Re: [PATCH] USB: serial: ftdi_sio: Helpful error on GPIO attempt

[Marc Zyngier <maz@kernel.org>]

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...