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

[Jolly Shah]

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.

Thanks,
Jolly Shah


>