Re: [PATCH 06/12] memory: stm32-fmc2-ebi: add RIF support

[Krzysztof Kozlowski]

On 12/02/2024 18:48, Christophe Kerello wrote:
> The FMC2 revision 2 supports security and isolation compliant with
> the Resource Isolation Framework (RIF). From RIF point of view,
> the FMC2 is composed of several independent resources, listed below,
> which can be assigned to different security and compartment domains:
>   - 0: Common FMC_CFGR register.
>   - 1: EBI controller for Chip Select 1.
>   - 2: EBI controller for Chip Select 2.
>   - 3: EBI controller for Chip Select 3.
>   - 4: EBI controller for Chip Select 4.
>   - 5: NAND controller.
> 


>  	regmap_update_bits(ebi->regmap, reg, mask, setup ? mask : 0);
>  
>  	return 0;
> @@ -990,6 +1023,107 @@ static const struct stm32_fmc2_prop stm32_fmc2_child_props[] = {
>  	},
>  };
>  
> +static int stm32_fmc2_ebi_check_rif(struct stm32_fmc2_ebi *ebi, u32 resource)
> +{
> +	u32 seccfgr, cidcfgr, semcr;
> +	int cid;
> +
> +	if (ebi->majrev < FMC2_VERR_MAJREV_2)
> +		return 0;
> +
> +	if (resource >= FMC2_MAX_RESOURCES)
> +		return -EINVAL;
> +
> +	regmap_read(ebi->regmap, FMC2_SECCFGR, &seccfgr);

No checking of read value?

> +	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!

>  }
>  
> @@ -1124,7 +1264,8 @@ static void stm32_fmc2_ebi_enable(struct stm32_fmc2_ebi *ebi)
>  	u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1_FMC2EN :
>  						      FMC2_CFGR_FMC2EN;
>  
> -	regmap_update_bits(ebi->regmap, reg, mask, mask);
> +	if (ebi->access_granted)
> +		regmap_update_bits(ebi->regmap, reg, mask, mask);
>  }
>  
>  static void stm32_fmc2_ebi_disable(struct stm32_fmc2_ebi *ebi)
> @@ -1133,7 +1274,8 @@ static void stm32_fmc2_ebi_disable(struct stm32_fmc2_ebi *ebi)
>  	u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1_FMC2EN :
>  						      FMC2_CFGR_FMC2EN;
>  
> -	regmap_update_bits(ebi->regmap, reg, mask, 0);
> +	if (ebi->access_granted)
> +		regmap_update_bits(ebi->regmap, reg, mask, 0);
>  }
>  
>  static int stm32_fmc2_ebi_setup_cs(struct stm32_fmc2_ebi *ebi,
> @@ -1190,6 +1332,13 @@ static int stm32_fmc2_ebi_parse_dt(struct stm32_fmc2_ebi *ebi)
>  			return -EINVAL;
>  		}
>  
> +		ret = stm32_fmc2_ebi_check_rif(ebi, bank + 1);
> +		if (ret) {
> +			dev_err(dev, "bank access failed: %d\n", bank);
> +			of_node_put(child);
> +			return ret;
> +		}
> +
>  		if (bank < FMC2_MAX_EBI_CE) {
>  			ret = stm32_fmc2_ebi_setup_cs(ebi, child, bank);
>  			if (ret) {
> @@ -1261,6 +1410,23 @@ static int stm32_fmc2_ebi_probe(struct platform_device *pdev)
>  	regmap_read(ebi->regmap, FMC2_VERR, &verr);
>  	ebi->majrev = FIELD_GET(FMC2_VERR_MAJREV, verr);
>  
> +	/* Check if CFGR register can be modified */
> +	ret = stm32_fmc2_ebi_check_rif(ebi, 0);
> +	if (!ret)
> +		ebi->access_granted = true;

I don't understand why you need to store it. If access is not granted,
what else is to do for this driver? Why even probing it? Why enabling
clocks and keep everything running if it cannot work?

> +
> +	/* In case of CFGR is secure, just check that the FMC2 is enabled */
> +	if (!ebi->access_granted) {

This is just "else", isn't it?

> +		u32 sr;
> +
> +		regmap_read(ebi->regmap, FMC2_SR, &sr);
> +		if (sr & FMC2_SR_ISOST) {
> +			dev_err(dev, "FMC2 is not ready to be used.\n");
> +			ret = -EACCES;
> +			goto err_release;
> +		}
> +	}
> +
>  	ret = stm32_fmc2_ebi_parse_dt(ebi);

>  

Best regards,
Krzysztof