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