mbox series

[0/5] firmware: xilinx: Add xilinx specific sysfs interface

Message ID 1575502159-11327-1-git-send-email-jolly.shah@xilinx.com (mailing list archive)
Headers show
Series firmware: xilinx: Add xilinx specific sysfs interface | expand

Message

Jolly Shah Dec. 4, 2019, 11:29 p.m. UTC
This patch series adds xilinx specific sysfs interface for below
purposes:
- Register access
- Set shutdown scope
- Set boot health status bit

Rajan Vaja (5):
  firmware: xilinx: Adds new eemi call for reg access
  firmware: xilinx: Add sysfs interface
  firmware: xilinx: Add system shutdown API interface
  firmware: xilinx: Add sysfs to set shutdown scope
  firmware: xilinx: Add sysfs and IOCTL to set boot health status

 Documentation/ABI/stable/sysfs-firmware-zynqmp | 103 +++++++
 drivers/firmware/xilinx/Makefile               |   2 +-
 drivers/firmware/xilinx/zynqmp-ggs.c           | 289 ++++++++++++++++++
 drivers/firmware/xilinx/zynqmp.c               | 392 +++++++++++++++++++++++++
 include/linux/firmware/xlnx-zynqmp.h           |  37 ++-
 5 files changed, 821 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/ABI/stable/sysfs-firmware-zynqmp
 create mode 100644 drivers/firmware/xilinx/zynqmp-ggs.c

Comments

Sudeep Holla Dec. 18, 2019, 2:45 p.m. UTC | #1
On Wed, Dec 04, 2019 at 03:29:14PM -0800, Jolly Shah wrote:
> This patch series adds xilinx specific sysfs interface for below
> purposes:
> - Register access
> - Set shutdown scope
> - Set boot health status bit

This series defeats the whole abstraction EEMI provides. By providing
direct register accesses, you are allowing user-space to do whatever it
wants. I had NACKed this idea before. Has anything changed ?

If you need it for testing firmware, better put them in debugfs which is
off on production builds.

--
Regards,
Sudeep
Jolly Shah Jan. 2, 2020, 9:01 p.m. UTC | #2
Hi Sudeep,

Thanks for the review.

> -----Original Message-----
> From: Sudeep Holla <sudeep.holla@arm.com>
> Sent: Wednesday, December 18, 2019 6:46 AM
> To: Jolly Shah <JOLLYS@xilinx.com>
> Cc: ard.biesheuvel@linaro.org; mingo@kernel.org;
> gregkh@linuxfoundation.org; matt@codeblueprint.co.uk;
> 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; Sudeep Holla <sudeep.holla@arm.com>
> Subject: Re: [PATCH 0/5] firmware: xilinx: Add xilinx specific sysfs interface
> 
> On Wed, Dec 04, 2019 at 03:29:14PM -0800, Jolly Shah wrote:
> > This patch series adds xilinx specific sysfs interface for below
> > purposes:
> > - Register access
> > - Set shutdown scope
> > - Set boot health status bit
> 
> This series defeats the whole abstraction EEMI provides. By providing
> direct register accesses, you are allowing user-space to do whatever it
> wants. I had NACKed this idea before. Has anything changed ?
>

Firmware checks for allowed accesses only and rejects rest. 
 
> If you need it for testing firmware, better put them in debugfs which is
> off on production builds.

Sure. Will reanalyze use cases and move to debugfs only if that suffices.

Thanks,
Jolly Shah


> 
> --
> Regards,
> Sudeep
Sudeep Holla Jan. 3, 2020, 11:31 a.m. UTC | #3
On Thu, Jan 02, 2020 at 09:01:58PM +0000, Jolly Shah wrote:
> Hi Sudeep,
>
> Thanks for the review.
>
> > -----Original Message-----
> > From: Sudeep Holla <sudeep.holla@arm.com>
> > Sent: Wednesday, December 18, 2019 6:46 AM
> > To: Jolly Shah <JOLLYS@xilinx.com>
> > Cc: ard.biesheuvel@linaro.org; mingo@kernel.org;
> > gregkh@linuxfoundation.org; matt@codeblueprint.co.uk;
> > 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; Sudeep Holla <sudeep.holla@arm.com>
> > Subject: Re: [PATCH 0/5] firmware: xilinx: Add xilinx specific sysfs interface
> >
> > On Wed, Dec 04, 2019 at 03:29:14PM -0800, Jolly Shah wrote:
> > > This patch series adds xilinx specific sysfs interface for below
> > > purposes:
> > > - Register access
> > > - Set shutdown scope
> > > - Set boot health status bit
> >
> > This series defeats the whole abstraction EEMI provides. By providing
> > direct register accesses, you are allowing user-space to do whatever it
> > wants. I had NACKed this idea before. Has anything changed ?
> >
>
> Firmware checks for allowed accesses only and rejects rest.
>

If that is always the case, why not abstract them and remove this direct
register access completely. It must go or we must remove EEMI abstraction
and just provide direct register access to the entire space. I really
don't like this mix-n-match approach here.

> > If you need it for testing firmware, better put them in debugfs which is
> > off on production builds.
>
> Sure. Will reanalyze use cases and move to debugfs only if that suffices.
>

Thanks.

--
Regards,
Sudeep