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

[Vadim Pasternak]

> -----Original Message-----
> From: Darren Hart [mailto:dvhart@xxxxxxxxxxxxx]
> Sent: Friday, May 25, 2018 3:31 AM
> To: Vadim Pasternak <vadimp@xxxxxxxxxxxx>
> Cc: andy.shevchenko@xxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx; jiri@xxxxxxxxxxx;
> Michael Shych <michaelsh@xxxxxxxxxxxx>; ivecera@xxxxxxxxxx
> 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@xxxxxxxxxxxx>
> > ---
> 
> One question on the attr init which I'm not familiar with... Andy, Greg - can you
> offer your opinion below...
> 
> > +static int mlxreg_io_attr_init(struct mlxreg_io_priv_data *priv) {
> > +	int i;
> > +
> > +	priv->group.attrs = devm_kzalloc(&priv->pdev->dev,
> > +					 priv->pdata->counter *
> > +					 sizeof(struct attribute *),
> > +					 GFP_KERNEL);
> > +	if (!priv->group.attrs)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < priv->pdata->counter; i++) {
> > +		priv->mlxreg_io_attr[i] =
> > +				&priv->mlxreg_io_dev_attr[i].dev_attr.attr;
> > +
> > +		/* Set attribute name as a label. */
> > +		priv->mlxreg_io_attr[i]->name =
> > +				devm_kasprintf(&priv->pdev->dev,
> GFP_KERNEL,
> > +					       priv->pdata->data[i].label);
> > +
> > +		if (!priv->mlxreg_io_attr[i]->name) {
> > +			dev_err(&priv->pdev->dev, "Memory allocation failed
> for sysfs attribute %d.\n",
> > +				i + 1);
> > +			return -ENOMEM;
> > +		}
> > +
> > +		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));
> 
> --
> Darren Hart
> VMware Open Source Technology Center