Re: gpiolib: why does gpio_set_bias() suppress ENOTSUPP?

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Thu, Mar 31, 2022 at 10:52:03AM +0800, Kent Gibson wrote:
> Hi all,

+Cc: Hans

> It has recently come to my attention that the setting of bias by the
> cdev uAPI is a best effort operation that quietly succeeds if bias is
> not supported by the hardware. That strikes me as being a bug.
> It seems I was aware of this when adding bias to the uAPI and intended
> to fix it, as shown in the comments of v4 of the corrsponding patch
> series[1]:

> > > The setting of bias is performed by gpio_set_bias, which is hooked into
> > > gpiod_direction_input and gpiod_direction_output.  It extends the setting
> > > of bias that was already present in gpiod_direction_input.
> > > In keeping with the existing behaviour, the bias is set on a best-effort
> > > basis - no error is returned to the user if an error is returned by the
> > > pinctrl driver.  Returning an error makes sense to me, particularly for
> > > the uAPI, but that conflicts with the existing gpiod_direction_input
> > > behaviour. So leave as best-effort, change gpiod_direction_input
> > > behaviour, or restructure to support both behaviours?
> 
> > Thomas: is there any reason not to check the return value of these
> > calls for errors other than -EOPNOTSUPP?
> 
> that being my comment, and Bart's followup question to Thomas.
> 
> That went unanswered AFAICT and the issue subsequently fell through the
> cracks.

My understanding that all constraints we have in kernel is due to
in-kernel use and possible (non-critical) issues.

For example, driver can set only selected values of bias. What to do when
the given value is not supported by hardware?

Followup question: Why do you think your choice is any better than another
one?

> I would like to fix the uAPI such that if the hardware does not support
> the requested configuration, or if it can't be emulated in the kernel,
> that fact is returned to userspace - bias being the sole counter example
> as far as I am aware.
> 
> The simplest fix involves changing gpio_set_bias() to call gpio_set_config()
> rather than gpio_set_config_with_argument_optional(), but as mentioned in
> my comment above, that would impact any existing users of
> gpiod_direction_input() that assume the best-effort behaviour.

Exactly, best effort is to supply it to the driver and <s>pray</s> hope for
the best form the hardware driver.

> I haven't been able to find any such usage, but that could just be proof
> that I'm not looking in the right place.
> Any input on that front would be greatly appreciated.
> 
> Also, fixing this as mentioned could be considered an uAPI ABI change.
> Is this a bug, so that is ok, or do I need to consider adding a strict
> mode flag or somesuch to the API?
> 
> Bart, I'm also hoping to extend the gpiosim to optionally not support
> bias in gc->set_config() to test this case.
> Any suggstions on a configfs interface extension to do that?
> 
> My apologies for the verbage rather than proffering a patch, but the
> different paths have vastly different costs, and the simplest solution
> has the potential to introduce breakage.

> [1] https://www.spinics.net/lists/linux-gpio/msg43579.html

-- 
With Best Regards,
Andy Shevchenko