lkml.org 
[lkml]   [2024]   [Feb]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 06/12] memory: stm32-fmc2-ebi: add RIF support
    From


    On 2/14/24 11:07, Krzysztof Kozlowski wrote:
    > On 13/02/2024 14:15, Christophe Kerello wrote:
    >>>> +
    >>>> + if (ebi->majrev < FMC2_VERR_MAJREV_2)
    >>>> + return 0;
    >>>> +
    >>>> + if (resource >= FMC2_MAX_RESOURCES)
    >>>> + return -EINVAL;
    >>>> +
    >>>> + regmap_read(ebi->regmap, FMC2_SECCFGR, &seccfgr);
    >>
    >> Hi Krzysztof,
    >>
    >>>
    >>> No checking of read value?
    >>>
    >>
    >> No, it should never failed.
    >
    > And you tested that neither smatch, sparse nor Coverity report here
    > warnings?
    >

    Hi Krzysztof,

    There is a lot of driver in the Kernel that are using same
    implementation, and I am surprised to not have had this comment 4 years
    ago when the driver was introduced.

    So, how should I proceed? Shall I initialize all local variables used by
    regmap_read? Or shall I check the return value of regmap_read?
    And, as there is a lot of regmap_read call in this driver, shall I fix
    them in a separate patch?

    >>
    >>>> + if (seccfgr & BIT(resource)) {
    >>>
    >>> Then on read failure this is random stack junk.
    >>>
    >>>> + if (resource)
    >>>> + dev_err(ebi->dev, "resource %d is configured as secure\n",
    >>>> + resource);
    >>>> +
    >>>> + return -EACCES;
    >>>> + }
    >>>> +
    >>>> + regmap_read(ebi->regmap, FMC2_CIDCFGR(resource), &cidcfgr);
    >>>> + if (!(cidcfgr & FMC2_CIDCFGR_CFEN))
    >>>> + /* CID filtering is turned off: access granted */
    >>>> + return 0;
    >>>> +
    >>>> + if (!(cidcfgr & FMC2_CIDCFGR_SEMEN)) {
    >>>> + /* Static CID mode */
    >>>> + cid = FIELD_GET(FMC2_CIDCFGR_SCID, cidcfgr);
    >>>> + if (cid != FMC2_CID1) {
    >>>> + if (resource)
    >>>> + dev_err(ebi->dev, "static CID%d set for resource %d\n",
    >>>> + cid, resource);
    >>>> +
    >>>> + return -EACCES;
    >>>> + }
    >>>> +
    >>>> + return 0;
    >>>> + }
    >>>> +
    >>>> + /* Pass-list with semaphore mode */
    >>>> + if (!(cidcfgr & FMC2_CIDCFGR_SEMWLC1)) {
    >>>> + if (resource)
    >>>> + dev_err(ebi->dev, "CID1 is block-listed for resource %d\n",
    >>>> + resource);
    >>>> +
    >>>> + return -EACCES;
    >>>> + }
    >>>> +
    >>>> + regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr);
    >>>> + if (!(semcr & FMC2_SEMCR_SEM_MUTEX)) {
    >>>> + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
    >>>> + FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX);
    >>>> + regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr);
    >>>> + }
    >>>> +
    >>>> + cid = FIELD_GET(FMC2_SEMCR_SEMCID, semcr);
    >>>> + if (cid != FMC2_CID1) {
    >>>> + if (resource)
    >>>> + dev_err(ebi->dev, "resource %d is already used by CID%d\n",
    >>>> + resource, cid);
    >>>> +
    >>>> + return -EACCES;
    >>>> + }
    >>>> +
    >>>> + ebi->sem_taken |= BIT(resource);
    >>>> +
    >>>> + return 0;
    >>>> +}
    >>>> +
    >>>> +static void stm32_fmc2_ebi_put_sems(struct stm32_fmc2_ebi *ebi)
    >>>> +{
    >>>> + unsigned int resource;
    >>>> +
    >>>> + if (ebi->majrev < FMC2_VERR_MAJREV_2)
    >>>> + return;
    >>>> +
    >>>> + for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) {
    >>>> + if (!(ebi->sem_taken & BIT(resource)))
    >>>> + continue;
    >>>> +
    >>>> + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
    >>>> + FMC2_SEMCR_SEM_MUTEX, 0);
    >>>> + }
    >>>> +}
    >>>> +
    >>>> +static void stm32_fmc2_ebi_get_sems(struct stm32_fmc2_ebi *ebi)
    >>>> +{
    >>>> + unsigned int resource;
    >>>> +
    >>>> + if (ebi->majrev < FMC2_VERR_MAJREV_2)
    >>>> + return;
    >>>> +
    >>>> + for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) {
    >>>> + if (!(ebi->sem_taken & BIT(resource)))
    >>>> + continue;
    >>>> +
    >>>> + regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
    >>>> + FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX);
    >>>> + }
    >>>> +}
    >>>> +
    >>>> static int stm32_fmc2_ebi_parse_prop(struct stm32_fmc2_ebi *ebi,
    >>>> struct device_node *dev_node,
    >>>> const struct stm32_fmc2_prop *prop,
    >>>> @@ -1057,6 +1191,9 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi)
    >>>> unsigned int cs;
    >>>>
    >>>> for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) {
    >>>> + if (!(ebi->bank_assigned & BIT(cs)))
    >>>> + continue;
    >>>> +
    >>>> regmap_read(ebi->regmap, FMC2_BCR(cs), &ebi->bcr[cs]);
    >>>> regmap_read(ebi->regmap, FMC2_BTR(cs), &ebi->btr[cs]);
    >>>> regmap_read(ebi->regmap, FMC2_BWTR(cs), &ebi->bwtr[cs]);
    >>>> @@ -1064,7 +1201,7 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi)
    >>>>
    >>>> if (ebi->majrev < FMC2_VERR_MAJREV_2)
    >>>> regmap_read(ebi->regmap, FMC2_PCSCNTR, &ebi->pcscntr);
    >>>> - else
    >>>> + else if (ebi->access_granted)
    >>>> regmap_read(ebi->regmap, FMC2_CFGR, &ebi->cfgr);
    >>>> }
    >>>>
    >>>> @@ -1073,6 +1210,9 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi)
    >>>> unsigned int cs;
    >>>>
    >>>> for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) {
    >>>> + if (!(ebi->bank_assigned & BIT(cs)))
    >>>> + continue;
    >>>> +
    >>>> regmap_write(ebi->regmap, FMC2_BCR(cs), ebi->bcr[cs]);
    >>>> regmap_write(ebi->regmap, FMC2_BTR(cs), ebi->btr[cs]);
    >>>> regmap_write(ebi->regmap, FMC2_BWTR(cs), ebi->bwtr[cs]);
    >>>> @@ -1080,7 +1220,7 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi)
    >>>>
    >>>> if (ebi->majrev < FMC2_VERR_MAJREV_2)
    >>>> regmap_write(ebi->regmap, FMC2_PCSCNTR, ebi->pcscntr);
    >>>> - else
    >>>> + else if (ebi->access_granted)
    >>>> regmap_write(ebi->regmap, FMC2_CFGR, ebi->cfgr);
    >>>
    >>> So this is kind of half-allowed-half-not. How is it supposed to work
    >>> with !access_granted? You configure some registers but some not. So will
    >>> it work or not? If yes, why even needing to write to FMC2_CFGR!
    >>>
    >>
    >> This register is considered as one resource and can be protected. If a
    >> companion (like optee_os) has configured this resource as secure, it
    >> means that the driver can not write into this register, and this
    >> register will be handled by the companion. If this register is let as
    >> non secure, the driver can handle this ressource.
    >
    > So this is not an error? Other places print errors and return -EACCESS,
    > so that's a bit confusing me.
    >

    It is not an error. We are saving registers values to restore them on
    low power cases. If registers are set as secure, it is the
    responsibility of the companion to restore them when it is the
    responsibility of the non secure driver to restore them if they are
    configured as non secure.

    Regards,
    Christophe Kerello.

    >
    > Best regards,
    > Krzysztof
    >

    \
     
     \ /
      Last update: 2024-05-27 15:04    [W:2.666 / U:0.268 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site