linux-kernel - Re: [PATCH] hwmon: max34451: Workaround for lost page

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: <CAD_4BXhUVRpNjORSHYiwhxXAGbAv5=4SYekWZhK+r9Wi=n5+Lw@mail.gmail.com>
Date: Wed, 2 Apr 2025 01:33:08 -0700
From: William Kennington <william@...nnington.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org, 
	linux-kernel@...r.kernel.org, openbmc@...ts.ozlabs.org
Subject: Re: [PATCH] hwmon: max34451: Workaround for lost page

On Tue, Apr 1, 2025 at 5:19 PM Guenter Roeck <linux@...ck-us.net> wrote:
>
> On 4/1/25 15:55, William Kennington wrote:
> > On Tue, Apr 1, 2025 at 3:52 PM Guenter Roeck <linux@...ck-us.net> wrote:
> >>
> >> On 4/1/25 15:08, William A. Kennington III wrote:
> >>> When requesting new pages from the max34451 we sometimes see that the
> >>> firmware doesn't update the page on the max34451 side fast enough. This
> >>> results in the kernel receiving data for a different page than what it
> >>> expects.
> >>>
> >>> To remedy this, the manufacturer recommends we wait 50-100us until
> >>> the firmware should be ready with the new page.
> >>>
> >>> Signed-off-by: William A. Kennington III <william@...nnington.com>
> >>> ---
> >>>    drivers/hwmon/pmbus/max34440.c | 7 +++++++
> >>>    1 file changed, 7 insertions(+)
> >>>
> >>> diff --git a/drivers/hwmon/pmbus/max34440.c b/drivers/hwmon/pmbus/max34440.c
> >>> index c9dda33831ff..ac3a26f7cff3 100644
> >>> --- a/drivers/hwmon/pmbus/max34440.c
> >>> +++ b/drivers/hwmon/pmbus/max34440.c
> >>> @@ -12,6 +12,7 @@
> >>>    #include <linux/init.h>
> >>>    #include <linux/err.h>
> >>>    #include <linux/i2c.h>
> >>> +#include <linux/delay.h>
> >>>    #include "pmbus.h"
> >>>
> >>>    enum chips { max34440, max34441, max34446, max34451, max34460, max34461 };
> >>> @@ -241,6 +242,12 @@ static int max34451_set_supported_funcs(struct i2c_client *client,
> >>>                if (rv < 0)
> >>>                        return rv;
> >>>
> >>> +             /* Firmware is sometimes not ready if we try and read the
> >>
> >> This is not the networking subsystem. Standard multi-line comments, please.
> >
> > Okay, let me fix that.
> >
> >>
> >>> +              * data from the page immediately after setting. Maxim
> >>> +              * recommends 50-100us delay.
> >>> +              */
> >>> +             fsleep(50);
> >>
> >> I would suggest to wait 100uS to be safe. The function is only called during probe,
> >> so that should be ok.
> >
> > Yeah, I don't think they did strenuous measurement of these values on
> > their end. We have been using this patch for 4-5 years now with
> > seemingly good robustness on the 50us value. I just pulled up an old
> > email from the vendor that gives this context.
> >
> >>
> >> Is this a generic problem with this chip when changing pages ?
> >
> > I believe that is the case, but this patch is pretty old at this
> > point. Is there somewhere to add in quirks for such chips that would
> > allow us to build in such a delay?
> >
>
> So far we only have delays for all accesses and for write operations.
> See access_delay and write_delay in struct pmbus_data. If the problem
> only affects page changes, we might have to add page_change_delay or
> something similar. Alternatively, maybe we could just set write_delay.
> If the chip has trouble with page changes, it might well be that it has
> trouble with write operations in general.
>

So I did some digging and asked the original contributors to the
patch. It would appear that it's specifically an issue with this IC
around page switches and not any arbitrary write command. There is an
issue where it does not correctly respond to the second command issued
after a PAGE switch occurs, if the commands come in too quickly. They
believe it's an issue with max34451 (and other derivative ICs) not
correctly clock stretching while the PAGE command is processed.

Let me know what approach you would prefer to take here. It seems like
it would be most optimal to have a quirk specifically to delay
commands after a PAGE.

> Thanks,
> Guenter
>