Message-ID: <CAMRc=MfUawHpDgxj=fP2OF_-qg1O+P3oM_cSvGsbvAdLRB=+hw@mail.gmail.com>
Date: Sat, 24 Jan 2026 16:07:12 -0500
From: Bartosz Golaszewski <brgl@...nel.org>
To: Robin Murphy <robin.murphy@....com>
Cc: Sebastian Reichel <sebastian.reichel@...labora.com>, 
	Bartosz Golaszewski <bartosz.golaszewski@....qualcomm.com>, Linus Walleij <linusw@...nel.org>, 
	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>, 
	Heiko Stübner <heiko@...ech.de>, 
	Bartosz Golaszewski <brgl@...nel.org>
Subject: Re: [PATCH] gpio: rockchip: mark the GPIO controller as sleeping

On Sat, 24 Jan 2026 00:45:25 +0100, Robin Murphy <robin.murphy@....com> said:
> On 2026-01-23 9:52 pm, Heiko Stübner wrote:
>> Am Freitag, 23. Januar 2026, 21:57:50 Mitteleuropäische Normalzeit schrieb Robin Murphy:
>>> 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?
>>>

Let me circle back to my earlier question. Would the following change work?

diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
index a6418ef782bc..1f95e54bd146 100644
--- a/drivers/media/rc/gpio-ir-recv.c
+++ b/drivers/media/rc/gpio-ir-recv.c
@@ -26,6 +26,11 @@ struct gpio_rc_dev {
 };

 static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)
+{
+	return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t gpio_ir_recv_irq_thread(int irq, void *dev_id)
 {
 	int val;
 	struct gpio_rc_dev *gpio_dev = dev_id;
@@ -120,9 +125,10 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)

 	platform_set_drvdata(pdev, gpio_dev);

-	return devm_request_irq(dev, gpio_dev->irq, gpio_ir_recv_irq,
-				IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
-				"gpio-ir-recv-irq", gpio_dev);
+	return devm_request_threaded_irq(dev, gpio_dev->irq, gpio_ir_recv_irq,
+					 gpio_ir_recv_irq_thread,
+					 IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
+					 "gpio-ir-recv-irq", gpio_dev);
 }

 static void gpio_ir_recv_remove(struct platform_device *pdev)

>>> 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.
>>>

Right, so gpiod_get_value() is a bit different from gpiod_set_value() because
it indeed can't descend into pinctrl.

>>> 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.
>>>

Well, the wording may be unfortunate and it's probably been this way for a
long time. I don't know the history here, this probably needs to be revised.

Please keep in mind: I don't deal with a single rockchip driver but with
a generic subsystem supporting hundreds of GPIO drivers and thousands of
users across the kernel tree. Not to mention all the API abuse like people
calling direction_output() from atomic context etc. We have all kinds of
combinations of sleeping and non-sleeping consumers and suppliers. It's
sometimes hard to find the middle ground.

Maybe the core GPIO code should not try to simulate open-source/open-drain
on non-sleeping chips for gpiod_set_value(). Or maybe we need a .uses_pinctrl
flag parallel to .cansleep which tells GPIO core that while set/get don't
sleep, direction_output/input may.

>
>> rockchip_pmx_gpio_set_direction()'s only function is to set the GPIO
>> pinmux - it does not handle the actual the actual direction.
>>
>> Can't we move the pinctrl_gpio_direction_input/_output() call just over
>> to the request callback of the gpiochip?
>

What if the user calls gpiod_direction_output() on a pin set to input?

> In fact, after an hour or so chasing through the code, is that not just
> pretty much this? (Not even compile tested as I'd rather go to bed now...)
>
> Cheers,
> Robin.
>
> ----->8-----
> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index 47174eb3ba76..118edd57c252 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c
> @@ -164,12 +164,6 @@ static int rockchip_gpio_set_direction(struct gpio_chip *chip,
>   	unsigned long flags;
>   	u32 data = input ? 0 : 1;
>
> -
> -	if (input)
> -		pinctrl_gpio_direction_input(chip, offset);
> -	else
> -		pinctrl_gpio_direction_output(chip, offset);
> -
>   	raw_spin_lock_irqsave(&bank->slock, flags);
>   	rockchip_gpio_writel_bit(bank, offset, data, bank->gpio_regs->port_ddr);
>   	raw_spin_unlock_irqrestore(&bank->slock, flags);
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
> index e44ef262beec..2fc67aeafdb3 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -3545,10 +3545,9 @@ static int rockchip_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
>   	return 0;
>   }
>
> -static int rockchip_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
> -					   struct pinctrl_gpio_range *range,
> -					   unsigned offset,
> -					   bool input)
> +static int rockchip_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
> +					    struct pinctrl_gpio_range *range,
> +					    unsigned int offset)
>   {
>   	struct rockchip_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
>   	struct rockchip_pin_bank *bank;
> @@ -3562,7 +3561,7 @@ static const struct pinmux_ops rockchip_pmx_ops = {
>   	.get_function_name	= rockchip_pmx_get_func_name,
>   	.get_function_groups	= rockchip_pmx_get_groups,
>   	.set_mux		= rockchip_pmx_set,
> -	.gpio_set_direction	= rockchip_pmx_gpio_set_direction,
> +	.gpio_request_enable	= rockchip_pmx_gpio_request_enable,
>   };
>
>   /*
>

I'm not sure what's going on here. You don't really need to call
pinctrl_gpio_direction_input/output()?

I'm putting it on my TODO list to figure out a proper way of interacting
with pinctrl. At the same time, I want to fix your problem so the .uses_pinctrl
flag sounds like a solution. This would make gpio-shared-proxy still use
a mutex but we wouldn't warn in gpiod_get_value(). Unless you can simply switch
to a threaded interrupt in the IR driver. Maybe the latency would be
negligible?

Bartosz

Powered by blists - more mailing lists