Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface

['Greg KH']

On Mon, Feb 10, 2020 at 04:57:01PM -0800, Jolly Shah wrote:
> Hi Greg,
> 
> > ------Original Message------
> > From: 'Greg Kh' <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Sent:  Friday, January 31, 2020 1:36AM
> > To: Rajan Vaja <RAJANV@xxxxxxxxxx>
> > Cc: Jolly Shah <JOLLYS@xxxxxxxxxx>, Ard Biesheuvel
> <ard.biesheuvel@xxxxxxxxxx>, Mingo <mingo@xxxxxxxxxx>, Matt
> <matt@xxxxxxxxxxxxxxxxxxx>, Sudeep Holla <sudeep.holla@xxxxxxx>, Hkallweit1
> <hkallweit1@xxxxxxxxx>, Keescook <keescook@xxxxxxxxxxxx>, Dmitry Torokhov
> <dmitry.torokhov@xxxxxxxxx>, Michal Simek <michals@xxxxxxxxxx>,
> Linux-arm-kernel <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>, Linux-kernel
> <linux-kernel@xxxxxxxxxxxxxxx>
> > Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
> >
> > On Fri, Jan 31, 2020 at 09:05:15AM +0000, Rajan Vaja wrote:
> > > Hi Greg,
> > > 
> > > > -----Original Message-----
> > > > From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > > Sent: 31 January 2020 11:41 AM
> > > > To: Jolly Shah <JOLLYS@xxxxxxxxxx>
> > > > Cc: ard.biesheuvel@xxxxxxxxxx; mingo@xxxxxxxxxx; matt@xxxxxxxxxxxxxxxxxxx;
> > > > sudeep.holla@xxxxxxx; hkallweit1@xxxxxxxxx; keescook@xxxxxxxxxxxx;
> > > > dmitry.torokhov@xxxxxxxxx; Michal Simek <michals@xxxxxxxxxx>; Rajan Vaja
> > > > <RAJANV@xxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> > > > kernel@xxxxxxxxxxxxxxx
> > > > Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
> > > > 
> > > > EXTERNAL EMAIL
> > > > 
> > > > On Thu, Jan 30, 2020 at 11:59:03PM +0000, Jolly Shah wrote:
> > > > > Hi Greg,
> > > > > 
> > > > > ïOn 1/27/20, 10:28 PM, "linux-kernel-owner@xxxxxxxxxxxxxxx on behalf of Greg
> > > > KH" <linux-kernel-owner@xxxxxxxxxxxxxxx on behalf of
> > > > gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > > > 
> > > > >      On Mon, Jan 27, 2020 at 11:01:27PM +0000, Jolly Shah wrote:
> > > > >      >     > > > +     ret = kstrtol(tok, 16, &value);
> > > > >      >     > > > +     if (ret) {
> > > > >      >     > > > +             ret = -EFAULT;
> > > > >      >     > > > +             goto err;
> > > > >      >     > > > +     }
> > > > >      >     > > > +
> > > > >      >     > > > +     ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
> > > > >      >     > >
> > > > >      >     > > This feels "tricky", if you tie this to the device you have your driver
> > > > >      >     > > bound to, will this make it easier instead of having to go through the
> > > > >      >     > > ioctl callback?
> > > > >      >     > >
> > > > >      >     >
> > > > >      >     > GGS(general global storage) registers are in PMU space and linux
> > > > doesn't have access to it
> > > > >      >     > Hence ioctl is used.
> > > > >      >
> > > > >      >     Why not just a "real" call to the driver to make this type of reading?
> > > > >      >     You don't have ioctls within the kernel for other drivers to call,
> > > > >      >     that's not needed at all.
> > > > >      >
> > > > >      > these registers are for users  and for special needs where users wants
> > > > >      > to retain values over resets. but as they belong to PMU address space,
> > > > >      > these interface APIs are provided. They donât allow access to any
> > > > >      > other registers.
> > > > > 
> > > > >      That's not the issue here.  The issue is you are using an "internal"
> > > > >      ioctl, instead just make a "real" call.
> > > > > 
> > > > > Sorry I am not clear. Do you mean that we should use linux standard
> > > > > ioctl interface instead of internal ioctl by mentioning "real" ?
> > > > 
> > > > No, you should just make a "real" function call to the exact thing you
> > > > want to do.  Not have an internal multi-plexor ioctl() call that others
> > > > then call.  This isn't a microkernel :)
> > > [Rajan] Sorry for multiple back and forth but as I understand, you are suggesting to create a new API for
> > > Read/write of GGS register instead of using PM_IOCTL API (eemi_ops->ioctl) for multiple purpose. Is my understanding correct?
> > 
> > That is correct.
> 
> 
> 
> Would like to clarify purpose of having ioctl API to avoid any confusion.
> eemi interface apis are defined to be platform independent and allows clock,
> reset, power etc management through firmware but apart from these generic
> operations, there are some operations which needs secure access through
> firmware. Examples are accessing some storage registers(ggs and pggs) for
> inter agent communication, configuring another agent(RPU) mode, boot device
> configuration etc. Those operations are covered as ioctls as they are very
> platform specific. Also only whitelisted operations are allowed through
> ioctl and is not exposed to user for any random read/write operation.
> 
> Olof earlier had same concerns. We had clarified the purpose and with his
> agreement, initial set of ioctls were accepted.
> (https://www.lkml.org/lkml/2018/9/24/1570)
> 
> Please suggest the best approach to handle this for current and future
> patches.

Ok, in looking further at this, it's both better than I thought, and
totally worse.

This interface you all are using where you ask the firmware driver for a
pointer to a set of operation functions and then make calls through that
is indicitive of an api that is NOT what we normally use in Linux at
all.

Just make the direct call to the firmware driver, no need to muck around
with tables of function pointers.  In fact, with the spectre changes,
you just made things slower than needed, and you can get back a bunch of
throughput by removing that whole middle layer.

So go do that first please, before adding any new stuff.

Now for the ioctl, yeah, that's not a "normal" pattern either.  But
right now you only have 2 "different" ioctls that you call.  So why not
just turn those 2 into real function calls as well that then makes the
"ioctl" call to the hardware?  That makes things a lot more obvious on
the kernel driver side exactly what is going on.

If you need to add more "ioctl" like calls, just add them as more
functions, no big deal.  How many more of these are you going to need
over time?

But that's not all that big of a deal right now, get rid of that whole
middle-layer first, that's more important to clean up.  You will get rid
of a lot of unneeded code and indirection that way, making it simpler
and easier to understand what exactly is happening.

thanks,

greg k-h