linux-kernel - Re: [PATCH v5 2/2] i2c: mv64xxx: add an optional bus-reset-gpios property

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: <89088ff0-d2c7-4270-bb2b-26f90d6c3e3c@alliedtelesis.co.nz>
Date:   Tue, 31 Oct 2023 19:59:27 +0000
From:   Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        "gregory.clement@...tlin.com" <gregory.clement@...tlin.com>,
        "andi.shyti@...nel.org" <andi.shyti@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "conor+dt@...nel.org" <conor+dt@...nel.org>,
        Abel Vesa <abel.vesa@...aro.org>,
        Mark Brown <broonie@...nel.org>
CC:     "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 2/2] i2c: mv64xxx: add an optional bus-reset-gpios
 property


On 31/10/23 19:01, Krzysztof Kozlowski wrote:
> On 29/10/2023 21:48, Chris Packham wrote:
>> On 28/10/23 00:37, Krzysztof Kozlowski wrote:
>>> On 27/10/2023 13:27, Krzysztof Kozlowski wrote:
>>>> On 27/10/2023 05:31, Chris Packham wrote:
>>>>> Some hardware designs have a GPIO used to control the reset of all the
>>>>> devices on and I2C bus. It's not possible for every child node to
>>>>> declare a reset-gpios property as only the first device probed would be
>>>>> able to successfully request it (the others will get -EBUSY). Represent
>>> Cc: Mark,
>>>
>>> Also this part is not true. If the bus is non-discoverable, then it is
>>> possible to have reset-gpios in each probed device. You can share GPIOs,
>>> so no problem with -EBUSY at all.
>> Last time I checked you couldn't share GPIOs. If that's no longer the
>> case then I can probably make what I need to happen work. It still
>> creates an issue that I have multiple PCA954x muxes connected to a
>> common reset GPIO so as each mux is probed the PCA954x driver will
>> toggle the reset. That's probably OK as the PCA954x is sufficiently
>> stateless that the extra resets won't do any harm but if it were a more
>> complicated device then there would be issues.
> I know, but this is a broader problem, not really specific to this one
> device. I also argue that your I2C controller does not actually have
> this reset line.

Yes absolutely. Because the reset line is common to multiple pca954x 
muxes the only option I have (I think) is to associate the reset line 
with the controller. It happens to be true for my case that everything 
connected to that bus is affected by the reset line but I can completely 
see that there may be other designs where there are a mix of muxes and 
other devices on the root bus.

So associating the reset line with the I2C controller is a pragmatic 
solution (or an egregious hack depending on your point of view) that 
works with this kind of hardware design.

Another complete hack I've experimented with is adding the muxes defined 
with `status = "disabled";` in the dts and having a custom driver that 
requests the GPIO and manipulates the live device tree. It works but is 
quite a lot more code and will invariably break if I need to tweak the 
device tree.

>> Having some kind of ref-counted reset controller that is implemented
>> with GPIOs is probably the better solution. I was kind of surprised that
>> nothing existed like that in drivers/reset.
> reset controller framework already supports this. The point is that GPIO
> reset is not a reset controller, so in terms of bindings "resets"
> property does not fit it.

So I need some way of representing a GPIO line associated with multiple 
devices that must be requested and driven appropriately before the 
devices are probed. In lieu of anything else a "bus-reset-gpios" 
property recognized by the generic I2C framework kind of sounds like the 
best solution so far. Unless maybe there's some kind of pinctrl type 
thing that would already work.

>>> The problem is doing reset:
>>> 1. in proper moment for all devices
>>> 2. without affecting other devices when one unbinds/remove()
>>>
>>> The (2) above is not solveable easy in kernel and we already had nice
>>> talks about it just few days ago:
>>> 1. Apple case:
>>> https://scanmail.trustwave.com/?c=20988&d=tZjA5R77ysliRyWfDvgX9JnmLZr-TqhRWpYjsNO-5A&u=https%3a%2f%2fsocial%2etreehouse%2esystems%2f%40marcan%2f111268780311634160
>>>
>>> 2. my WSA884x:
>>> https://scanmail.trustwave.com/?c=20988&d=tZjA5R77ysliRyWfDvgX9JnmLZr-TqhRWpZ9tI7vvw&u=https%3a%2f%2flore%2ekernel%2eorg%2falsa-devel%2f84f9f1c4-0627-4986-8160-b4ab99469b81%40linaro%2eorg%2f
>> Apologies for the mangled links (they're more secure now at least that's
>> what our IS team have been sold).
>>> Last,
>>> I would like to apologize to you Chris. I understand that bringing such
>>> feedback at v5 is not that good. I had plenty of time to say something
>>> earlier, so this is not really professional from my side. I am sorry,
>>> just my brain did not connect all these topics together.
>>>
>>> I apologize.
>> Actually I kind of expected this feedback. I figured I could start with
>> the driver that is currently causing me issues and once the dt-binding
>> was considered good enough it might migrate to the i2c core.
>>
>
> Best regards,
> Krzysztof
>