> -----Original Message-----
> From: Antoine Tenart <antoine.tenart@xxxxxxxxxxx>
> Sent: Tuesday, July 30, 2019 11:08 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: [PATCHv2 2/3] crypto: inside-secure - add support for PCI based FPGA
> development board
>
> 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.
>
I think the rename makes sense within the context of this patch, as the
patch allows it to be run on our development board which may not contain
Marvell hardware (see also the answer below). The rename makes it clear
from the code, at least, that for now only Marvell hardware is supported.
> 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).
>
Yes, there is a very good reason. These flags control features that are
very specific to those three Marvell socs and have nothing to do with
'generic' EIP97IES's, EIP197B's or EIP197D's (these are just high-level
marketing/sales denominators and do not cover all the intricate config
details of the actual delivery that the driver needs to know about, so
this naive approach would not be maintainable scaling to other devices)
Therefore, I wanted to make that abundantly clear, hence the prefix.
(Renaming them to the specific Marvell SoC names was another option,
if only I knew which engine ended up in which SoC ...)
While there are many SoC's out there with EIP97 and EIP197 engines,
the driver in its current form will NOT work (properly) on them, for
that there is still much work to be done beyond the patches I already
submitted. I already have the implementation for that (you tested that
already!), but chopping it into bits and submitting it all will take
a lot more time. But you have to understand that, without that, it's
totally useless to either me or Verimatrix.
TL;DR: These flags are really just for controlling anything specific
to those particular Marvell instances and nothing else.
> > - 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.
>
The EIP97 being the exception as the only firmware-less engine.
This makes EIP197_DEVBRD fall through to EIP197B firmware until
my patches supporting other EIP197 configs eventually get merged,
after which this part will change anyway.
> > - if (priv->version != EIP197B)
> > + if (!(priv->version == EIP197B_MRVL))
>
> '!=' ?
>
Yes, that one looks funny ;-) May have been some global search and
replace going awry. Will fix.
> > - /* 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).
>
Not according to the Linux coding style (which only makes an exception
for /net) and not in most other crypto code I've seen. So maybe both
styles are allowed(?) and they are certainly both used, but this style
seems to be prevalent ...
> > /* 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).
>
Same situation as with the crypto mode: I know for a fact the EIP97
is the *only* configuration that *doesn't* need this code. So why
would I have a long list of configurations there (that will keep
growing indefinitely) that *do* need that code? That will for sure
not improve maintainability ...
> > @@ -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?
>
All algorithms provided at this moment are available from all engines
currently supported. So the whole mechanism, so far, is redundant.
This will change as I add support for generic (non-Marvell) engines,
but the approach taken here is not scalable or maintainable. So I will
end up doing it differently, eventually. I don't see the point in
maintaining dead/unused/redundant code I'm about to replace anyway.
> > @@ -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.
>
I don't see why or how. Same reasoning here: the EIP97 is the
only known exception so why bother with a full list of "the rest".
That's just more code to maintain and more error prone.
(and this code, like previous similar cases, will shortly need to
change when adding generic engine support, so this is just (hopefully
very) temporary anyway)
> > + if (IS_ENABLED(CONFIG_PCI) && (priv->version == EIP197_DEVBRD)) {
>
> You have extra parenthesis here.
>
Our internal coding style (as well as my personal preference)
actually mandates to put parenthesis around everything so expect
that to happen a lot going forward as I've been doing it like that
for nearly 30 years now.
Does the Linux coding style actually *prohibit* the use of these
"extra" parenthesis? I don't recall reading that anywhere ...
> > - platform_set_drvdata(pdev, priv);
>
> Side note: this is why you had to send the patch fixing rmmod.
>
Ah ... so that's where it accidentally disappeared :-)
> > 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.
>
Same comment as before
> > + 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?
>
Actually, no. The way I see it, priv->version does not convey any engine
information, just integration context (i.e. a specific Marvell SoC or, in
this case, our FPGA dev board), see also my explanation at the beginning.
Conveying engine information through a simple set of flags or some
integer or whatever is just not going to fly. No two of our engines
are ever the same, so that would quickly blow up in your face.
> 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.
>
As a result of that discussion, I kept your original version field
as intact as I could, knowing what I know and where I want to go.
But to truly support generic engines, we really need all the stuff
that I added. Because it simply has that many parameters that are
different for each individual instance. But at the same time these
parameters can all be probed from the hardware directly, so
maintaining huge if statements all over the place decoding some
artificial version field is not the way to go (not maintainable).
Just probe all the parameters from the hardware and use them
directly where needed ... which my future patch set will do.
> > +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))
>
Oops, I missed that one, will fix.
> Thanks!
> Antoine
>
> --
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Thanks,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
