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

[Jolly Shah <jolly.shah@xilinx.com>]

Hi Greg,

Thanks for the response.

 > ------Original Message------
 > From: 'Greg Kh' <gregkh@linuxfoundation.org>
 > Sent:  Friday, February 14, 2020 9:10AM
 > To: Jolly Shah <jolly.shah@xilinx.com>
 > Cc: Rajan Vaja <RAJANV@xilinx.com>, Ard.biesheuvel@linaro.org 
<ard.biesheuvel@linaro.org>, Mingo@kernel.org <mingo@kernel.org>, 
Matt@codeblueprint.co.uk <matt@codeblueprint.co.uk>, 
Sudeep.holla@arm.com <sudeep.holla@arm.com>, Hkallweit1@gmail.com 
<hkallweit1@gmail.com>, Keescook@chromium.org <keescook@chromium.org>, 
Dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com>, Michal Simek 
<michals@xilinx.com>, Linux-arm-kernel@lists.infradead.org 
<linux-arm-kernel@lists.infradead.org>, Linux-kernel@vger.kernel.org 
<linux-kernel@vger.kernel.org>
 > 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@linuxfoundation.org>
>>> Sent:  Friday, January 31, 2020 1:36AM
>>> To: Rajan Vaja <RAJANV@xilinx.com>
>>> Cc: Jolly Shah <JOLLYS@xilinx.com>, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>, Mingo <mingo@kernel.org>, Matt
>> <matt@codeblueprint.co.uk>, Sudeep Holla <sudeep.holla@arm.com>, Hkallweit1
>> <hkallweit1@gmail.com>, Keescook <keescook@chromium.org>, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com>, Michal Simek <michals@xilinx.com>,
>> Linux-arm-kernel <linux-arm-kernel@lists.infradead.org>, Linux-kernel
>> <linux-kernel@vger.kernel.org>
>>> 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@linuxfoundation.org>
>>>>> Sent: 31 January 2020 11:41 AM
>>>>> To: Jolly Shah <JOLLYS@xilinx.com>
>>>>> Cc: ard.biesheuvel@linaro.org; mingo@kernel.org; matt@codeblueprint.co.uk;
>>>>> sudeep.holla@arm.com; hkallweit1@gmail.com; keescook@chromium.org;
>>>>> dmitry.torokhov@gmail.com; Michal Simek <michals@xilinx.com>; Rajan Vaja
>>>>> <RAJANV@xilinx.com>; linux-arm-kernel@lists.infradead.org; linux-
>>>>> kernel@vger.kernel.org
>>>>> 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@vger.kernel.org on behalf of Greg
>>>>> KH" <linux-kernel-owner@vger.kernel.org on behalf of
>>>>> gregkh@linuxfoundation.org> 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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel