linux-kernel - Re: [PATCH v1] pinctrl: mcp23s08: Reset all output latches to default at probe

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

Message-ID: <20a23403-2852-4422-993c-0e949144c06f@topic.nl>
Date: Mon, 6 Oct 2025 14:18:34 +0200
From: Mike Looijmans <mike.looijmans@...ic.nl>
To: Maxim Kiselev <bigunclemax@...il.com>
CC: Linus Walleij <linus.walleij@...aro.org>, linux-gpio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] pinctrl: mcp23s08: Reset all output latches to default
 at probe

On 06-10-2025 12:18, Maxim Kiselev wrote:
> Hi
>
> пн, 6 окт. 2025 г. в 11:04, Mike Looijmans <mike.looijmans@...ic.nl>:
>> On 06-10-2025 09:49, bigunclemax@...il.com wrote:
>>> From: "Maksim Kiselev" <bigunclemax@...il.com>
>>>
>>> It appears that resetting only the direction register is not sufficient,
>>> it's also necessary to reset the OLAT register to its default values.
>>>
>>> Otherwise, the following situation can occur:
>>>
>>> If a pin was configured as OUT=1 before driver probe(Ex: IODIR=1,IOLAT=1),
>>> then after loading the MCP driver, the cache will be populated from
>>> reg_defaults with IOLAT=0 (while the actual value in the chip is 1).
>>> A subsequent setting OUT=0 will fail because
>>> mcp_update_bits(mcp, MCP_OLAT, ...) calls regmap_update_bits(),
>>> which will check that the value to be set (0) matches the cached value (0)
>>> and thus skip writing actual value to the MCP chip.
>> Maybe it's be better to fix the underlying issue: This driver should not be
>> using a pre-populated regmap cache. Unless it performs a hard reset, the
>> driver has no way of knowing what the initial values are, it should just read
>> them from the chip.
>>
> I agree with you here, thought about it, but consider such a change
> too radical :)

Yeah, I had the same thought when I created the patch that resets the 
direction register :)


> Okay, I'll remove the reg_defaults in the second version.

I think it's the proper solution. You should remove the direction register 
reset as well (basically revert my patch).

M.

>
>>> To avoid this, the OLAT register must be explicitly reset at probe.
>>>
>>> Fixes: 3ede3f8b4b4b ("pinctrl: mcp23s08: Reset all pins to input at probe")
>>> Signed-off-by: Maksim Kiselev <bigunclemax@...il.com>
>>> ---
>>>    drivers/pinctrl/pinctrl-mcp23s08.c | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
>>> index 78ff7930649d..23af441aa468 100644
>>> --- a/drivers/pinctrl/pinctrl-mcp23s08.c
>>> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c
>>> @@ -622,6 +622,11 @@ int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
>>>        if (ret < 0)
>>>                return ret;
>>>
>>> +     /* Also reset all out latches to default values */
>>> +     ret = mcp_write(mcp, MCP_OLAT, 0x0);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>>        /* verify MCP_IOCON.SEQOP = 0, so sequential reads work,
>>>         * and MCP_IOCON.HAEN = 1, so we work with all chips.
>>>         */
>> Mike.
>>
>>
>>