Re: [PATCH 1/8] gpiolib: check the return value of gpio_chip::get_direction()

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Wed, Feb 19, 2025 at 10:14 AM Marek Szyprowski
<m.szyprowski@xxxxxxxxxxx> wrote:
>
> On 19.02.2025 09:50, Bartosz Golaszewski wrote:
> > On Wed, Feb 19, 2025 at 9:38 AM Marek Szyprowski
> > <m.szyprowski@xxxxxxxxxxx> wrote:
> >> On 10.02.2025 11:51, Bartosz Golaszewski wrote:
> >>> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >>>
> >>> As per the API contract - gpio_chip::get_direction() may fail and return
> >>> a negative error number. However, we treat it as if it always returned 0
> >>> or 1. Check the return value of the callback and propagate the error
> >>> number up the stack.
> >>>
> >> This change breaks bcm2835 pincontrol/gpio driver (and probably others)
> >> in next-20250218. The problem is that some gpio lines are initially
> >> configured as alternate function (i.e. uart) and .get_direction returns
> >> -EINVAL for them, what in turn causes the whole gpio chip fail to
> >> register. Here is the log with WARN_ON() added to line
> >> drivers/pinctrl/bcm/pinctrl-bcm2835.c:350 from Raspberry Pi 4B:
> >>
> >> Any suggestions how to fix this issue? Should we add
> >> GPIO_LINE_DIRECTION_UNKNOWN?
> >>
> > That would be quite an intrusive change and not something for the
> > middle of the release cycle. I think we need to revert to the previous
> > behavior for this particular use-case: check ret for EINVAL and assume
> > it means input as it's the "safe" setting. Now the question is - can
> > this only happen during the chip registration or should we filter out
> > EINVAL at each gpiod_get_direction() call?
>
> IMHO it will be enough to use that workaround only in the
> gpiochip_add_data_with_key() function. The other functions modified by
> the $subject patch are strictly related to input or output gpio mode of
> operation, so having the line set to proper input/output state seems to
> be justified.
>

Cc'ing Florian

After a quick glance at existing get_direction() callbacks, it seems
this is the only driver that does it. I'm wondering if it wouldn't
make sense to change the driver behavior instead and make it assume
input for unknown functions.

Bart