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

From: Sudeep Holla <sudeep.holla@arm.com>
To: Greg KH <gregkh@linuxfoundation.org>, Jolly Shah <JOLLYS@xilinx.com>
Cc: "ard.biesheuvel@linaro.org" <ard.biesheuvel@linaro.org>,
	"mingo@kernel.org" <mingo@kernel.org>,
	"matt@codeblueprint.co.uk" <matt@codeblueprint.co.uk>,
	"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>, Rajan Vaja <RAJANV@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
Date: Fri, 24 Jan 2020 11:32:39 +0000	[thread overview]
Message-ID: <20200124113239.GB40307@bogus> (raw)
In-Reply-To: <20200124060339.GB2906795@kroah.com>

Hi Greg,

Thanks for raising various issues that I have repeatedly asked and
constantly ignored.

On Fri, Jan 24, 2020 at 07:03:39AM +0100, Greg KH wrote:
> On Thu, Jan 23, 2020 at 11:47:57PM +0000, Jolly Shah wrote:
> > Hi Greg,
> >

[...]

> > Firmware driver was changed later to be platform driver but these
> > interfaces were defined earlier and are in use.
>
> Defined and in use where?  Not in the upstream kernel tree, right?  Or
> am I missing them somewhere else?
>

Exactly and they keep saying there partners are using these for 3-4 years
and they want to retain that. I have told them to change several times.

> > > > +	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.
>

Oh yes, this is so broken in design. This firmware is designed to abstract
the power and configuration management on their platform, but they decided
to add direct register access to some registers anyway. Weird use case,
don't even ask. But I strongly objected such interface in sysfs even if
they moved under platform device. If they need this at any cost, I have
suggested debugfs.

[...]

> >
> > Agree it will be simpler but to as firmware driver was changed to be
> > platform driver, to keep paths same, we used sysfs.
>
> Keep them same from what?  Use the platform device as that is what it is
> there for, do not go create new apis when they are not needed at all.
>

+1, and please don't add any sysfs that can read/write those GGS registers
directly from userspace. Move them to debugfs if you are desperate to have
something.

--
Regards,
Sudeep