> -----Original Message-----
> From: Darren Hart [mailto:dvh...@infradead.org]
> Sent: Friday, May 25, 2018 3:31 AM
> To: Vadim Pasternak <vad...@mellanox.com>
> Cc: andy.shevche...@gmail.com; gre...@linuxfoundation.org; linux-
> ker...@vger.kernel.org; platform-driver-...@vger.kernel.org; j...@resnulli.us;
> Michael Shych <michae...@mellanox.com>; ivec...@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 <vad...@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
