lkml.org 
[lkml]   [2020]   [Feb]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
From
Date
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
>

\
 
 \ /
  Last update: 2020-02-15 01:37    [W:0.100 / U:8.208 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site