Re: [PATCH v2 6/7] platform/mellanox: Introduce support for Mellanox register access driver

[Darren Hart <dvhart@infradead.org>]

On Sat, May 26, 2018 at 11:15:35AM +0000, Vadim Pasternak wrote:
> 
> 
> > -----Original Message-----
> > From: Darren Hart [mailto:dvhart@infradead.org]
> > Sent: Friday, May 25, 2018 3:31 AM
> > To: Vadim Pasternak <vadimp@mellanox.com>
> > Cc: andy.shevchenko@gmail.com; gregkh@linuxfoundation.org; linux-
> > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; jiri@resnulli.us;
> > Michael Shych <michaelsh@mellanox.com>; ivecera@redhat.com
> > Subject: Re: [PATCH v2 6/7] platform/mellanox: Introduce support for Mellanox
> > register access driver
> > 
> > On Mon, May 07, 2018 at 06:48:54AM +0000, Vadim Pasternak wrote:
> > > Introduce new Mellanox platform driver to allow access to Mellanox
> > > programmable device register space trough sysfs interface.
> > > The driver purpose is to provide sysfs interface for user space for
> > > the registers essential for system control and monitoring.
> > > The sets of registers for sysfs access are supposed to be defined per
> > > system type bases and include the registers related to system resets
> > > operation, system reset causes monitoring and some kinds of mux selection.
> > >
> > > Signed-off-by: Vadim Pasternak <vadimp@mellanox.com>
> > > ---
> > 
> > One question on the attr init which I'm not familiar with... Andy, Greg - can you
> > offer your opinion below...
> > 
...
> > > +		priv->mlxreg_io_dev_attr[i].dev_attr.attr.mode =
> > > +						priv->pdata->data[i].mode;
> > > +		switch (priv->pdata->data[i].mode) {
> > 
> > This seemed a bit odd to me. Do we need to do this conditional assignment
> > within the kernel, or can these just be assigned, and the mode will guard against
> > the user being able to call store on a read only attr?
> > 
> > > +		case 0200:
> > > +			priv->mlxreg_io_dev_attr[i].dev_attr.store =
> > > +							mlxreg_io_attr_store;
> > > +			break;
> > > +
> > > +		case 0444:
> > > +			priv->mlxreg_io_dev_attr[i].dev_attr.show =
> > > +							mlxreg_io_attr_show;
> > > +			break;
> > > +
> > > +		case 0644:
> > > +			priv->mlxreg_io_dev_attr[i].dev_attr.show =
> > > +							mlxreg_io_attr_show;
> > > +			priv->mlxreg_io_dev_attr[i].dev_attr.store =
> > > +							mlxreg_io_attr_store;
> > > +			break;
> > 
> > If this is necessary, we can simplify this by checking for the read mask and the
> > write mask and setting each once - rather than duplicating this for r, w, and rw.
> > As it is a 0400 would not assign the show function, even though it is readable by
> > somebody.
> 
> Maybe I really can add something like
> static struct device_attribute mlxreg_io_devattr_rw = {
> 	.show	= mlxreg_io_attr_show,
> 	.store	= mlxreg_io_attr_store,
> };
> 
> And replace this whole switch statement just with:
> 		memcpy(&priv->mlxreg_io_dev_attr[i].dev_attr,
> 		       &mlxreg_io_devattr_rw, sizeof(struct device_attribute));

This is certainly preferable if it doesn't present any functional problems.
Seems to me it must be doable because the OS has to deny write for Group and
Other and allow for User with 644, similarly for read is other perm conditions.

-- 
Darren Hart
VMware Open Source Technology Center