Re: [PATCHv2 2/3] crypto: inside-secure - add support for PCI based FPGA development board

[Antoine Tenart <antoine.tenart@bootlin.com>]

Hi Pascal,

On Fri, Jul 26, 2019 at 02:43:24PM +0200, Pascal van Leeuwen wrote:
> +	if (priv->version == EIP197D_MRVL) {

I see you renamed EIP197D to EIP197D_MRVL in the v2. Such a rename
should not be part of this patch, as it has nothing to do with the
engine you're adding support for.

Is there a reason to have this one linked to Marvell? Aren't there other
EIP197 (or EIP97) engines not on Marvell SoCs? (I'm pretty sure I know
at least one).

> -	switch (priv->version) {
> -	case EIP197B:
> -		dir = "eip197b";
> -		break;
> -	case EIP197D:
> -		dir = "eip197d";
> -		break;
> -	default:
> +	if (priv->version == EIP97IES_MRVL)
>  		/* No firmware is required */
>  		return 0;
> -	}
> +	else if (priv->version == EIP197D_MRVL)
> +		dir = "eip197d";
> +	else
> +		/* Default to minimum EIP197 config */
> +		dir = "eip197b";

You're moving the default choice from "no firmware" to being a specific
one.

> -			if (priv->version != EIP197B)
> +			if (!(priv->version == EIP197B_MRVL))

'!=' ?

> -			/* Fallback to the old firmware location for the
> +			/*
> +			 * Fallback to the old firmware location for the

This is actually the expected comment style in net/ and crypto/. (There
are other examples).

>  	/* For EIP197 set maximum number of TX commands to 2^5 = 32 */
> -	if (priv->version == EIP197B || priv->version == EIP197D)
> +	if (priv->version != EIP97IES_MRVL)

I would really prefer having explicit checks here. More engines will be
supported in the future and doing will help. (There are others).

> @@ -869,9 +898,6 @@ static int safexcel_register_algorithms(struct safexcel_crypto_priv *priv)
>  	for (i = 0; i < ARRAY_SIZE(safexcel_algs); i++) {
>  		safexcel_algs[i]->priv = priv;
>  
> -		if (!(safexcel_algs[i]->engines & priv->version))
> -			continue;

You should remove the 'engines' flag in a separate patch. I'm really not
sure about this. I don't think all the IS EIP engines support the same
sets of algorithms?

> @@ -925,22 +945,18 @@ static void safexcel_configure(struct safexcel_crypto_priv *priv)
>  	val = readl(EIP197_HIA_AIC_G(priv) + EIP197_HIA_OPTIONS);
>  
>  	/* Read number of PEs from the engine */
> -	switch (priv->version) {
> -	case EIP197B:
> -	case EIP197D:
> -		mask = EIP197_N_PES_MASK;
> -		break;
> -	default:
> +	if (priv->version == EIP97IES_MRVL)
>  		mask = EIP97_N_PES_MASK;
> -	}
> +	else
> +		mask = EIP197_N_PES_MASK;
> +

You also lose readability here.

> +	if (IS_ENABLED(CONFIG_PCI) && (priv->version == EIP197_DEVBRD)) {

You have extra parenthesis here.

> -	platform_set_drvdata(pdev, priv);

Side note: this is why you had to send the patch fixing rmmod.

>  static void safexcel_hw_reset_rings(struct safexcel_crypto_priv *priv)
> @@ -1160,6 +1139,78 @@ static void safexcel_hw_reset_rings(struct safexcel_crypto_priv *priv)
>  	}
>  }
>  
> +#if (IS_ENABLED(CONFIG_OF))

No need for the extra parenthesis here.

> +	if (priv->version == EIP197_DEVBRD) {

It seems to me this is mixing an engine version information and a board
were the engine is integrated. Are there differences in the engine
itself, or only in the way it's wired?

We had this discussion on the v1. Your point was that you wanted this
information to be in .data. One solution I proposed then was to use a
struct (with both a 'version' and a 'flag' variable inside) instead of
a single 'version' variable, so that we still can make checks on the
version itself and not on something too specific.

> +static int __init crypto_is_init(void)
> +{
> +	int rc;
> +
> +	#if (IS_ENABLED(CONFIG_OF))
> +		/* Register platform driver */
> +		platform_driver_register(&crypto_safexcel);
> +	#endif

When used in the code directly, you should use:

  if (IS_ENABLED(CONFIG_OF))

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com