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: <20200128223813.GA13354@roeck-us.net>
Date:   Tue, 28 Jan 2020 14:38:13 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     "Jones, Michael-A1" <Michael-A1.Jones@...log.com>
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.

Hi Mike,

On Tue, Jan 28, 2020 at 09:35:49PM +0000, Jones, Michael-A1 wrote:
> Guentar,
> 
> Yes, LTC3883. Slip of the finger.
> 
> I am happy as is. If LTC3883 starts selling like hotcakes, I'll update it later.
> 
> This is my first attempt at doing things the linux way, so if you see something I should do different, let me know. I left off Tested-by, assuming that Signed-off-by was good enough. But I do test the changes using a DLN-2 and demo boards.
> 

Don't top post, and split your lines at 80 columns. Tested-by: only makes
sense if someone else tested your code; that you have tested it yourself
is (or should be) implied in the Signed-off-by: tag (that people sometimes
send patches which don't even compile is no excuse).

Thanks,
Guenter

> Mike
> 
> -----Original Message-----
> From: Guenter Roeck <groeck7@...il.com> On Behalf Of Guenter Roeck
> Sent: Tuesday, January 28, 2020 2:31 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 09:16:39PM +0000, Jones, Michael-A1 wrote:
> > 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 assume you mean LTC3883, not LTC3888.
> 
> > 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.
> > 
> 
> Your call, really. My major concern was that bit 5 is no longer polled on LTC3883. From the above, it looks like it actually _is_ the pending bit (5) that isn't supported on LTC3883. With that in mind, I'll apply the series as-is and add the Fixes: tag myself; no need to resend.
> 
> Thanks,
> Guenter
> 
> > 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