| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 9 Aug 2017 18:15:24 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Eddie James <eajames@...ux.vnet.ibm.com>
Cc: jdelvare@...e.com, linux-hwmon@...r.kernel.org,
linux-kernel@...r.kernel.org, joel@....id.au, jk@...abs.org,
andrew@...id.au, cbostic@...ux.vnet.ibm.com,
"Edward A. James" <eajames@...ibm.com>
Subject: Re: [PATCH 4/4] drivers/hwmon/pmbus: Add debugfs for status registers
On Wed, Aug 09, 2017 at 05:19:17PM -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@...ibm.com>
>
> Export all the available status registers through debugfs, if the client
> driver wants them.
>
> Signed-off-by: Edward A. James <eajames@...ibm.com>
> ---
> drivers/hwmon/pmbus/pmbus.c | 24 +++++-
> drivers/hwmon/pmbus/pmbus.h | 11 +++
> drivers/hwmon/pmbus/pmbus_core.c | 175 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 209 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/pmbus/pmbus.c b/drivers/hwmon/pmbus/pmbus.c
> index 7718e58..fc27417 100644
> --- a/drivers/hwmon/pmbus/pmbus.c
> +++ b/drivers/hwmon/pmbus/pmbus.c
That won't work: SENSORS_PMBUS is independent of PMBUS and does not have
to be enabled even if PMBUS is enabled. This will have to be in pmbus_core.c.
I would suggest to do the same as done with other drivers. Move
pmbus_debugfs_dir into pmbus_core.c and create the directory on probe
if it has not already been created.
> @@ -18,6 +18,7 @@
> * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> */
>
> +#include <linux/debugfs.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/init.h>
> @@ -230,7 +231,28 @@ static int pmbus_probe(struct i2c_client *client,
> .id_table = pmbus_id,
> };
>
> -module_i2c_driver(pmbus_driver);
> +struct dentry *pmbus_debugfs_dir;
> +EXPORT_SYMBOL_GPL(pmbus_debugfs_dir);
> +
> +static int __init pmbus_init(void)
> +{
> + pmbus_debugfs_dir = debugfs_create_dir(pmbus_driver.driver.name, NULL);
> + if (IS_ERR(pmbus_debugfs_dir))
> + /* Don't fail out if we don't have debugfs support. */
> + pmbus_debugfs_dir = NULL;
> +
> + return i2c_add_driver(&pmbus_driver);
> +}
> +
> +static void __exit pmbus_exit(void)
> +{
> + debugfs_remove_recursive(pmbus_debugfs_dir);
> +
> + i2c_del_driver(&pmbus_driver);
> +}
> +
> +module_init(pmbus_init);
> +module_exit(pmbus_exit);
>
> MODULE_AUTHOR("Guenter Roeck");
> MODULE_DESCRIPTION("Generic PMBus driver");
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index bfcb13b..c772b83 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -25,6 +25,8 @@
> #include <linux/bitops.h>
> #include <linux/regulator/driver.h>
>
> +struct dentry;
> +
> /*
> * Registers
> */
> @@ -383,6 +385,12 @@ struct pmbus_driver_info {
> /* Regulator functionality, if supported by this chip driver. */
> int num_regulators;
> const struct regulator_desc *reg_desc;
> +
> + /*
> + * Controls whether or not to create debugfs entries for this driver's
> + * devices.
> + */
> + bool debugfs;
> };
>
> /* Regulator ops */
> @@ -401,6 +409,9 @@ struct pmbus_driver_info {
> .owner = THIS_MODULE, \
> }
>
> +/* Handle for debugfs directory */
> +extern struct dentry *pmbus_debugfs_dir;
> +
> /* Function declarations */
>
> void pmbus_clear_cache(struct i2c_client *client);
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 1a51f8f..afbd364 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -19,6 +19,7 @@
> * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> */
>
> +#include <linux/debugfs.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/init.h>
> @@ -49,6 +50,9 @@
> #define PB_STATUS_FAN34_BASE (PB_STATUS_FAN_BASE + PMBUS_PAGES)
> #define PB_STATUS_TEMP_BASE (PB_STATUS_FAN34_BASE + PMBUS_PAGES)
> #define PB_STATUS_INPUT_BASE (PB_STATUS_TEMP_BASE + PMBUS_PAGES)
> +#define PB_STATUS_CML_BASE (PB_STATUS_INPUT_BASE + PMBUS_PAGES)
> +#define PB_STATUS_OTHER_BASE (PB_STATUS_CML_BASE + PMBUS_PAGES)
> +#define PB_STATUS_MFR_BASE (PB_STATUS_OTHER_BASE + PMBUS_PAGES)
We are not actively using those status registers, are we ?
I am a bit concerned that we'll continuously read those status registers
but don't do anything with it most of the time. Ultimately I'd prefer
to get rid of all caching, not more, since it gets quite expensive
on chips with many pages.
Maybe just display uncached status register values in debugfs ?
> #define PB_STATUS_VMON_BASE (PB_STATUS_INPUT_BASE + 1)
>
> #define PB_NUM_STATUS_REG (PB_STATUS_VMON_BASE + 1)
> @@ -101,6 +105,7 @@ struct pmbus_data {
> int num_attributes;
> struct attribute_group group;
> const struct attribute_group *groups[2];
> + struct dentry *debugfs; /* debugfs device directory */
>
> struct pmbus_sensor *sensors;
>
> @@ -119,6 +124,11 @@ struct pmbus_data {
> u8 currpage;
> };
>
> +struct pmbus_debugfs_entry {
> + struct device *dev;
> + int index;
> +};
> +
> void pmbus_clear_cache(struct i2c_client *client)
> {
> struct pmbus_data *data = i2c_get_clientdata(client);
> @@ -422,6 +432,19 @@ static struct pmbus_data *pmbus_update_device(struct device *dev)
> = _pmbus_read_byte_data(client, i,
> s->reg);
> }
> +
> + if (!data->debugfs)
> + continue;
> +
> + data->status[PB_STATUS_CML_BASE + i]
> + = _pmbus_read_byte_data(client, i,
> + PMBUS_STATUS_CML);
> + data->status[PB_STATUS_OTHER_BASE + i]
> + = _pmbus_read_byte_data(client, i,
> + PMBUS_STATUS_OTHER);
> + data->status[PB_STATUS_MFR_BASE + i]
> + = _pmbus_read_byte_data(client, i,
> + PMBUS_STATUS_MFR_SPECIFIC);
> }
>
> if (info->func[0] & PMBUS_HAVE_STATUS_INPUT)
> @@ -1891,6 +1914,151 @@ static int pmbus_regulator_register(struct pmbus_data *data)
> }
> #endif
>
> +static int pmbus_debugfs_get(void *data, u64 *val)
> +{
> + struct pmbus_debugfs_entry *entry = data;
> + struct pmbus_data *pdata = pmbus_update_device(entry->dev);
> +
> + *val = pdata->status[entry->index];
> +
> + return 0;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops8, pmbus_debugfs_get, NULL,
> + "0x%02llx\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops16, pmbus_debugfs_get, NULL,
> + "0x%04llx\n");
> +
> +static int pmbus_init_debugfs(struct i2c_client *client,
> + struct pmbus_data *data)
> +{
> + int i, idx = 0;
> + struct dentry *dbg;
> + char name[PMBUS_NAME_SIZE];
> + struct pmbus_debugfs_entry *entries;
> +
> + /*
> + * Either user didn't request debugfs or debugfs is not enabled in
> + * kernel. Exit but don't throw an error in these cases.
> + */
Here might be the place to initialize pmbus_debugfs_dir
if it is not yet initialized.
> + if (!data->info->debugfs || !pmbus_debugfs_dir)
> + return 0;
> +
> + /*
> + * Create the debugfs directory for this device. Use the hwmon device
> + * name to avoid conflicts (hwmon numbers are globally unique).
> + */
> + data->debugfs = debugfs_create_dir(dev_name(data->hwmon_dev),
> + pmbus_debugfs_dir);
> + if (IS_ERR_OR_NULL(data->debugfs)) {
> + data->debugfs = NULL;
> + return -ENODEV;
> + }
> +
> + /* Allocate the max possible entries we need. */
> + entries = devm_kzalloc(data->dev,
> + sizeof(*entries) * (data->info->pages * 10),
> + GFP_KERNEL);
> + if (!entries)
> + return -ENOMEM;
> +
> + for (i = 0; i < data->info->pages; ++i) {
> + /* check accessibility of status register if it's not page 0 */
> + if (!i || pmbus_check_status_register(client, i)) {
> + entries[idx].dev = data->hwmon_dev;
> + entries[idx].index = PB_STATUS_BASE + i;
> + snprintf(name, PMBUS_NAME_SIZE, "status%d", i);
> + dbg = debugfs_create_file(name, 0444, data->debugfs,
> + &entries[idx++],
> + &pmbus_debugfs_ops16);
What is the point of the dbg variable ?
> + }
> +
> + if (data->info->func[i] & PMBUS_HAVE_STATUS_VOUT) {
> + entries[idx].dev = data->hwmon_dev;
> + entries[idx].index = PB_STATUS_VOUT_BASE + i;
> + snprintf(name, PMBUS_NAME_SIZE, "status%d_vout", i);
> + dbg = debugfs_create_file(name, 0444, data->debugfs,
> + &entries[idx++],
> + &pmbus_debugfs_ops8);
> + }
> +
> + if (data->info->func[i] & PMBUS_HAVE_STATUS_IOUT) {
> + entries[idx].dev = data->hwmon_dev;
> + entries[idx].index = PB_STATUS_IOUT_BASE + i;
> + snprintf(name, PMBUS_NAME_SIZE, "status%d_iout", i);
> + dbg = debugfs_create_file(name, 0444, data->debugfs,
> + &entries[idx++],
> + &pmbus_debugfs_ops8);
> + }
> +
> + if (data->info->func[i] & PMBUS_HAVE_STATUS_INPUT) {
> + entries[idx].dev = data->hwmon_dev;
> + entries[idx].index = PB_STATUS_INPUT_BASE + i;
> + snprintf(name, PMBUS_NAME_SIZE, "status%d_input", i);
> + dbg = debugfs_create_file(name, 0444, data->debugfs,
> + &entries[idx++],
> + &pmbus_debugfs_ops8);
> + }
> +
> + if (data->info->func[i] & PMBUS_HAVE_STATUS_TEMP) {
> + entries[idx].dev = data->hwmon_dev;
> + entries[idx].index = PB_STATUS_TEMP_BASE + i;
> + snprintf(name, PMBUS_NAME_SIZE, "status%d_temp", i);
> + dbg = debugfs_create_file(name, 0444, data->debugfs,
> + &entries[idx++],
> + &pmbus_debugfs_ops8);
> + }
> +
> + if (pmbus_check_byte_register(client, i, PMBUS_STATUS_CML)) {
> + entries[idx].dev = data->hwmon_dev;
> + entries[idx].index = PB_STATUS_CML_BASE + i;
> + snprintf(name, PMBUS_NAME_SIZE, "status%d_cml", i);
> + dbg = debugfs_create_file(name, 0444, data->debugfs,
> + &entries[idx++],
> + &pmbus_debugfs_ops8);
> + }
> +
> + if (pmbus_check_byte_register(client, i, PMBUS_STATUS_OTHER)) {
> + entries[idx].dev = data->hwmon_dev;
> + entries[idx].index = PB_STATUS_OTHER_BASE + i;
> + snprintf(name, PMBUS_NAME_SIZE, "status%d_other", i);
> + dbg = debugfs_create_file(name, 0444, data->debugfs,
> + &entries[idx++],
> + &pmbus_debugfs_ops8);
> + }
> +
> + if (pmbus_check_byte_register(client, i,
> + PMBUS_STATUS_MFR_SPECIFIC)) {
> + entries[idx].dev = data->hwmon_dev;
> + entries[idx].index = PB_STATUS_MFR_BASE + i;
> + snprintf(name, PMBUS_NAME_SIZE, "status%d_mfr", i);
> + dbg = debugfs_create_file(name, 0444, data->debugfs,
> + &entries[idx++],
> + &pmbus_debugfs_ops8);
> + }
> +
> + if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN12) {
> + entries[idx].dev = data->hwmon_dev;
> + entries[idx].index = PB_STATUS_FAN_BASE + i;
> + snprintf(name, PMBUS_NAME_SIZE, "status%d_fan12", i);
> + dbg = debugfs_create_file(name, 0444, data->debugfs,
> + &entries[idx++],
> + &pmbus_debugfs_ops8);
> + }
> +
> + if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN34) {
> + entries[idx].dev = data->hwmon_dev;
> + entries[idx].index = PB_STATUS_FAN34_BASE + i;
> + snprintf(name, PMBUS_NAME_SIZE, "status%d_fan34", i);
> + dbg = debugfs_create_file(name, 0444, data->debugfs,
> + &entries[idx++],
> + &pmbus_debugfs_ops8);
> + }
> + }
> +
> + return 0;
> +}
> +
Typically debugfs code is conditional. What happens if DEBUGFS
is not enabled ? See below.
> int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
> struct pmbus_driver_info *info)
> {
> @@ -1950,6 +2118,10 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
> if (ret)
> goto out_unregister;
>
> + ret = pmbus_init_debugfs(client, data);
> + if (ret)
> + dev_warn(dev, "Failed to register debugfs\n");
I think this warning will be generated if debugfs is disabled.
We should not do that. Consider putting the debugfs code into
#fidef and have a dummy pmbus_init_debugfs() returning 0 if
it is disabled.
> +
> return 0;
>
> out_unregister:
> @@ -1963,6 +2135,9 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
> int pmbus_do_remove(struct i2c_client *client)
> {
> struct pmbus_data *data = i2c_get_clientdata(client);
> +
> + debugfs_remove_recursive(data->debugfs);
> +
> hwmon_device_unregister(data->hwmon_dev);
> kfree(data->group.attrs);
> return 0;
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists