Hi Pascal,
On Thu, Jun 20, 2019 at 02:47:30PM +0000, Pascal Van Leeuwen wrote:
> > From: Antoine Tenart <antoine.tenart@xxxxxxxxxxx>
> > On Wed, Jun 19, 2019 at 02:22:19PM +0000, Pascal Van Leeuwen wrote:
> > > > From: Antoine Tenart <antoine.tenart@xxxxxxxxxxx>
> > > > On Tue, Jun 18, 2019 at 07:56:23AM +0200, Pascal van Leeuwen wrote:
> > > > >
> > > > > /* Fallback to the old firmware location for the
> > > > > @@ -294,6 +291,9 @@ static int safexcel_hw_init(struct safexcel_crypto_priv *priv)
> > > > >
> > > > > + dev_info(priv->dev, "EIP(1)97 HW init: burst size %d beats, using %d pipe(s) and %d
> > > > ring(s)",
> > > > > + 16, priv->config.pes, priv->config.rings);
> > > >
> > > > Adding custom messages in the kernel log has to be done carefully.
> > > > Although it's not considered stable it could be difficult to rework
> > > > later on. Also, if all driver were to print custom messages the log
> > > > would be very hard to read. But you can also argue that a single message
> > > > when probing a driver is also done in other drivers.
> > > >
> > > Hmm ... don't know what the rules for logging are exactly, but from my
> > > perspective, I'm dealing with a zillion different HW configurations so
> > > some feedback whether the driver detected the *correct* HW parameters -
> > > or actually, whether I stuffed the correct image into my FPGA :o) - is
> > > very convenient to have. And not just for my local development, but also
> > > to debug deployments in the field at customer sites.
> >
> > I understand it can be convenient, it's just a matter of having a
> > logging message for you that will end up in many builds for many users.
> > They do not necessarily have the same needs. So it's a matter of
> > compromise, one or two messages at boot can be OK, more is likely to
> > become an issue.
> >
> OK, got it. So I have to stuff all my logging into one or two very long lines :-P
> (just kidding)
Hehe :-)
> > > > For this one particularly, the probe could fail later on. So if we were
> > > > to add this output, it should be done at the very end of the probe.
> > > >
> > > I'm in doubt about this one. I understand that you want to reduce the
> > > logging in that case, but at the same time that message can convey
> > > information as to WHY the probing fails later on ...
> >
> > If the drivers fails to probe, there will be other messages. In that
> > case is this one really needed? I'm not sure.
> >
> > > i.e. if it detects, say, 4 pipes on a device that, in fact, only has
> > > 2, then that may be the very reason for the FW init to fail later on.
> >
> > In case of failure you'll need anyway to debug and understand what's
> > going on. By adding new prints, or enabling debugging messages.
> >
> If it fails for me locally, I can do that. If it somehow fails "in the field",
> I think most people won't be able to recompile their own Linux
> kernel with debug messages let alone add their own debug messages.
>
> Anyway, I'll just make everything dev_dbg to avoid further discussion.
There's always the 'loglevel' command-line parameter, but yes, that
probably do not cover all cases.
> > > So in my humble opinion, version was the correct location, it
> > > is just a confusing name. (i.e. you can have many *versions*
> > > of an EIP197B, for instance ...)
> >
> > That would be an issue with the driver. We named the 'version' given the
> > knowledge we had of the h/w, it might not be specific enough. Or maybe
> > you can think of this as being a "family of engine versions". The idea
> > is the version is what the h/w is capable of, not how it's being
> > wired/accessed.
>
> Well ... I want to avoid the whole discussion about the naming of the
> variable (which can be trivially changed) and what the intention may
> have been, if you allow me.
>
> Fact is ... this variable is what receives .data / .driver_data from the
> OF or PCI match table. So it is a means of conveying a value that is
> specific to the table entry that was matched. No more, no less.
> In "your" device tree case you want to distinguish between
> Armada 39x, Armada 7K/8K and Armada 9K. In "my" PCI case I
> want to potentially distinguish multiple FPGA boards/images.
>
> It wouldn't make much sense to me to do the vendor/subvendor/
> device/subdevice decoding all over again in my probe routine.
> So what exactly is so very wrong with the way I'm doing this?
I think what is an issue for me here is the re-use of a variable
intended to only control the version of the engine. And the way this
engine is probed/accessed has nothing to do with this.
One solution, that I think would work for both of us, would be to still
keep this information in .data (as you did) but to organise it within a
struct so that the version information is split from the way the device
is accessed. Would that work for you?
('version' can probably be a value and not a bitfield then).
I'm sorry if the discussion about this point seems disproportionate
compared to technical aspect, but I would like to avoid possible
maintenance issues in the future with conditions looking like:
if (version == EIP197)
Which could easily be merged in a big patch but would break the
existing, given on what h/w the submitter tested the changes.
> > > > > @@ -1189,13 +1249,12 @@ static int safexcel_remove(struct platform_device *pdev)
> > > > > .compatible = "inside-secure,safexcel-eip197d",
> > > > > .data = (void *)EIP197D,
> > > > > },
> > > > > + /* For backward compatibiliry and intended for generic use */
> > > > > {
> > > > > - /* Deprecated. Kept for backward compatibility. */
> > > > > .compatible = "inside-secure,safexcel-eip97",
> > > > > .data = (void *)EIP97IES,
> > > > > },
> > > > > {
> > > > > - /* Deprecated. Kept for backward compatibility. */
> > > > > .compatible = "inside-secure,safexcel-eip197",
> > > > > .data = (void *)EIP197B,
> > > > > },
> > > >
> > > > I'm not sure about this. The compatible should describe what the
> > > > hardware is, and the driver can then decide if it has special things to
> > > > do or not. It is not used to configure the driver to be used with a
> > > > generic use or not.
> > > >
> > > > Do you have a practical reason to do this?
> > >
> > > I have to admit I don't fully understand how these compatible
> > > strings work. All I wanted to achieve is provide some generic
> > > device tree entry to point to this driver, to be used for
> > > devices other than Marvell. No need to convey b/d that way
> > > (or even eip97/197, for that matter) as that can all be probed.
> >
> > Compatibles are used in device trees, which intend to be a description
> > of the hardware (not the configuration of how the hardware should be
> > used). So we can't have a compatible being a restricted configuration
> > use of a given hardware. But I think here you're right, and there is
> > room for a more generic eip197 compatible: the b/d versions only have
> > few differences and are part of the same family, so we can have a very
> > specific compatible plus a "family" one. Something like:
> >
> > compatible = "inside-secure,safexcel-eip197d", "inside-secure,safexcel-eip197";
> >
> > This would need to be in a separated patch, and this should be
> > documented in:
> > Documentation/devicetree/bindings/crypto/inside-secure-safexcel.txt
> >
> Ok, then I'll leave that part untouched for now.
> (I only changed the comments anyway ...)
Feel free to send a patch later on :) (Even if it's only about the
comment, it is important as well).
> > > > > +static struct pci_driver crypto_is_pci_driver = {
> > > > > + .name = "crypto-safexcel",
> > > > > + .id_table = crypto_is_pci_ids,
> > > > > + .probe = crypto_is_pci_probe,
> > > > > + .remove = crypto_is_pci_remove,
> > > > > +};
> > > >
> > > > More generally, you should protect all the PCI specific functions and
> > > > definitions between #ifdef.
> > > >
> > > I asked the mailing list and the answer was I should NOT use #ifdef,
> > > but instead use IS_ENABLED to *only* remove relevant function bodies.
> > > Which is exactly what I did (or tried to do, anyway).
> >
> > My bad, I realise there's a mistake in my comment. That should have
> > been: you should protect all the PCI specific functions and definitions
> > with #if IS_ENABLED(...). When part of a function should be excluded
> > you can use if(IS_ENABLED(...)), but if the entire function can be left
> > out, #if is the way to go.
> >
> Ok #if instead of if or #ifdef,that makes sense.
> So can I just put all the PCI stuff into one big #if then?
Right.
You may also want to check for for helpers only defined if CONFIG_OF
is selected, as the driver could be compiled for a kernel with only
CONFIG_PCI enabled.
Thanks!
Antoine
--
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
