[PATCH 0/4] USB: ftdio_sio: GPIO validity fixes

[PATCH 0/4] USB: ftdio_sio: GPIO validity fixes

[Marc Zyngier]

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.

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

[PATCH 1/4] gpiolib: cdev: Flag invalid GPIOs as used

[Marc Zyngier]

When reporting the state of a GPIO to userspace, we never check
for the actual validity of the line, meaning we report invalid
lines as being usable. A subsequent request will fail though,
which is an inconsistent behaviour from a userspace perspective.

Instead, let's check for the validity of the line and report it
as used if it is invalid. This allows a tool such as gpioinfo
to report something sensible:

gpiochip3 - 4 lines:
	line   0:      unnamed       unused   input  active-high
	line   1:      unnamed       kernel   input  active-high [used]
	line   2:      unnamed       kernel   input  active-high [used]
	line   3:      unnamed       unused   input  active-high

In this example, lines 1 and 2 are invalid, and cannot be used by
userspace.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/gpio/gpiolib-cdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index e9faeaf65d14..a0fcb4ccaa02 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1910,6 +1910,7 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 	    test_bit(FLAG_USED_AS_IRQ, &desc->flags) ||
 	    test_bit(FLAG_EXPORT, &desc->flags) ||
 	    test_bit(FLAG_SYSFS, &desc->flags) ||
+	    !gpiochip_line_is_valid(gc, info->offset) ||
 	    !ok_for_pinctrl)
 		info->flags |= GPIO_V2_LINE_FLAG_USED;
 
-- 
2.28.0

[PATCH 2/4] USB: serial: ftdi_sio: Report the valid GPIO lines to gpiolib

[Marc Zyngier]

Since it is pretty common for only some of the CBUS lines to be
valid as GPIO lines, let's report such validity to the rest of
the kernel.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/usb/serial/ftdi_sio.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index e0f4c3d9649c..13e575f16bcd 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -2002,6 +2002,19 @@ static int ftdi_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio,
 	return result;
 }
 
+static int ftdi_gpio_init_valid_mask(struct gpio_chip *gc,
+				     unsigned long *valid_mask,
+				     unsigned int ngpios)
+{
+	struct usb_serial_port *port = gpiochip_get_data(gc);
+	struct ftdi_private *priv = usb_get_serial_port_data(port);
+	unsigned long map = priv->gpio_altfunc;
+
+	bitmap_complement(valid_mask, &map, ngpios);
+
+	return 0;
+}
+
 static int ftdi_read_eeprom(struct usb_serial *serial, void *dst, u16 addr,
 				u16 nbytes)
 {
@@ -2173,6 +2186,7 @@ static int ftdi_gpio_init(struct usb_serial_port *port)
 	priv->gc.get_direction = ftdi_gpio_direction_get;
 	priv->gc.direction_input = ftdi_gpio_direction_input;
 	priv->gc.direction_output = ftdi_gpio_direction_output;
+	priv->gc.init_valid_mask = ftdi_gpio_init_valid_mask;
 	priv->gc.get = ftdi_gpio_get;
 	priv->gc.set = ftdi_gpio_set;
 	priv->gc.get_multiple = ftdi_gpio_get_multiple;
-- 
2.28.0

[PATCH 3/4] USB: serial: ftdi_sio: Log the CBUS GPIO validity

[Marc Zyngier]

The validity of the ftdi CBUS GPIO is pretty hidden so far,
and finding out *why* some GPIOs don't work is sometimes
hard to identify. So let's help the user by displaying the
map of the CBUS pins that are valid for a GPIO.

Also, tell the user about the magic ftx-prog tool that can
make GPIOs appear: https://github.com/richardeoin/ftx-prog

Suggested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/usb/serial/ftdi_sio.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 13e575f16bcd..b9d3b33891fc 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -2012,6 +2012,15 @@ static int ftdi_gpio_init_valid_mask(struct gpio_chip *gc,
 
 	bitmap_complement(valid_mask, &map, ngpios);
 
+	if (bitmap_empty(valid_mask, ngpios))
+		dev_warn(&port->dev, "No usable GPIO\n");
+	else
+		dev_info(&port->dev, "Enabling CBUS%*pbl for GPIO\n",
+			 ngpios, valid_mask);
+
+	if (!bitmap_full(valid_mask, ngpios))
+		dev_warn_once(&port->dev, "Consider using a tool such as ftx-prog to enable GPIOs if required\n");
+
 	return 0;
 }
 
-- 
2.28.0

[PATCH 4/4] USB: serial: ftdi_sio: Drop GPIO line checking dead code

[Marc Zyngier]

Now that the code code can track the validity of GPIO pins,
there is no need to check whether the line is valid.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/usb/serial/ftdi_sio.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index b9d3b33891fc..2c8a0b9aac92 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1841,9 +1841,6 @@ 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))
-		return -ENODEV;
-
 	mutex_lock(&priv->gpio_lock);
 	if (!priv->gpio_used) {
 		/* Set default pin states, as we cannot get them from device */
-- 
2.28.0

Re: [PATCH 0/4] USB: ftdio_sio: GPIO validity fixes

[Andy Shevchenko]

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
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] USB: ftdio_sio: GPIO validity fixes

[Johan Hovold]

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

Re: [PATCH 1/4] gpiolib: cdev: Flag invalid GPIOs as used

[Johan Hovold]

On Fri, Dec 04, 2020 at 04:47:36PM +0000, Marc Zyngier wrote:
> When reporting the state of a GPIO to userspace, we never check
> for the actual validity of the line, meaning we report invalid
> lines as being usable. A subsequent request will fail though,
> which is an inconsistent behaviour from a userspace perspective.
> 
> Instead, let's check for the validity of the line and report it
> as used if it is invalid. This allows a tool such as gpioinfo
> to report something sensible:
> 
> gpiochip3 - 4 lines:
> 	line   0:      unnamed       unused   input  active-high
> 	line   1:      unnamed       kernel   input  active-high [used]
> 	line   2:      unnamed       kernel   input  active-high [used]
> 	line   3:      unnamed       unused   input  active-high
> 
> In this example, lines 1 and 2 are invalid, and cannot be used by
> userspace.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/gpio/gpiolib-cdev.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index e9faeaf65d14..a0fcb4ccaa02 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -1910,6 +1910,7 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
>  	    test_bit(FLAG_USED_AS_IRQ, &desc->flags) ||
>  	    test_bit(FLAG_EXPORT, &desc->flags) ||
>  	    test_bit(FLAG_SYSFS, &desc->flags) ||
> +	    !gpiochip_line_is_valid(gc, info->offset) ||
>  	    !ok_for_pinctrl)
>  		info->flags |= GPIO_V2_LINE_FLAG_USED;

So this is somewhat separate from the rest of the series in case it
applies also to gpio chips with reserved ranges (e.g.
"gpio-reserved-ranges" devicetree property). Are they currently reported
as available?

Looks like this will work well also for USB gpio controllers with static
muxing configured in EEPROM, especially as that is how we already report
pins reported as unavailable by pinctrl (i.e. ok_for_pinctrl).

Johan

Re: [PATCH 3/4] USB: serial: ftdi_sio: Log the CBUS GPIO validity

[Johan Hovold]

On Fri, Dec 04, 2020 at 04:47:38PM +0000, Marc Zyngier wrote:
> The validity of the ftdi CBUS GPIO is pretty hidden so far,
> and finding out *why* some GPIOs don't work is sometimes
> hard to identify. So let's help the user by displaying the
> map of the CBUS pins that are valid for a GPIO.
> 
> Also, tell the user about the magic ftx-prog tool that can
> make GPIOs appear: https://github.com/richardeoin/ftx-prog
> 
> Suggested-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/usb/serial/ftdi_sio.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index 13e575f16bcd..b9d3b33891fc 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -2012,6 +2012,15 @@ static int ftdi_gpio_init_valid_mask(struct gpio_chip *gc,
>  
>  	bitmap_complement(valid_mask, &map, ngpios);
>  
> +	if (bitmap_empty(valid_mask, ngpios))
> +		dev_warn(&port->dev, "No usable GPIO\n");

This isn't an error of any kind, and certainly not something that
deserves a warning in the system log on every probe. Not everyone cares
about the GPIO interface.

> +	else
> +		dev_info(&port->dev, "Enabling CBUS%*pbl for GPIO\n",
> +			 ngpios, valid_mask);

And while printing this mask has some worth I'm still reluctant to be
spamming the logs with it. Just like gpolib has a dev_dbg() for
registering chips, this should probably be demoted to KERN_DEBUG as
well.

> +
> +	if (!bitmap_full(valid_mask, ngpios))
> +		dev_warn_once(&port->dev, "Consider using a tool such as ftx-prog to enable GPIOs if required\n");
> +

And again, this is not something that belongs in the logs of just about
every system with an attached ftdi device.

While not possible to combine with the valid_mask approach, this is
something which we could otherwise add to the request() callback for the
first request that fails due to the mux configuration.

>  	return 0;
>  }

Johan

Re: [PATCH 0/4] USB: ftdio_sio: GPIO validity fixes

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/4] gpiolib: cdev: Flag invalid GPIOs as used

[Marc Zyngier]

On 2020-12-07 14:16, Johan Hovold wrote:
> On Fri, Dec 04, 2020 at 04:47:36PM +0000, Marc Zyngier wrote:
>> When reporting the state of a GPIO to userspace, we never check
>> for the actual validity of the line, meaning we report invalid
>> lines as being usable. A subsequent request will fail though,
>> which is an inconsistent behaviour from a userspace perspective.
>> 
>> Instead, let's check for the validity of the line and report it
>> as used if it is invalid. This allows a tool such as gpioinfo
>> to report something sensible:
>> 
>> gpiochip3 - 4 lines:
>> 	line   0:      unnamed       unused   input  active-high
>> 	line   1:      unnamed       kernel   input  active-high [used]
>> 	line   2:      unnamed       kernel   input  active-high [used]
>> 	line   3:      unnamed       unused   input  active-high
>> 
>> In this example, lines 1 and 2 are invalid, and cannot be used by
>> userspace.
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  drivers/gpio/gpiolib-cdev.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
>> index e9faeaf65d14..a0fcb4ccaa02 100644
>> --- a/drivers/gpio/gpiolib-cdev.c
>> +++ b/drivers/gpio/gpiolib-cdev.c
>> @@ -1910,6 +1910,7 @@ static void gpio_desc_to_lineinfo(struct 
>> gpio_desc *desc,
>>  	    test_bit(FLAG_USED_AS_IRQ, &desc->flags) ||
>>  	    test_bit(FLAG_EXPORT, &desc->flags) ||
>>  	    test_bit(FLAG_SYSFS, &desc->flags) ||
>> +	    !gpiochip_line_is_valid(gc, info->offset) ||
>>  	    !ok_for_pinctrl)
>>  		info->flags |= GPIO_V2_LINE_FLAG_USED;
> 
> So this is somewhat separate from the rest of the series in case it
> applies also to gpio chips with reserved ranges (e.g.
> "gpio-reserved-ranges" devicetree property). Are they currently 
> reported
> as available?

I don't have any HW that uses this, but gpiolib-of.c makes use of it to
expose the valid GPIO range. I expect these systems suffer from the same
issue.

> Looks like this will work well also for USB gpio controllers with 
> static
> muxing configured in EEPROM, especially as that is how we already 
> report
> pins reported as unavailable by pinctrl (i.e. ok_for_pinctrl).

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 3/4] USB: serial: ftdi_sio: Log the CBUS GPIO validity

[Marc Zyngier]

On 2020-12-07 14:29, Johan Hovold wrote:
> On Fri, Dec 04, 2020 at 04:47:38PM +0000, Marc Zyngier wrote:
>> The validity of the ftdi CBUS GPIO is pretty hidden so far,
>> and finding out *why* some GPIOs don't work is sometimes
>> hard to identify. So let's help the user by displaying the
>> map of the CBUS pins that are valid for a GPIO.
>> 
>> Also, tell the user about the magic ftx-prog tool that can
>> make GPIOs appear: https://github.com/richardeoin/ftx-prog
>> 
>> Suggested-by: Linus Walleij <linus.walleij@linaro.org>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  drivers/usb/serial/ftdi_sio.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/drivers/usb/serial/ftdi_sio.c 
>> b/drivers/usb/serial/ftdi_sio.c
>> index 13e575f16bcd..b9d3b33891fc 100644
>> --- a/drivers/usb/serial/ftdi_sio.c
>> +++ b/drivers/usb/serial/ftdi_sio.c
>> @@ -2012,6 +2012,15 @@ static int ftdi_gpio_init_valid_mask(struct 
>> gpio_chip *gc,
>> 
>>  	bitmap_complement(valid_mask, &map, ngpios);
>> 
>> +	if (bitmap_empty(valid_mask, ngpios))
>> +		dev_warn(&port->dev, "No usable GPIO\n");
> 
> This isn't an error of any kind, and certainly not something that
> deserves a warning in the system log on every probe. Not everyone cares
> about the GPIO interface.
> 
>> +	else
>> +		dev_info(&port->dev, "Enabling CBUS%*pbl for GPIO\n",
>> +			 ngpios, valid_mask);
> 
> And while printing this mask has some worth I'm still reluctant to be
> spamming the logs with it. Just like gpolib has a dev_dbg() for
> registering chips, this should probably be demoted to KERN_DEBUG as
> well.
> 
>> +
>> +	if (!bitmap_full(valid_mask, ngpios))
>> +		dev_warn_once(&port->dev, "Consider using a tool such as ftx-prog 
>> to enable GPIOs if required\n");
>> +
> 
> And again, this is not something that belongs in the logs of just about
> every system with an attached ftdi device.

Fine by me, this patch can be dropped without issue. After all,
I now know how to deal with these chips.

> While not possible to combine with the valid_mask approach, this is
> something which we could otherwise add to the request() callback for 
> the
> first request that fails due to the mux configuration.

That was Linus' initial suggestion. But I think a consistent user
API is more important than free advise in the kernel log.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 0/4] USB: ftdio_sio: GPIO validity fixes

[Johan Hovold]

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

Re: [PATCH 3/4] USB: serial: ftdi_sio: Log the CBUS GPIO validity

[Johan Hovold]

On Mon, Dec 07, 2020 at 03:00:37PM +0000, Marc Zyngier wrote:
> On 2020-12-07 14:29, Johan Hovold wrote:
> > On Fri, Dec 04, 2020 at 04:47:38PM +0000, Marc Zyngier wrote:

> >> +	if (!bitmap_full(valid_mask, ngpios))
> >> +		dev_warn_once(&port->dev, "Consider using a tool such as ftx-prog 
> >> to enable GPIOs if required\n");
> >> +
> > 
> > And again, this is not something that belongs in the logs of just about
> > every system with an attached ftdi device.
> 
> Fine by me, this patch can be dropped without issue. After all,
> I now know how to deal with these chips.
> 
> > While not possible to combine with the valid_mask approach, this is
> > something which we could otherwise add to the request() callback for 
> > the
> > first request that fails due to the mux configuration.
> 
> That was Linus' initial suggestion. But I think a consistent user
> API is more important than free advise in the kernel log.

I tend to agree. So since your valid-mask approach clearly has some
merit in that it marks the lines in use when using the new cdev
interface, perhaps we should stick with that.

Johan

Re: [PATCH 0/4] USB: ftdio_sio: GPIO validity fixes

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 0/4] USB: ftdio_sio: GPIO validity fixes

[Johan Hovold]

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

Re: [PATCH 0/4] USB: ftdio_sio: GPIO validity fixes

[Linus Walleij]

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

Re: [PATCH 1/4] gpiolib: cdev: Flag invalid GPIOs as used

[Linus Walleij]

On Fri, Dec 4, 2020 at 5:47 PM Marc Zyngier <maz@kernel.org> wrote:

> When reporting the state of a GPIO to userspace, we never check
> for the actual validity of the line, meaning we report invalid
> lines as being usable. A subsequent request will fail though,
> which is an inconsistent behaviour from a userspace perspective.
>
> Instead, let's check for the validity of the line and report it
> as used if it is invalid. This allows a tool such as gpioinfo
> to report something sensible:
>
> gpiochip3 - 4 lines:
>         line   0:      unnamed       unused   input  active-high
>         line   1:      unnamed       kernel   input  active-high [used]
>         line   2:      unnamed       kernel   input  active-high [used]
>         line   3:      unnamed       unused   input  active-high
>
> In this example, lines 1 and 2 are invalid, and cannot be used by
> userspace.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

This makes sense, so patch applied (unless Bartosz tells me
otherwise, then I'll pull it out again).

Yours,
Linus Walleij

Re: [PATCH 2/4] USB: serial: ftdi_sio: Report the valid GPIO lines to gpiolib

[Linus Walleij]

On Fri, Dec 4, 2020 at 5:47 PM Marc Zyngier <maz@kernel.org> wrote:

> Since it is pretty common for only some of the CBUS lines to be
> valid as GPIO lines, let's report such validity to the rest of
> the kernel.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 3/4] USB: serial: ftdi_sio: Log the CBUS GPIO validity

[Linus Walleij]

On Mon, Dec 7, 2020 at 4:19 PM Johan Hovold <johan@kernel.org> wrote:
> On Mon, Dec 07, 2020 at 03:00:37PM +0000, Marc Zyngier wrote:
> > On 2020-12-07 14:29, Johan Hovold wrote:
> > > On Fri, Dec 04, 2020 at 04:47:38PM +0000, Marc Zyngier wrote:
>
> > >> +  if (!bitmap_full(valid_mask, ngpios))
> > >> +          dev_warn_once(&port->dev, "Consider using a tool such as ftx-prog
> > >> to enable GPIOs if required\n");
> > >> +
> > >
> > > And again, this is not something that belongs in the logs of just about
> > > every system with an attached ftdi device.
> >
> > Fine by me, this patch can be dropped without issue. After all,
> > I now know how to deal with these chips.
> >
> > > While not possible to combine with the valid_mask approach, this is
> > > something which we could otherwise add to the request() callback for
> > > the
> > > first request that fails due to the mux configuration.
> >
> > That was Linus' initial suggestion. But I think a consistent user
> > API is more important than free advise in the kernel log.
>
> I tend to agree. So since your valid-mask approach clearly has some
> merit in that it marks the lines in use when using the new cdev
> interface, perhaps we should stick with that.

It sounds like we agree that this patch sans prints is acceptable.

It makes things better so let's go with that.

The problem for the user is that the line looks to be
"used by the kernel" (true in some sense) but they have no
idea what to do about it and that the ftx-prog will solve
their hacking problem.

It's a matter of taste admittedly, I have noticed that some
subsystem maintainers are "dmesg minimalists" and want
as little as possible in dmesg while some are "dmesg maximalists"
and want as much messages and help as possible for
users in dmesg. I tend toward the latter but it's not like
I don't see the beauty and feeling of control that comes
with a clean dmesg.

My usual argument is that different loglevels exist for a
reason and those who don't want advice can just filter out
anything but errors or worse. But it seems they don't really wanna
hear that because on their pet systems KERN_INFO it is on
by default so it bothers them.

Yours,
Linus Walleij

Re: [PATCH 0/4] USB: ftdio_sio: GPIO validity fixes

[Johan Hovold]

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

Re: [PATCH 3/4] USB: serial: ftdi_sio: Log the CBUS GPIO validity

[Johan Hovold]

On Wed, Dec 09, 2020 at 10:35:53AM +0100, Linus Walleij wrote:
> On Mon, Dec 7, 2020 at 4:19 PM Johan Hovold <johan@kernel.org> wrote:
> > On Mon, Dec 07, 2020 at 03:00:37PM +0000, Marc Zyngier wrote:
> > > On 2020-12-07 14:29, Johan Hovold wrote:
> > > > On Fri, Dec 04, 2020 at 04:47:38PM +0000, Marc Zyngier wrote:
> >
> > > >> +  if (!bitmap_full(valid_mask, ngpios))
> > > >> +          dev_warn_once(&port->dev, "Consider using a tool such as ftx-prog
> > > >> to enable GPIOs if required\n");
> > > >> +
> > > >
> > > > And again, this is not something that belongs in the logs of just about
> > > > every system with an attached ftdi device.
> > >
> > > Fine by me, this patch can be dropped without issue. After all,
> > > I now know how to deal with these chips.
> > >
> > > > While not possible to combine with the valid_mask approach, this is
> > > > something which we could otherwise add to the request() callback for
> > > > the
> > > > first request that fails due to the mux configuration.
> > >
> > > That was Linus' initial suggestion. But I think a consistent user
> > > API is more important than free advise in the kernel log.
> >
> > I tend to agree. So since your valid-mask approach clearly has some
> > merit in that it marks the lines in use when using the new cdev
> > interface, perhaps we should stick with that.
> 
> It sounds like we agree that this patch sans prints is acceptable.
> 
> It makes things better so let's go with that.

Sounds good.

I'm about to apply patches 2, 3 and 4 with some smaller changes like
demoting the printk messages to KERN_DEBUG and dropping the ftx-progs
warning.

> The problem for the user is that the line looks to be
> "used by the kernel" (true in some sense) but they have no
> idea what to do about it and that the ftx-prog will solve
> their hacking problem.

Right, it's not ideal, but the datasheets for these devices clearly
states that the configuration of the CBUS pins is done in EEPROM and the
vendor provides some tool to do that. Then there's a bunch of open
source implementations for the same including ftx-progs (which can only
be used for a subset of these devices).

I'd be fine with a dev_err() on the first request that fails saying that
the CBUS pin is not configured for GPIO use (perhaps even on every
request if its not something that a non-root user can trigger). But we
cannot have both that and have the line marked in-use through the
chardev interface currently.

I'm admittedly a bit torn on which is preferable.

Johan

Re: [PATCH 3/4] USB: serial: ftdi_sio: Log the CBUS GPIO validity

[Johan Hovold]

On Wed, Dec 09, 2020 at 06:05:58PM +0100, Johan Hovold wrote:
> On Wed, Dec 09, 2020 at 10:35:53AM +0100, Linus Walleij wrote:

> > It sounds like we agree that this patch sans prints is acceptable.
> > 
> > It makes things better so let's go with that.
> 
> Sounds good.
> 
> I'm about to apply patches 2, 3 and 4 with some smaller changes like
> demoting the printk messages to KERN_DEBUG and dropping the ftx-progs
> warning.
>
> > The problem for the user is that the line looks to be
> > "used by the kernel" (true in some sense) but they have no
> > idea what to do about it and that the ftx-prog will solve
> > their hacking problem.
> 
> Right, it's not ideal, but the datasheets for these devices clearly
> states that the configuration of the CBUS pins is done in EEPROM and the
> vendor provides some tool to do that. Then there's a bunch of open
> source implementations for the same including ftx-progs (which can only
> be used for a subset of these devices).
> 
> I'd be fine with a dev_err() on the first request that fails saying that
> the CBUS pin is not configured for GPIO use (perhaps even on every
> request if its not something that a non-root user can trigger). But we
> cannot have both that and have the line marked in-use through the
> chardev interface currently.
> 
> I'm admittedly a bit torn on which is preferable.

I've applied the patches now. Having this reported through the chardev
interface must be better than having to match up a failed request with
something in the system log.

Johan