diff mbox series

[v1] HID: ft260: fix device removal due to USB disconnect

Message ID 20210720144954.5392-1-michael.zaidman@gmail.com
State Superseded
Headers show
Series [v1] HID: ft260: fix device removal due to USB disconnect | expand

Commit Message

Michael Zaidman July 20, 2021, 2:49 p.m. UTC
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(-)

Comments

Jiri Kosina July 28, 2021, 9:47 a.m. UTC | #1
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
Jiri Kosina July 29, 2021, 10:39 a.m. UTC | #2
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
diff mbox series

Patch

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);