* [PATCH] USB: serial: ftdi_sio: Helpful error on GPIO attempt @ 2020-12-01 14:10 Linus Walleij 2020-12-01 15:01 ` Marc Zyngier 0 siblings, 1 reply; 5+ messages in thread From: Linus Walleij @ 2020-12-01 14:10 UTC (permalink / raw) To: Johan Hovold; +Cc: linux-usb, Linus Walleij, Marc Zyngier The FTDI adapters present all potentially available GPIO lines to userspace, and they are often also visibly available on things like breakout boards. These are appetizing targets for random GPIO tinkering such as bit-banging or other industrial control over USB. When a user attempts to use one of the GPIO lines, they can get the opaque error -ENODEV, because the flashed configuration says that the line is not in GPIO mode but another alternative function. We had one user run into this, debug and finally fix the problem using ftx-prog. Give the user some more helpful dmesg text and a pointer to ftx-prog when the error occurs. Reported-by: Marc Zyngier <maz@kernel.org> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/usb/serial/ftdi_sio.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c index e0f4c3d9649c..405fec78f2fc 100644 --- a/drivers/usb/serial/ftdi_sio.c +++ b/drivers/usb/serial/ftdi_sio.c @@ -1841,8 +1841,11 @@ static int ftdi_gpio_request(struct gpio_chip *gc, unsigned int offset) struct ftdi_private *priv = usb_get_serial_port_data(port); int result; - if (priv->gpio_altfunc & BIT(offset)) + if (priv->gpio_altfunc & BIT(offset)) { + dev_err(&port->dev, "FTDI firmware says line is not in GPIO mode\n"); + dev_err(&port->dev, "if you really know what you're doing the flash can be reconfigured using ftx-prog\n"); return -ENODEV; + } mutex_lock(&priv->gpio_lock); if (!priv->gpio_used) { -- 2.26.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] USB: serial: ftdi_sio: Helpful error on GPIO attempt 2020-12-01 14:10 [PATCH] USB: serial: ftdi_sio: Helpful error on GPIO attempt Linus Walleij @ 2020-12-01 15:01 ` Marc Zyngier 2020-12-02 18:13 ` Marc Zyngier 0 siblings, 1 reply; 5+ messages in thread From: Marc Zyngier @ 2020-12-01 15:01 UTC (permalink / raw) To: Linus Walleij; +Cc: Johan Hovold, linux-usb Hi Linus, On 2020-12-01 14:10, Linus Walleij wrote: > The FTDI adapters present all potentially available GPIO > lines to userspace, and they are often also visibly > available on things like breakout boards. These are > appetizing targets for random GPIO tinkering such as > bit-banging or other industrial control over USB. > > When a user attempts to use one of the GPIO lines, they > can get the opaque error -ENODEV, because the flashed > configuration says that the line is not in GPIO mode > but another alternative function. > > We had one user run into this, debug and finally fix the > problem using ftx-prog. Well, you gave me 2/3 of the solution ;-). How about adding a pointer to this tool? [1] > > Give the user some more helpful dmesg text and a pointer > to ftx-prog when the error occurs. > > Reported-by: Marc Zyngier <maz@kernel.org> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/usb/serial/ftdi_sio.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/serial/ftdi_sio.c > b/drivers/usb/serial/ftdi_sio.c > index e0f4c3d9649c..405fec78f2fc 100644 > --- a/drivers/usb/serial/ftdi_sio.c > +++ b/drivers/usb/serial/ftdi_sio.c > @@ -1841,8 +1841,11 @@ static int ftdi_gpio_request(struct gpio_chip > *gc, unsigned int offset) > struct ftdi_private *priv = usb_get_serial_port_data(port); > int result; > > - if (priv->gpio_altfunc & BIT(offset)) > + if (priv->gpio_altfunc & BIT(offset)) { > + dev_err(&port->dev, "FTDI firmware says line is not in GPIO > mode\n"); > + dev_err(&port->dev, "if you really know what you're doing the flash > can be reconfigured using ftx-prog\n"); > return -ENODEV; > + } > > mutex_lock(&priv->gpio_lock); > if (!priv->gpio_used) { It occurs to me that since the driver already knows which of the CBUS pins are unusable, we should maybe find a way to expose the line as "reserved", one way or another? Generic tools such as gpioinfo would (or should?) be able to display the status of the pin to the user. enum gpio_v2_line_flag doesn't have a "reserved" flag, so maybe GPIO_V2_LINE_FLAG_USED is an adequate way to mark the line as being unavailable for userspace? Thanks, M. [1] https://github.com/richardeoin/ftx-prog -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] USB: serial: ftdi_sio: Helpful error on GPIO attempt 2020-12-01 15:01 ` Marc Zyngier @ 2020-12-02 18:13 ` Marc Zyngier 2020-12-08 8:39 ` Linus Walleij 0 siblings, 1 reply; 5+ messages in thread From: Marc Zyngier @ 2020-12-02 18:13 UTC (permalink / raw) To: Linus Walleij; +Cc: Johan Hovold, linux-usb On 2020-12-01 15:01, Marc Zyngier wrote: > Hi Linus, > > On 2020-12-01 14:10, Linus Walleij wrote: >> The FTDI adapters present all potentially available GPIO >> lines to userspace, and they are often also visibly >> available on things like breakout boards. These are >> appetizing targets for random GPIO tinkering such as >> bit-banging or other industrial control over USB. >> >> When a user attempts to use one of the GPIO lines, they >> can get the opaque error -ENODEV, because the flashed >> configuration says that the line is not in GPIO mode >> but another alternative function. >> >> We had one user run into this, debug and finally fix the >> problem using ftx-prog. > > Well, you gave me 2/3 of the solution ;-). How about adding > a pointer to this tool? [1] > >> >> Give the user some more helpful dmesg text and a pointer >> to ftx-prog when the error occurs. >> >> Reported-by: Marc Zyngier <maz@kernel.org> >> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> >> --- >> drivers/usb/serial/ftdi_sio.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/serial/ftdi_sio.c >> b/drivers/usb/serial/ftdi_sio.c >> index e0f4c3d9649c..405fec78f2fc 100644 >> --- a/drivers/usb/serial/ftdi_sio.c >> +++ b/drivers/usb/serial/ftdi_sio.c >> @@ -1841,8 +1841,11 @@ static int ftdi_gpio_request(struct gpio_chip >> *gc, unsigned int offset) >> struct ftdi_private *priv = usb_get_serial_port_data(port); >> int result; >> >> - if (priv->gpio_altfunc & BIT(offset)) >> + if (priv->gpio_altfunc & BIT(offset)) { >> + dev_err(&port->dev, "FTDI firmware says line is not in GPIO >> mode\n"); >> + dev_err(&port->dev, "if you really know what you're doing the flash >> can be reconfigured using ftx-prog\n"); >> return -ENODEV; >> + } >> >> mutex_lock(&priv->gpio_lock); >> if (!priv->gpio_used) { > > It occurs to me that since the driver already knows which of the CBUS > pins are unusable, we should maybe find a way to expose the line as > "reserved", one way or another? Generic tools such as gpioinfo would > (or should?) be able to display the status of the pin to the user. > > enum gpio_v2_line_flag doesn't have a "reserved" flag, so maybe > GPIO_V2_LINE_FLAG_USED is an adequate way to mark the line as > being unavailable for userspace? And to clarify what I mean, here's a patchlet that does the trick. maz@tiger-roach:~$ sudo gpioinfo gpiochip3 gpiochip3 - 4 lines: line 0: unnamed unused output active-high line 1: "AltFunc" kernel input active-high [used] line 2: "AltFunc" kernel input active-high [used] line 3: "AltFunc" kernel input active-high [used] It at least make clear that you can't grab the GPIO. Of course, you don't get the message that you just added... Thoughts? M. diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c index e0f4c3d9649c..00da3f42139f 100644 --- a/drivers/usb/serial/ftdi_sio.c +++ b/drivers/usb/serial/ftdi_sio.c @@ -44,6 +44,8 @@ #include "ftdi_sio.h" #include "ftdi_sio_ids.h" +#include "../../gpio/gpiolib.h" + #define DRIVER_AUTHOR "Greg Kroah-Hartman <greg@kroah.com>, Bill Ryder <bryder@sgi.com>, Kuba Ober <kuba@mareimbrium.org>, Andreas Mohr, Johan Hovold <jhovold@gmail.com>" #define DRIVER_DESC "USB FTDI Serial Converters Driver" @@ -2143,11 +2145,13 @@ static int ftdi_gpio_init_ftx(struct usb_serial_port *port) return result; } +static const char *altfunc = "AltFunc"; + static int ftdi_gpio_init(struct usb_serial_port *port) { struct ftdi_private *priv = usb_get_serial_port_data(port); struct usb_serial *serial = port->serial; - int result; + int result, i; switch (priv->chip_type) { case FT232H: @@ -2183,10 +2187,23 @@ static int ftdi_gpio_init(struct usb_serial_port *port) priv->gc.can_sleep = true; result = gpiochip_add_data(&priv->gc, port); - if (!result) - priv->gpio_registered = true; + if (result) + return result; - return result; + priv->gpio_registered = true; + + for (i = 0; i < priv->gc.ngpio; i++) { + struct gpio_desc *desc; + + if (!(priv->gpio_altfunc & BIT(i))) + continue; + + desc = gpiochip_get_desc(&priv->gc, i); + desc->flags |= BIT(FLAG_REQUESTED); + desc->name = altfunc; + } + + return 0; } static void ftdi_gpio_remove(struct usb_serial_port *port) -- Jazz is not dead. It just smells funny... ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] USB: serial: ftdi_sio: Helpful error on GPIO attempt 2020-12-02 18:13 ` Marc Zyngier @ 2020-12-08 8:39 ` Linus Walleij 2020-12-08 9:02 ` Johan Hovold 0 siblings, 1 reply; 5+ messages in thread From: Linus Walleij @ 2020-12-08 8:39 UTC (permalink / raw) To: Marc Zyngier; +Cc: Johan Hovold, linux-usb On Wed, Dec 2, 2020 at 7:13 PM Marc Zyngier <maz@kernel.org> wrote: > And to clarify what I mean, here's a patchlet that does the trick. > > maz@tiger-roach:~$ sudo gpioinfo gpiochip3 > gpiochip3 - 4 lines: > line 0: unnamed unused output active-high > line 1: "AltFunc" kernel input active-high [used] > line 2: "AltFunc" kernel input active-high [used] > line 3: "AltFunc" kernel input active-high [used] > > It at least make clear that you can't grab the GPIO. Of course, you > don't get the message that you just added... I think this looks good. > +#include "../../gpio/gpiolib.h" I suppose I can live with this because it's for a noble cause. > Thoughts? Could you combine it with a patch similar to min giving some probe() time information that some GPIO lines are altfunc and where to get the tool and send a proper patch? Yours, Linus Walleij ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] USB: serial: ftdi_sio: Helpful error on GPIO attempt 2020-12-08 8:39 ` Linus Walleij @ 2020-12-08 9:02 ` Johan Hovold 0 siblings, 0 replies; 5+ messages in thread From: Johan Hovold @ 2020-12-08 9:02 UTC (permalink / raw) To: Linus Walleij; +Cc: Marc Zyngier, Johan Hovold, linux-usb On Tue, Dec 08, 2020 at 09:39:25AM +0100, Linus Walleij wrote: > On Wed, Dec 2, 2020 at 7:13 PM Marc Zyngier <maz@kernel.org> wrote: > > > And to clarify what I mean, here's a patchlet that does the trick. > > > > maz@tiger-roach:~$ sudo gpioinfo gpiochip3 > > gpiochip3 - 4 lines: > > line 0: unnamed unused output active-high > > line 1: "AltFunc" kernel input active-high [used] > > line 2: "AltFunc" kernel input active-high [used] > > line 3: "AltFunc" kernel input active-high [used] > > > > It at least make clear that you can't grab the GPIO. Of course, you > > don't get the message that you just added... > > I think this looks good. > > > +#include "../../gpio/gpiolib.h" > > I suppose I can live with this because it's for a noble cause. Not needed, Marc fixed that bit up in his series. > > Thoughts? > > Could you combine it with a patch similar to min giving some probe() time > information that some GPIO lines are altfunc and where to get the tool > and send a proper patch? He already did that, but I'm sceptical to spamming the logs with this (we don't explain how people should setup up pinmuxing in their device trees in the logs either). Please take a look at the discussion following Marc's series. Johan ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-08 9:03 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-01 14:10 [PATCH] USB: serial: ftdi_sio: Helpful error on GPIO attempt Linus Walleij 2020-12-01 15:01 ` Marc Zyngier 2020-12-02 18:13 ` Marc Zyngier 2020-12-08 8:39 ` Linus Walleij 2020-12-08 9:02 ` Johan Hovold
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.