Messages in this thread | | | Subject | Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface | From | Jolly Shah <> | Date | Fri, 14 Feb 2020 16:37:16 -0800 |
| |
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 >
| |