LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v1] HID: ft260: fix device removal due to USB disconnect
@ 2021-07-20 14:49 Michael Zaidman
  2021-07-28  9:47 ` Jiri Kosina
  0 siblings, 1 reply; 4+ messages in thread
From: Michael Zaidman @ 2021-07-20 14:49 UTC (permalink / raw)
  To: jikos, benjamin.tissoires, aaron.jones
  Cc: linux-kernel, linux-input, linux-i2c, Michael Zaidman

This commit fixes a regression introduced by the commit 82f09a637dd3
("HID: ft260: improve error handling of ft260_hid_feature_report_get()")
when upon USB disconnect, the ft260 i2c device is still available within
the /dev folder.

Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
Acked-by: Aaron Jones (FTDI-UK) <aaron.jones@ftdichip.com>
---
 drivers/hid/hid-ft260.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index f43a8406cb9a..fa73a35aaf98 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -742,8 +742,8 @@ static int ft260_is_interface_enabled(struct hid_device *hdev)
 	int ret;
 
 	ret = ft260_get_system_config(hdev, &cfg);
-	if (ret)
-		return ret;
+	if (ret < 0)
+		return 0;
 
 	ft260_dbg("interface:  0x%02x\n", interface);
 	ft260_dbg("chip mode:  0x%02x\n", cfg.chip_mode);
@@ -754,23 +754,16 @@ static int ft260_is_interface_enabled(struct hid_device *hdev)
 	switch (cfg.chip_mode) {
 	case FT260_MODE_ALL:
 	case FT260_MODE_BOTH:
-		if (interface == 1) {
+		if (interface == 1)
 			hid_info(hdev, "uart interface is not supported\n");
-			return 0;
-		}
-		ret = 1;
+		else
+			ret = 1;
 		break;
 	case FT260_MODE_UART:
-		if (interface == 0) {
-			hid_info(hdev, "uart is unsupported on interface 0\n");
-			ret = 0;
-		}
+		hid_info(hdev, "uart interface is not supported\n");
 		break;
 	case FT260_MODE_I2C:
-		if (interface == 1) {
-			hid_info(hdev, "i2c is unsupported on interface 1\n");
-			ret = 0;
-		}
+		ret = 1;
 		break;
 	}
 	return ret;
@@ -1004,11 +997,9 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 static void ft260_remove(struct hid_device *hdev)
 {
-	int ret;
 	struct ft260_device *dev = hid_get_drvdata(hdev);
 
-	ret = ft260_is_interface_enabled(hdev);
-	if (ret <= 0)
+	if (!dev)
 		return;
 
 	sysfs_remove_group(&hdev->dev.kobj, &ft260_attr_group);
-- 
2.25.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v1] HID: ft260: fix device removal due to USB disconnect
  2021-07-20 14:49 [PATCH v1] HID: ft260: fix device removal due to USB disconnect Michael Zaidman
@ 2021-07-28  9:47 ` Jiri Kosina
  2021-07-29 10:26   ` [PATCH v2] " Michael Zaidman
  0 siblings, 1 reply; 4+ messages in thread
From: Jiri Kosina @ 2021-07-28  9:47 UTC (permalink / raw)
  To: Michael Zaidman
  Cc: benjamin.tissoires, aaron.jones, linux-kernel, linux-input, linux-i2c

On Tue, 20 Jul 2021, Michael Zaidman wrote:

> This commit fixes a regression introduced by the commit 82f09a637dd3
> ("HID: ft260: improve error handling of ft260_hid_feature_report_get()")
> when upon USB disconnect, the ft260 i2c device is still available within
> the /dev folder.

Thanks for the fix. Could you please make the changelog a little bit more 
verbose though? Namely describe the issue in the code the patch is fixing 
(and how). Thanks,

-- 
Jiri Kosina
SUSE Labs


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH v2] HID: ft260: fix device removal due to USB disconnect
  2021-07-28  9:47 ` Jiri Kosina
@ 2021-07-29 10:26   ` Michael Zaidman
  2021-07-29 10:39     ` Jiri Kosina
  0 siblings, 1 reply; 4+ messages in thread
From: Michael Zaidman @ 2021-07-29 10:26 UTC (permalink / raw)
  To: jikos, benjamin.tissoires, aaron.jones
  Cc: linux-kernel, linux-input, linux-i2c, Michael Zaidman

This commit fixes a functional regression introduced by the commit 82f09a637dd3
("HID: ft260: improve error handling of ft260_hid_feature_report_get()")
when upon USB disconnect, the FTDI FT260 i2c device is still available within
the /dev folder.

In my company's product, where the host USB to FT260 USB connection is
hard-wired in the PCB, the issue is not reproducible. To reproduce it, I used
the VirtualBox Ubuntu 20.04 VM and the UMFT260EV1A development module for the
FTDI FT260 chip:

Plug the UMFT260EV1A module into a USB port and attach it to VM.

The VM shows 2 i2c devices under the /dev:
    michael@michael-VirtualBox:~$ ls /dev/i2c-*
    /dev/i2c-0  /dev/i2c-1

The i2c-0 is not related to the FTDI FT260:
    michael@michael-VirtualBox:~$ cat /sys/bus/i2c/devices/i2c-0/name
    SMBus PIIX4 adapter at 4100

The i2c-1 is created by hid-ft260.ko:
    michael@michael-VirtualBox:~$ cat /sys/bus/i2c/devices/i2c-1/name
    FT260 usb-i2c bridge on hidraw1

Now, detach the FTDI FT260 USB device from VM. We expect the /dev/i2c-1
to disappear, but it's still here:
    michael@michael-VirtualBox:~$ ls /dev/i2c-*
    /dev/i2c-0  /dev/i2c-1

And the kernel log shows:
    [  +0.001202] usb 2-2: USB disconnect, device number 3
    [  +0.000109] ft260 0003:0403:6030.0002: failed to retrieve system status
    [  +0.000316] ft260 0003:0403:6030.0003: failed to retrieve system status

It happens because the commit 82f09a637dd3 changed the ft260_get_system_config()
return logic. This caused the ft260_is_interface_enabled() to exit with error
upon the FT260 device USB disconnect, which in turn, aborted the ft260_remove()
before deleting the FT260 i2c device and cleaning its sysfs stuff.

This commit restores the FT260 USB removal functionality and improves the
ft260_is_interface_enabled() code to handle correctly all chip modes defined
by the device interface configuration pins DCNF0 and DCNF1.

Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
Acked-by: Aaron Jones (FTDI-UK) <aaron.jones@ftdichip.com>
---
 drivers/hid/hid-ft260.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index f43a8406cb9a..e73776ae6976 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -742,7 +742,7 @@ static int ft260_is_interface_enabled(struct hid_device *hdev)
 	int ret;
 
 	ret = ft260_get_system_config(hdev, &cfg);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	ft260_dbg("interface:  0x%02x\n", interface);
@@ -754,23 +754,16 @@ static int ft260_is_interface_enabled(struct hid_device *hdev)
 	switch (cfg.chip_mode) {
 	case FT260_MODE_ALL:
 	case FT260_MODE_BOTH:
-		if (interface == 1) {
+		if (interface == 1)
 			hid_info(hdev, "uart interface is not supported\n");
-			return 0;
-		}
-		ret = 1;
+		else
+			ret = 1;
 		break;
 	case FT260_MODE_UART:
-		if (interface == 0) {
-			hid_info(hdev, "uart is unsupported on interface 0\n");
-			ret = 0;
-		}
+		hid_info(hdev, "uart interface is not supported\n");
 		break;
 	case FT260_MODE_I2C:
-		if (interface == 1) {
-			hid_info(hdev, "i2c is unsupported on interface 1\n");
-			ret = 0;
-		}
+		ret = 1;
 		break;
 	}
 	return ret;
@@ -1004,11 +997,9 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 static void ft260_remove(struct hid_device *hdev)
 {
-	int ret;
 	struct ft260_device *dev = hid_get_drvdata(hdev);
 
-	ret = ft260_is_interface_enabled(hdev);
-	if (ret <= 0)
+	if (!dev)
 		return;
 
 	sysfs_remove_group(&hdev->dev.kobj, &ft260_attr_group);
-- 
2.25.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] HID: ft260: fix device removal due to USB disconnect
  2021-07-29 10:26   ` [PATCH v2] " Michael Zaidman
@ 2021-07-29 10:39     ` Jiri Kosina
  0 siblings, 0 replies; 4+ messages in thread
From: Jiri Kosina @ 2021-07-29 10:39 UTC (permalink / raw)
  To: Michael Zaidman
  Cc: benjamin.tissoires, aaron.jones, linux-kernel, linux-input, linux-i2c

On Thu, 29 Jul 2021, Michael Zaidman wrote:

> This commit fixes a functional regression introduced by the commit 82f09a637dd3
> ("HID: ft260: improve error handling of ft260_hid_feature_report_get()")
> when upon USB disconnect, the FTDI FT260 i2c device is still available within
> the /dev folder.
> 
> In my company's product, where the host USB to FT260 USB connection is
> hard-wired in the PCB, the issue is not reproducible. To reproduce it, I used
> the VirtualBox Ubuntu 20.04 VM and the UMFT260EV1A development module for the
> FTDI FT260 chip:
> 
> Plug the UMFT260EV1A module into a USB port and attach it to VM.
> 
> The VM shows 2 i2c devices under the /dev:
>     michael@michael-VirtualBox:~$ ls /dev/i2c-*
>     /dev/i2c-0  /dev/i2c-1
> 
> The i2c-0 is not related to the FTDI FT260:
>     michael@michael-VirtualBox:~$ cat /sys/bus/i2c/devices/i2c-0/name
>     SMBus PIIX4 adapter at 4100
> 
> The i2c-1 is created by hid-ft260.ko:
>     michael@michael-VirtualBox:~$ cat /sys/bus/i2c/devices/i2c-1/name
>     FT260 usb-i2c bridge on hidraw1
> 
> Now, detach the FTDI FT260 USB device from VM. We expect the /dev/i2c-1
> to disappear, but it's still here:
>     michael@michael-VirtualBox:~$ ls /dev/i2c-*
>     /dev/i2c-0  /dev/i2c-1
> 
> And the kernel log shows:
>     [  +0.001202] usb 2-2: USB disconnect, device number 3
>     [  +0.000109] ft260 0003:0403:6030.0002: failed to retrieve system status
>     [  +0.000316] ft260 0003:0403:6030.0003: failed to retrieve system status
> 
> It happens because the commit 82f09a637dd3 changed the ft260_get_system_config()
> return logic. This caused the ft260_is_interface_enabled() to exit with error
> upon the FT260 device USB disconnect, which in turn, aborted the ft260_remove()
> before deleting the FT260 i2c device and cleaning its sysfs stuff.
> 
> This commit restores the FT260 USB removal functionality and improves the
> ft260_is_interface_enabled() code to handle correctly all chip modes defined
> by the device interface configuration pins DCNF0 and DCNF1.
> 
> Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
> Acked-by: Aaron Jones (FTDI-UK) <aaron.jones@ftdichip.com>

Thanks for the respin. Queued in for-5.14/upstream-fixes.

-- 
Jiri Kosina
SUSE Labs


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-07-29 10:39 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-20 14:49 [PATCH v1] HID: ft260: fix device removal due to USB disconnect Michael Zaidman
2021-07-28  9:47 ` Jiri Kosina
2021-07-29 10:26   ` [PATCH v2] " Michael Zaidman
2021-07-29 10:39     ` Jiri Kosina

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).