| 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
| ||
|
Message-ID: <YGNdoYq5lyERVGLO@hatter.bewilderbeest.net>
Date: Tue, 30 Mar 2021 12:19:29 -0500
From: Zev Weiss <zev@...ilderbeest.net>
To: Mark Brown <broonie@...nel.org>
Cc: Guenter Roeck <linux@...ck-us.net>,
Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org,
Andrew Jeffery <andrew@...id.au>,
Liam Girdwood <lgirdwood@...il.com>,
linux-kernel@...r.kernel.org, openbmc@...ts.ozlabs.org
Subject: Re: Enabling pmbus power control
On Tue, Mar 30, 2021 at 06:22:54AM CDT, Mark Brown wrote:
>On Tue, Mar 30, 2021 at 03:34:16AM -0700, Guenter Roeck wrote:
>
>> (and I don't know if the userspace consumer code is appropriate - you
>> might want to check with the regulator maintainer on that).
>
>It's not, you should never see this in a production system.
>
Sorry, can you clarify what exactly "this" refers to here?
>> > first attempt at this ran into problems with all the
>> > reg-userspace-consumer instances getting attached to the first
>> > regulator device, I think due to all of the regulators ending up under
>> > the same name in the global namespace of regulator_map_list. I worked
>> > around that by adding an ID counter to produce a unique name for each,
>> > though that changes device names in userspace-visible ways that I'm
>> > not sure would be considered OK for backwards compatibility. (I'm not
>> > familiar enough with the regulator code to know if there's a better
>> > way of fixing that problem.) The #if-ing to keep it behind a Kconfig
>
>> Maybe ask that question on the regulator mailing list.
>
>I can't really tell what the issue is here without more context, the
>global name list should not be relevant for much in a system that's well
>configured so it sounds like it's user error.
My initial attempt I guess followed the existing ltc2978 code a little
too closely and I ended up with all my lm25066 regulators registered
under the same (static) name, so when I went to attach the
reg-userspace-consumer instances to them by way of that name I got this:
# ls -l /sys/kernel/debug/regulator/7-004?-vout0
/sys/kernel/debug/regulator/7-0040-vout0:
-r--r--r-- 1 root root 0 Jan 1 1970 bypass_count
-r--r--r-- 1 root root 0 Jan 1 1970 open_count
drwxr-xr-x 2 root root 0 Jan 1 1970 reg-userspace-consumer.0.auto-vout0
drwxr-xr-x 2 root root 0 Jan 1 1970 reg-userspace-consumer.1.auto-vout0
drwxr-xr-x 2 root root 0 Jan 1 1970 reg-userspace-consumer.2.auto-vout0
-r--r--r-- 1 root root 0 Jan 1 1970 use_count
/sys/kernel/debug/regulator/7-0041-vout0:
-r--r--r-- 1 root root 0 Jan 1 1970 bypass_count
-r--r--r-- 1 root root 0 Jan 1 1970 open_count
-r--r--r-- 1 root root 0 Jan 1 1970 use_count
/sys/kernel/debug/regulator/7-0043-vout0:
-r--r--r-- 1 root root 0 Jan 1 1970 bypass_count
-r--r--r-- 1 root root 0 Jan 1 1970 open_count
-r--r--r-- 1 root root 0 Jan 1 1970 use_count
(When of course the intent is to have one reg-userspace-consumer for
each regulator.)
I now have a revised version that takes Guenter's comments into account
and puts this logic in the lm25066 driver instead of the pmbus core
though, and in the process arrived at what I expect is a better solution
to the name-collision problem at least (below).
Thanks,
Zev
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 32d2fc850621..d9905e95d510 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -120,6 +120,21 @@ config SENSORS_LM25066
This driver can also be built as a module. If so, the module will
be called lm25066.
+config SENSORS_LM25066_REGULATOR
+ bool "Regulator support for LM25066 and compatibles"
+ depends on SENSORS_LM25066 && REGULATOR
+ help
+ If you say yes here you get regulator support for National
+ Semiconductor LM25056, LM25066, LM5064, and LM5066.
+
+config SENSORS_LM25066_USERSPACE_REGULATOR_CONSUMER
+ bool "Userspace regulator consumer support for PMBus devices"
+ depends on SENSORS_LM25066_REGULATOR && REGULATOR_USERSPACE_CONSUMER
+ help
+ If you say yes here, regulator-enabled PMBus devices such as
+ the LM25066 and LTC2978 will have their on/off state
+ controllable from userspace via sysfs.
+
config SENSORS_LTC2978
tristate "Linear Technologies LTC2978 and compatibles"
help
diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
index e9a66fd9e144..4e7e66d1ca8c 100644
--- a/drivers/hwmon/pmbus/lm25066.c
+++ b/drivers/hwmon/pmbus/lm25066.c
@@ -14,6 +14,9 @@
#include <linux/slab.h>
#include <linux/i2c.h>
#include <linux/log2.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/userspace-consumer.h>
+#include <linux/platform_device.h>
#include "pmbus.h"
enum chips { lm25056, lm25066, lm5064, lm5066, lm5066i };
@@ -207,6 +210,13 @@ struct lm25066_data {
int id;
u16 rlimit; /* Maximum register value */
struct pmbus_driver_info info;
+
+#if IS_ENABLED(CONFIG_SENSORS_LM25066_USERSPACE_REGULATOR_CONSUMER)
+ struct regulator_desc reg_desc;
+ struct regulator_bulk_data reg_supply;
+ struct regulator_userspace_consumer_data consumerdata;
+ struct platform_device platdev;
+#endif
};
#define to_lm25066_data(x) container_of(x, struct lm25066_data, info)
@@ -413,9 +423,15 @@ static int lm25066_write_word_data(struct i2c_client *client, int page, int reg,
return ret;
}
+#if IS_ENABLED(CONFIG_SENSORS_LM25066_REGULATOR)
+static const struct regulator_desc lm25066_reg_desc[] = {
+ PMBUS_REGULATOR("vout", 0),
+};
+#endif
+
static int lm25066_probe(struct i2c_client *client)
{
- int config;
+ int config, status;
struct lm25066_data *data;
struct pmbus_driver_info *info;
struct __coeff *coeff;
@@ -483,7 +499,46 @@ static int lm25066_probe(struct i2c_client *client)
info->b[PSC_POWER] = coeff[PSC_POWER].b;
}
- return pmbus_do_probe(client, info);
+#if IS_ENABLED(CONFIG_SENSORS_LM25066_REGULATOR)
+ info->num_regulators = 1;
+
+#if IS_ENABLED(CONFIG_SENSORS_LM25066_USERSPACE_REGULATOR_CONSUMER)
+ data->reg_desc = lm25066_reg_desc[0];
+ data->reg_desc.name = devm_kasprintf(&client->dev, GFP_KERNEL, "%s.%s",
+ lm25066_reg_desc[0].name,
+ dev_name(&client->dev));
+ if (!data->reg_desc.name)
+ return -ENOMEM;
+
+ info->reg_desc = &data->reg_desc;
+ info->reg_valid_ops = REGULATOR_CHANGE_STATUS;
+#else
+ info->reg_desc = &lm25066_reg_desc[0];
+#endif
+#endif
+
+ status = pmbus_do_probe(client, info);
+ if (status)
+ return status;
+
+#if IS_ENABLED(CONFIG_SENSORS_LM25066_USERSPACE_REGULATOR_CONSUMER)
+ data->reg_supply.supply = data->reg_desc.name;
+ data->consumerdata = (struct regulator_userspace_consumer_data) {
+ .name = data->reg_desc.name,
+ .num_supplies = 1,
+ .supplies = &data->reg_supply,
+ .init_on = true,
+ };
+ data->platdev = (struct platform_device) {
+ .name = "reg-userspace-consumer",
+ .id = PLATFORM_DEVID_AUTO,
+ .dev = { .platform_data = &data->consumerdata, },
+ };
+
+ status = platform_device_register(&data->platdev);
+#endif
+
+ return status;
}
static const struct i2c_device_id lm25066_id[] = {
diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index 4c30ec89f5bf..153220e2c363 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -451,6 +451,7 @@ struct pmbus_driver_info {
/* Regulator functionality, if supported by this chip driver. */
int num_regulators;
const struct regulator_desc *reg_desc;
+ unsigned int reg_valid_ops;
/* custom attributes */
const struct attribute_group **groups;
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index aadea85fe630..805e3a8d8bd9 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2314,6 +2314,8 @@ static int pmbus_regulator_register(struct pmbus_data *data)
info->reg_desc[i].name);
return PTR_ERR(rdev);
}
+
+ rdev->constraints->valid_ops_mask |= info->reg_valid_ops;
}
return 0;
Powered by blists - more mailing lists