| Message ID | 20211206131648.1521868-1-hverkuil-cisco@xs4all.nl |
|---|---|
| Headers | show |
| Series | pinctrl: can_sleep and pinctrl_gpio_direction | expand |
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
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.
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
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, >