lkml.org 
[lkml]   [2018]   [May]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v2 6/7] platform/mellanox: Introduce support for Mellanox register access driver
Date


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

\
 
 \ /
  Last update: 2018-05-26 13:15    [W:0.213 / U:0.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site