[RFC PATCH 0/3] pinctrl: can_sleep and pinctrl_gpio_direction

[RFC PATCH 0/3] pinctrl: can_sleep and pinctrl_gpio_direction

[Hans Verkuil]

Hi all,

Based on this discussion:

https://lore.kernel.org/linux-gpio/CACRpkdb3q4-9O3dHS6QDWnZZ5JJjXWXS9KPvwXVaowLMRhcejA@mail.gmail.com/T/#t

I propose this RFC series.

The first patch adds a check if can_sleep is true when pinctrl_gpio_direction()
is called. It does this only once per pin controller (a new field is added to
keep track if this has been checked or not). If the gpio driver set can_sleep
to false, then it makes no sense that pinctrl_gpio_direction() is called since
it takes two different mutexes. This way you get at least a warning that something
is wrong, rather than only if CONFIG_DEBUG_ATOMIC_SLEEP is set.

However, if there are gpio drivers that call pinctrl_gpio_direction() always from
non-atomic contexts (so never via gpiod_get/set_value() etc.) then I'm not sure if
this check is possible here.

The second and third patches convert the bcm2835 and sunxi pinctrl drivers to
set the direction in those drivers directly, rather than by calling
pinctrl_gpio_direction_input/output. Both set can_sleep to false, so going
through a code path that uses mutexes is not a good idea.

This series has been tested with the cec-gpio driver on a Raspberry Pi 4b and
with an A10 Cubieboard. Both pincontroller drivers set can_sleep to false, but
call pinctrl_gpio_direction_input/output().

Those last two patches look sane to me.

Regards,

	Hans

Hans Verkuil (3):
  pinctrl/core: check that can_sleep is true in pinctrl_gpio_direction()
  pinctrl-bcm2835: don't call pinctrl_gpio_direction()
  pinctrl-sunxi: don't call pinctrl_gpio_direction()

 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 12 +++++++++---
 drivers/pinctrl/core.c                | 11 +++++++++++
 drivers/pinctrl/core.h                |  4 ++++
 drivers/pinctrl/sunxi/pinctrl-sunxi.c |  8 ++++++--
 4 files changed, 30 insertions(+), 5 deletions(-)

-- 
2.33.0

[RFC PATCH 1/3] pinctrl/core: check that can_sleep is true in pinctrl_gpio_direction()

[Hans Verkuil]

Since pinctrl_gpio_direction() takes a mutex, the gpio driver must
have can_sleep set to true. If not, then this function will call
WARN_ON (only once per pin controller) to tell the developer that
something must be done: either set can_sleep to true, or set the
direction in the combine gpio/pinctrl driver itself without calling
pinctrl_gpio_direction_input/output.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/pinctrl/core.c | 11 +++++++++++
 drivers/pinctrl/core.h |  4 ++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index ffe39336fcac..886242d08dcf 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -845,6 +845,17 @@ static int pinctrl_gpio_direction(unsigned gpio, bool input)
 
 	mutex_lock(&pctldev->mutex);
 
+	if (!pctldev->checked_can_sleep) {
+		struct gpio_chip *chip = gpio_to_chip(gpio);
+
+		pctldev->checked_can_sleep = true;
+		/*
+		 * This pin controller can sleep, so check that the gpio
+		 * controller sets can_sleep to true.
+		 */
+		WARN_ON(chip && !chip->can_sleep);
+	}
+
 	/* Convert to the pin controllers number space */
 	pin = gpio_to_pin(range, gpio);
 	ret = pinmux_gpio_direction(pctldev, range, pin, input);
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 840103c40c14..ae14b4e69b7b 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -38,6 +38,9 @@ struct pinctrl_gpio_range;
  * @hog_sleep: sleep state for pins hogged by this device
  * @mutex: mutex taken on each pin controller specific action
  * @device_root: debugfs root for this device
+ * @checked_can_sleep: used internally to warn once if pinctrl_gpio_direction()
+ *	is called from a gpio driver that didn't set @can_sleep to true.
+ *	pinctrl_gpio_direction() uses mutexes, so it can sleep.
  */
 struct pinctrl_dev {
 	struct list_head node;
@@ -62,6 +65,7 @@ struct pinctrl_dev {
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *device_root;
 #endif
+	bool checked_can_sleep;
 };
 
 /**
-- 
2.33.0

[RFC PATCH 2/3] pinctrl-bcm2835: don't call pinctrl_gpio_direction()

[Hans Verkuil]

Set the direction directly without calling pinctrl_gpio_direction().
This avoids the mutex_lock() calls in that function, which would
invalid the can_sleep = false.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index 2abcc6ce4eba..2f51fc3b24eb 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -313,7 +313,10 @@ static inline void bcm2835_pinctrl_fsel_set(
 
 static int bcm2835_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 {
-	return pinctrl_gpio_direction_input(chip->base + offset);
+	struct bcm2835_pinctrl *pc = gpiochip_get_data(chip);
+
+	bcm2835_pinctrl_fsel_set(pc, offset, BCM2835_FSEL_GPIO_IN);
+	return 0;
 }
 
 static int bcm2835_gpio_get(struct gpio_chip *chip, unsigned offset)
@@ -348,8 +351,11 @@ static void bcm2835_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 static int bcm2835_gpio_direction_output(struct gpio_chip *chip,
 		unsigned offset, int value)
 {
-	bcm2835_gpio_set(chip, offset, value);
-	return pinctrl_gpio_direction_output(chip->base + offset);
+	struct bcm2835_pinctrl *pc = gpiochip_get_data(chip);
+
+	bcm2835_gpio_set_bit(pc, value ? GPSET0 : GPCLR0, offset);
+	bcm2835_pinctrl_fsel_set(pc, offset, BCM2835_FSEL_GPIO_OUT);
+	return 0;
 }
 
 static const struct gpio_chip bcm2835_gpio_chip = {
-- 
2.33.0

[RFC PATCH 3/3] pinctrl-sunxi: don't call pinctrl_gpio_direction()

[Hans Verkuil]

Set the direction directly without calling pinctrl_gpio_direction().
This avoids the mutex_lock() calls in that function, which would
invalid the can_sleep = false.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/pinctrl/sunxi/pinctrl-sunxi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index 862c84efb718..80d6750c74a6 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -835,7 +835,9 @@ static const struct pinmux_ops sunxi_pmx_ops = {
 static int sunxi_pinctrl_gpio_direction_input(struct gpio_chip *chip,
 					unsigned offset)
 {
-	return pinctrl_gpio_direction_input(chip->base + offset);
+	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
+
+	return sunxi_pmx_gpio_set_direction(pctl->pctl_dev, NULL, offset, true);
 }
 
 static int sunxi_pinctrl_gpio_get(struct gpio_chip *chip, unsigned offset)
@@ -885,8 +887,10 @@ static void sunxi_pinctrl_gpio_set(struct gpio_chip *chip,
 static int sunxi_pinctrl_gpio_direction_output(struct gpio_chip *chip,
 					unsigned offset, int value)
 {
+	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
+
 	sunxi_pinctrl_gpio_set(chip, offset, value);
-	return pinctrl_gpio_direction_output(chip->base + offset);
+	return sunxi_pmx_gpio_set_direction(pctl->pctl_dev, NULL, offset, false);
 }
 
 static int sunxi_pinctrl_gpio_of_xlate(struct gpio_chip *gc,
-- 
2.33.0

Re: [RFC PATCH 2/3] pinctrl-bcm2835: don't call pinctrl_gpio_direction()

[Florian Fainelli]

On 12/6/21 5:16 AM, Hans Verkuil wrote:
> Set the direction directly without calling pinctrl_gpio_direction().
> This avoids the mutex_lock() calls in that function, which would
> invalid the can_sleep = false.

make invalid, or invalidate?

With that fixed:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [RFC PATCH 0/3] pinctrl: can_sleep and pinctrl_gpio_direction

[Linus Walleij]

On Tue, Dec 7, 2021 at 11:14 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Monday, December 6, 2021, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>
>> Hi all,
>>
>> Based on this discussion:
>>
>> https://lore.kernel.org/linux-gpio/CACRpkdb3q4-9O3dHS6QDWnZZ5JJjXWXS9KPvwXVaowLMRhcejA@mail.gmail.com/T/#t
>>
>> I propose this RFC series.
>
>
> When I first saw your report I was thinking about actually adding a new callback ->set_direction_atomic()
> and then make pinctrl use it, otherwise like you do, I.e. issue a warning when it’s called in atomic context

The problem is inside of pinctrl core, not in any driver. It takes a
mutex when going over
the GPIO ranges.

I suggested maybe just replacing these mutexes with spinlocks, or RCU.

Yours,
Linus Walleij

Re: [RFC PATCH 0/3] pinctrl: can_sleep and pinctrl_gpio_direction

[Hans Verkuil]

On 08/12/2021 01:30, Linus Walleij wrote:
> On Tue, Dec 7, 2021 at 11:14 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Monday, December 6, 2021, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>>
>>> Hi all,
>>>
>>> Based on this discussion:
>>>
>>> https://lore.kernel.org/linux-gpio/CACRpkdb3q4-9O3dHS6QDWnZZ5JJjXWXS9KPvwXVaowLMRhcejA@mail.gmail.com/T/#t
>>>
>>> I propose this RFC series.
>>
>>
>> When I first saw your report I was thinking about actually adding a new callback ->set_direction_atomic()
>> and then make pinctrl use it, otherwise like you do, I.e. issue a warning when it’s called in atomic context
> 
> The problem is inside of pinctrl core, not in any driver. It takes a
> mutex when going over
> the GPIO ranges.
> 
> I suggested maybe just replacing these mutexes with spinlocks, or RCU.

RCU or spinlock would most likely work as a replacement for pinctrldev_list_mutex, but
not for pctldev->mutex. I didn't see any obvious way of replacing it with something else.

I'm open to any suggestions, but for now this was the best I could come up with, given
my limited knowledge of the gpio/pinctrl subsystems. It at least warns you that something
is wrong.

Personally I think that for combined gpio/pinctrl drivers it doesn't make sense to take
this additional loop through the pinctrl core, regardless of whatever locking mechanism
is used. I actually think that it obfuscates those drivers a bit, but that might just be
me.

Regards,

	Hans

> 
> Yours,
> Linus Walleij
> 

Re: [RFC PATCH 0/3] pinctrl: can_sleep and pinctrl_gpio_direction

[Andy Shevchenko]

On Wed, Dec 8, 2021 at 11:26 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> On 08/12/2021 01:30, Linus Walleij wrote:
> > On Tue, Dec 7, 2021 at 11:14 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> >> On Monday, December 6, 2021, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:

> >>> Based on this discussion:
> >>>
> >>> https://lore.kernel.org/linux-gpio/CACRpkdb3q4-9O3dHS6QDWnZZ5JJjXWXS9KPvwXVaowLMRhcejA@mail.gmail.com/T/#t
> >>>
> >>> I propose this RFC series.
> >>
> >>
> >> When I first saw your report I was thinking about actually adding a new callback ->set_direction_atomic()
> >> and then make pinctrl use it, otherwise like you do, I.e. issue a warning when it’s called in atomic context
> >
> > The problem is inside of pinctrl core, not in any driver. It takes a
> > mutex when going over
> > the GPIO ranges.
> >
> > I suggested maybe just replacing these mutexes with spinlocks, or RCU.
>
> RCU or spinlock would most likely work as a replacement for pinctrldev_list_mutex, but
> not for pctldev->mutex. I didn't see any obvious way of replacing it with something else.

You can't get rid of locking even with RCU. AFAIR the locking protects
"writer" side and it can well be mutex, doesn't really matter. The
question about RCU is what we are actually _doing_ in the call we are
talking about. If it's a simple _reader_ of some (shared) array of
data which is not being updated during this traversing, then it's a
very good fit for it. Otherwise other means should be considered.

> I'm open to any suggestions, but for now this was the best I could come up with, given
> my limited knowledge of the gpio/pinctrl subsystems. It at least warns you that something
> is wrong.
>
> Personally I think that for combined gpio/pinctrl drivers it doesn't make sense to take
> this additional loop through the pinctrl core, regardless of whatever locking mechanism
> is used. I actually think that it obfuscates those drivers a bit, but that might just be
> me.


-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 2/3] pinctrl-bcm2835: don't call pinctrl_gpio_direction()

[Linus Walleij]

On Mon, Dec 6, 2021 at 2:16 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:

> Set the direction directly without calling pinctrl_gpio_direction().
> This avoids the mutex_lock() calls in that function, which would
> invalid the can_sleep = false.
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Patch applied.

Yours,
Linus Walleij

Re: [RFC PATCH 3/3] pinctrl-sunxi: don't call pinctrl_gpio_direction()

[Linus Walleij]

On Mon, Dec 6, 2021 at 2:16 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:

> Set the direction directly without calling pinctrl_gpio_direction().
> This avoids the mutex_lock() calls in that function, which would
> invalid the can_sleep = false.
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Patch applied.

Yours,
Linus Walleij

[PATCH for 5.17] pinctrl-sunxi: sunxi_pinctrl_gpio_direction_in/output: use correct offset

[Hans Verkuil]

The commit that sets the direction directly without calling
pinctrl_gpio_direction(), forgot to add chip->base to the offset when
calling sunxi_pmx_gpio_set_direction().

This caused failures for various Allwinner boards which have two
GPIO blocks.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reported-by: 5kft <5kft@5kft.org>
Suggested-by: 5kft <5kft@5kft.org>
Reported-by: Corentin Labbe <clabbe.montjoie@gmail.com>
Fixes: 8df89a7cbc63 (pinctrl-sunxi: don't call pinctrl_gpio_direction())
---
Corentin, can you please test this patch to verify that this fixes your
issue on the orangepiPC?
---
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index 80d6750c74a6..061323eab8b1 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -837,7 +837,8 @@ static int sunxi_pinctrl_gpio_direction_input(struct gpio_chip *chip,
 {
 	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);

-	return sunxi_pmx_gpio_set_direction(pctl->pctl_dev, NULL, offset, true);
+	return sunxi_pmx_gpio_set_direction(pctl->pctl_dev, NULL,
+					    chip->base + offset, true);
 }

 static int sunxi_pinctrl_gpio_get(struct gpio_chip *chip, unsigned offset)
@@ -890,7 +891,8 @@ static int sunxi_pinctrl_gpio_direction_output(struct gpio_chip *chip,
 	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);

 	sunxi_pinctrl_gpio_set(chip, offset, value);
-	return sunxi_pmx_gpio_set_direction(pctl->pctl_dev, NULL, offset, false);
+	return sunxi_pmx_gpio_set_direction(pctl->pctl_dev, NULL,
+					    chip->base + offset, false);
 }

 static int sunxi_pinctrl_gpio_of_xlate(struct gpio_chip *gc,

Re: [PATCH for 5.17] pinctrl-sunxi: sunxi_pinctrl_gpio_direction_in/output: use correct offset

[Corentin Labbe]

Le Wed, Jan 26, 2022 at 12:02:04PM +0100, Hans Verkuil a écrit :
> The commit that sets the direction directly without calling
> pinctrl_gpio_direction(), forgot to add chip->base to the offset when
> calling sunxi_pmx_gpio_set_direction().
> 
> This caused failures for various Allwinner boards which have two
> GPIO blocks.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Reported-by: 5kft <5kft@5kft.org>
> Suggested-by: 5kft <5kft@5kft.org>
> Reported-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> Fixes: 8df89a7cbc63 (pinctrl-sunxi: don't call pinctrl_gpio_direction())
> ---
> Corentin, can you please test this patch to verify that this fixes your
> issue on the orangepiPC?

Hello

Yes, it fixes the issue.
Tested-by: Corentin Labbe <clabbe.montjoie@gmail.com>

Thanks

Re: [PATCH for 5.17] pinctrl-sunxi: sunxi_pinctrl_gpio_direction_in/output: use correct offset

[Jernej Škrabec]

Hi Hans!

Dne sreda, 26. januar 2022 ob 12:02:04 CET je Hans Verkuil napisal(a):
> The commit that sets the direction directly without calling
> pinctrl_gpio_direction(), forgot to add chip->base to the offset when
> calling sunxi_pmx_gpio_set_direction().
> 
> This caused failures for various Allwinner boards which have two
> GPIO blocks.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Reported-by: 5kft <5kft@5kft.org>
> Suggested-by: 5kft <5kft@5kft.org>
> Reported-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> Fixes: 8df89a7cbc63 (pinctrl-sunxi: don't call pinctrl_gpio_direction())
> Tested-by: Corentin Labbe <clabbe.montjoie@gmail.com>

Next time please send to all sunxi maintainers/reviewers and mailing list.

Tested-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

> ---
> Corentin, can you please test this patch to verify that this fixes your
> issue on the orangepiPC?
> ---
> 
> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/
pinctrl-sunxi.c
> index 80d6750c74a6..061323eab8b1 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> @@ -837,7 +837,8 @@ static int sunxi_pinctrl_gpio_direction_input(struct 
gpio_chip *chip,
>  {
>  	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
> 
> -	return sunxi_pmx_gpio_set_direction(pctl->pctl_dev, NULL, offset, 
true);
> +	return sunxi_pmx_gpio_set_direction(pctl->pctl_dev, NULL,
> +					    chip->base + offset, 
true);
>  }
> 
>  static int sunxi_pinctrl_gpio_get(struct gpio_chip *chip, unsigned offset)
> @@ -890,7 +891,8 @@ static int sunxi_pinctrl_gpio_direction_output(struct 
gpio_chip *chip,
>  	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
> 
>  	sunxi_pinctrl_gpio_set(chip, offset, value);
> -	return sunxi_pmx_gpio_set_direction(pctl->pctl_dev, NULL, offset, 
false);
> +	return sunxi_pmx_gpio_set_direction(pctl->pctl_dev, NULL,
> +					    chip->base + offset, 
false);
>  }
> 
>  static int sunxi_pinctrl_gpio_of_xlate(struct gpio_chip *gc,
> 

Re: [PATCH for 5.17] pinctrl-sunxi: sunxi_pinctrl_gpio_direction_in/output: use correct offset

[Guenter Roeck]

On Wed, Jan 26, 2022 at 12:02:04PM +0100, Hans Verkuil wrote:
> The commit that sets the direction directly without calling
> pinctrl_gpio_direction(), forgot to add chip->base to the offset when
> calling sunxi_pmx_gpio_set_direction().
> 
> This caused failures for various Allwinner boards which have two
> GPIO blocks.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Reported-by: 5kft <5kft@5kft.org>
> Suggested-by: 5kft <5kft@5kft.org>
> Reported-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> Fixes: 8df89a7cbc63 (pinctrl-sunxi: don't call pinctrl_gpio_direction())
> Tested-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> Tested-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Tested-by: Guenter Roeck <linux@roeck-us.net>

Guenter

> ---
> Corentin, can you please test this patch to verify that this fixes your
> issue on the orangepiPC?
> ---
> 
> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> index 80d6750c74a6..061323eab8b1 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> @@ -837,7 +837,8 @@ static int sunxi_pinctrl_gpio_direction_input(struct gpio_chip *chip,
>  {
>  	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
> 
> -	return sunxi_pmx_gpio_set_direction(pctl->pctl_dev, NULL, offset, true);
> +	return sunxi_pmx_gpio_set_direction(pctl->pctl_dev, NULL,
> +					    chip->base + offset, true);
>  }
> 
>  static int sunxi_pinctrl_gpio_get(struct gpio_chip *chip, unsigned offset)
> @@ -890,7 +891,8 @@ static int sunxi_pinctrl_gpio_direction_output(struct gpio_chip *chip,
>  	struct sunxi_pinctrl *pctl = gpiochip_get_data(chip);
> 
>  	sunxi_pinctrl_gpio_set(chip, offset, value);
> -	return sunxi_pmx_gpio_set_direction(pctl->pctl_dev, NULL, offset, false);
> +	return sunxi_pmx_gpio_set_direction(pctl->pctl_dev, NULL,
> +					    chip->base + offset, false);
>  }
> 
>  static int sunxi_pinctrl_gpio_of_xlate(struct gpio_chip *gc,

Re: [PATCH for 5.17] pinctrl-sunxi: sunxi_pinctrl_gpio_direction_in/output: use correct offset

[Guenter Roeck]

Hi,

On Wed, Jan 26, 2022 at 12:02:04PM +0100, Hans Verkuil wrote:
> The commit that sets the direction directly without calling
> pinctrl_gpio_direction(), forgot to add chip->base to the offset when
> calling sunxi_pmx_gpio_set_direction().
> 
> This caused failures for various Allwinner boards which have two
> GPIO blocks.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Reported-by: 5kft <5kft@5kft.org>
> Suggested-by: 5kft <5kft@5kft.org>
> Reported-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> Fixes: 8df89a7cbc63 (pinctrl-sunxi: don't call pinctrl_gpio_direction())
> Tested-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> Tested-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> Tested-by: Guenter Roeck <linux@roeck-us.net>

Unfortunately, this patch causes (or exposes) a secondary problem.
When applied, the following traceback is seen during reboot.

Requesting system reboot
[   30.899594]
[   30.899685] ============================================
[   30.899757] WARNING: possible recursive locking detected
[   30.899938] 5.17.0-rc3-00394-gc849047c2473 #1 Not tainted
[   30.900055] --------------------------------------------
[   30.900124] init/307 is trying to acquire lock:
[   30.900246] c2dfe27c (&irq_desc_lock_class){-.-.}-{2:2}, at: __irq_get_desc_lock+0x58/0xa0
[   30.900900]
[   30.900900] but task is already holding lock:
[   30.900974] c3c0ac7c (&irq_desc_lock_class){-.-.}-{2:2}, at: __irq_get_desc_lock+0x58/0xa0
[   30.901101]
[   30.901101] other info that might help us debug this:
[   30.901188]  Possible unsafe locking scenario:
[   30.901188]
[   30.901262]        CPU0
[   30.901301]        ----
[   30.901339]   lock(&irq_desc_lock_class);
[   30.901411]   lock(&irq_desc_lock_class);
[   30.901480]
[   30.901480]  *** DEADLOCK ***
[   30.901480]
[   30.901554]  May be due to missing lock nesting notation
[   30.901554]
[   30.901657] 4 locks held by init/307:
[   30.901724]  #0: c1f29f18 (system_transition_mutex){+.+.}-{3:3}, at: __do_sys_reboot+0x90/0x23c
[   30.901889]  #1: c20f7760 (&dev->mutex){....}-{3:3}, at: device_shutdown+0xf4/0x224
[   30.902016]  #2: c2e804d8 (&dev->mutex){....}-{3:3}, at: device_shutdown+0x104/0x224
[   30.902138]  #3: c3c0ac7c (&irq_desc_lock_class){-.-.}-{2:2}, at: __irq_get_desc_lock+0x58/0xa0
[   30.902281]
[   30.902281] stack backtrace:
[   30.902462] CPU: 0 PID: 307 Comm: init Not tainted 5.17.0-rc3-00394-gc849047c2473 #1
[   30.902572] Hardware name: Allwinner sun8i Family
[   30.902781]  unwind_backtrace from show_stack+0x10/0x14
[   30.902895]  show_stack from dump_stack_lvl+0x68/0x90
[   30.902970]  dump_stack_lvl from __lock_acquire+0x1680/0x31a0
[   30.903047]  __lock_acquire from lock_acquire+0x148/0x3dc
[   30.903118]  lock_acquire from _raw_spin_lock_irqsave+0x50/0x6c
[   30.903197]  _raw_spin_lock_irqsave from __irq_get_desc_lock+0x58/0xa0
[   30.903282]  __irq_get_desc_lock from irq_set_irq_wake+0x2c/0x19c
[   30.903366]  irq_set_irq_wake from irq_set_irq_wake+0x13c/0x19c
[   30.903442]  irq_set_irq_wake from gpio_keys_suspend+0x80/0x1a4
[   30.903523]  gpio_keys_suspend from gpio_keys_shutdown+0x10/0x2c
[   30.903603]  gpio_keys_shutdown from device_shutdown+0x180/0x224
[   30.903685]  device_shutdown from __do_sys_reboot+0x134/0x23c
[   30.903764]  __do_sys_reboot from ret_fast_syscall+0x0/0x1c
[   30.903894] Exception stack(0xc584ffa8 to 0xc584fff0)
[   30.904013] ffa0:                   01234567 000c623f fee1dead 28121969 01234567 00000000
[   30.904117] ffc0: 01234567 000c623f 00000001 00000058 000d85c0 00000000 00000000 00000000
[   30.904213] ffe0: 000d8298 be84ddf4 000918bc b6eb0edc
[   30.905189] reboot: Restarting system
------------

Bisect log is attached.

Guenter

---
# bad: [c849047c2473f78306791b27ec7c3e0ed552727d] Merge branch 'for-linux-next-fixes' of git://anongit.freedesktop.org/drm/drm-misc
# good: [dfd42facf1e4ada021b939b4e19c935dcdd55566] Linux 5.17-rc3
git bisect start 'HEAD' 'v5.17-rc3'
# good: [a0eafda3873b900f2bfa2bac738583493b458338] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git
git bisect good a0eafda3873b900f2bfa2bac738583493b458338
# good: [b7bbfc1f46f45e896928c301cd02fb530ed426f3] Merge branch 'fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git
git bisect good b7bbfc1f46f45e896928c301cd02fb530ed426f3
# bad: [2af1645572f8fef201a7d2a891f328ed94509135] Merge branch 'rtc-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git
git bisect bad 2af1645572f8fef201a7d2a891f328ed94509135
# bad: [e3d76bb86c683b05afe4a3b73fd1d50ea7a294be] Merge branch 'hwmon' of git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
git bisect bad e3d76bb86c683b05afe4a3b73fd1d50ea7a294be
# good: [b55a65e66f178b3507554260b4f3d56bc7b445b6] Merge branch 'fixes' of git://linuxtv.org/mchehab/media-next.git
git bisect good b55a65e66f178b3507554260b4f3d56bc7b445b6
# good: [2b0ecccb55310a4b8ad5d59c703cf8c821be6260] KVM: x86: nSVM: deal with L1 hypervisor that intercepts interrupts but lets L2 control them
git bisect good 2b0ecccb55310a4b8ad5d59c703cf8c821be6260
# good: [fcb732d8f8cf6084f8480015ad41d25fb023a4dd] KVM: x86/xen: Fix runstate updates to be atomic when preempting vCPU
git bisect good fcb732d8f8cf6084f8480015ad41d25fb023a4dd
# bad: [bb9bb9c75482aa008cfc62b5cb88681de8408fa3] hwmon: (ntc_thermistor) Underscore Samsung thermistor
git bisect bad bb9bb9c75482aa008cfc62b5cb88681de8408fa3
# bad: [3c5412cdec9f6e417e7757974040e461df4a7238] pinctrl-sunxi: sunxi_pinctrl_gpio_direction_in/output: use correct offset
git bisect bad 3c5412cdec9f6e417e7757974040e461df4a7238
# first bad commit: [3c5412cdec9f6e417e7757974040e461df4a7238] pinctrl-sunxi: sunxi_pinctrl_gpio_direction_in/output: use correct offset

Re: [PATCH for 5.17] pinctrl-sunxi: sunxi_pinctrl_gpio_direction_in/output: use correct offset

[Hans Verkuil]

Hi Guenter,

On 2/13/22 21:42, Guenter Roeck wrote:
> Hi,
> 
> On Wed, Jan 26, 2022 at 12:02:04PM +0100, Hans Verkuil wrote:
>> The commit that sets the direction directly without calling
>> pinctrl_gpio_direction(), forgot to add chip->base to the offset when
>> calling sunxi_pmx_gpio_set_direction().
>>
>> This caused failures for various Allwinner boards which have two
>> GPIO blocks.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Reported-by: 5kft <5kft@5kft.org>
>> Suggested-by: 5kft <5kft@5kft.org>
>> Reported-by: Corentin Labbe <clabbe.montjoie@gmail.com>
>> Fixes: 8df89a7cbc63 (pinctrl-sunxi: don't call pinctrl_gpio_direction())
>> Tested-by: Corentin Labbe <clabbe.montjoie@gmail.com>
>> Tested-by: Jernej Skrabec <jernej.skrabec@gmail.com>
>> Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>
>> Tested-by: Guenter Roeck <linux@roeck-us.net>
> 
> Unfortunately, this patch causes (or exposes) a secondary problem.
> When applied, the following traceback is seen during reboot.

I've been digging through the code, but I can't really see how this patch would
give this result. The backtrace isn't very informative.

I suspect it ends up in sunxi_pinctrl_irq_set_wake(). Can you debug this a bit?
Esp. logging the d->hwirq value would be useful (and comparing it with and without
this patch). Of course, I may be wrong and the issue isn't in that function at all.

One possibility is that there is another offset missing somewhere in this code
that hasn't been noticed before.

Regards,

	Hans

> 
> Requesting system reboot
> [   30.899594]
> [   30.899685] ============================================
> [   30.899757] WARNING: possible recursive locking detected
> [   30.899938] 5.17.0-rc3-00394-gc849047c2473 #1 Not tainted
> [   30.900055] --------------------------------------------
> [   30.900124] init/307 is trying to acquire lock:
> [   30.900246] c2dfe27c (&irq_desc_lock_class){-.-.}-{2:2}, at: __irq_get_desc_lock+0x58/0xa0
> [   30.900900]
> [   30.900900] but task is already holding lock:
> [   30.900974] c3c0ac7c (&irq_desc_lock_class){-.-.}-{2:2}, at: __irq_get_desc_lock+0x58/0xa0
> [   30.901101]
> [   30.901101] other info that might help us debug this:
> [   30.901188]  Possible unsafe locking scenario:
> [   30.901188]
> [   30.901262]        CPU0
> [   30.901301]        ----
> [   30.901339]   lock(&irq_desc_lock_class);
> [   30.901411]   lock(&irq_desc_lock_class);
> [   30.901480]
> [   30.901480]  *** DEADLOCK ***
> [   30.901480]
> [   30.901554]  May be due to missing lock nesting notation
> [   30.901554]
> [   30.901657] 4 locks held by init/307:
> [   30.901724]  #0: c1f29f18 (system_transition_mutex){+.+.}-{3:3}, at: __do_sys_reboot+0x90/0x23c
> [   30.901889]  #1: c20f7760 (&dev->mutex){....}-{3:3}, at: device_shutdown+0xf4/0x224
> [   30.902016]  #2: c2e804d8 (&dev->mutex){....}-{3:3}, at: device_shutdown+0x104/0x224
> [   30.902138]  #3: c3c0ac7c (&irq_desc_lock_class){-.-.}-{2:2}, at: __irq_get_desc_lock+0x58/0xa0
> [   30.902281]
> [   30.902281] stack backtrace:
> [   30.902462] CPU: 0 PID: 307 Comm: init Not tainted 5.17.0-rc3-00394-gc849047c2473 #1
> [   30.902572] Hardware name: Allwinner sun8i Family
> [   30.902781]  unwind_backtrace from show_stack+0x10/0x14
> [   30.902895]  show_stack from dump_stack_lvl+0x68/0x90
> [   30.902970]  dump_stack_lvl from __lock_acquire+0x1680/0x31a0
> [   30.903047]  __lock_acquire from lock_acquire+0x148/0x3dc
> [   30.903118]  lock_acquire from _raw_spin_lock_irqsave+0x50/0x6c
> [   30.903197]  _raw_spin_lock_irqsave from __irq_get_desc_lock+0x58/0xa0
> [   30.903282]  __irq_get_desc_lock from irq_set_irq_wake+0x2c/0x19c
> [   30.903366]  irq_set_irq_wake from irq_set_irq_wake+0x13c/0x19c
> [   30.903442]  irq_set_irq_wake from gpio_keys_suspend+0x80/0x1a4
> [   30.903523]  gpio_keys_suspend from gpio_keys_shutdown+0x10/0x2c
> [   30.903603]  gpio_keys_shutdown from device_shutdown+0x180/0x224
> [   30.903685]  device_shutdown from __do_sys_reboot+0x134/0x23c
> [   30.903764]  __do_sys_reboot from ret_fast_syscall+0x0/0x1c
> [   30.903894] Exception stack(0xc584ffa8 to 0xc584fff0)
> [   30.904013] ffa0:                   01234567 000c623f fee1dead 28121969 01234567 00000000
> [   30.904117] ffc0: 01234567 000c623f 00000001 00000058 000d85c0 00000000 00000000 00000000
> [   30.904213] ffe0: 000d8298 be84ddf4 000918bc b6eb0edc
> [   30.905189] reboot: Restarting system
> ------------
> 
> Bisect log is attached.
> 
> Guenter
> 
> ---
> # bad: [c849047c2473f78306791b27ec7c3e0ed552727d] Merge branch 'for-linux-next-fixes' of git://anongit.freedesktop.org/drm/drm-misc
> # good: [dfd42facf1e4ada021b939b4e19c935dcdd55566] Linux 5.17-rc3
> git bisect start 'HEAD' 'v5.17-rc3'
> # good: [a0eafda3873b900f2bfa2bac738583493b458338] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git
> git bisect good a0eafda3873b900f2bfa2bac738583493b458338
> # good: [b7bbfc1f46f45e896928c301cd02fb530ed426f3] Merge branch 'fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git
> git bisect good b7bbfc1f46f45e896928c301cd02fb530ed426f3
> # bad: [2af1645572f8fef201a7d2a891f328ed94509135] Merge branch 'rtc-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git
> git bisect bad 2af1645572f8fef201a7d2a891f328ed94509135
> # bad: [e3d76bb86c683b05afe4a3b73fd1d50ea7a294be] Merge branch 'hwmon' of git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
> git bisect bad e3d76bb86c683b05afe4a3b73fd1d50ea7a294be
> # good: [b55a65e66f178b3507554260b4f3d56bc7b445b6] Merge branch 'fixes' of git://linuxtv.org/mchehab/media-next.git
> git bisect good b55a65e66f178b3507554260b4f3d56bc7b445b6
> # good: [2b0ecccb55310a4b8ad5d59c703cf8c821be6260] KVM: x86: nSVM: deal with L1 hypervisor that intercepts interrupts but lets L2 control them
> git bisect good 2b0ecccb55310a4b8ad5d59c703cf8c821be6260
> # good: [fcb732d8f8cf6084f8480015ad41d25fb023a4dd] KVM: x86/xen: Fix runstate updates to be atomic when preempting vCPU
> git bisect good fcb732d8f8cf6084f8480015ad41d25fb023a4dd
> # bad: [bb9bb9c75482aa008cfc62b5cb88681de8408fa3] hwmon: (ntc_thermistor) Underscore Samsung thermistor
> git bisect bad bb9bb9c75482aa008cfc62b5cb88681de8408fa3
> # bad: [3c5412cdec9f6e417e7757974040e461df4a7238] pinctrl-sunxi: sunxi_pinctrl_gpio_direction_in/output: use correct offset
> git bisect bad 3c5412cdec9f6e417e7757974040e461df4a7238
> # first bad commit: [3c5412cdec9f6e417e7757974040e461df4a7238] pinctrl-sunxi: sunxi_pinctrl_gpio_direction_in/output: use correct offset

Re: [PATCH for 5.17] pinctrl-sunxi: sunxi_pinctrl_gpio_direction_in/output: use correct offset

[Guenter Roeck]

On 2/14/22 00:44, Hans Verkuil wrote:
> Hi Guenter,
> 
> On 2/13/22 21:42, Guenter Roeck wrote:
>> Hi,
>>
>> On Wed, Jan 26, 2022 at 12:02:04PM +0100, Hans Verkuil wrote:
>>> The commit that sets the direction directly without calling
>>> pinctrl_gpio_direction(), forgot to add chip->base to the offset when
>>> calling sunxi_pmx_gpio_set_direction().
>>>
>>> This caused failures for various Allwinner boards which have two
>>> GPIO blocks.
>>>
>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>> Reported-by: 5kft <5kft@5kft.org>
>>> Suggested-by: 5kft <5kft@5kft.org>
>>> Reported-by: Corentin Labbe <clabbe.montjoie@gmail.com>
>>> Fixes: 8df89a7cbc63 (pinctrl-sunxi: don't call pinctrl_gpio_direction())
>>> Tested-by: Corentin Labbe <clabbe.montjoie@gmail.com>
>>> Tested-by: Jernej Skrabec <jernej.skrabec@gmail.com>
>>> Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>
>>> Tested-by: Guenter Roeck <linux@roeck-us.net>
>>
>> Unfortunately, this patch causes (or exposes) a secondary problem.
>> When applied, the following traceback is seen during reboot.
> 
> I've been digging through the code, but I can't really see how this patch would
> give this result. The backtrace isn't very informative.
> 
> I suspect it ends up in sunxi_pinctrl_irq_set_wake(). Can you debug this a bit?
> Esp. logging the d->hwirq value would be useful (and comparing it with and without
> this patch). Of course, I may be wrong and the issue isn't in that function at all.
> 
> One possibility is that there is another offset missing somewhere in this code
> that hasn't been noticed before.
> 

Initial finding is that your second patch doesn't introduce the problem. it appears
that it uncovers a problem that was hidden by commit 8df89a7cbc ("pinctrl-sunxi:
don't call pinctrl_gpio_direction()"). If I revert both this patch and commit
8df89a7cbc in linux-next, I also see the traceback. I do not see the traceback
in v5.16. I do see the traceback in v5.17-rc4 after applying this patch.

I am currently trying to bisect between v5.16 and v5.17-rc4.

Guenter

> Regards,
> 
> 	Hans
> 
>>
>> Requesting system reboot
>> [   30.899594]
>> [   30.899685] ============================================
>> [   30.899757] WARNING: possible recursive locking detected
>> [   30.899938] 5.17.0-rc3-00394-gc849047c2473 #1 Not tainted
>> [   30.900055] --------------------------------------------
>> [   30.900124] init/307 is trying to acquire lock:
>> [   30.900246] c2dfe27c (&irq_desc_lock_class){-.-.}-{2:2}, at: __irq_get_desc_lock+0x58/0xa0
>> [   30.900900]
>> [   30.900900] but task is already holding lock:
>> [   30.900974] c3c0ac7c (&irq_desc_lock_class){-.-.}-{2:2}, at: __irq_get_desc_lock+0x58/0xa0
>> [   30.901101]
>> [   30.901101] other info that might help us debug this:
>> [   30.901188]  Possible unsafe locking scenario:
>> [   30.901188]
>> [   30.901262]        CPU0
>> [   30.901301]        ----
>> [   30.901339]   lock(&irq_desc_lock_class);
>> [   30.901411]   lock(&irq_desc_lock_class);
>> [   30.901480]
>> [   30.901480]  *** DEADLOCK ***
>> [   30.901480]
>> [   30.901554]  May be due to missing lock nesting notation
>> [   30.901554]
>> [   30.901657] 4 locks held by init/307:
>> [   30.901724]  #0: c1f29f18 (system_transition_mutex){+.+.}-{3:3}, at: __do_sys_reboot+0x90/0x23c
>> [   30.901889]  #1: c20f7760 (&dev->mutex){....}-{3:3}, at: device_shutdown+0xf4/0x224
>> [   30.902016]  #2: c2e804d8 (&dev->mutex){....}-{3:3}, at: device_shutdown+0x104/0x224
>> [   30.902138]  #3: c3c0ac7c (&irq_desc_lock_class){-.-.}-{2:2}, at: __irq_get_desc_lock+0x58/0xa0
>> [   30.902281]
>> [   30.902281] stack backtrace:
>> [   30.902462] CPU: 0 PID: 307 Comm: init Not tainted 5.17.0-rc3-00394-gc849047c2473 #1
>> [   30.902572] Hardware name: Allwinner sun8i Family
>> [   30.902781]  unwind_backtrace from show_stack+0x10/0x14
>> [   30.902895]  show_stack from dump_stack_lvl+0x68/0x90
>> [   30.902970]  dump_stack_lvl from __lock_acquire+0x1680/0x31a0
>> [   30.903047]  __lock_acquire from lock_acquire+0x148/0x3dc
>> [   30.903118]  lock_acquire from _raw_spin_lock_irqsave+0x50/0x6c
>> [   30.903197]  _raw_spin_lock_irqsave from __irq_get_desc_lock+0x58/0xa0
>> [   30.903282]  __irq_get_desc_lock from irq_set_irq_wake+0x2c/0x19c
>> [   30.903366]  irq_set_irq_wake from irq_set_irq_wake+0x13c/0x19c
>> [   30.903442]  irq_set_irq_wake from gpio_keys_suspend+0x80/0x1a4
>> [   30.903523]  gpio_keys_suspend from gpio_keys_shutdown+0x10/0x2c
>> [   30.903603]  gpio_keys_shutdown from device_shutdown+0x180/0x224
>> [   30.903685]  device_shutdown from __do_sys_reboot+0x134/0x23c
>> [   30.903764]  __do_sys_reboot from ret_fast_syscall+0x0/0x1c
>> [   30.903894] Exception stack(0xc584ffa8 to 0xc584fff0)
>> [   30.904013] ffa0:                   01234567 000c623f fee1dead 28121969 01234567 00000000
>> [   30.904117] ffc0: 01234567 000c623f 00000001 00000058 000d85c0 00000000 00000000 00000000
>> [   30.904213] ffe0: 000d8298 be84ddf4 000918bc b6eb0edc
>> [   30.905189] reboot: Restarting system
>> ------------
>>
>> Bisect log is attached.
>>
>> Guenter
>>
>> ---
>> # bad: [c849047c2473f78306791b27ec7c3e0ed552727d] Merge branch 'for-linux-next-fixes' of git://anongit.freedesktop.org/drm/drm-misc
>> # good: [dfd42facf1e4ada021b939b4e19c935dcdd55566] Linux 5.17-rc3
>> git bisect start 'HEAD' 'v5.17-rc3'
>> # good: [a0eafda3873b900f2bfa2bac738583493b458338] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git
>> git bisect good a0eafda3873b900f2bfa2bac738583493b458338
>> # good: [b7bbfc1f46f45e896928c301cd02fb530ed426f3] Merge branch 'fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git
>> git bisect good b7bbfc1f46f45e896928c301cd02fb530ed426f3
>> # bad: [2af1645572f8fef201a7d2a891f328ed94509135] Merge branch 'rtc-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git
>> git bisect bad 2af1645572f8fef201a7d2a891f328ed94509135
>> # bad: [e3d76bb86c683b05afe4a3b73fd1d50ea7a294be] Merge branch 'hwmon' of git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
>> git bisect bad e3d76bb86c683b05afe4a3b73fd1d50ea7a294be
>> # good: [b55a65e66f178b3507554260b4f3d56bc7b445b6] Merge branch 'fixes' of git://linuxtv.org/mchehab/media-next.git
>> git bisect good b55a65e66f178b3507554260b4f3d56bc7b445b6
>> # good: [2b0ecccb55310a4b8ad5d59c703cf8c821be6260] KVM: x86: nSVM: deal with L1 hypervisor that intercepts interrupts but lets L2 control them
>> git bisect good 2b0ecccb55310a4b8ad5d59c703cf8c821be6260
>> # good: [fcb732d8f8cf6084f8480015ad41d25fb023a4dd] KVM: x86/xen: Fix runstate updates to be atomic when preempting vCPU
>> git bisect good fcb732d8f8cf6084f8480015ad41d25fb023a4dd
>> # bad: [bb9bb9c75482aa008cfc62b5cb88681de8408fa3] hwmon: (ntc_thermistor) Underscore Samsung thermistor
>> git bisect bad bb9bb9c75482aa008cfc62b5cb88681de8408fa3
>> # bad: [3c5412cdec9f6e417e7757974040e461df4a7238] pinctrl-sunxi: sunxi_pinctrl_gpio_direction_in/output: use correct offset
>> git bisect bad 3c5412cdec9f6e417e7757974040e461df4a7238
>> # first bad commit: [3c5412cdec9f6e417e7757974040e461df4a7238] pinctrl-sunxi: sunxi_pinctrl_gpio_direction_in/output: use correct offset

Re: [PATCH for 5.17] pinctrl-sunxi: sunxi_pinctrl_gpio_direction_in/output: use correct offset

[Guenter Roeck]

On 2/14/22 00:44, Hans Verkuil wrote:
> Hi Guenter,
> 
> On 2/13/22 21:42, Guenter Roeck wrote:
>> Hi,
>>
>> On Wed, Jan 26, 2022 at 12:02:04PM +0100, Hans Verkuil wrote:
>>> The commit that sets the direction directly without calling
>>> pinctrl_gpio_direction(), forgot to add chip->base to the offset when
>>> calling sunxi_pmx_gpio_set_direction().
>>>
>>> This caused failures for various Allwinner boards which have two
>>> GPIO blocks.
>>>
>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>> Reported-by: 5kft <5kft@5kft.org>
>>> Suggested-by: 5kft <5kft@5kft.org>
>>> Reported-by: Corentin Labbe <clabbe.montjoie@gmail.com>
>>> Fixes: 8df89a7cbc63 (pinctrl-sunxi: don't call pinctrl_gpio_direction())
>>> Tested-by: Corentin Labbe <clabbe.montjoie@gmail.com>
>>> Tested-by: Jernej Skrabec <jernej.skrabec@gmail.com>
>>> Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>
>>> Tested-by: Guenter Roeck <linux@roeck-us.net>
>>
>> Unfortunately, this patch causes (or exposes) a secondary problem.
>> When applied, the following traceback is seen during reboot.
> 
> I've been digging through the code, but I can't really see how this patch would
> give this result. The backtrace isn't very informative.
> 
> I suspect it ends up in sunxi_pinctrl_irq_set_wake(). Can you debug this a bit?
> Esp. logging the d->hwirq value would be useful (and comparing it with and without
> this patch). Of course, I may be wrong and the issue isn't in that function at all.
> 
> One possibility is that there is another offset missing somewhere in this code
> that hasn't been noticed before.
> 

Turns out the real 'culprit' is commit 145988cff2a1 ("ARM: dts: sun8i: Adjust power
key nodes") which introduces wakeup-source for orangepi-pc and with it the call to
sunxi_pinctrl_irq_set_wake and the warning traceback.

Guenter

> Regards,
> 
> 	Hans
> 
>>
>> Requesting system reboot
>> [   30.899594]
>> [   30.899685] ============================================
>> [   30.899757] WARNING: possible recursive locking detected
>> [   30.899938] 5.17.0-rc3-00394-gc849047c2473 #1 Not tainted
>> [   30.900055] --------------------------------------------
>> [   30.900124] init/307 is trying to acquire lock:
>> [   30.900246] c2dfe27c (&irq_desc_lock_class){-.-.}-{2:2}, at: __irq_get_desc_lock+0x58/0xa0
>> [   30.900900]
>> [   30.900900] but task is already holding lock:
>> [   30.900974] c3c0ac7c (&irq_desc_lock_class){-.-.}-{2:2}, at: __irq_get_desc_lock+0x58/0xa0
>> [   30.901101]
>> [   30.901101] other info that might help us debug this:
>> [   30.901188]  Possible unsafe locking scenario:
>> [   30.901188]
>> [   30.901262]        CPU0
>> [   30.901301]        ----
>> [   30.901339]   lock(&irq_desc_lock_class);
>> [   30.901411]   lock(&irq_desc_lock_class);
>> [   30.901480]
>> [   30.901480]  *** DEADLOCK ***
>> [   30.901480]
>> [   30.901554]  May be due to missing lock nesting notation
>> [   30.901554]
>> [   30.901657] 4 locks held by init/307:
>> [   30.901724]  #0: c1f29f18 (system_transition_mutex){+.+.}-{3:3}, at: __do_sys_reboot+0x90/0x23c
>> [   30.901889]  #1: c20f7760 (&dev->mutex){....}-{3:3}, at: device_shutdown+0xf4/0x224
>> [   30.902016]  #2: c2e804d8 (&dev->mutex){....}-{3:3}, at: device_shutdown+0x104/0x224
>> [   30.902138]  #3: c3c0ac7c (&irq_desc_lock_class){-.-.}-{2:2}, at: __irq_get_desc_lock+0x58/0xa0
>> [   30.902281]
>> [   30.902281] stack backtrace:
>> [   30.902462] CPU: 0 PID: 307 Comm: init Not tainted 5.17.0-rc3-00394-gc849047c2473 #1
>> [   30.902572] Hardware name: Allwinner sun8i Family
>> [   30.902781]  unwind_backtrace from show_stack+0x10/0x14
>> [   30.902895]  show_stack from dump_stack_lvl+0x68/0x90
>> [   30.902970]  dump_stack_lvl from __lock_acquire+0x1680/0x31a0
>> [   30.903047]  __lock_acquire from lock_acquire+0x148/0x3dc
>> [   30.903118]  lock_acquire from _raw_spin_lock_irqsave+0x50/0x6c
>> [   30.903197]  _raw_spin_lock_irqsave from __irq_get_desc_lock+0x58/0xa0
>> [   30.903282]  __irq_get_desc_lock from irq_set_irq_wake+0x2c/0x19c
>> [   30.903366]  irq_set_irq_wake from irq_set_irq_wake+0x13c/0x19c
>> [   30.903442]  irq_set_irq_wake from gpio_keys_suspend+0x80/0x1a4
>> [   30.903523]  gpio_keys_suspend from gpio_keys_shutdown+0x10/0x2c
>> [   30.903603]  gpio_keys_shutdown from device_shutdown+0x180/0x224
>> [   30.903685]  device_shutdown from __do_sys_reboot+0x134/0x23c
>> [   30.903764]  __do_sys_reboot from ret_fast_syscall+0x0/0x1c
>> [   30.903894] Exception stack(0xc584ffa8 to 0xc584fff0)
>> [   30.904013] ffa0:                   01234567 000c623f fee1dead 28121969 01234567 00000000
>> [   30.904117] ffc0: 01234567 000c623f 00000001 00000058 000d85c0 00000000 00000000 00000000
>> [   30.904213] ffe0: 000d8298 be84ddf4 000918bc b6eb0edc
>> [   30.905189] reboot: Restarting system
>> ------------
>>
>> Bisect log is attached.
>>
>> Guenter
>>
>> ---
>> # bad: [c849047c2473f78306791b27ec7c3e0ed552727d] Merge branch 'for-linux-next-fixes' of git://anongit.freedesktop.org/drm/drm-misc
>> # good: [dfd42facf1e4ada021b939b4e19c935dcdd55566] Linux 5.17-rc3
>> git bisect start 'HEAD' 'v5.17-rc3'
>> # good: [a0eafda3873b900f2bfa2bac738583493b458338] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git
>> git bisect good a0eafda3873b900f2bfa2bac738583493b458338
>> # good: [b7bbfc1f46f45e896928c301cd02fb530ed426f3] Merge branch 'fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git
>> git bisect good b7bbfc1f46f45e896928c301cd02fb530ed426f3
>> # bad: [2af1645572f8fef201a7d2a891f328ed94509135] Merge branch 'rtc-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git
>> git bisect bad 2af1645572f8fef201a7d2a891f328ed94509135
>> # bad: [e3d76bb86c683b05afe4a3b73fd1d50ea7a294be] Merge branch 'hwmon' of git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
>> git bisect bad e3d76bb86c683b05afe4a3b73fd1d50ea7a294be
>> # good: [b55a65e66f178b3507554260b4f3d56bc7b445b6] Merge branch 'fixes' of git://linuxtv.org/mchehab/media-next.git
>> git bisect good b55a65e66f178b3507554260b4f3d56bc7b445b6
>> # good: [2b0ecccb55310a4b8ad5d59c703cf8c821be6260] KVM: x86: nSVM: deal with L1 hypervisor that intercepts interrupts but lets L2 control them
>> git bisect good 2b0ecccb55310a4b8ad5d59c703cf8c821be6260
>> # good: [fcb732d8f8cf6084f8480015ad41d25fb023a4dd] KVM: x86/xen: Fix runstate updates to be atomic when preempting vCPU
>> git bisect good fcb732d8f8cf6084f8480015ad41d25fb023a4dd
>> # bad: [bb9bb9c75482aa008cfc62b5cb88681de8408fa3] hwmon: (ntc_thermistor) Underscore Samsung thermistor
>> git bisect bad bb9bb9c75482aa008cfc62b5cb88681de8408fa3
>> # bad: [3c5412cdec9f6e417e7757974040e461df4a7238] pinctrl-sunxi: sunxi_pinctrl_gpio_direction_in/output: use correct offset
>> git bisect bad 3c5412cdec9f6e417e7757974040e461df4a7238
>> # first bad commit: [3c5412cdec9f6e417e7757974040e461df4a7238] pinctrl-sunxi: sunxi_pinctrl_gpio_direction_in/output: use correct offset