linux-kernel - RE: [PATCH 2/3] hwmon: (pmbus/ltc2978): Fix PMBus polling of MFR

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <SN6PR03MB40329A1D480256447C4565CDF60A0@SN6PR03MB4032.namprd03.prod.outlook.com>
Date:   Tue, 28 Jan 2020 21:16:39 +0000
From:   "Jones, Michael-A1" <Michael-A1.Jones@...log.com>
To:     Guenter Roeck <linux@...ck-us.net>
CC:     "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jdelvare@...e.com" <jdelvare@...e.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "corbet@....net" <corbet@....net>
Subject: RE: [PATCH 2/3] hwmon: (pmbus/ltc2978): Fix PMBus polling of
 MFR_COMMON definitions.

Guenter,

The decision to not poll PEND was based on some other non-driver code based on /dev/i2c that looked like this:

// Set to 0 for LTC3883 which does not support PEND
#define USE_PEND        1

#define NOT_BUSY        1 << 6
#define NOT_TRANS       1 << 4
#if (USE_PEND)
#define NOT_PENDING     1 << 5
#else
#define NOT_PENDING     0
#endif

My recollection is that came from the datasheet, many years ago.

I talked to the designer, and if the above is correct, it has not been correct since 2012. The designer was not interested in researching artifacts that far back in history. So we know it has been in the part for at least 8 years.

There seems to be two choices:

A) Leave it as is
B) Poll the PEND bit and possibly break compatibility on ancient hardware

Generally, unused status bits in this kind are high when reserved or not used. That is good for polling.

The shipping volume of LTC3888 has always been very low compared to other parts, so exposure is very small, certainly Cisco/Juniper type companies would not be effected.

I would feel ok with polling PEND on this part. Let me know your opinion.

On a related matter, bit 4 is asserted low when the output is changing value. Hwmon cannot cause this because it only performs telemetry.

A user application could change VOUT and cause this to happen. Telemetry would reflect the last measured value from a 100ms internal polling loop, which may be a before, after, or during value. I have always judged that checking this bit has no value, and it can be problematic. If the part is set to have a very long transition rate, like 5 seconds, this would hang the call for a long time. That seemed bad to me, and is why I did not poll this bit.

Mike

-----Original Message-----
From: Guenter Roeck <groeck7@...il.com> On Behalf Of Guenter Roeck
Sent: Tuesday, January 28, 2020 12:13 PM
To: Jones, Michael-A1 <Michael-A1.Jones@...log.com>
Cc: linux-hwmon@...r.kernel.org; devicetree@...r.kernel.org; linux-doc@...r.kernel.org; linux-kernel@...r.kernel.org; jdelvare@...e.com; robh+dt@...nel.org; corbet@....net
Subject: Re: [PATCH 2/3] hwmon: (pmbus/ltc2978): Fix PMBus polling of MFR_COMMON definitions.

[External]

On Tue, Jan 28, 2020 at 10:59:59AM -0700, Mike Jones wrote:
> Change 21537dc driver PMBus polling of MFR_COMMON from bits 5/4 to 
> bits 6/5. This fixs a LTC297X family bug where polling always returns 
> not busy even when the part is busy. This fixes a LTC388X and LTM467X 
> bug where polling used PEND and NOT_IN_TRANS, and BUSY was not polled, 
> which can lead to NACKing of commands. LTC388X and LTM467X modules now 
> poll BUSY and PEND, increasing reliability by eliminating NACKing of 
> commands.
> 
> Signed-off-by: Mike Jones <michael-a1.jones@...log.com>

Fixes: e04d1ce9bbb49 ("hwmon: (ltc2978) Add polling for chips requiring it")

> ---
>  drivers/hwmon/pmbus/ltc2978.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/ltc2978.c 
> b/drivers/hwmon/pmbus/ltc2978.c index f01f488..a91ed01 100644
> --- a/drivers/hwmon/pmbus/ltc2978.c
> +++ b/drivers/hwmon/pmbus/ltc2978.c
> @@ -82,8 +82,8 @@ enum chips { ltc2974, ltc2975, ltc2977, ltc2978, 
> ltc2980, ltc3880, ltc3882,
>  
>  #define LTC_POLL_TIMEOUT		100	/* in milli-seconds */
>  
> -#define LTC_NOT_BUSY			BIT(5)
> -#define LTC_NOT_PENDING			BIT(4)
> +#define LTC_NOT_BUSY			BIT(6)
> +#define LTC_NOT_PENDING			BIT(5)
>  

In ltc_wait_ready(), we have:

	/*
         * LTC3883 does not support LTC_NOT_PENDING, even though
         * the datasheet claims that it does.
         */
        mask = LTC_NOT_BUSY;
        if (data->id != ltc3883)
                mask |= LTC_NOT_PENDING;

The semantics of this code is now different: It means that on
LTC3883 only bit 6 is checked; previously, it was bit 5. I agree that the above change makes sense, but it doesn't seem correct to drop the check for bit 5 on LTC3883. Maybe remove the if() above and always check for bit 5 and 6 ? Or should bit 4 be checked on parts other than LTC3883 ?

#define LTC_NOT_TRANSITIONING		BIT(4)
...
        mask = LTC_NOT_BUSY | LTC_NOT_PENDING;
        if (data->id != ltc3883)
                mask |= LTC_NOT_TRANSITIONING;

Thanks,
Guenter