Hi Antoine,
Thanks for the review and I agree with all of your comments below.
So I'm willing to fix those but I'm a bit unclear of the procedure now,
since you acked part of the patch set already.
Should I resend just the subpatches that need fixes or should I resend
the whole patchset? Please advise :-)
> -----Original Message-----
> From: linux-crypto-owner@xxxxxxxxxxxxxxx <linux-crypto-owner@xxxxxxxxxxxxxxx> On Behalf Of
> Antoine Tenart
> Sent: Monday, August 5, 2019 10:36 AM
> To: Pascal van Leeuwen <pascalvanl@xxxxxxxxx>
> Cc: linux-crypto@xxxxxxxxxxxxxxx; antoine.tenart@xxxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx;
> davem@xxxxxxxxxxxxx; Pascal Van Leeuwen <pvanleeuwen@xxxxxxxxxxxxxx>
> Subject: Re: [PATCHv3 3/4] crypto: inside-secure - add support for PCI based FPGA
> development board
>
> 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
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
