* gpiolib: why does gpio_set_bias() suppress ENOTSUPP? @ 2022-03-31 2:52 Kent Gibson 2022-03-31 13:53 ` Andy Shevchenko 0 siblings, 1 reply; 13+ messages in thread From: Kent Gibson @ 2022-03-31 2:52 UTC (permalink / raw) To: linux-gpio, brgl, andriy.shevchenko, thomas.petazzoni; +Cc: linus.walleij Hi all, 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. 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. 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. Cheers, Kent. [1] https://www.spinics.net/lists/linux-gpio/msg43579.html ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: gpiolib: why does gpio_set_bias() suppress ENOTSUPP? 2022-03-31 2:52 gpiolib: why does gpio_set_bias() suppress ENOTSUPP? Kent Gibson @ 2022-03-31 13:53 ` Andy Shevchenko 2022-03-31 14:15 ` Kent Gibson 0 siblings, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2022-03-31 13:53 UTC (permalink / raw) To: Kent Gibson, Hans de Goede Cc: linux-gpio, brgl, thomas.petazzoni, linus.walleij 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: gpiolib: why does gpio_set_bias() suppress ENOTSUPP? 2022-03-31 13:53 ` Andy Shevchenko @ 2022-03-31 14:15 ` Kent Gibson 2022-03-31 17:19 ` Andy Shevchenko 2022-04-01 10:36 ` Hans de Goede 0 siblings, 2 replies; 13+ messages in thread From: Kent Gibson @ 2022-03-31 14:15 UTC (permalink / raw) To: Andy Shevchenko Cc: Hans de Goede, linux-gpio, brgl, thomas.petazzoni, linus.walleij On Thu, Mar 31, 2022 at 04:53:23PM +0300, Andy Shevchenko wrote: > 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'm probably missing your point here. What makes gpiolib best placed to decide that bias not being supported by hardware is non-critical? Why not just propagate the ENOTSUPP to the caller and let them decide? Is it because setting bias is piggy-backed onto gpiod_direction_input() rather than being separate, so then you can't tell whether it is input or bias that is not supported? Anyway, if that interface is required for internal use then there is no option but to refactor gpiod_direction_input() and provide an alternate interface so that cdev can determine if bias is supported or not. Cheers, Kent. > > 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 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: gpiolib: why does gpio_set_bias() suppress ENOTSUPP? 2022-03-31 14:15 ` Kent Gibson @ 2022-03-31 17:19 ` Andy Shevchenko 2022-04-01 0:08 ` Kent Gibson 2022-04-01 10:36 ` Hans de Goede 1 sibling, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2022-03-31 17:19 UTC (permalink / raw) To: Kent Gibson Cc: Hans de Goede, linux-gpio, brgl, thomas.petazzoni, linus.walleij On Thu, Mar 31, 2022 at 10:15:24PM +0800, Kent Gibson wrote: > On Thu, Mar 31, 2022 at 04:53:23PM +0300, Andy Shevchenko wrote: > > On Thu, Mar 31, 2022 at 10:52:03AM +0800, Kent Gibson wrote: > > > 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'm probably missing your point here. > > What makes gpiolib best placed to decide that bias not being supported > by hardware is non-critical? Why not just propagate the ENOTSUPP to the > caller and let them decide? First of all, ENOTSUPP may not be sent to user, it's wrong error code. The returning any other error code make behaviour for the _very same_ GPIO line _different_ when it being configured in kernel (via firmware) and user space. That's unacceptable. So, it means we have to have synchronized behaviour, means either error in both case or ignoring it. The latter one is current state of affairs, the former might break the cases where driver and firmware are not synchronized well. > Is it because setting bias is piggy-backed onto > gpiod_direction_input() rather than being separate, so then you can't > tell whether it is input or bias that is not supported? > > Anyway, if that interface is required for internal use then there is no > option but to refactor gpiod_direction_input() and provide an alternate > interface so that cdev can determine if bias is supported or not. > > > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: gpiolib: why does gpio_set_bias() suppress ENOTSUPP? 2022-03-31 17:19 ` Andy Shevchenko @ 2022-04-01 0:08 ` Kent Gibson 2022-04-01 10:42 ` Andy Shevchenko 0 siblings, 1 reply; 13+ messages in thread From: Kent Gibson @ 2022-04-01 0:08 UTC (permalink / raw) To: Andy Shevchenko Cc: Hans de Goede, linux-gpio, brgl, thomas.petazzoni, linus.walleij On Thu, Mar 31, 2022 at 08:19:39PM +0300, Andy Shevchenko wrote: > On Thu, Mar 31, 2022 at 10:15:24PM +0800, Kent Gibson wrote: > > On Thu, Mar 31, 2022 at 04:53:23PM +0300, Andy Shevchenko wrote: > > > On Thu, Mar 31, 2022 at 10:52:03AM +0800, Kent Gibson wrote: > > > > 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'm probably missing your point here. > > > > What makes gpiolib best placed to decide that bias not being supported > > by hardware is non-critical? Why not just propagate the ENOTSUPP to the > > caller and let them decide? > > First of all, ENOTSUPP may not be sent to user, it's wrong error code. > The returning any other error code make behaviour for the _very same_ > GPIO line _different_ when it being configured in kernel (via firmware) > and user space. That's unacceptable. So, it means we have to have > synchronized behaviour, means either error in both case or ignoring it. > The latter one is current state of affairs, the former might break the > cases where driver and firmware are not synchronized well. > First of all, I was referring to in-kernel users of gpiolib, so gpiolib would propagate ENOTSUPP. You will note I said "caller", not "user" to try to emphasise that point. cdev, being the caller here, would translate the ENOTSUPP to EOPNOTSUPP for user space consumption. But back to the point... Are you saying that user space should not be given an error if bias is not supported as it was already decided by gpiolib that the kernel driver wouldn't be given one? That makes no sense to me, so I'm probably missing something. Can you clarify? What I read from your answer is "that's the way it is". I get that, but was after why. I don't see dropping errors as a way to get anything in sync. Quite the opposite - it ensures we can never tell if they are. If the original problem was that there are two possible causes for ENOTSUPP, and one of them is critical and one is not, then the solution should be to split the functionality into separate calls, not just drop the error from the "non-critical" one. Again, gpiolib has no business in that criticality decision, IMHO. And again when I say gpiolib here I mean the core - gpiolib.c. Cheers, Kent. > > Is it because setting bias is piggy-backed onto > > gpiod_direction_input() rather than being separate, so then you can't > > tell whether it is input or bias that is not supported? > > > > Anyway, if that interface is required for internal use then there is no > > option but to refactor gpiod_direction_input() and provide an alternate > > interface so that cdev can determine if bias is supported or not. > > > > > 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 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: gpiolib: why does gpio_set_bias() suppress ENOTSUPP? 2022-04-01 0:08 ` Kent Gibson @ 2022-04-01 10:42 ` Andy Shevchenko 2022-04-02 1:45 ` Kent Gibson 0 siblings, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2022-04-01 10:42 UTC (permalink / raw) To: Kent Gibson Cc: Hans de Goede, linux-gpio, brgl, thomas.petazzoni, linus.walleij On Fri, Apr 01, 2022 at 08:08:37AM +0800, Kent Gibson wrote: > On Thu, Mar 31, 2022 at 08:19:39PM +0300, Andy Shevchenko wrote: > > On Thu, Mar 31, 2022 at 10:15:24PM +0800, Kent Gibson wrote: > > > On Thu, Mar 31, 2022 at 04:53:23PM +0300, Andy Shevchenko wrote: > > > > On Thu, Mar 31, 2022 at 10:52:03AM +0800, Kent Gibson wrote: > > > > > 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'm probably missing your point here. > > > > > > What makes gpiolib best placed to decide that bias not being supported > > > by hardware is non-critical? Why not just propagate the ENOTSUPP to the > > > caller and let them decide? > > > > First of all, ENOTSUPP may not be sent to user, it's wrong error code. > > The returning any other error code make behaviour for the _very same_ > > GPIO line _different_ when it being configured in kernel (via firmware) > > and user space. That's unacceptable. So, it means we have to have > > synchronized behaviour, means either error in both case or ignoring it. > > The latter one is current state of affairs, the former might break the > > cases where driver and firmware are not synchronized well. > > > > First of all, I was referring to in-kernel users of gpiolib, so gpiolib > would propagate ENOTSUPP. You will note I said "caller", not "user" > to try to emphasise that point. > cdev, being the caller here, would translate the ENOTSUPP to EOPNOTSUPP > for user space consumption. Ah, okay! > But back to the point... > Are you saying that user space should not be given an error if bias is > not supported as it was already decided by gpiolib that the kernel > driver wouldn't be given one? > That makes no sense to me, so I'm probably missing something. > Can you clarify? I think that deviating of the behaviour would confuse some. Why the configuration of the very same line described in DT and used in user space will behave differently? > What I read from your answer is "that's the way it is". > I get that, but was after why. > I don't see dropping errors as a way to get anything in sync. > Quite the opposite - it ensures we can never tell if they are. > > If the original problem was that there are two possible causes for > ENOTSUPP, and one of them is critical and one is not, then the solution > should be to split the functionality into separate calls, not just drop > the error from the "non-critical" one. Again, gpiolib has no business > in that criticality decision, IMHO. And again when I say gpiolib here > I mean the core - gpiolib.c. I got your point. But to me it is a way of breaking the prototyping and actual product. If you strict at prototyping and reuse same DT(b) over several generations of the platform, some of them (due to subtle hardware differences) may ignore errors (due to in-kernel behaviour) while during prototyping that was strict. The best approach here is to add a new IOCTL "enable extended error reporting" or alike and expect all of GPIO ABI calls to fail. In such case it won't alter status quo, not break ABI. > > > Is it because setting bias is piggy-backed onto > > > gpiod_direction_input() rather than being separate, so then you can't > > > tell whether it is input or bias that is not supported? > > > > > > Anyway, if that interface is required for internal use then there is no > > > option but to refactor gpiod_direction_input() and provide an alternate > > > interface so that cdev can determine if bias is supported or not. > > > > > > > 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 > > > > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: gpiolib: why does gpio_set_bias() suppress ENOTSUPP? 2022-04-01 10:42 ` Andy Shevchenko @ 2022-04-02 1:45 ` Kent Gibson 0 siblings, 0 replies; 13+ messages in thread From: Kent Gibson @ 2022-04-02 1:45 UTC (permalink / raw) To: Andy Shevchenko Cc: Hans de Goede, linux-gpio, brgl, thomas.petazzoni, linus.walleij On Fri, Apr 01, 2022 at 01:42:54PM +0300, Andy Shevchenko wrote: > On Fri, Apr 01, 2022 at 08:08:37AM +0800, Kent Gibson wrote: > > On Thu, Mar 31, 2022 at 08:19:39PM +0300, Andy Shevchenko wrote: > > > On Thu, Mar 31, 2022 at 10:15:24PM +0800, Kent Gibson wrote: > > > > On Thu, Mar 31, 2022 at 04:53:23PM +0300, Andy Shevchenko wrote: > > > > > On Thu, Mar 31, 2022 at 10:52:03AM +0800, Kent Gibson wrote: > > > > > > 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'm probably missing your point here. > > > > > > > > What makes gpiolib best placed to decide that bias not being supported > > > > by hardware is non-critical? Why not just propagate the ENOTSUPP to the > > > > caller and let them decide? > > > > > > First of all, ENOTSUPP may not be sent to user, it's wrong error code. > > > The returning any other error code make behaviour for the _very same_ > > > GPIO line _different_ when it being configured in kernel (via firmware) > > > and user space. That's unacceptable. So, it means we have to have > > > synchronized behaviour, means either error in both case or ignoring it. > > > The latter one is current state of affairs, the former might break the > > > cases where driver and firmware are not synchronized well. > > > > > > > First of all, I was referring to in-kernel users of gpiolib, so gpiolib > > would propagate ENOTSUPP. You will note I said "caller", not "user" > > to try to emphasise that point. > > cdev, being the caller here, would translate the ENOTSUPP to EOPNOTSUPP > > for user space consumption. > > Ah, okay! > > > But back to the point... > > Are you saying that user space should not be given an error if bias is > > not supported as it was already decided by gpiolib that the kernel > > driver wouldn't be given one? > > That makes no sense to me, so I'm probably missing something. > > Can you clarify? > > I think that deviating of the behaviour would confuse some. > Why the configuration of the very same line described in DT > and used in user space will behave differently? > Ok, you are looking at the uAPI using a DT lens. How many users would be using both DT and uAPI? If the bias is already set via DT why set it again via uAPI? The more likely use case is that users don't know anything about DT, and are surprised to find that bias doesn't work - despite the uAPI accepting bias settings without complaint. > > What I read from your answer is "that's the way it is". > > I get that, but was after why. > > I don't see dropping errors as a way to get anything in sync. > > Quite the opposite - it ensures we can never tell if they are. > > > > If the original problem was that there are two possible causes for > > ENOTSUPP, and one of them is critical and one is not, then the solution > > should be to split the functionality into separate calls, not just drop > > the error from the "non-critical" one. Again, gpiolib has no business > > in that criticality decision, IMHO. And again when I say gpiolib here > > I mean the core - gpiolib.c. > > I got your point. But to me it is a way of breaking the prototyping and > actual product. If you strict at prototyping and reuse same DT(b) over > several generations of the platform, some of them (due to subtle hardware > differences) may ignore errors (due to in-kernel behaviour) while during > prototyping that was strict. > I understand why you want application of pin description from DT to be best-effort - I was unsure why that was done in gpiod_direction_input(). But Hans got me over that hump. > The best approach here is to add a new IOCTL "enable extended error reporting" > or alike and expect all of GPIO ABI calls to fail. In such case it won't > alter status quo, not break ABI. > Agreed, although I'm thinking a flag in the gpio_v2_line_request rather than a new ioctl. Or maybe in gpio_v2_line_config. Thanks for the assistance - much appreciated. Cheers, Kent. > > > > Is it because setting bias is piggy-backed onto > > > > gpiod_direction_input() rather than being separate, so then you can't > > > > tell whether it is input or bias that is not supported? > > > > > > > > Anyway, if that interface is required for internal use then there is no > > > > option but to refactor gpiod_direction_input() and provide an alternate > > > > interface so that cdev can determine if bias is supported or not. > > > > > > > > > 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 > > > > > > > > -- > With Best Regards, > Andy Shevchenko > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: gpiolib: why does gpio_set_bias() suppress ENOTSUPP? 2022-03-31 14:15 ` Kent Gibson 2022-03-31 17:19 ` Andy Shevchenko @ 2022-04-01 10:36 ` Hans de Goede 2022-04-02 1:45 ` Kent Gibson 1 sibling, 1 reply; 13+ messages in thread From: Hans de Goede @ 2022-04-01 10:36 UTC (permalink / raw) To: Kent Gibson, Andy Shevchenko Cc: linux-gpio, brgl, thomas.petazzoni, linus.walleij Hi Kent, On 3/31/22 16:15, Kent Gibson wrote: > On Thu, Mar 31, 2022 at 04:53:23PM +0300, Andy Shevchenko wrote: >> 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'm probably missing your point here. > > What makes gpiolib best placed to decide that bias not being supported > by hardware is non-critical? Why not just propagate the ENOTSUPP to the > caller and let them decide? > > Is it because setting bias is piggy-backed onto > gpiod_direction_input() rather than being separate, so then you can't > tell whether it is input or bias that is not supported? Right, gpiod_direction_input() check if there is a bias described for the pin in the firmware description of the pin (devicetree or ACPI) and those might contain a bias setting even if the pinctrl/gpio chip is not capable of it (and instead it is actually e.g. applied by external resistors on the PCB). The idea behind this is to make things just work for most drivers using/consuming GPIOS without the drivers needing to know about the firmware description details. To make sure this actually just works and does not cause drivers to see unexpected (and usually not a problem) errors the ENOTSUPP error from the set bias call is not propagated from gpiod_direction_input(). > Anyway, if that interface is required for internal use then there is no > option but to refactor gpiod_direction_input() and provide an alternate > interface so that cdev can determine if bias is supported or not. I'm not very familiar with the cdev userspace API, but I think that that is correct. Assuming the cdev userspace API has separate set-direction and set-bias calls then I would expect the set-direction calls to work as the in kernel one does, so try to apply any bias from the firmware pin description, but ignore ENOTSUPP errors. Although I guess in most (all probably even?) cases since we just get a GPIO-chip + hw-index there is no firmware pin-description in the cdev uapi case. Where as explicit set_bias calls should indeed probably not ignore the ENOTSUPP error. I do wonder though if such a change would not consider breakage of existing userspace, esp. on popular boards like the raspberry pi where there is a lot of existing code poking the GPIOs from userspace. As long as you don't actually change the function prototype of gpiod_direction_input() making changes for this should be fine. You can rename the original function to something else and give it an extra flag, or split it in 2 functions or some such + use a wrapper with the old name. But having to modify all callers for this would be bad IMHO. Regards, Hans > > Cheers, > Kent. > >>> 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 >> >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: gpiolib: why does gpio_set_bias() suppress ENOTSUPP? 2022-04-01 10:36 ` Hans de Goede @ 2022-04-02 1:45 ` Kent Gibson 2022-04-02 9:11 ` Hans de Goede 0 siblings, 1 reply; 13+ messages in thread From: Kent Gibson @ 2022-04-02 1:45 UTC (permalink / raw) To: Hans de Goede Cc: Andy Shevchenko, linux-gpio, brgl, thomas.petazzoni, linus.walleij On Fri, Apr 01, 2022 at 12:36:57PM +0200, Hans de Goede wrote: > Hi Kent, > > On 3/31/22 16:15, Kent Gibson wrote: > > On Thu, Mar 31, 2022 at 04:53:23PM +0300, Andy Shevchenko wrote: > >> 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'm probably missing your point here. > > > > What makes gpiolib best placed to decide that bias not being supported > > by hardware is non-critical? Why not just propagate the ENOTSUPP to the > > caller and let them decide? > > > > Is it because setting bias is piggy-backed onto > > gpiod_direction_input() rather than being separate, so then you can't > > tell whether it is input or bias that is not supported? > > Right, gpiod_direction_input() check if there is a bias described for > the pin in the firmware description of the pin (devicetree or ACPI) and > those might contain a bias setting even if the pinctrl/gpio chip is not > capable of it (and instead it is actually e.g. applied by external > resistors on the PCB). > So the pin description may extend beyond the gpio chip itself, and describe part of the external circuit? Ok, hadn't considered that a possibility. > The idea behind this is to make things just work for most > drivers using/consuming GPIOS without the drivers needing to know > about the firmware description details. > > To make sure this actually just works and does not cause drivers > to see unexpected (and usually not a problem) errors the ENOTSUPP > error from the set bias call is not propagated from > gpiod_direction_input(). > Ok, just to make sure I have this straight: The drivers get their pin description from devicetree or ACPI and then apply it using gpiod_direction_input() or gpiod_direction_output(), as appropriate. The gpiod_direction_input() was leveraged to do that, rather than having to extend the gpiolib API and change all the drivers, as drivers were already using it. The application of the pin description, specifically the bias aspect, is best-effort as the pin description may not match the capabilities of the hardware, or more precisely the pinctrl. The driver is acting as the middleman between the source of the pin description and gpiolib/pinctrl, and so wouldn't know how to deal with any inconsistency, so gpiod_direction_input() suppresses the error. > > > Anyway, if that interface is required for internal use then there is no > > option but to refactor gpiod_direction_input() and provide an alternate > > interface so that cdev can determine if bias is supported or not. > > I'm not very familiar with the cdev userspace API, but I think > that that is correct. > > Assuming the cdev userspace API has separate set-direction and set-bias > calls then I would expect the set-direction calls to work as the in > kernel one does, so try to apply any bias from the firmware pin > description, but ignore ENOTSUPP errors. > > Although I guess in most (all probably even?) cases since we just > get a GPIO-chip + hw-index there is no firmware pin-description in > the cdev uapi case. > > Where as explicit set_bias calls should indeed probably not ignore > the ENOTSUPP error. > In fact the uAPI doesn't have separate calls. The user effectively creates their own pin description and that is applied with a single ioctl call - which results in a call to gpiod_direction_input(). The current problem is that users can request bias and then be surprised to find that it doesn't work on their platform - despite the uAPI accepting the bias setting without complaint. > I do wonder though if such a change would not consider > breakage of existing userspace, esp. on popular boards like > the raspberry pi where there is a lot of existing code > poking the GPIOs from userspace. > I've wondered the same. It would only be breakage for cases where platforms don't actually support bias, but users were assuming that it does. The RPi supports bias (when adding bias to the uAPI I explicitly did testing on RPis), so it wouldn't be a problem there. OTOH, I can't say how many other platforms it could cause breakage on. Probably best to extend the uAPI to add a strict mode and leave existing usage unchanged. > As long as you don't actually change the function prototype > of gpiod_direction_input() making changes for this should be > fine. You can rename the original function to something else > and give it an extra flag, or split it in 2 functions or some > such + use a wrapper with the old name. But having to modify > all callers for this would be bad IMHO. > Agreed - refactoring and providing a wrapper for existing usage looks the way to go. I was hoping to avoid that, to avoid having to find a name for that new function, as naming is always the hardest part. Thanks for your assistance - much appreciated. Cheers, Kent. > Regards, > > Hans > > > > > > > > > > > Cheers, > > Kent. > > > >>> 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 > >> > >> > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: gpiolib: why does gpio_set_bias() suppress ENOTSUPP? 2022-04-02 1:45 ` Kent Gibson @ 2022-04-02 9:11 ` Hans de Goede 2022-04-04 9:23 ` Andy Shevchenko 0 siblings, 1 reply; 13+ messages in thread From: Hans de Goede @ 2022-04-02 9:11 UTC (permalink / raw) To: Kent Gibson Cc: Andy Shevchenko, linux-gpio, brgl, thomas.petazzoni, linus.walleij Hi, On 4/2/22 03:45, Kent Gibson wrote: > On Fri, Apr 01, 2022 at 12:36:57PM +0200, Hans de Goede wrote: >> Hi Kent, >> >> On 3/31/22 16:15, Kent Gibson wrote: >>> On Thu, Mar 31, 2022 at 04:53:23PM +0300, Andy Shevchenko wrote: >>>> 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'm probably missing your point here. >>> >>> What makes gpiolib best placed to decide that bias not being supported >>> by hardware is non-critical? Why not just propagate the ENOTSUPP to the >>> caller and let them decide? >>> >>> Is it because setting bias is piggy-backed onto >>> gpiod_direction_input() rather than being separate, so then you can't >>> tell whether it is input or bias that is not supported? >> >> Right, gpiod_direction_input() check if there is a bias described for >> the pin in the firmware description of the pin (devicetree or ACPI) and >> those might contain a bias setting even if the pinctrl/gpio chip is not >> capable of it (and instead it is actually e.g. applied by external >> resistors on the PCB). >> > > So the pin description may extend beyond the gpio chip itself, and > describe part of the external circuit? It is not necessary / really supposed to, but copy and paste from previous platforms may lead to this. Also something like the active low/high bit of the pin description may take the presence of an external (inverting) buffer/level-convertor into account. So yes it is somewhat tied to the external circuit too. Because it e.g. also tells the kernel if the pin should be driven electrically low/high to make it logically high which may depend on external circuitry. > Ok, hadn't considered that a possibility. > >> The idea behind this is to make things just work for most >> drivers using/consuming GPIOS without the drivers needing to know >> about the firmware description details. >> >> To make sure this actually just works and does not cause drivers >> to see unexpected (and usually not a problem) errors the ENOTSUPP >> error from the set bias call is not propagated from >> gpiod_direction_input(). >> > > Ok, just to make sure I have this straight: > > The drivers get their pin description from devicetree or ACPI and > then apply it using gpiod_direction_input() or gpiod_direction_output(), > as appropriate. Yes, but note that it is not the drivers which interpret the firmware data, this is all done in the core code. A driver basically says give me my <label> (e.g. "reset") pin and initially configure it as output with a logical low value. Many drivers never call gpiod_direction_input() or gpiod_direction_output() they pass the direction they want initially (+ value when direction is output) when calling gpiod_get() and then never have to change the direction. > The gpiod_direction_input() was leveraged to do that, rather than having > to extend the gpiolib API and change all the drivers, as drivers were > already using it. Right, except that this mostly relies on gpiod_get() calling gpiod_direction_input() when called with the GPIOD_IN flag for setting the initial direction. > The application of the pin description, specifically the bias aspect, > is best-effort as the pin description may not match the capabilities of > the hardware, or more precisely the pinctrl. The driver is acting as > the middleman between the source of the pin description and > gpiolib/pinctrl, and so wouldn't know how to deal with any inconsistency, > so gpiod_direction_input() suppresses the error. Right that pretty much sums it up. >>> Anyway, if that interface is required for internal use then there is no >>> option but to refactor gpiod_direction_input() and provide an alternate >>> interface so that cdev can determine if bias is supported or not. >> >> I'm not very familiar with the cdev userspace API, but I think >> that that is correct. >> >> Assuming the cdev userspace API has separate set-direction and set-bias >> calls then I would expect the set-direction calls to work as the in >> kernel one does, so try to apply any bias from the firmware pin >> description, but ignore ENOTSUPP errors. >> >> Although I guess in most (all probably even?) cases since we just >> get a GPIO-chip + hw-index there is no firmware pin-description in >> the cdev uapi case. >> >> Where as explicit set_bias calls should indeed probably not ignore >> the ENOTSUPP error. >> > > In fact the uAPI doesn't have separate calls. The user effectively > creates their own pin description and that is applied with a single > ioctl call - which results in a call to gpiod_direction_input(). > > The current problem is that users can request bias and then be surprised > to find that it doesn't work on their platform - despite the uAPI > accepting the bias setting without complaint. > >> I do wonder though if such a change would not consider >> breakage of existing userspace, esp. on popular boards like >> the raspberry pi where there is a lot of existing code >> poking the GPIOs from userspace. >> > > I've wondered the same. > It would only be breakage for cases where platforms don't actually > support bias, but users were assuming that it does. > The RPi supports bias (when adding bias to the uAPI I explicitly did > testing on RPis), so it wouldn't be a problem there. > OTOH, I can't say how many other platforms it could cause breakage on. Actually with all the Pi clones, I think that the silent ignore bias not support behavior makes some sense for the uAPI too, some of the clones might not support this, while users will have a tendency to just re-use some python libs to support peripherals without modifying them. OTOH getting a bias not supported error might help them figure out that the pi compatible hat which they are using on their Pi clone is only going to work if they add some external pull-ups / -downs... > Probably best to extend the uAPI to add a strict mode and leave > existing usage unchanged. Agreed, adding a strict mode to the uAPI seems best. And if you do it this way, you should probably also make the kernel log (using a ratelimited log function) why (e.g. bias setting not supported) the call is failing since errno is not going to tell the user enough here I think. >> As long as you don't actually change the function prototype >> of gpiod_direction_input() making changes for this should be >> fine. You can rename the original function to something else >> and give it an extra flag, or split it in 2 functions or some >> such + use a wrapper with the old name. But having to modify >> all callers for this would be bad IMHO. >> > > Agreed - refactoring and providing a wrapper for existing usage looks > the way to go. I was hoping to avoid that, to avoid having to find a > name for that new function, as naming is always the hardest part. Well you need to come up with a name for the uAPI, if you are going with strict there, you could add a _strict to the non-wrapped function and give it a strict boolean argument? Not sure if using strict is good for the uAPI though, what I'm trying to say if that if you solve the naming issue for the uAPI you can then mirror that name on the kernel side. Regards, Hans >>>>> 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 >>>> >>>> >>> >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: gpiolib: why does gpio_set_bias() suppress ENOTSUPP? 2022-04-02 9:11 ` Hans de Goede @ 2022-04-04 9:23 ` Andy Shevchenko 2022-04-04 10:06 ` Kent Gibson 0 siblings, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2022-04-04 9:23 UTC (permalink / raw) To: Hans de Goede Cc: Kent Gibson, Andy Shevchenko, open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Thomas Petazzoni, Linus Walleij On Sun, Apr 3, 2022 at 6:34 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 4/2/22 03:45, Kent Gibson wrote: > > On Fri, Apr 01, 2022 at 12:36:57PM +0200, Hans de Goede wrote: ... > > Probably best to extend the uAPI to add a strict mode and leave > > existing usage unchanged. > > Agreed, adding a strict mode to the uAPI seems best. > > And if you do it this way, you should probably also make > the kernel log (using a ratelimited log function) why (e.g. > bias setting not supported) the call is failing since errno is > not going to tell the user enough here I think. ...which reminds me this one: https://lwn.net/Articles/657341/ -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: gpiolib: why does gpio_set_bias() suppress ENOTSUPP? 2022-04-04 9:23 ` Andy Shevchenko @ 2022-04-04 10:06 ` Kent Gibson 2022-04-04 10:16 ` Andy Shevchenko 0 siblings, 1 reply; 13+ messages in thread From: Kent Gibson @ 2022-04-04 10:06 UTC (permalink / raw) To: Andy Shevchenko Cc: Hans de Goede, Andy Shevchenko, open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Thomas Petazzoni, Linus Walleij On Mon, Apr 04, 2022 at 12:23:18PM +0300, Andy Shevchenko wrote: > On Sun, Apr 3, 2022 at 6:34 PM Hans de Goede <hdegoede@redhat.com> wrote: > > On 4/2/22 03:45, Kent Gibson wrote: > > > On Fri, Apr 01, 2022 at 12:36:57PM +0200, Hans de Goede wrote: > > ... > > > > Probably best to extend the uAPI to add a strict mode and leave > > > existing usage unchanged. > > > > Agreed, adding a strict mode to the uAPI seems best. > > > > And if you do it this way, you should probably also make > > the kernel log (using a ratelimited log function) why (e.g. > > bias setting not supported) the call is failing since errno is > > not going to tell the user enough here I think. > > ...which reminds me this one: https://lwn.net/Articles/657341/ > In this case I'd be more inclined to return a sanitised config along with the error code. So the user gets "the config you requested isn't doable, but this one is". They could even repeat the request with the sanitised config, though I'm not sure if that would provide any benefit compared to just not requesting strict in the first place. Cheers, Kent. > -- > With Best Regards, > Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: gpiolib: why does gpio_set_bias() suppress ENOTSUPP? 2022-04-04 10:06 ` Kent Gibson @ 2022-04-04 10:16 ` Andy Shevchenko 0 siblings, 0 replies; 13+ messages in thread From: Andy Shevchenko @ 2022-04-04 10:16 UTC (permalink / raw) To: Kent Gibson Cc: Hans de Goede, Andy Shevchenko, open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Thomas Petazzoni, Linus Walleij On Mon, Apr 4, 2022 at 1:06 PM Kent Gibson <warthog618@gmail.com> wrote: > On Mon, Apr 04, 2022 at 12:23:18PM +0300, Andy Shevchenko wrote: > > On Sun, Apr 3, 2022 at 6:34 PM Hans de Goede <hdegoede@redhat.com> wrote: > > > On 4/2/22 03:45, Kent Gibson wrote: > > > > On Fri, Apr 01, 2022 at 12:36:57PM +0200, Hans de Goede wrote: ... > > > > Probably best to extend the uAPI to add a strict mode and leave > > > > existing usage unchanged. > > > > > > Agreed, adding a strict mode to the uAPI seems best. > > > > > > And if you do it this way, you should probably also make > > > the kernel log (using a ratelimited log function) why (e.g. > > > bias setting not supported) the call is failing since errno is > > > not going to tell the user enough here I think. > > > > ...which reminds me this one: https://lwn.net/Articles/657341/ > > > > In this case I'd be more inclined to return a sanitised config along > with the error code. So the user gets "the config you requested isn't > doable, but this one is". They could even repeat the request with the > sanitised config, though I'm not sure if that would provide any benefit > compared to just not requesting strict in the first place. Yeah, being "too smart" sometimes becomes an evil result. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-04-04 10:17 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-31 2:52 gpiolib: why does gpio_set_bias() suppress ENOTSUPP? Kent Gibson 2022-03-31 13:53 ` Andy Shevchenko 2022-03-31 14:15 ` Kent Gibson 2022-03-31 17:19 ` Andy Shevchenko 2022-04-01 0:08 ` Kent Gibson 2022-04-01 10:42 ` Andy Shevchenko 2022-04-02 1:45 ` Kent Gibson 2022-04-01 10:36 ` Hans de Goede 2022-04-02 1:45 ` Kent Gibson 2022-04-02 9:11 ` Hans de Goede 2022-04-04 9:23 ` Andy Shevchenko 2022-04-04 10:06 ` Kent Gibson 2022-04-04 10:16 ` Andy Shevchenko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.