linux-kernel - Re: [PATCH 1/3] pinctrl: rockchip: add support for io-domain dependency

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: <367a8ee4-2047-879d-22ae-a0082a04f92f@sholland.org>
Date:   Fri, 15 Sep 2023 23:59:59 -0500
From:   Samuel Holland <samuel@...lland.org>
To:     Saravana Kannan <saravanak@...gle.com>,
        Sascha Hauer <s.hauer@...gutronix.de>
Cc:     Chen-Yu Tsai <wens@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-rockchip@...ts.infradead.org,
        Heiko Stuebner <heiko@...ech.de>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, linux-gpio@...r.kernel.org,
        kernel@...gutronix.de,
        Quentin Schulz <quentin.schulz@...obroma-systems.com>,
        Michael Riesch <michael.riesch@...fvision.net>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Robin Murphy <robin.murphy@....com>
Subject: Re: [PATCH 1/3] pinctrl: rockchip: add support for io-domain
 dependency

On 9/13/23 15:48, Saravana Kannan wrote:
> On Tue, Sep 12, 2023 at 11:58 PM Sascha Hauer <s.hauer@...gutronix.de> wrote:
>> On Wed, Sep 13, 2023 at 12:37:54PM +0800, Chen-Yu Tsai wrote:
>>> On Tue, Sep 12, 2023 at 4:07 PM Linus Walleij <linus.walleij@...aro.org> wrote:
>>>>
>>>> Top posting to bring Saravana Kannan into this discussion.
>>>>
>>>> This looks like a big hack to me, Saravana has been working
>>>> tirelessly to make the device tree probe order "sort itself out"
>>>> and I am pretty sure this issue needs to be fixed at the DT
>>>> core level and not in a driver.
>>>
>>> We could merge all the IO domain stuff into the pinctrl node/driver,
>>> like is done for Allwinner? Maybe that would simplify things a bit?
>>
>> I thought about this as well. On Rockchip the pinctrl driver and the IO
>> domain driver even work on the same register space, so putting these
>> into a single node/driver would even feel more natural than what we have
>> now.
> 
> Then we should try to do this and fix any issues blocking us.
> 
>> However, with that the pinctrl node would get the supplies that the IO
>> domain node now has and we would never get into the probe of the pinctrl
>> driver due to the circular dependencies.
> 
> From a fw_devlink perspective, the circular dependency shouldn't be a
> problem. It's smart enough to recognize all cycle possibilities (since
> 6.3) and not enforce ordering between nodes in a cycle.
> 
> So, this is really only a matter of pinctrl not trying to do
> regulator_get() in its probe function. You need to do the
> regulator_get() when the pins that depend on the io-domain are
> requested. And if the regulator isn't ready yet, return -EPROBE_DEFER?
> 
> Is there something that prevents us from doing that?

Calling regulator_get() from the pin request function does not solve the
problem on its own. We already do that in the Allwinner driver (in
sunxi_pmx_request()), and we still have the circular dependency:

  __driver_probe_device(I2C/RSB controller)
    pinctrl_bind_pins(I2C/RSB controller)
      pinctrl_select_state(I2C/RSB controller default pins)
        pinmux_enable_setting()
          pin_request()
            sunxi_pmx_request()
              regulator_get(vcc-pl)
                [depends on the PMIC/regulator driver]
                  [depends on the I2C/RSB controller driver]

To break the cycle, you need to defer the regulator_get() call during
this specific call to the pin request function, then come back later and
call regulator_get() once the regulator is actually registered.

If we have a DT property somewhere that provides an initial voltage for
the I/O domain, then regulator_get() returning -EPROBE_DEFER would not
be an error. Instead, we would configure the I/O domain based on the DT
property, and add the pair (IO domain, regulator OF node) to a list.
Then register a notifier for new regulator class devices. Check each new
 device's OF node against the list; if it is found, hook up the voltage
notifier and remove the list entry. When the list is empty, remove the
regulator class notifier.

I thought about (ab)using the pinctrl "init" state so pin_request() gets
called a second time inside pinctrl_init_done() after the PMIC's bus
controller gets probed, but that would rely on the regulator getting
registered synchronously by some recursive call inside the bus
controller probe function. So it would break if probing the
PMIC/regulator driver got deferred for any reason.

So the suggestion from my perspective ends up being the same as what
Robin just suggested elsewhere in the thread. :)

Regards,
Samuel