| Message ID | 1525675735-125547-4-git-send-email-vadimp@mellanox.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Darren Hart |
| Headers | show |
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.
> -----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
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.
diff --git a/drivers/platform/mellanox/Kconfig b/drivers/platform/mellanox/Kconfig index 591bccd..ddfae9fc 100644 --- a/drivers/platform/mellanox/Kconfig +++ b/drivers/platform/mellanox/Kconfig @@ -23,4 +23,15 @@ config MLXREG_HOTPLUG This driver handles hot-plug events for the power suppliers, power cables and fans on the wide range Mellanox IB and Ethernet systems. +config MLXREG_IO + tristate "Mellanox platform register access driver support" + depends on REGMAP + depends on HWMON + help + This driver allows access to Mellanox programmable device register + space trough sysfs interface. The sets of registers for sysfs access + are defined per system type bases and includes the registers related + to system resets operation, system reset causes monitoring and some + kinds of mux selection. + endif # MELLANOX_PLATFORM diff --git a/drivers/platform/mellanox/Makefile b/drivers/platform/mellanox/Makefile index 7c8385e..57074d9c 100644 --- a/drivers/platform/mellanox/Makefile +++ b/drivers/platform/mellanox/Makefile @@ -4,3 +4,4 @@ # Mellanox Platform-Specific Drivers # obj-$(CONFIG_MLXREG_HOTPLUG) += mlxreg-hotplug.o +obj-$(CONFIG_MLXREG_IO) += mlxreg-io.o diff --git a/drivers/platform/mellanox/mlxreg-io.c b/drivers/platform/mellanox/mlxreg-io.c new file mode 100644 index 0000000..f573b65 --- /dev/null +++ b/drivers/platform/mellanox/mlxreg-io.c @@ -0,0 +1,221 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Mellanox register access driver + * + * Copyright (C) 2018 Mellanox Technologies + * Copyright (C) 2018 Vadim Pasternak <vadimp@mellanox.com> + */ + +#include <linux/bitops.h> +#include <linux/device.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/platform_data/mlxreg.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +/* Attribute parameters. */ +#define MLXREG_IO_ATT_SIZE 10 +#define MLXREG_IO_ATT_NUM 48 + +/** + * struct mlxreg_io_priv_data - driver's private data: + * + * @pdev: platform device; + * @pdata: platform data; + * @hwmon: hwmon device; + * @mlxreg_io_attr: sysfs attributes array; + * @mlxreg_io_dev_attr: sysfs sensor device attribute array; + * @group: sysfs attribute group; + * @groups: list of sysfs attribute group for hwmon registration; + */ +struct mlxreg_io_priv_data { + struct platform_device *pdev; + struct mlxreg_core_platform_data *pdata; + struct device *hwmon; + struct attribute *mlxreg_io_attr[MLXREG_IO_ATT_NUM + 1]; + struct sensor_device_attribute mlxreg_io_dev_attr[MLXREG_IO_ATT_NUM]; + struct attribute_group group; + const struct attribute_group *groups[2]; +}; + +static ssize_t +mlxreg_io_attr_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct mlxreg_io_priv_data *priv = dev_get_drvdata(dev); + int index = to_sensor_dev_attr(attr)->index; + struct mlxreg_core_data *data = priv->pdata->data + index; + u32 regval = 0; + int ret; + + ret = regmap_read(priv->pdata->regmap, data->reg, ®val); + if (ret) + goto access_error; + + if (!data->bit) + regval = !!(regval & ~data->mask); + + return sprintf(buf, "%u\n", regval); + +access_error: + return ret; +} + +static ssize_t +mlxreg_io_attr_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t len) +{ + struct mlxreg_io_priv_data *priv = dev_get_drvdata(dev); + int index = to_sensor_dev_attr(attr)->index; + struct mlxreg_core_data *data = priv->pdata->data + index; + u32 val, regval; + int ret; + + ret = kstrtou32(buf, MLXREG_IO_ATT_SIZE, &val); + if (ret) + return ret; + + ret = regmap_read(priv->pdata->regmap, data->reg, ®val); + if (ret) + goto access_error; + + regval &= data->mask; + + val = !!val; + if (val) + regval |= ~data->mask; + else + regval &= data->mask; + + ret = regmap_write(priv->pdata->regmap, data->reg, regval); + if (ret) + goto access_error; + + return len; + +access_error: + dev_err(&priv->pdev->dev, "Bus access error\n"); + return ret; +} + +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) { + 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; + + default: + dev_err(&priv->pdev->dev, "Bad access mode %u for attribute %s.\n", + priv->pdata->data[i].mode, + priv->mlxreg_io_attr[i]->name); + return -EINVAL; + } + + priv->mlxreg_io_dev_attr[i].dev_attr.attr.name = + priv->mlxreg_io_attr[i]->name; + priv->mlxreg_io_dev_attr[i].index = i; + sysfs_attr_init(&priv->mlxreg_io_dev_attr[i].dev_attr.attr); + } + + priv->group.attrs = priv->mlxreg_io_attr; + priv->groups[0] = &priv->group; + priv->groups[1] = NULL; + + return 0; +} + +static int mlxreg_io_probe(struct platform_device *pdev) +{ + struct mlxreg_io_priv_data *priv; + int err; + + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->pdata = dev_get_platdata(&pdev->dev); + if (!priv->pdata) { + dev_err(&pdev->dev, "Failed to get platform data.\n"); + return -EINVAL; + } + + priv->pdev = pdev; + + err = mlxreg_io_attr_init(priv); + if (err) { + dev_err(&priv->pdev->dev, "Failed to allocate attributes: %d\n", + err); + return err; + } + + priv->hwmon = devm_hwmon_device_register_with_groups(&pdev->dev, + "mlxreg_io", + priv, + priv->groups); + if (IS_ERR(priv->hwmon)) { + dev_err(&pdev->dev, "Failed to register hwmon device %ld\n", + PTR_ERR(priv->hwmon)); + return PTR_ERR(priv->hwmon); + } + + dev_set_drvdata(&pdev->dev, priv); + + return 0; +} + +static struct platform_driver mlxreg_io_driver = { + .driver = { + .name = "mlxreg-io", + }, + .probe = mlxreg_io_probe, +}; + +module_platform_driver(mlxreg_io_driver); + +MODULE_AUTHOR("Vadim Pasternak <vadimp@mellanox.com>"); +MODULE_DESCRIPTION("Mellanox regmap I/O access driver"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:mlxreg-io");
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> --- v1->v2: Changed added by Vadim: - Change ---help--- to help in Kconfig, according to new requirements; --- drivers/platform/mellanox/Kconfig | 11 ++ drivers/platform/mellanox/Makefile | 1 + drivers/platform/mellanox/mlxreg-io.c | 221 ++++++++++++++++++++++++++++++++++ 3 files changed, 233 insertions(+) create mode 100644 drivers/platform/mellanox/mlxreg-io.c