Hello Bartosz Golaszewski,
The patch 65a828bab158: "gpiolib: use a mutex to protect the list of
GPIO devices" from Dec 15, 2023 (linux-next), leads to the following
Smatch static checker warning:
drivers/net/wireless/ath/ath9k/hw.c:2836 ath9k_hw_gpio_get()
warn: sleeping in atomic context
drivers/net/wireless/ath/ath9k/hw.c
2826 val = MS_REG_READ(AR9285, gpio);
2827 else if (AR_SREV_9280(ah))
2828 val = MS_REG_READ(AR928X, gpio);
2829 else if (AR_DEVID_7010(ah))
2830 val = REG_READ(ah, AR7010_GPIO_IN) & BIT(gpio);
2831 else if (AR_SREV_9300_20_OR_LATER(ah))
2832 val = REG_READ(ah, AR_GPIO_IN(ah)) & BIT(gpio);
2833 else
2834 val = MS_REG_READ(AR, gpio);
2835 } else if (BIT(gpio) & ah->caps.gpio_requested) {
--> 2836 val = gpio_get_value(gpio) & BIT(gpio);
^^^^^^^^^^^^^^
2837 } else {
2838 WARN_ON(1);
2839 }
2840
2841 return !!val;
2842 }
Before gpio_get_value() took a spinlock but now it takes a mutex
(actually a rw semaphor now). The call tree where we are in atomic
context is:
ath_btcoex_period_timer() <- disables preempt
-> ath_detect_bt_priority()
-> ath9k_hw_gpio_get()
Another warning this change causes is:
drivers/input/keyboard/matrix_keypad.c:95 enable_row_irqs() warn: sleeping in atomic context
matrix_keypad_scan() <- disables preempt
-> enable_row_irqs()
drivers/input/keyboard/matrix_keypad.c:108 disable_row_irqs() warn: sleeping in atomic context
matrix_keypad_interrupt() <- disables preempt
-> disable_row_irqs()
regards,
dan carpenter
On Mon, 15 Jan 2024 at 09:50, Dan Carpenter <[email protected]> wrote:
>
> Hello Bartosz Golaszewski,
>
> The patch 65a828bab158: "gpiolib: use a mutex to protect the list of
> GPIO devices" from Dec 15, 2023 (linux-next), leads to the following
> Smatch static checker warning:
>
> drivers/net/wireless/ath/ath9k/hw.c:2836 ath9k_hw_gpio_get()
> warn: sleeping in atomic context
>
> drivers/net/wireless/ath/ath9k/hw.c
> 2826 val = MS_REG_READ(AR9285, gpio);
> 2827 else if (AR_SREV_9280(ah))
> 2828 val = MS_REG_READ(AR928X, gpio);
> 2829 else if (AR_DEVID_7010(ah))
> 2830 val = REG_READ(ah, AR7010_GPIO_IN) & BIT(gpio);
> 2831 else if (AR_SREV_9300_20_OR_LATER(ah))
> 2832 val = REG_READ(ah, AR_GPIO_IN(ah)) & BIT(gpio);
> 2833 else
> 2834 val = MS_REG_READ(AR, gpio);
> 2835 } else if (BIT(gpio) & ah->caps.gpio_requested) {
> --> 2836 val = gpio_get_value(gpio) & BIT(gpio);
> ^^^^^^^^^^^^^^
>
> 2837 } else {
> 2838 WARN_ON(1);
> 2839 }
> 2840
> 2841 return !!val;
> 2842 }
>
> Before gpio_get_value() took a spinlock but now it takes a mutex
> (actually a rw semaphor now). The call tree where we are in atomic
> context is:
>
> ath_btcoex_period_timer() <- disables preempt
> -> ath_detect_bt_priority()
> -> ath9k_hw_gpio_get()
>
> Another warning this change causes is:
>
> drivers/input/keyboard/matrix_keypad.c:95 enable_row_irqs() warn: sleeping in atomic context
> matrix_keypad_scan() <- disables preempt
> -> enable_row_irqs()
>
> drivers/input/keyboard/matrix_keypad.c:108 disable_row_irqs() warn: sleeping in atomic context
> matrix_keypad_interrupt() <- disables preempt
> -> disable_row_irqs()
>
> regards,
> dan carpenter
Ah, the legacy API is at it again. Every legacy gpio_get/set_value()
call results in calling gpio_to_desc() after which a chip lookup
follows, taking the sleeping lock). Ugh, I will have to revert this. I
think that the only way forward will be an SCRU protected list.
That is what 15 years of technical debt looks like sadly. :(
Bartosz
On Mon, Jan 15, 2024 at 11:50:52AM +0300, Dan Carpenter wrote:
> Hello Bartosz Golaszewski,
>
> The patch 65a828bab158: "gpiolib: use a mutex to protect the list of
> GPIO devices" from Dec 15, 2023 (linux-next), leads to the following
> Smatch static checker warning:
>
> drivers/net/wireless/ath/ath9k/hw.c:2836 ath9k_hw_gpio_get()
> warn: sleeping in atomic context
>
> drivers/net/wireless/ath/ath9k/hw.c
> 2826 val = MS_REG_READ(AR9285, gpio);
> 2827 else if (AR_SREV_9280(ah))
> 2828 val = MS_REG_READ(AR928X, gpio);
> 2829 else if (AR_DEVID_7010(ah))
> 2830 val = REG_READ(ah, AR7010_GPIO_IN) & BIT(gpio);
> 2831 else if (AR_SREV_9300_20_OR_LATER(ah))
> 2832 val = REG_READ(ah, AR_GPIO_IN(ah)) & BIT(gpio);
> 2833 else
> 2834 val = MS_REG_READ(AR, gpio);
> 2835 } else if (BIT(gpio) & ah->caps.gpio_requested) {
> --> 2836 val = gpio_get_value(gpio) & BIT(gpio);
> ^^^^^^^^^^^^^^
>
> 2837 } else {
> 2838 WARN_ON(1);
> 2839 }
> 2840
> 2841 return !!val;
> 2842 }
>
> Before gpio_get_value() took a spinlock but now it takes a mutex
> (actually a rw semaphor now). The call tree where we are in atomic
> context is:
>
> ath_btcoex_period_timer() <- disables preempt
> -> ath_detect_bt_priority()
> -> ath9k_hw_gpio_get()
>
> Another warning this change causes is:
>
> drivers/input/keyboard/matrix_keypad.c:95 enable_row_irqs() warn: sleeping in atomic context
> matrix_keypad_scan() <- disables preempt
> -> enable_row_irqs()
>
> drivers/input/keyboard/matrix_keypad.c:108 disable_row_irqs() warn: sleeping in atomic context
> matrix_keypad_interrupt() <- disables preempt
> -> disable_row_irqs()
Now that that operation becomes heavier we should convert row GPIOs into
interrupt numbers in probe() and then this issue will go away.
Thanks.
--
Dmitry