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

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

Hello Pascal,

The patch looks mostly good, just a few comments below.

On Wed, Jul 31, 2019 at 05:29:18PM +0200, Pascal van Leeuwen wrote:
> @@ -381,10 +383,11 @@ static int safexcel_hw_init(struct safexcel_crypto_priv *priv)
>  		       EIP197_HIA_DxE_CFG_MAX_DATA_SIZE(8);
>  		val |= EIP197_HIA_DxE_CFG_DATA_CACHE_CTRL(WR_CACHE_3BITS);
>  		val |= EIP197_HIA_DSE_CFG_ALWAYS_BUFFERABLE;
> -		/* FIXME: instability issues can occur for EIP97 but disabling it impact
> -		 * performances.
> +		/*
> +		 * FIXME: instability issues can occur for EIP97 but disabling
> +		 * it impacts performance.
>  		 */

No need to change the comment style here.

> @@ -514,7 +517,8 @@ void safexcel_dequeue(struct safexcel_crypto_priv *priv, int ring)
>  	struct safexcel_context *ctx;
>  	int ret, nreq = 0, cdesc = 0, rdesc = 0, commands, results;
> 
> -	/* If a request wasn't properly dequeued because of a lack of resources,
> +	/*
> +	 * If a request wasn't properly dequeued because of a lack of resources,
>  	 * proceeded it first,
>  	 */

Ditto.

> @@ -543,7 +547,8 @@ void safexcel_dequeue(struct safexcel_crypto_priv *priv, int ring)
> 
> -		/* In case the send() helper did not issue any command to push
> +		/*
> +		 * In case the send() helper did not issue any command to push

Ditto.

> -	/* Not enough resources to handle all the requests. Bail out and save
> +	/*
> +	 * Not enough resources to handle all the requests. Bail out and save

Ditto.

> @@ -731,7 +738,8 @@ static inline void safexcel_handle_result_descriptor(struct safexcel_crypto_priv
>  		       EIP197_xDR_PROC_xD_COUNT(tot_descs * priv->config.rd_offset),
>  		       EIP197_HIA_RDR(priv, ring) + EIP197_HIA_xDR_PROC_COUNT);
> 
> -	/* If the number of requests overflowed the counter, try to proceed more
> +	/*
> +	 * If the number of requests overflowed the counter, try to proceed more

Ditto.

> +#if IS_ENABLED(CONFIG_OF)
> +/*
> + * for Device Tree platform driver
> + */

Single line comment should be:

/* comment */

> +#if IS_ENABLED(CONFIG_PCI)
> +/*
> + * PCIE devices - i.e. Inside Secure development boards
> + */

Here as well.

> +static int crypto_is_pci_probe(struct pci_dev *pdev,
> +			       const struct pci_device_id *ent)

The whole driver uses the "safexcel_" prefix for functions. You should
use it here as well for consistency.

> +void crypto_is_pci_remove(struct pci_dev *pdev)

Here as well.

> +static const struct pci_device_id crypto_is_pci_ids[] = {

Here as well.

> +static struct pci_driver crypto_is_pci_driver = {

Here as well.

> +static int __init crypto_is_init(void)

Here as well.

> +static void __exit crypto_is_exit(void)

Here as well.

Thanks,
Antoine

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