Message-ID: <e9fd0005-bfbb-4052-8c2a-9200eb0b60ac@arm.com>
Date: Fri, 23 Jan 2026 20:57:50 +0000
From: Robin Murphy <robin.murphy@....com>
To: Bartosz Golaszewski <brgl@...nel.org>
Cc: Sebastian Reichel <sebastian.reichel@...labora.com>,
 Bartosz Golaszewski <bartosz.golaszewski@....qualcomm.com>,
 Linus Walleij <linusw@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 linux-gpio@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
 stable@...r.kernel.org, Marek Szyprowski <m.szyprowski@...sung.com>
Subject: Re: [PATCH] gpio: rockchip: mark the GPIO controller as sleeping

On 2026-01-23 7:27 pm, Bartosz Golaszewski wrote:
> On Fri, Jan 23, 2026 at 2:27 PM Robin Murphy <robin.murphy@....com> wrote:
>>
>>>>
>>>> It's not a big issue for the hdmirx driver specifically, but I wonder
>>>> how many more (less often tested) rockchip drivers use GPIOs from their
>>>> IRQ handler.
>>
>> Yeah, seems this finally reached my distro kernel and now the kernel log
>> on one of my boards is totally flooded from gpio_ir_recv_irq()
>> (legitimately) calling gpio_get_value()... that's not really OK :/
>>
> 
> This has always been a sleeping driver. The driver does not know the
> firmware configuration it'll be passed and - as I explained above -
> depending on the lookup flags, we may call .direction_output() and
> descend into pinctrl which uses mutexes. Ideally, we'd make
> GPIO-facing pinctrl operations not sleeping but this is a long-time
> project and quite complex. Telling the GPIO core that it cannot sleep
> is simply incorrect - even if it worked for this particular use-case -
> and has an impact on paths we're choosing.
> 
> Can the GPIO reading in the gpio-ir-recv driver be done from a
> high-priority workqueue by any chance? Or can we make it a threaded
> interrupt?

rockchip_gpio_get() is essentially nothing but a readl(), please explain 
how that could sleep? Saying that countless in-tree and out-of-tree 
arbitrary GPIO consumer drivers should pointlessly refactor just to 
avoid the GPIO core spewing spurious WARN()s is not reasonable.

I appreciate there are cases where the warning most definitely *is* 
relevant, which is why I picked up this discussion rather than proposing 
a revert, even though the documentation says:

  * @can_sleep: flag must be set iff get()/set() methods sleep, as they

where since neither rockchip_gpio_get() nor rockchip_gpio_set() 
themselves sleep, apparently this flag must *not* be set. It's 
irrelevant that a higher-level gpiod_set_value() invocation might end up 
calling .set_direction before it gets as far as calling .set - that's 
not the gpio_chip's fault, and gpiolib knows exactly what it's doing.

What I'm getting at is that getting, and even (directly) setting a 
GPIO's value can reasonably be expected to be more common and applicable 
in a wider range of circumstances than changing its configuration, so 
the former should not be unfairly penalised because of the latter. This 
case is clearly distinct from external GPIO expanders on buses that 
fundamentally can't guarantee fast memory-mapped access at all, so 
trying to conflate it under the same flag doesn't fit, and that needs 
fixing ASAP, before the reverts *do* start piling in. Maybe that just 
means some new dir_can_sleep or more abstract dir_needs_pinctrl flag 
might suffice, maybe it's something more involved; I don't really know, 
and I don't have the bandwidth to take this on myself.

Thanks,
Robin.

Powered by blists - more mailing lists