| 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, 14 Apr 2021 02:27:18 +0200
From: Václav Kubernát <kubernat@...net.cz>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Jean Delvare <jdelvare@...e.com>, Jonathan Corbet <corbet@....net>,
linux-hwmon@...r.kernel.org, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] hwmon: Add driver for fsp-3y PSUs and PDUs
út 13. 4. 2021 v 16:59 odesílatel Guenter Roeck <linux@...ck-us.net> napsal:
>
> On 4/13/21 3:42 AM, Václav Kubernát wrote:
> > This patch adds support for these devices:
> > - YH-5151E - the PDU
> > - YM-2151E - the PSU
> >
> > The device datasheet says that the devices support PMBus 1.2, but in my
> > testing, a lot of the commands aren't supported and if they are, they
> > sometimes behave strangely or inconsistently. For example, writes to the
> > PAGE command requires using PEC, otherwise the write won't work and the
> > page won't switch, even though, the standard says that PEC is opiotnal.
>
> optional
Done.
>
> > On the other hand, writes the SMBALERT don't require PEC. Because of
>
> s/writes the/writes to/ ?
Done.
>
> > this, the driver is mostly reverse engineered with the help of a tool
> > called pmbus_peek written by David Brownell (and later adopted by my
> > colleague Jan Kundrát).
> >
> > The device also has some sort of a timing issue when switching pages,
> > which is explained further in the code.
> >
> > Because of this, the driver support is limited. It exposes only the
> > values, that have been tested to work correctly.
> >
>
> You might want to add those details into the driver code, below the
> copyright line. It would be more valuable there than in the commit log.
>
Done.
> > Signed-off-by: Václav Kubernát <kubernat@...net.cz>
> > ---
> > Documentation/hwmon/fsp-3y.rst | 26 ++++
>
> Needs to be added to index.rst.
Done.
>
> > drivers/hwmon/pmbus/Kconfig | 10 ++
> > drivers/hwmon/pmbus/Makefile | 1 +
> > drivers/hwmon/pmbus/fsp-3y.c | 239 +++++++++++++++++++++++++++++++++
> > 4 files changed, 276 insertions(+)
> > create mode 100644 Documentation/hwmon/fsp-3y.rst
> > create mode 100644 drivers/hwmon/pmbus/fsp-3y.c
> >
> > diff --git a/Documentation/hwmon/fsp-3y.rst b/Documentation/hwmon/fsp-3y.rst
> > new file mode 100644
> > index 000000000000..68a547021846
> > --- /dev/null
> > +++ b/Documentation/hwmon/fsp-3y.rst
> > @@ -0,0 +1,26 @@
> > +Kernel driver fsp3y
> > +======================
> > +Supported devices:
> > + * 3Y POWER YH-5151E
> > + * 3Y POWER YM-2151E
> > +
> > +Author: Václav Kubernát <kubernat@...net.cz>
> > +
> > +Description
> > +-----------
> > +This driver implements limited support for two 3Y POWER devices.
> > +
> > +Sysfs entries
> > +-------------
> > +in1_input input voltage
> > +in2_input 12V output voltage
> > +in3_input 5V output voltage
> > +curr1_input input current
> > +curr2_input 12V output current
> > +curr3_input 5V output current
> > +fan1_input fan rpm
> > +temp1_input temperature 1
> > +temp2_input temperature 2
> > +temp3_input temperature 3
> > +power1_input input power
> > +power2_input output power
> > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> > index 03606d4298a4..9d12d446396c 100644
> > --- a/drivers/hwmon/pmbus/Kconfig
> > +++ b/drivers/hwmon/pmbus/Kconfig
> > @@ -56,6 +56,16 @@ config SENSORS_BEL_PFE
> > This driver can also be built as a module. If so, the module will
> > be called bel-pfe.
> >
> > +config SENSORS_FSP_3Y
> > + tristate "FSP/3Y-Power power supplies"
> > + help
> > + If you say yes here you get hardware monitoring support for
> > + FSP/3Y-Power hot-swap power supplies.
> > + Supported models: YH-5151E, YM-2151E
> > +
> > + This driver can also be built as a module. If so, the module will
> > + be called fsp-3y.
> > +
> > config SENSORS_IBM_CFFPS
> > tristate "IBM Common Form Factor Power Supply"
> > depends on LEDS_CLASS
> > diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> > index 6a4ba0fdc1db..bfe218ad898f 100644
> > --- a/drivers/hwmon/pmbus/Makefile
> > +++ b/drivers/hwmon/pmbus/Makefile
> > @@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
> > obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
> > obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
> > obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
> > +obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o
> > obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
> > obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
> > obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
> > diff --git a/drivers/hwmon/pmbus/fsp-3y.c b/drivers/hwmon/pmbus/fsp-3y.c
> > new file mode 100644
> > index 000000000000..2185ab119fd2
> > --- /dev/null
> > +++ b/drivers/hwmon/pmbus/fsp-3y.c
> > @@ -0,0 +1,239 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Hardware monitoring driver for FSP 3Y-Power PSUs
> > + *
> > + * Copyright (c) 2021 Václav Kubernát, CESNET
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include "pmbus.h"
> > +
> > +#define YM2151_PAGE_12V_LOG 0x00
> > +#define YM2151_PAGE_12V_REAL 0x00
> > +#define YM2151_PAGE_5VSB_LOG 0x01
> > +#define YM2151_PAGE_5VSB_REAL 0x20
> > +#define YH5151E_PAGE_12V_LOG 0x00
> > +#define YH5151E_PAGE_12V_REAL 0x00
> > +#define YH5151E_PAGE_5V_LOG 0x01
> > +#define YH5151E_PAGE_5V_REAL 0x10
> > +#define YH5151E_PAGE_3V3_LOG 0x02
> > +#define YH5151E_PAGE_3V3_REAL 0x11
> > +
> > +enum chips {
> > + ym2151e,
> > + yh5151e
> > +};
> > +
> > +struct fsp3y_data {
> > + struct pmbus_driver_info info;
> > + int chip;
> > + int page;
> > +};
> > +
> > +#define to_fsp3y_data(x) container_of(x, struct fsp3y_data, info)
> > +
> > +static int page_log_to_page_real(int page_log, enum chips chip)
> > +{
> > + switch (chip) {
> > + case ym2151e:
> > + switch (page_log) {
> > + case YM2151_PAGE_12V_LOG:
> > + return YM2151_PAGE_12V_REAL;
> > + case YM2151_PAGE_5VSB_LOG:
> > + return YM2151_PAGE_5VSB_REAL;
> > + }
> > + return -EINVAL;
> > + case yh5151e:
> > + switch (page_log) {
> > + case YH5151E_PAGE_12V_LOG:
> > + return YH5151E_PAGE_12V_REAL;
> > + case YH5151E_PAGE_5V_LOG:
> > + return YH5151E_PAGE_5V_LOG;
> > + case YH5151E_PAGE_3V3_LOG:
> > + return YH5151E_PAGE_3V3_REAL;
> > + }
> > + return -EINVAL;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int set_page(struct i2c_client *client, int page_log)
> > +{
> > + const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> > + struct fsp3y_data *data = to_fsp3y_data(info);
> > + int rv;
> > + int page_real;
> > +
> > + if (page_log < 0)
> > + return 0;
> > +
> > + page_real = page_log_to_page_real(page_log, data->chip);
> > + if (page_real < 0)
> > + return page_real;
> > +
> > + if (data->page != page_real) {
> > + rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page_real);
> > + if (rv < 0)
> > + return rv;
> > +
> > + data->page = page_real;
> > +
> > + /* Testing showed that the device has a timing issue. After
>
> Somehow network subsystem multi-line alignments slipped in.
> Not in hwmon, please. I cringe at those; it makes my brain focus on the
> comment (because it is asynchronous) instead of the code.
>
Done.
> > + * setting a page, it takes a while, before the device actually
> > + * gives the correct values from the correct page. 20 ms was
> > + * tested to be enough to not give wrong values (15 ms wasn't
> > + * enough)
> > + */
> > + usleep_range(20000, 30000);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int fsp3y_read_byte_data(struct i2c_client *client, int page, int reg)
> > +{
> > + int rv;
> > +
> > + rv = set_page(client, page);
> > + if (rv < 0)
> > + return rv;
> > +
> > + return i2c_smbus_read_byte_data(client, reg);
> > +}
> > +
> > +static int fsp3y_read_word_data(struct i2c_client *client, int page, int phase, int reg)
> > +{
> > + int rv;
> > +
> > + /* This masks commands which weren't tested to work correctly. Some of the masked commands
> > + * return either 0xFFFF. These would probably get tagged as invalid by pmbus_core. Other
>
> s/either// ?
Done.
>
> > + * ones do return values, which might be useful (that is, they are not 0xFFFF), but their
>
> s/values,/values/
Done.
>
> > + * encoding is unknown, and so they are unsupported.
> > + */
> > + switch (reg) {
> > + case PMBUS_READ_FAN_SPEED_1:
> > + case PMBUS_READ_IIN:
> > + case PMBUS_READ_IOUT:
> > + case PMBUS_READ_PIN:
> > + case PMBUS_READ_POUT:
> > + case PMBUS_READ_TEMPERATURE_1:
> > + case PMBUS_READ_TEMPERATURE_2:
> > + case PMBUS_READ_TEMPERATURE_3:
> > + case PMBUS_READ_VIN:
> > + case PMBUS_READ_VOUT:
> > + case PMBUS_STATUS_WORD:
> > + break;
> > + default:
> > + return -ENXIO;
> > + }
> > +
> > + rv = set_page(client, page);
> > + if (rv < 0)
> > + return rv;
> > +
> > + return i2c_smbus_read_word_data(client, reg);
> > +}
> > +
> > +struct pmbus_driver_info fsp3y_info[] = {
> > + [ym2151e] = {
> > + .pages = 2,
> > + .func[YM2151_PAGE_12V_LOG] =
> > + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> > + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
> > + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
> > + PMBUS_HAVE_VIN | PMBUS_HAVE_IIN |
> > + PMBUS_HAVE_FAN12,
> > + .func[YM2151_PAGE_5VSB_LOG] =
> > + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT,
> > + PMBUS_HAVE_IIN,
> > + .read_word_data = fsp3y_read_word_data,
> > + .read_byte_data = fsp3y_read_byte_data,
> > + },
> > + [yh5151e] = {
> > + .pages = 3,
> > + .func[YH5151E_PAGE_12V_LOG] =
> > + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> > + PMBUS_HAVE_POUT |
> > + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3,
> > + .func[YH5151E_PAGE_5V_LOG] =
> > + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> > + PMBUS_HAVE_POUT,
> > + .func[YH5151E_PAGE_3V3_LOG] =
> > + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> > + PMBUS_HAVE_POUT,
> > + .read_word_data = fsp3y_read_word_data,
> > + .read_byte_data = fsp3y_read_byte_data,
> > + }
> > +};
> > +
> > +static int fsp3y_detect(struct i2c_client *client)
> > +{
> > + int rv;
> > + u8 buf[I2C_SMBUS_BLOCK_MAX];
> > +
> > + rv = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
> > + if (rv < 0)
> > + return rv;
> > +
> > + if (rv == 8 && !strncmp(buf, "YM-2151E", strlen("YM-2151E")))
> > + return ym2151e;
> > + else if (rv == 8 && !strncmp(buf, "YH-5151E", strlen("YH-5151E")))
> > + return yh5151e;
>
> better
> if (rv == 8) {
> /* rest of check */
> }
>
Done.
> > +
> > + dev_err(&client->dev, "Unsupported model %.*s\n", rv, buf);
>
> Sorry I didn't notice before. This assumes that the buffer is zero-terminated,
> which may not be the case. For this to work, add another byte to the buffer
> (u8 buf[I2C_SMBUS_BLOCK_MAX + 1];), and add
> buf[rv] = '\0';
>
> You could actually do that before checking the returned strings and then just
> use strcmp() for the model comparisons, without bothering about the return
> length.
>
Done.
> > + return -ENODEV;
> > +}
> > +
> > +static const struct i2c_device_id fsp3y_id[] = {
> > + {"ym2151e", ym2151e},
> > + {"yh5151e", yh5151e}
> > +};
> > +
> > +static int fsp3y_probe(struct i2c_client *client)
> > +{
> > + struct fsp3y_data *data;
> > + const struct i2c_device_id *id;
> > + int rv;
> > +
> > + data = devm_kzalloc(&client->dev, sizeof(struct fsp3y_data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + data->chip = fsp3y_detect(client);
> > + if (data->chip < 0)
> > + return data->chip;
> > +
> > + id = i2c_match_id(fsp3y_id, client);
> > + if (data->chip != id->driver_data)
> > + dev_warn(&client->dev, "Device mismatch: Configured %s (%d), detected %d\n", id->name, (int)id->driver_data, data->chip);
> > +
> > + rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
> > + if (rv < 0)
> > + return rv;
> > + data->page = rv;
> > +
> > + data->info = fsp3y_info[data->chip];
> > +
> > + return pmbus_do_probe(client, &data->info);
> > +}
> > +
> > +MODULE_DEVICE_TABLE(i2c, pmbus_id);
> > +
> > +/* This is the driver that will be inserted */
>
> Nit: Useless comment
>
Done.
> > +static struct i2c_driver fsp3y_driver = {
> > + .driver = {
> > + .name = "fsp3y",
> > + },
> > + .probe_new = fsp3y_probe,
> > + .id_table = fsp3y_id
> > +};
> > +
> > +module_i2c_driver(fsp3y_driver);
> > +
> > +MODULE_AUTHOR("Václav Kubernát");
> > +MODULE_DESCRIPTION("PMBus driver for FSP/3Y-Power power supplies");
> > +MODULE_LICENSE("GPL");
> >
>
Powered by blists - more mailing lists