| Message ID | 20210419052136.2475373-1-tzungbi@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RESEND] pinctrl: mediatek: use spin lock in mtk_rmw | expand |
On Mon, Apr 19, 2021 at 8:32 AM Tzung-Bi Shih <tzungbi@google.com> wrote: > > Commit 42a46434e9b1 ("pinctrl: add lock in mtk_rmw function.") uses > mutex lock in mtk_rmw. However the function is possible called from > atomic context. > > For example call trace: Please, shrink it to the necessary minimum. > mutex_lock+0x28/0x64 > mtk_rmw+0x38/0x80 > mtk_hw_set_value+0x100/0x138 > mtk_gpio_set+0x48/0x58 > gpiod_set_raw_value_commit+0xf4/0x110 > gpiod_set_value_nocheck+0x4c/0x80 > gpiod_set_value+0x4c/0x6c > max98357a_daiops_trigger+0x8c/0x9c > soc_pcm_trigger+0x5c/0x10c > > The max98357a_daiops_trigger() could run in either atomic or non-atomic > context. As a result, dmesg shows some similar messages: "BUG: sleeping > function called from invalid context at kernel/locking/mutex.c:254". > > Uses spin lock in mtk_rmw instead. It's nice, but now the question is, can you have this function be called in a non-IRQ context concurrently with IRQ accessing registers?
On Mon, Apr 19, 2021 at 4:31 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Apr 19, 2021 at 8:32 AM Tzung-Bi Shih <tzungbi@google.com> wrote: > > For example call trace: > > Please, shrink it to the necessary minimum. I'll snip the stack a bit in the next version. > > Uses spin lock in mtk_rmw instead. > > It's nice, but now the question is, can you have this function be > called in a non-IRQ context concurrently with IRQ accessing registers? Fixed in v2. See https://patchwork.ozlabs.org/project/linux-gpio/list/?series=239787 .
diff --git a/drivers/pinctrl/mediatek/pinctrl-moore.c b/drivers/pinctrl/mediatek/pinctrl-moore.c index f77921957f15..3a4a23c40a71 100644 --- a/drivers/pinctrl/mediatek/pinctrl-moore.c +++ b/drivers/pinctrl/mediatek/pinctrl-moore.c @@ -619,7 +619,7 @@ int mtk_moore_pinctrl_probe(struct platform_device *pdev, hw->nbase = hw->soc->nbase_names; - mutex_init(&hw->lock); + spin_lock_init(&hw->lock); /* Copy from internal struct mtk_pin_desc to register to the core */ pins = devm_kmalloc_array(&pdev->dev, hw->soc->npins, sizeof(*pins), diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c index fcf7c3eeee4a..9ffe01a8ceca 100644 --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c @@ -58,14 +58,14 @@ void mtk_rmw(struct mtk_pinctrl *pctl, u8 i, u32 reg, u32 mask, u32 set) { u32 val; - mutex_lock(&pctl->lock); + spin_lock(&pctl->lock); val = mtk_r32(pctl, i, reg); val &= ~mask; val |= set; mtk_w32(pctl, i, reg, val); - mutex_unlock(&pctl->lock); + spin_unlock(&pctl->lock); } static int mtk_hw_pin_field_lookup(struct mtk_pinctrl *hw, diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.h b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.h index 65eac708a3b3..13836bb188b7 100644 --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.h +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.h @@ -252,7 +252,7 @@ struct mtk_pinctrl { struct mtk_pinctrl_group *groups; const char **grp_names; /* lock pin's register resource to avoid multiple threads issue*/ - struct mutex lock; + spinlock_t lock; }; void mtk_rmw(struct mtk_pinctrl *pctl, u8 i, u32 reg, u32 mask, u32 set); diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c index 48e823f6d293..85db2e4377f0 100644 --- a/drivers/pinctrl/mediatek/pinctrl-paris.c +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c @@ -970,7 +970,7 @@ int mtk_paris_pinctrl_probe(struct platform_device *pdev, hw->nbase = hw->soc->nbase_names; - mutex_init(&hw->lock); + spin_lock_init(&hw->lock); err = mtk_pctrl_build_state(pdev); if (err) {
Commit 42a46434e9b1 ("pinctrl: add lock in mtk_rmw function.") uses mutex lock in mtk_rmw. However the function is possible called from atomic context. For example call trace: mutex_lock+0x28/0x64 mtk_rmw+0x38/0x80 mtk_hw_set_value+0x100/0x138 mtk_gpio_set+0x48/0x58 gpiod_set_raw_value_commit+0xf4/0x110 gpiod_set_value_nocheck+0x4c/0x80 gpiod_set_value+0x4c/0x6c max98357a_daiops_trigger+0x8c/0x9c soc_pcm_trigger+0x5c/0x10c The max98357a_daiops_trigger() could run in either atomic or non-atomic context. As a result, dmesg shows some similar messages: "BUG: sleeping function called from invalid context at kernel/locking/mutex.c:254". Uses spin lock in mtk_rmw instead. Fixes: 42a46434e9b1 ("pinctrl: add lock in mtk_rmw function.") Signed-off-by: Tzung-Bi Shih <tzungbi@google.com> --- drivers/pinctrl/mediatek/pinctrl-moore.c | 2 +- drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c | 4 ++-- drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.h | 2 +- drivers/pinctrl/mediatek/pinctrl-paris.c | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-)