Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
From: Jolly Shah
Date: Fri Feb 14 2020 - 19:37:46 EST
Hi Greg,
Thanks for the response.
> ------Original Message------
> From: 'Greg Kh' <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Friday, February 14, 2020 9:10AM
> To: Jolly Shah <jolly.shah@xxxxxxxxxx>
> Cc: Rajan Vaja <RAJANV@xxxxxxxxxx>, Ard.biesheuvel@xxxxxxxxxx
<ard.biesheuvel@xxxxxxxxxx>, Mingo@xxxxxxxxxx <mingo@xxxxxxxxxx>,
Matt@xxxxxxxxxxxxxxxxxxx <matt@xxxxxxxxxxxxxxxxxxx>,
Sudeep.holla@xxxxxxx <sudeep.holla@xxxxxxx>, Hkallweit1@xxxxxxxxx
<hkallweit1@xxxxxxxxx>, Keescook@xxxxxxxxxxxx <keescook@xxxxxxxxxxxx>,
Dmitry.torokhov@xxxxxxxxx <dmitry.torokhov@xxxxxxxxx>, Michal Simek
<michals@xxxxxxxxxx>, Linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
<linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>, Linux-kernel@xxxxxxxxxxxxxxx
<linux-kernel@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
>
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.
arm,scpi is doing the same way and we thought this approach will be more
acceptable than direct function calls but happy to change as suggested.
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.
Sure as i understand firmware driver will provide real function calls to
be used by user drivers and underneath it will call ioctl for desired
operation. Please correct if I misunderstood.
Thanks,
Jolly Shah
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