| Message ID | 20201204164739.781812-1-maz@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | USB: ftdio_sio: GPIO validity fixes | expand |
On Fri, Dec 4, 2020 at 6:49 PM Marc Zyngier <maz@kernel.org> wrote: > > Having recently tried to use the CBUS GPIOs that come thanks to the > ftdio_sio driver, it occurred to me that the driver has a couple of > usability issues: > > - it advertises potential GPIOs that are reserved to other uses (LED > control, or something else) > > - it returns an odd error (-ENODEV), instead of the expected -EINVAL > when a line is unavailable, leading to a difficult diagnostic > > We address the issues in a number of ways: > > - Stop reporting invalid GPIO lines as valid to userspace. It > definitely seems odd to do so. Instead, report the line as being > used, making the userspace interface a bit more consistent. > > - Implement the init_valid_mask() callback in the ftdi_sio driver, > allowing it to report which lines are actually valid. > > - As suggested by Linus, give an indication to the user of why some of > the GPIO lines are unavailable, and point them to a useful tool > (once per boot). It is a bit sad that there next to no documentation > on how to use these CBUS pins. > > - Drop the error reporting code, which has become useless at this > point. > > Tested with a couple of FTDI devices (FT230X and FT231X) and various > CBUS configurations. Series looks pretty good to me, FWIW, Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Marc Zyngier (4): > gpiolib: cdev: Flag invalid GPIOs as used > USB: serial: ftdi_sio: Report the valid GPIO lines to gpiolib > USB: serial: ftdi_sio: Log the CBUS GPIO validity > USB: serial: ftdi_sio: Drop GPIO line checking dead code > > drivers/gpio/gpiolib-cdev.c | 1 + > drivers/usb/serial/ftdi_sio.c | 26 +++++++++++++++++++++++--- > 2 files changed, 24 insertions(+), 3 deletions(-) > > -- > 2.28.0 >
On Fri, Dec 04, 2020 at 04:47:35PM +0000, Marc Zyngier wrote: > Having recently tried to use the CBUS GPIOs that come thanks to the > ftdio_sio driver, it occurred to me that the driver has a couple of > usability issues: > > - it advertises potential GPIOs that are reserved to other uses (LED > control, or something else) Consider the alternative, that the gpio offsets (for CBUS0, CBUS1, CBUS2 or CBUS4) varies depending on how the pins have been muxed. Hardly very user friendly. > - it returns an odd error (-ENODEV), instead of the expected -EINVAL > when a line is unavailable, leading to a difficult diagnostic Hmm, maybe. Several gpio driver return -ENODEV when trying to request reserved pins. Even gpiolib returns -ENODEV when a pins is not yet available due to probe deferal. -EBUSY could also be an alternative, but that's used to indicate that a line is already in use as a gpio. > We address the issues in a number of ways: > > - Stop reporting invalid GPIO lines as valid to userspace. It > definitely seems odd to do so. Instead, report the line as being > used, making the userspace interface a bit more consistent. > > - Implement the init_valid_mask() callback in the ftdi_sio driver, > allowing it to report which lines are actually valid. > > - As suggested by Linus, give an indication to the user of why some of > the GPIO lines are unavailable, and point them to a useful tool > (once per boot). It is a bit sad that there next to no documentation > on how to use these CBUS pins. Don't be sad, Marc; write some documentation. ;) Johan
On 2020-12-07 14:01, Johan Hovold wrote: > On Fri, Dec 04, 2020 at 04:47:35PM +0000, Marc Zyngier wrote: >> Having recently tried to use the CBUS GPIOs that come thanks to the >> ftdio_sio driver, it occurred to me that the driver has a couple of >> usability issues: >> >> - it advertises potential GPIOs that are reserved to other uses (LED >> control, or something else) > > Consider the alternative, that the gpio offsets (for CBUS0, CBUS1, > CBUS2 > or CBUS4) varies depending on how the pins have been muxed. Hardly very > user friendly. That's not what I suggest. If you want fixed GPIO offsets, fine by me. But telling the user "these are GPIOs you can use", and then "on second though, you can't" is not exactly consistent. >> - it returns an odd error (-ENODEV), instead of the expected -EINVAL >> when a line is unavailable, leading to a difficult diagnostic > > Hmm, maybe. Several gpio driver return -ENODEV when trying to request > reserved pins. Even gpiolib returns -ENODEV when a pins is not yet > available due to probe deferal. -ENODEV really means "no GPIOchip" in this context. The fact that other drivers return -ENODEV for reserved pins looks like a bug to me. > -EBUSY could also be an alternative, but that's used to indicate that a > line is already in use as a gpio. Or something else. Which is exactly the case, as it's been allocated to another function. >> We address the issues in a number of ways: >> >> - Stop reporting invalid GPIO lines as valid to userspace. It >> definitely seems odd to do so. Instead, report the line as being >> used, making the userspace interface a bit more consistent. >> >> - Implement the init_valid_mask() callback in the ftdi_sio driver, >> allowing it to report which lines are actually valid. >> >> - As suggested by Linus, give an indication to the user of why some of >> the GPIO lines are unavailable, and point them to a useful tool >> (once per boot). It is a bit sad that there next to no documentation >> on how to use these CBUS pins. > > Don't be sad, Marc; write some documentation. ;) I sure will, right after I have fixed the rest of the kernel bugs I have introduced. With a bit of luck, that's right after I finally kick the bucket. M.
On Mon, Dec 07, 2020 at 02:41:03PM +0000, Marc Zyngier wrote: > On 2020-12-07 14:01, Johan Hovold wrote: > > On Fri, Dec 04, 2020 at 04:47:35PM +0000, Marc Zyngier wrote: > >> Having recently tried to use the CBUS GPIOs that come thanks to the > >> ftdio_sio driver, it occurred to me that the driver has a couple of > >> usability issues: > >> > >> - it advertises potential GPIOs that are reserved to other uses (LED > >> control, or something else) > > > > Consider the alternative, that the gpio offsets (for CBUS0, CBUS1, > > CBUS2 > > or CBUS4) varies depending on how the pins have been muxed. Hardly very > > user friendly. > > That's not what I suggest. If you want fixed GPIO offsets, fine by me. > But telling the user "these are GPIOs you can use", and then > "on second though, you can't" is not exactly consistent. It's really no different from any other gpio chip which registers all its lines, including those which may have been muxed for other purposes. > >> - it returns an odd error (-ENODEV), instead of the expected -EINVAL > >> when a line is unavailable, leading to a difficult diagnostic > > > > Hmm, maybe. Several gpio driver return -ENODEV when trying to request > > reserved pins. Even gpiolib returns -ENODEV when a pins is not yet > > available due to probe deferal. > > -ENODEV really means "no GPIOchip" in this context. The fact that > other drivers return -ENODEV for reserved pins looks like a bug to me. No, the chip is there. The -ENODEV is what you get when requesting the line, because the line isn't available. > > -EBUSY could also be an alternative, but that's used to indicate that a > > line is already in use as a gpio. > > Or something else. Which is exactly the case, as it's been allocated > to another function. Right, there are invalid requests (e.g. requesting line five of a four line chip), lines that are already in use, and lines not available due to muxing. And then there's the question of whether to use the same or distinct errnos for these. I believe using distinct errnos provides more feedback, but we can certainly pick another errno for this if it's really that confusing. > >> We address the issues in a number of ways: > >> > >> - Stop reporting invalid GPIO lines as valid to userspace. It > >> definitely seems odd to do so. Instead, report the line as being > >> used, making the userspace interface a bit more consistent. > >> > >> - Implement the init_valid_mask() callback in the ftdi_sio driver, > >> allowing it to report which lines are actually valid. > >> > >> - As suggested by Linus, give an indication to the user of why some of > >> the GPIO lines are unavailable, and point them to a useful tool > >> (once per boot). It is a bit sad that there next to no documentation > >> on how to use these CBUS pins. > > > > Don't be sad, Marc; write some documentation. ;) > > I sure will, right after I have fixed the rest of the kernel bugs > I have introduced. With a bit of luck, that's right after I finally > kick the bucket. Hear, hear. Johan
On 2020-12-07 15:08, Johan Hovold wrote: > On Mon, Dec 07, 2020 at 02:41:03PM +0000, Marc Zyngier wrote: >> On 2020-12-07 14:01, Johan Hovold wrote: >> > On Fri, Dec 04, 2020 at 04:47:35PM +0000, Marc Zyngier wrote: >> >> Having recently tried to use the CBUS GPIOs that come thanks to the >> >> ftdio_sio driver, it occurred to me that the driver has a couple of >> >> usability issues: >> >> >> >> - it advertises potential GPIOs that are reserved to other uses (LED >> >> control, or something else) >> > >> > Consider the alternative, that the gpio offsets (for CBUS0, CBUS1, >> > CBUS2 >> > or CBUS4) varies depending on how the pins have been muxed. Hardly very >> > user friendly. >> >> That's not what I suggest. If you want fixed GPIO offsets, fine by me. >> But telling the user "these are GPIOs you can use", and then >> "on second though, you can't" is not exactly consistent. > > It's really no different from any other gpio chip which registers all > its lines, including those which may have been muxed for other > purposes. If they claim that their lines are available, and then refuse to let the user play with it, that's just a bug willing to be fixed. >> >> - it returns an odd error (-ENODEV), instead of the expected -EINVAL >> >> when a line is unavailable, leading to a difficult diagnostic >> > >> > Hmm, maybe. Several gpio driver return -ENODEV when trying to request >> > reserved pins. Even gpiolib returns -ENODEV when a pins is not yet >> > available due to probe deferal. >> >> -ENODEV really means "no GPIOchip" in this context. The fact that >> other drivers return -ENODEV for reserved pins looks like a bug to me. > > No, the chip is there. The -ENODEV is what you get when requesting the > line, because the line isn't available. I still believe that ENODEV is the wrong error. The device is there, but the request is invalid because the line is used by something else. EINVAL, EBUSY, ENXIO would all be (sort of) OK. > >> > -EBUSY could also be an alternative, but that's used to indicate that a >> > line is already in use as a gpio. >> >> Or something else. Which is exactly the case, as it's been allocated >> to another function. > > Right, there are invalid requests (e.g. requesting line five of a four > line chip), lines that are already in use, and lines not available due > to muxing. > > And then there's the question of whether to use the same or distinct > errnos for these. I believe using distinct errnos provides more > feedback, but we can certainly pick another errno for this if it's > really that confusing. Fundamentally, I don't think the backend driver should be in charge of the error reporting. That should be the char device's job. Leaving it to the individual drivers is a sure way to have an inconsistent API. Thanks, M.
On Mon, Dec 07, 2020 at 03:34:23PM +0000, Marc Zyngier wrote: > On 2020-12-07 15:08, Johan Hovold wrote: > > On Mon, Dec 07, 2020 at 02:41:03PM +0000, Marc Zyngier wrote: > >> On 2020-12-07 14:01, Johan Hovold wrote: > >> > On Fri, Dec 04, 2020 at 04:47:35PM +0000, Marc Zyngier wrote: > >> >> Having recently tried to use the CBUS GPIOs that come thanks to the > >> >> ftdio_sio driver, it occurred to me that the driver has a couple of > >> >> usability issues: > >> >> > >> >> - it advertises potential GPIOs that are reserved to other uses (LED > >> >> control, or something else) > >> > > >> > Consider the alternative, that the gpio offsets (for CBUS0, CBUS1, > >> > CBUS2 > >> > or CBUS4) varies depending on how the pins have been muxed. Hardly very > >> > user friendly. > >> > >> That's not what I suggest. If you want fixed GPIO offsets, fine by me. > >> But telling the user "these are GPIOs you can use", and then > >> "on second though, you can't" is not exactly consistent. > > > > It's really no different from any other gpio chip which registers all > > its lines, including those which may have been muxed for other > > purposes. > > If they claim that their lines are available, and then refuse to > let the user play with it, that's just a bug willing to be fixed. My point was that this is how *all* gpio drivers work, and that muxing is somewhat orthogonal to the gpio controller implementation. Not sure how you would even "fix" that since muxing can often be changed at runtime while the number of lines is typically a hardware feature (which we report to the user). The resource is still there but it may not be available for use. > >> >> - it returns an odd error (-ENODEV), instead of the expected -EINVAL > >> >> when a line is unavailable, leading to a difficult diagnostic > >> > > >> > Hmm, maybe. Several gpio driver return -ENODEV when trying to request > >> > reserved pins. Even gpiolib returns -ENODEV when a pins is not yet > >> > available due to probe deferal. > >> > >> -ENODEV really means "no GPIOchip" in this context. The fact that > >> other drivers return -ENODEV for reserved pins looks like a bug to me. > > > > No, the chip is there. The -ENODEV is what you get when requesting the > > line, because the line isn't available. > > I still believe that ENODEV is the wrong error. The device is there, > but the request is invalid because the line is used by something else. > EINVAL, EBUSY, ENXIO would all be (sort of) OK. Fair enough. > >> > -EBUSY could also be an alternative, but that's used to indicate that a > >> > line is already in use as a gpio. > >> > >> Or something else. Which is exactly the case, as it's been allocated > >> to another function. > > > > Right, there are invalid requests (e.g. requesting line five of a four > > line chip), lines that are already in use, and lines not available due > > to muxing. > > > > And then there's the question of whether to use the same or distinct > > errnos for these. I believe using distinct errnos provides more > > feedback, but we can certainly pick another errno for this if it's > > really that confusing. > > Fundamentally, I don't think the backend driver should be in charge > of the error reporting. That should be the char device's job. Leaving it > to the individual drivers is a sure way to have an inconsistent API. I agree, and your valid-mask approach takes care of the static mux- configuration case nicely. Johan
On Mon, Dec 7, 2020 at 4:48 PM Johan Hovold <johan@kernel.org> wrote: > On Mon, Dec 07, 2020 at 03:34:23PM +0000, Marc Zyngier wrote: > > If they claim that their lines are available, and then refuse to > > let the user play with it, that's just a bug willing to be fixed. > > My point was that this is how *all* gpio drivers work, and that muxing > is somewhat orthogonal to the gpio controller implementation. This is true. It's because it is orthogonal that the separate subsystem for pin control including pin muxing exists. Should I be really overly picky, the drivers that can mux lines like this should be implementing the pin control mux driver side as well just to make Linux aware of this. But if the muxing cannot be changed by the kernel (albeit with special tools) then it would be pretty overengineered for this case. Things would be much easier if this wasn't some flashing configuration but more of a runtime thing (which is kind of the implicit assumption in pin control land). We don't really have many drivers that are "muxable by (intrusive) flashing" as opposed to "muxable by setting some bits" so in that way these FTDI drivers and siblings are special. So this needs some special considerations to become user friendly I think. Yours, Linus Walleij
On Wed, Dec 09, 2020 at 10:20:38AM +0100, Linus Walleij wrote: > On Mon, Dec 7, 2020 at 4:48 PM Johan Hovold <johan@kernel.org> wrote: > > On Mon, Dec 07, 2020 at 03:34:23PM +0000, Marc Zyngier wrote: > > > > If they claim that their lines are available, and then refuse to > > > let the user play with it, that's just a bug willing to be fixed. > > > > My point was that this is how *all* gpio drivers work, and that muxing > > is somewhat orthogonal to the gpio controller implementation. > > This is true. It's because it is orthogonal that the separate subsystem > for pin control including pin muxing exists. > > Should I be really overly picky, the drivers that can mux lines like > this should be implementing the pin control mux driver side as > well just to make Linux aware of this. But if the muxing cannot > be changed by the kernel (albeit with special tools) then it would > be pretty overengineered for this case. Things would be much > easier if this wasn't some flashing configuration but more of a > runtime thing (which is kind of the implicit assumption in pin > control land). We'd still have problem of how to configure these hot-pluggable devices at runtime, so it's not necessarily easier. If I remember correctly the xr_serial driver under review is doing something like muxing at runtime, but by simply having whichever interface (tty or gpio) that claims the resource first implicitly set the mux configuration. I have to revisit that. > We don't really have many drivers that are "muxable by > (intrusive) flashing" as opposed to "muxable by setting some > bits" so in that way these FTDI drivers and siblings are special. Yeah, but the gpio-reserved-range (valid-mask) feature which Marc used comes close here. Johan