linux-kernel - Re: [PATCH] [v1 1/1]hwmon:(pmbus) Validate the data for chip supporting vout_mode (PMBUS_HAVE

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <085e90e5-d21e-9068-a2e1-6f7e07fa64df@roeck-us.net>
Date:   Tue, 11 Oct 2022 18:35:13 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     karthik gengan <gengankarthik@...il.com>,
        Jean Delvare <jdelvare@...e.com>
Cc:     linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] [v1 1/1]hwmon:(pmbus) Validate the data for chip
 supporting vout_mode (PMBUS_HAVE_VOUT) in the linear config.

On 10/11/22 16:59, karthik gengan wrote:
> Linear mode config calculation is based on the exponent value
> derived from vout_mode. So vout_mode value should be valid
> for PMBUS_VOUT_MODE command-supported chips.
> 

We can not do this. The operational word is "should". See comment
below "Not all chips support the VOUT_MODE command". It is what it is.
We can not just refuse to support such chips because they don't
support what we expect them to support.

Sure, those chips will (likely) report wrong values since the
exponent will default to 0. That can be adjusted in user space,
or whoever finds such a chip can provide a back-end driver
with the appropriate values configured (for example by providing
a dummy VOUT_MODE command response). That is better than just
rejecting the chip entirely.

 From a practical perspective, if you know about an affected chip
one that would refuse to instantiate after your patch is applied,
I would suggest to submit (or improve) a back-end driver with
an explanation instead.

Thanks,
Guenter

> Signed-off-by: karthik.gengan <gengankarthik@...il.com <mailto:gengankarthik@...il.com>>
> ---
>   drivers/hwmon/pmbus/pmbus_core.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 7ec04934747e..5f80c3b8f245 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2507,9 +2507,17 @@ static int pmbus_identify_common(struct i2c_client *client,
>   {
>      int vout_mode = -1;
> 
> -   if (pmbus_check_byte_register(client, page, PMBUS_VOUT_MODE))
> +   if (pmbus_check_byte_register(client, page, PMBUS_VOUT_MODE)) {
>          vout_mode = _pmbus_read_byte_data(client, page,
>                            PMBUS_VOUT_MODE);
> +       /*
> +        * If the client page supports PMBUS_VOUT_MODE,
> +        * then the output of the VOUT_MODE command should
> +        * be a valid value for linear mode calculation.
> +        */
> +       if ((data->info->format[PSC_VOLTAGE_OUT] == linear) && (vout_mode < 0))
> +           return -ENODEV;
> +   }
>      if (vout_mode >= 0 && vout_mode != 0xff) {
>          /*
>           * Not all chips support the VOUT_MODE command,
> --
> 2.25.1
>