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

[Jolly Shah]

Hi Greg,

> ------Original Message------
> From: 'Greg Kh' <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent:  Friday, February 14, 2020 4:52PM
> 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 Fri, Feb 14, 2020 at 04:37:16PM -0800, Jolly Shah wrote:
> > > 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.
>
> Just because one random tiny thing does it the wrong way does not mean
> to focus on that design pattern and ignore the thousands of other
> apis/interfaces in the kernel that do not do it that way :)
>
> > > 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.
>
> You do not misunderstand.

Submitted v3 with required changes. Please review.

Thanks,
Jolly Shah

> thanks,
>
> greg k-h