Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 00/11] USB: serial: mos7840: type detection and clean ups
@ 2019-11-07 13:28 Johan Hovold
  2019-11-07 13:28 ` [PATCH 01/11] USB: serial: mos7840: clean up device-type handling Johan Hovold
                   ` (11 more replies)
  0 siblings, 12 replies; 14+ messages in thread
From: Johan Hovold @ 2019-11-07 13:28 UTC (permalink / raw)
  To: Johan Hovold; +Cc: linux-usb

The mos7840 device-type detection is fragile and cannot generally be
relied upon (e.g. as recently reported for Moxa UPort 2210 which was
detected as a four-port device).

The first couple of patches adds support for encoding known chip
features in the device-id table, and documents the underlying
assumptions for the mcs7810-detection hack.

Turns out we have a lot of legacy cruft in this driver, and the
remaining patches rips that out.

Johan


Johan Hovold (11):
  USB: serial: mos7840: clean up device-type handling
  USB: serial: mos7840: document MCS7810 detection hack
  USB: serial: mos7840: fix probe error handling
  USB: serial: mos7840: rip out broken interrupt handling
  USB: serial: mos7840: drop redundant urb context check
  USB: serial: mos7840: drop paranoid port checks
  USB: serial: mos7840: drop paranoid serial checks
  USB: serial: mos7840: drop serial struct accessor
  USB: serial: mos7840: drop port driver data accessors
  USB: serial: mos7840: drop read-urb check
  USB: serial: mos7840: drop port open flag

 drivers/usb/serial/mos7840.c | 770 +++++------------------------------
 1 file changed, 102 insertions(+), 668 deletions(-)

-- 
2.23.0


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

* [PATCH 01/11] USB: serial: mos7840: clean up device-type handling
  2019-11-07 13:28 [PATCH 00/11] USB: serial: mos7840: type detection and clean ups Johan Hovold
@ 2019-11-07 13:28 ` Johan Hovold
  2019-11-07 13:28 ` [PATCH 02/11] USB: serial: mos7840: document MCS7810 detection hack Johan Hovold
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Johan Hovold @ 2019-11-07 13:28 UTC (permalink / raw)
  To: Johan Hovold; +Cc: linux-usb

The current device-type detection is fragile and can't really be relied
upon. Instead of sprinkling device-id conditionals throughout the
driver, let's use the device-id table to encode the number of ports and
whether the device has a driver-controlled activity LED (MCS7810).

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/mos7840.c | 121 +++++++++++++----------------------
 1 file changed, 44 insertions(+), 77 deletions(-)

diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index ab4bf8d6d7df..f13cf723fa6c 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -89,17 +89,10 @@
 /* For higher baud Rates use TIOCEXBAUD */
 #define TIOCEXBAUD     0x5462
 
-/* vendor id and device id defines */
-
-/* The native mos7840/7820 component */
-#define USB_VENDOR_ID_MOSCHIP           0x9710
-#define MOSCHIP_DEVICE_ID_7840          0x7840
-#define MOSCHIP_DEVICE_ID_7843          0x7843
-#define MOSCHIP_DEVICE_ID_7820          0x7820
-#define MOSCHIP_DEVICE_ID_7810          0x7810
-/* The native component can have its vendor/device id's overridden
- * in vendor-specific implementations.  Such devices can be handled
- * by making a change here, in id_table.
+/*
+ * Vendor id and device id defines
+ *
+ * NOTE: Do not add new defines, add entries directly to the id_table instead.
  */
 #define USB_VENDOR_ID_BANDB              0x0856
 #define BANDB_DEVICE_ID_USO9ML2_2        0xAC22
@@ -116,18 +109,6 @@
 #define BANDB_DEVICE_ID_USOPTL4_4P       0xBC03
 #define BANDB_DEVICE_ID_USOPTL2_4        0xAC24
 
-/* This driver also supports
- * ATEN UC2324 device using Moschip MCS7840
- * ATEN UC2322 device using Moschip MCS7820
- * MOXA UPort 2210 device using Moschip MCS7820
- */
-#define USB_VENDOR_ID_ATENINTL		0x0557
-#define ATENINTL_DEVICE_ID_UC2324	0x2011
-#define ATENINTL_DEVICE_ID_UC2322	0x7820
-
-#define USB_VENDOR_ID_MOXA		0x110a
-#define MOXA_DEVICE_ID_2210		0x2210
-
 /* Interrupt Routine Defines    */
 
 #define SERIAL_IIR_RLS      0x06
@@ -179,27 +160,34 @@ enum mos7840_flag {
 	MOS7840_FLAG_LED_BUSY,
 };
 
+#define MCS_PORT_MASK	GENMASK(2, 0)
+#define MCS_PORTS(nr)	((nr) & MCS_PORT_MASK)
+#define MCS_LED		BIT(3)
+
+#define MCS_DEVICE(vid, pid, flags) \
+		USB_DEVICE((vid), (pid)), .driver_info = (flags)
+
 static const struct usb_device_id id_table[] = {
-	{USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7840)},
-	{USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7843)},
-	{USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7820)},
-	{USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7810)},
-	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_2)},
-	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_2P)},
-	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_4)},
-	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_4P)},
-	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_US9ML2_2)},
-	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_US9ML2_4)},
-	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USPTL4_2)},
-	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USPTL4_4)},
-	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USOPTL4_2)},
-	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USOPTL4_2P)},
-	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USOPTL4_4)},
-	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USOPTL4_4P)},
-	{USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USOPTL2_4)},
-	{USB_DEVICE(USB_VENDOR_ID_ATENINTL, ATENINTL_DEVICE_ID_UC2324)},
-	{USB_DEVICE(USB_VENDOR_ID_ATENINTL, ATENINTL_DEVICE_ID_UC2322)},
-	{USB_DEVICE(USB_VENDOR_ID_MOXA, MOXA_DEVICE_ID_2210)},
+	{ MCS_DEVICE(0x0557, 0x2011, MCS_PORTS(4)) },	/* ATEN UC2324 */
+	{ MCS_DEVICE(0x0557, 0x7820, MCS_PORTS(2)) },	/* ATEN UC2322 */
+	{ MCS_DEVICE(0x110a, 0x2210, MCS_PORTS(2)) },	/* Moxa UPort 2210 */
+	{ MCS_DEVICE(0x9710, 0x7810, MCS_PORTS(1) | MCS_LED) }, /* ASIX MCS7810 */
+	{ MCS_DEVICE(0x9710, 0x7820, MCS_PORTS(2)) },	/* MosChip MCS7820 */
+	{ MCS_DEVICE(0x9710, 0x7840, MCS_PORTS(4)) },	/* MosChip MCS7840 */
+	{ MCS_DEVICE(0x9710, 0x7843, MCS_PORTS(3)) },	/* ASIX MCS7840 3 port */
+	{ USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_2) },
+	{ USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_2P) },
+	{ USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_4) },
+	{ USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_4P) },
+	{ USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_US9ML2_2) },
+	{ USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_US9ML2_4) },
+	{ USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USPTL4_2) },
+	{ USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USPTL4_4) },
+	{ USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USOPTL4_2) },
+	{ USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USOPTL4_2P) },
+	{ USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USOPTL4_4) },
+	{ USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USOPTL4_4P) },
+	{ USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USOPTL2_4) },
 	{}			/* terminating entry */
 };
 MODULE_DEVICE_TABLE(usb, id_table);
@@ -2024,22 +2012,12 @@ static int mos7810_check(struct usb_serial *serial)
 static int mos7840_probe(struct usb_serial *serial,
 				const struct usb_device_id *id)
 {
-	u16 product = le16_to_cpu(serial->dev->descriptor.idProduct);
-	u16 vid = le16_to_cpu(serial->dev->descriptor.idVendor);
+	unsigned long device_flags = id->driver_info;
 	u8 *buf;
-	int device_type;
 
-	if (product == MOSCHIP_DEVICE_ID_7810 ||
-		product == MOSCHIP_DEVICE_ID_7820 ||
-		product == MOSCHIP_DEVICE_ID_7843) {
-		device_type = product;
+	/* Skip device-type detection if we already have device flags. */
+	if (device_flags)
 		goto out;
-	}
-
-	if (vid == USB_VENDOR_ID_MOXA && product == MOXA_DEVICE_ID_2210) {
-		device_type = MOSCHIP_DEVICE_ID_7820;
-		goto out;
-	}
 
 	buf = kzalloc(VENDOR_READ_LENGTH, GFP_KERNEL);
 	if (!buf)
@@ -2051,15 +2029,15 @@ static int mos7840_probe(struct usb_serial *serial,
 
 	/* For a MCS7840 device GPIO0 must be set to 1 */
 	if (buf[0] & 0x01)
-		device_type = MOSCHIP_DEVICE_ID_7840;
+		device_flags = MCS_PORTS(4);
 	else if (mos7810_check(serial))
-		device_type = MOSCHIP_DEVICE_ID_7810;
+		device_flags = MCS_PORTS(1) | MCS_LED;
 	else
-		device_type = MOSCHIP_DEVICE_ID_7820;
+		device_flags = MCS_PORTS(2);
 
 	kfree(buf);
 out:
-	usb_set_serial_data(serial, (void *)(unsigned long)device_type);
+	usb_set_serial_data(serial, (void *)device_flags);
 
 	return 0;
 }
@@ -2067,19 +2045,10 @@ static int mos7840_probe(struct usb_serial *serial,
 static int mos7840_calc_num_ports(struct usb_serial *serial,
 					struct usb_serial_endpoints *epds)
 {
-	int device_type = (unsigned long)usb_get_serial_data(serial);
-	int num_ports;
+	unsigned long device_flags = (unsigned long)usb_get_serial_data(serial);
+	int num_ports = MCS_PORTS(device_flags);
 
-	if (device_type == MOSCHIP_DEVICE_ID_7843)
-		num_ports = 3;
-	else
-		num_ports = (device_type >> 4) & 0x000F;
-
-	/*
-	 * num_ports is currently never zero as device_type is one of
-	 * MOSCHIP_DEVICE_ID_78{1,2,4}0.
-	 */
-	if (num_ports == 0)
+	if (num_ports == 0 || num_ports > 4)
 		return -ENODEV;
 
 	if (epds->num_bulk_in < num_ports || epds->num_bulk_out < num_ports) {
@@ -2093,7 +2062,7 @@ static int mos7840_calc_num_ports(struct usb_serial *serial,
 static int mos7840_port_probe(struct usb_serial_port *port)
 {
 	struct usb_serial *serial = port->serial;
-	int device_type = (unsigned long)usb_get_serial_data(serial);
+	unsigned long device_flags = (unsigned long)usb_get_serial_data(serial);
 	struct moschip_port *mos7840_port;
 	int status;
 	int pnum;
@@ -2255,12 +2224,10 @@ static int mos7840_port_probe(struct usb_serial_port *port)
 		goto error;
 	}
 
-	mos7840_port->has_led = false;
+	mos7840_port->has_led = device_flags & MCS_LED;
 
 	/* Initialize LED timers */
-	if (device_type == MOSCHIP_DEVICE_ID_7810) {
-		mos7840_port->has_led = true;
-
+	if (mos7840_port->has_led) {
 		mos7840_port->led_urb = usb_alloc_urb(0, GFP_KERNEL);
 		mos7840_port->led_dr = kmalloc(sizeof(*mos7840_port->led_dr),
 								GFP_KERNEL);
-- 
2.23.0


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

* [PATCH 02/11] USB: serial: mos7840: document MCS7810 detection hack
  2019-11-07 13:28 [PATCH 00/11] USB: serial: mos7840: type detection and clean ups Johan Hovold
  2019-11-07 13:28 ` [PATCH 01/11] USB: serial: mos7840: clean up device-type handling Johan Hovold
@ 2019-11-07 13:28 ` Johan Hovold
  2019-11-07 13:28 ` [PATCH 03/11] USB: serial: mos7840: fix probe error handling Johan Hovold
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Johan Hovold @ 2019-11-07 13:28 UTC (permalink / raw)
  To: Johan Hovold; +Cc: linux-usb

Document the MCS7810 detection hack which relies on having the GPO and
GPI pins connected as recommended by ASIX.

Note that GPO (pin 42) is really RTS of the third port which will be
toggled for the corresponding physical port on two- and four-port
devices.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/mos7840.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index f13cf723fa6c..6de41a3c2dab 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -1955,6 +1955,13 @@ static int mos7840_ioctl(struct tty_struct *tty,
 	return -ENOIOCTLCMD;
 }
 
+/*
+ * Check if GPO (pin 42) is connected to GPI (pin 33) as recommended by ASIX
+ * for MCS7810 by bit-banging a 16-bit word.
+ *
+ * Note that GPO is really RTS of the third port so this will toggle RTS of
+ * port two or three on two- and four-port devices.
+ */
 static int mos7810_check(struct usb_serial *serial)
 {
 	int i, pass_count = 0;
-- 
2.23.0


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

* [PATCH 03/11] USB: serial: mos7840: fix probe error handling
  2019-11-07 13:28 [PATCH 00/11] USB: serial: mos7840: type detection and clean ups Johan Hovold
  2019-11-07 13:28 ` [PATCH 01/11] USB: serial: mos7840: clean up device-type handling Johan Hovold
  2019-11-07 13:28 ` [PATCH 02/11] USB: serial: mos7840: document MCS7810 detection hack Johan Hovold
@ 2019-11-07 13:28 ` Johan Hovold
  2019-11-07 13:28 ` [PATCH 04/11] USB: serial: mos7840: rip out broken interrupt handling Johan Hovold
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Johan Hovold @ 2019-11-07 13:28 UTC (permalink / raw)
  To: Johan Hovold; +Cc: linux-usb

The driver would return success and leave the port structures
half-initialised if any of the register accesses during probe fails.

This would specifically leave the port control urb unallocated,
something which could trigger a NULL pointer dereference on interrupt
events.

Fortunately the interrupt implementation is completely broken and has
never even been enabled...

Note that the zero-length-enable register write used to set the zle-flag
for all ports is moved to attach.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/mos7840.c | 48 +++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index 6de41a3c2dab..ddd8db3be110 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -2066,6 +2066,23 @@ static int mos7840_calc_num_ports(struct usb_serial *serial,
 	return num_ports;
 }
 
+static int mos7840_attach(struct usb_serial *serial)
+{
+	struct device *dev = &serial->interface->dev;
+	int status;
+	u16 val;
+
+	/* Zero Length flag enable */
+	val = 0x0f;
+	status = mos7840_set_reg_sync(serial->port[0], ZLP_REG5, val);
+	if (status < 0)
+		dev_dbg(dev, "Writing ZLP_REG5 failed status-0x%x\n", status);
+	else
+		dev_dbg(dev, "ZLP_REG5 Writing success status%d\n", status);
+
+	return status;
+}
+
 static int mos7840_port_probe(struct usb_serial_port *port)
 {
 	struct usb_serial *serial = port->serial;
@@ -2123,7 +2140,7 @@ static int mos7840_port_probe(struct usb_serial_port *port)
 			mos7840_port->ControlRegOffset, &Data);
 	if (status < 0) {
 		dev_dbg(&port->dev, "Reading ControlReg failed status-0x%x\n", status);
-		goto out;
+		goto error;
 	} else
 		dev_dbg(&port->dev, "ControlReg Reading success val is %x, status%d\n", Data, status);
 	Data |= 0x08;	/* setting driver done bit */
@@ -2135,7 +2152,7 @@ static int mos7840_port_probe(struct usb_serial_port *port)
 			mos7840_port->ControlRegOffset, Data);
 	if (status < 0) {
 		dev_dbg(&port->dev, "Writing ControlReg failed(rx_disable) status-0x%x\n", status);
-		goto out;
+		goto error;
 	} else
 		dev_dbg(&port->dev, "ControlReg Writing success(rx_disable) status%d\n", status);
 
@@ -2146,7 +2163,7 @@ static int mos7840_port_probe(struct usb_serial_port *port)
 			(__u16) (mos7840_port->DcrRegOffset + 0), Data);
 	if (status < 0) {
 		dev_dbg(&port->dev, "Writing DCR0 failed status-0x%x\n", status);
-		goto out;
+		goto error;
 	} else
 		dev_dbg(&port->dev, "DCR0 Writing success status%d\n", status);
 
@@ -2155,7 +2172,7 @@ static int mos7840_port_probe(struct usb_serial_port *port)
 			(__u16) (mos7840_port->DcrRegOffset + 1), Data);
 	if (status < 0) {
 		dev_dbg(&port->dev, "Writing DCR1 failed status-0x%x\n", status);
-		goto out;
+		goto error;
 	} else
 		dev_dbg(&port->dev, "DCR1 Writing success status%d\n", status);
 
@@ -2164,7 +2181,7 @@ static int mos7840_port_probe(struct usb_serial_port *port)
 			(__u16) (mos7840_port->DcrRegOffset + 2), Data);
 	if (status < 0) {
 		dev_dbg(&port->dev, "Writing DCR2 failed status-0x%x\n", status);
-		goto out;
+		goto error;
 	} else
 		dev_dbg(&port->dev, "DCR2 Writing success status%d\n", status);
 
@@ -2173,7 +2190,7 @@ static int mos7840_port_probe(struct usb_serial_port *port)
 	status = mos7840_set_reg_sync(port, CLK_START_VALUE_REGISTER, Data);
 	if (status < 0) {
 		dev_dbg(&port->dev, "Writing CLK_START_VALUE_REGISTER failed status-0x%x\n", status);
-		goto out;
+		goto error;
 	} else
 		dev_dbg(&port->dev, "CLK_START_VALUE_REGISTER Writing success status%d\n", status);
 
@@ -2190,7 +2207,7 @@ static int mos7840_port_probe(struct usb_serial_port *port)
 	status = mos7840_set_uart_reg(port, SCRATCH_PAD_REGISTER, Data);
 	if (status < 0) {
 		dev_dbg(&port->dev, "Writing SCRATCH_PAD_REGISTER failed status-0x%x\n", status);
-		goto out;
+		goto error;
 	} else
 		dev_dbg(&port->dev, "SCRATCH_PAD_REGISTER Writing success status%d\n", status);
 
@@ -2204,7 +2221,7 @@ static int mos7840_port_probe(struct usb_serial_port *port)
 				(__u16)(ZLP_REG1 + ((__u16) mos7840_port->port_num)));
 		if (status < 0) {
 			dev_dbg(&port->dev, "Writing ZLP_REG%d failed status-0x%x\n", pnum + 2, status);
-			goto out;
+			goto error;
 		} else
 			dev_dbg(&port->dev, "ZLP_REG%d Writing success status%d\n", pnum + 2, status);
 	} else {
@@ -2216,7 +2233,7 @@ static int mos7840_port_probe(struct usb_serial_port *port)
 				(__u16)(ZLP_REG1 + ((__u16) mos7840_port->port_num) - 0x1));
 		if (status < 0) {
 			dev_dbg(&port->dev, "Writing ZLP_REG%d failed status-0x%x\n", pnum + 1, status);
-			goto out;
+			goto error;
 		} else
 			dev_dbg(&port->dev, "ZLP_REG%d Writing success status%d\n", pnum + 1, status);
 
@@ -2254,17 +2271,7 @@ static int mos7840_port_probe(struct usb_serial_port *port)
 		/* Turn off LED */
 		mos7840_set_led_sync(port, MODEM_CONTROL_REGISTER, 0x0300);
 	}
-out:
-	if (pnum == serial->num_ports - 1) {
-		/* Zero Length flag enable */
-		Data = 0x0f;
-		status = mos7840_set_reg_sync(serial->port[0], ZLP_REG5, Data);
-		if (status < 0) {
-			dev_dbg(&port->dev, "Writing ZLP_REG5 failed status-0x%x\n", status);
-			goto error;
-		} else
-			dev_dbg(&port->dev, "ZLP_REG5 Writing success status%d\n", status);
-	}
+
 	return 0;
 error:
 	kfree(mos7840_port->led_dr);
@@ -2320,6 +2327,7 @@ static struct usb_serial_driver moschip7840_4port_device = {
 	.unthrottle = mos7840_unthrottle,
 	.calc_num_ports = mos7840_calc_num_ports,
 	.probe = mos7840_probe,
+	.attach = mos7840_attach,
 	.ioctl = mos7840_ioctl,
 	.get_serial = mos7840_get_serial_info,
 	.set_termios = mos7840_set_termios,
-- 
2.23.0


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

* [PATCH 04/11] USB: serial: mos7840: rip out broken interrupt handling
  2019-11-07 13:28 [PATCH 00/11] USB: serial: mos7840: type detection and clean ups Johan Hovold
                   ` (2 preceding siblings ...)
  2019-11-07 13:28 ` [PATCH 03/11] USB: serial: mos7840: fix probe error handling Johan Hovold
@ 2019-11-07 13:28 ` Johan Hovold
  2019-11-07 13:28 ` [PATCH 05/11] USB: serial: mos7840: drop redundant urb context check Johan Hovold
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Johan Hovold @ 2019-11-07 13:28 UTC (permalink / raw)
  To: Johan Hovold; +Cc: linux-usb

The interrupt handling is completely broken and has in fact never been
been enabled due to an inverted test for an interrupt endpoint in
open() that prevented the interrupt URB from being submitted.

Other highlights include missing interrupt URB resubmission (had it
ever been submitted), missing locking when managing the open-port count,
and NULL-pointer dereferences that could have been triggered by a
malicious device.

Rip out this broken crap which is beyond repair instead of pretending
we support this feature.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/mos7840.c | 294 +----------------------------------
 1 file changed, 3 insertions(+), 291 deletions(-)

diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index ddd8db3be110..9c5956fbd600 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -156,7 +156,6 @@
 #define LED_OFF_MS	500
 
 enum mos7840_flag {
-	MOS7840_FLAG_CTRL_BUSY,
 	MOS7840_FLAG_LED_BUSY,
 };
 
@@ -200,18 +199,12 @@ struct moschip_port {
 	__u8 shadowLCR;		/* last LCR value received */
 	__u8 shadowMCR;		/* last MCR value received */
 	char open;
-	char open_ports;
 	struct usb_serial_port *port;	/* loop back to the owner of this object */
 
 	/* Offsets */
 	__u8 SpRegOffset;
 	__u8 ControlRegOffset;
 	__u8 DcrRegOffset;
-	/* for processing control URBS in interrupt context */
-	struct urb *control_urb;
-	struct usb_ctrlrequest *dr;
-	char *ctrl_buf;
-	int MsrLsr;
 
 	spinlock_t pool_lock;
 	struct urb *write_urb_pool[NUM_URBS];
@@ -371,55 +364,6 @@ static inline struct moschip_port *mos7840_get_port_private(struct
 	return (struct moschip_port *)usb_get_serial_port_data(port);
 }
 
-static void mos7840_handle_new_msr(struct moschip_port *port, __u8 new_msr)
-{
-	struct moschip_port *mos7840_port;
-	struct async_icount *icount;
-	mos7840_port = port;
-	if (new_msr &
-	    (MOS_MSR_DELTA_CTS | MOS_MSR_DELTA_DSR | MOS_MSR_DELTA_RI |
-	     MOS_MSR_DELTA_CD)) {
-		icount = &mos7840_port->port->icount;
-
-		/* update input line counters */
-		if (new_msr & MOS_MSR_DELTA_CTS)
-			icount->cts++;
-		if (new_msr & MOS_MSR_DELTA_DSR)
-			icount->dsr++;
-		if (new_msr & MOS_MSR_DELTA_CD)
-			icount->dcd++;
-		if (new_msr & MOS_MSR_DELTA_RI)
-			icount->rng++;
-
-		wake_up_interruptible(&port->port->port.delta_msr_wait);
-	}
-}
-
-static void mos7840_handle_new_lsr(struct moschip_port *port, __u8 new_lsr)
-{
-	struct async_icount *icount;
-
-	if (new_lsr & SERIAL_LSR_BI) {
-		/*
-		 * Parity and Framing errors only count if they
-		 * occur exclusive of a break being
-		 * received.
-		 */
-		new_lsr &= (__u8) (SERIAL_LSR_OE | SERIAL_LSR_BI);
-	}
-
-	/* update input line counters */
-	icount = &port->port->icount;
-	if (new_lsr & SERIAL_LSR_BI)
-		icount->brk++;
-	if (new_lsr & SERIAL_LSR_OE)
-		icount->overrun++;
-	if (new_lsr & SERIAL_LSR_PE)
-		icount->parity++;
-	if (new_lsr & SERIAL_LSR_FE)
-		icount->frame++;
-}
-
 /************************************************************************/
 /************************************************************************/
 /*            U S B  C A L L B A C K   F U N C T I O N S                */
@@ -427,76 +371,6 @@ static void mos7840_handle_new_lsr(struct moschip_port *port, __u8 new_lsr)
 /************************************************************************/
 /************************************************************************/
 
-static void mos7840_control_callback(struct urb *urb)
-{
-	unsigned char *data;
-	struct moschip_port *mos7840_port;
-	struct device *dev = &urb->dev->dev;
-	__u8 regval = 0x0;
-	int status = urb->status;
-
-	mos7840_port = urb->context;
-
-	switch (status) {
-	case 0:
-		/* success */
-		break;
-	case -ECONNRESET:
-	case -ENOENT:
-	case -ESHUTDOWN:
-		/* this urb is terminated, clean up */
-		dev_dbg(dev, "%s - urb shutting down with status: %d\n", __func__, status);
-		goto out;
-	default:
-		dev_dbg(dev, "%s - nonzero urb status received: %d\n", __func__, status);
-		goto out;
-	}
-
-	dev_dbg(dev, "%s urb buffer size is %d\n", __func__, urb->actual_length);
-	if (urb->actual_length < 1)
-		goto out;
-
-	dev_dbg(dev, "%s mos7840_port->MsrLsr is %d port %d\n", __func__,
-		mos7840_port->MsrLsr, mos7840_port->port_num);
-	data = urb->transfer_buffer;
-	regval = (__u8) data[0];
-	dev_dbg(dev, "%s data is %x\n", __func__, regval);
-	if (mos7840_port->MsrLsr == 0)
-		mos7840_handle_new_msr(mos7840_port, regval);
-	else if (mos7840_port->MsrLsr == 1)
-		mos7840_handle_new_lsr(mos7840_port, regval);
-out:
-	clear_bit_unlock(MOS7840_FLAG_CTRL_BUSY, &mos7840_port->flags);
-}
-
-static int mos7840_get_reg(struct moschip_port *mcs, __u16 Wval, __u16 reg,
-			   __u16 *val)
-{
-	struct usb_device *dev = mcs->port->serial->dev;
-	struct usb_ctrlrequest *dr = mcs->dr;
-	unsigned char *buffer = mcs->ctrl_buf;
-	int ret;
-
-	if (test_and_set_bit_lock(MOS7840_FLAG_CTRL_BUSY, &mcs->flags))
-		return -EBUSY;
-
-	dr->bRequestType = MCS_RD_RTYPE;
-	dr->bRequest = MCS_RDREQ;
-	dr->wValue = cpu_to_le16(Wval);	/* 0 */
-	dr->wIndex = cpu_to_le16(reg);
-	dr->wLength = cpu_to_le16(2);
-
-	usb_fill_control_urb(mcs->control_urb, dev, usb_rcvctrlpipe(dev, 0),
-			     (unsigned char *)dr, buffer, 2,
-			     mos7840_control_callback, mcs);
-	mcs->control_urb->transfer_buffer_length = 2;
-	ret = usb_submit_urb(mcs->control_urb, GFP_ATOMIC);
-	if (ret)
-		clear_bit_unlock(MOS7840_FLAG_CTRL_BUSY, &mcs->flags);
-
-	return ret;
-}
-
 static void mos7840_set_led_callback(struct urb *urb)
 {
 	switch (urb->status) {
@@ -572,100 +446,6 @@ static void mos7840_led_activity(struct usb_serial_port *port)
 				jiffies + msecs_to_jiffies(LED_ON_MS));
 }
 
-/*****************************************************************************
- * mos7840_interrupt_callback
- *	this is the callback function for when we have received data on the
- *	interrupt endpoint.
- *****************************************************************************/
-
-static void mos7840_interrupt_callback(struct urb *urb)
-{
-	int result;
-	int length;
-	struct moschip_port *mos7840_port;
-	struct usb_serial *serial;
-	__u16 Data;
-	unsigned char *data;
-	__u8 sp[5];
-	int i, rv = 0;
-	__u16 wval, wreg = 0;
-	int status = urb->status;
-
-	switch (status) {
-	case 0:
-		/* success */
-		break;
-	case -ECONNRESET:
-	case -ENOENT:
-	case -ESHUTDOWN:
-		/* this urb is terminated, clean up */
-		dev_dbg(&urb->dev->dev, "%s - urb shutting down with status: %d\n",
-			__func__, status);
-		return;
-	default:
-		dev_dbg(&urb->dev->dev, "%s - nonzero urb status received: %d\n",
-			__func__, status);
-		goto exit;
-	}
-
-	length = urb->actual_length;
-	data = urb->transfer_buffer;
-
-	serial = urb->context;
-
-	/* Moschip get 5 bytes
-	 * Byte 1 IIR Port 1 (port.number is 0)
-	 * Byte 2 IIR Port 2 (port.number is 1)
-	 * Byte 3 IIR Port 3 (port.number is 2)
-	 * Byte 4 IIR Port 4 (port.number is 3)
-	 * Byte 5 FIFO status for both */
-
-	if (length > 5) {
-		dev_dbg(&urb->dev->dev, "%s", "Wrong data !!!\n");
-		return;
-	}
-
-	sp[0] = (__u8) data[0];
-	sp[1] = (__u8) data[1];
-	sp[2] = (__u8) data[2];
-	sp[3] = (__u8) data[3];
-
-	for (i = 0; i < serial->num_ports; i++) {
-		mos7840_port = mos7840_get_port_private(serial->port[i]);
-		wval = ((__u16)serial->port[i]->port_number + 1) << 8;
-		if (mos7840_port->open) {
-			if (sp[i] & 0x01) {
-				dev_dbg(&urb->dev->dev, "SP%d No Interrupt !!!\n", i);
-			} else {
-				switch (sp[i] & 0x0f) {
-				case SERIAL_IIR_RLS:
-					dev_dbg(&urb->dev->dev, "Serial Port %d: Receiver status error or \n", i);
-					dev_dbg(&urb->dev->dev, "address bit detected in 9-bit mode\n");
-					mos7840_port->MsrLsr = 1;
-					wreg = LINE_STATUS_REGISTER;
-					break;
-				case SERIAL_IIR_MS:
-					dev_dbg(&urb->dev->dev, "Serial Port %d: Modem status change\n", i);
-					mos7840_port->MsrLsr = 0;
-					wreg = MODEM_STATUS_REGISTER;
-					break;
-				}
-				rv = mos7840_get_reg(mos7840_port, wval, wreg, &Data);
-			}
-		}
-	}
-	if (!(rv < 0))
-		/* the completion handler for the control urb will resubmit */
-		return;
-exit:
-	result = usb_submit_urb(urb, GFP_ATOMIC);
-	if (result) {
-		dev_err(&urb->dev->dev,
-			"%s - Error %d submitting interrupt urb\n",
-			__func__, result);
-	}
-}
-
 static int mos7840_port_paranoia_check(struct usb_serial_port *port,
 				       const char *function)
 {
@@ -836,7 +616,6 @@ static int mos7840_open(struct tty_struct *tty, struct usb_serial_port *port)
 	__u16 Data;
 	int status;
 	struct moschip_port *mos7840_port;
-	struct moschip_port *port0;
 
 	if (mos7840_port_paranoia_check(port, __func__))
 		return -ENODEV;
@@ -847,14 +626,11 @@ static int mos7840_open(struct tty_struct *tty, struct usb_serial_port *port)
 		return -ENODEV;
 
 	mos7840_port = mos7840_get_port_private(port);
-	port0 = mos7840_get_port_private(serial->port[0]);
-
-	if (mos7840_port == NULL || port0 == NULL)
+	if (mos7840_port == NULL)
 		return -ENODEV;
 
 	usb_clear_halt(serial->dev, port->write_urb->pipe);
 	usb_clear_halt(serial->dev, port->read_urb->pipe);
-	port0->open_ports++;
 
 	/* Initialising the write urb pool */
 	for (j = 0; j < NUM_URBS; ++j) {
@@ -1005,41 +781,6 @@ static int mos7840_open(struct tty_struct *tty, struct usb_serial_port *port)
 	status = mos7840_set_reg_sync(port, mos7840_port->ControlRegOffset,
 									Data);
 
-	/* Check to see if we've set up our endpoint info yet    *
-	 * (can't set it up in mos7840_startup as the structures *
-	 * were not set up at that time.)                        */
-	if (port0->open_ports == 1) {
-		/* FIXME: Buffer never NULL, so URB is not submitted. */
-		if (serial->port[0]->interrupt_in_buffer == NULL) {
-			/* set up interrupt urb */
-			usb_fill_int_urb(serial->port[0]->interrupt_in_urb,
-				serial->dev,
-				usb_rcvintpipe(serial->dev,
-				serial->port[0]->interrupt_in_endpointAddress),
-				serial->port[0]->interrupt_in_buffer,
-				serial->port[0]->interrupt_in_urb->
-				transfer_buffer_length,
-				mos7840_interrupt_callback,
-				serial,
-				serial->port[0]->interrupt_in_urb->interval);
-
-			/* start interrupt read for mos7840 */
-			response =
-			    usb_submit_urb(serial->port[0]->interrupt_in_urb,
-					   GFP_KERNEL);
-			if (response) {
-				dev_err(&port->dev, "%s - Error %d submitting "
-					"interrupt urb\n", __func__, response);
-			}
-
-		}
-
-	}
-
-	/* see if we've set up our endpoint info yet   *
-	 * (can't set it up in mos7840_startup as the  *
-	 * structures were not set up at that time.)   */
-
 	dev_dbg(&port->dev, "port number is %d\n", port->port_number);
 	dev_dbg(&port->dev, "minor number is %d\n", port->minor);
 	dev_dbg(&port->dev, "Bulkin endpoint is %d\n", port->bulk_in_endpointAddress);
@@ -1142,7 +883,6 @@ static void mos7840_close(struct usb_serial_port *port)
 {
 	struct usb_serial *serial;
 	struct moschip_port *mos7840_port;
-	struct moschip_port *port0;
 	int j;
 	__u16 Data;
 
@@ -1154,9 +894,7 @@ static void mos7840_close(struct usb_serial_port *port)
 		return;
 
 	mos7840_port = mos7840_get_port_private(port);
-	port0 = mos7840_get_port_private(serial->port[0]);
-
-	if (mos7840_port == NULL || port0 == NULL)
+	if (mos7840_port == NULL)
 		return;
 
 	for (j = 0; j < NUM_URBS; ++j)
@@ -1173,15 +911,6 @@ static void mos7840_close(struct usb_serial_port *port)
 	usb_kill_urb(mos7840_port->read_urb);
 	mos7840_port->read_urb_busy = false;
 
-	port0->open_ports--;
-	dev_dbg(&port->dev, "%s in close%d\n", __func__, port0->open_ports);
-	if (port0->open_ports == 0) {
-		if (serial->port[0]->interrupt_in_urb) {
-			dev_dbg(&port->dev, "Shutdown interrupt_in_urb\n");
-			usb_kill_urb(serial->port[0]->interrupt_in_urb);
-		}
-	}
-
 	Data = 0x0;
 	mos7840_set_uart_reg(port, MODEM_CONTROL_REGISTER, Data);
 
@@ -2238,15 +1967,6 @@ static int mos7840_port_probe(struct usb_serial_port *port)
 			dev_dbg(&port->dev, "ZLP_REG%d Writing success status%d\n", pnum + 1, status);
 
 	}
-	mos7840_port->control_urb = usb_alloc_urb(0, GFP_KERNEL);
-	mos7840_port->ctrl_buf = kmalloc(16, GFP_KERNEL);
-	mos7840_port->dr = kmalloc(sizeof(struct usb_ctrlrequest),
-			GFP_KERNEL);
-	if (!mos7840_port->control_urb || !mos7840_port->ctrl_buf ||
-			!mos7840_port->dr) {
-		status = -ENOMEM;
-		goto error;
-	}
 
 	mos7840_port->has_led = device_flags & MCS_LED;
 
@@ -2276,9 +1996,6 @@ static int mos7840_port_probe(struct usb_serial_port *port)
 error:
 	kfree(mos7840_port->led_dr);
 	usb_free_urb(mos7840_port->led_urb);
-	kfree(mos7840_port->dr);
-	kfree(mos7840_port->ctrl_buf);
-	usb_free_urb(mos7840_port->control_urb);
 	kfree(mos7840_port);
 
 	return status;
@@ -2301,10 +2018,7 @@ static int mos7840_port_remove(struct usb_serial_port *port)
 		usb_free_urb(mos7840_port->led_urb);
 		kfree(mos7840_port->led_dr);
 	}
-	usb_kill_urb(mos7840_port->control_urb);
-	usb_free_urb(mos7840_port->control_urb);
-	kfree(mos7840_port->ctrl_buf);
-	kfree(mos7840_port->dr);
+
 	kfree(mos7840_port);
 
 	return 0;
@@ -2334,12 +2048,10 @@ static struct usb_serial_driver moschip7840_4port_device = {
 	.break_ctl = mos7840_break,
 	.tiocmget = mos7840_tiocmget,
 	.tiocmset = mos7840_tiocmset,
-	.tiocmiwait = usb_serial_generic_tiocmiwait,
 	.get_icount = usb_serial_generic_get_icount,
 	.port_probe = mos7840_port_probe,
 	.port_remove = mos7840_port_remove,
 	.read_bulk_callback = mos7840_bulk_in_callback,
-	.read_int_callback = mos7840_interrupt_callback,
 };
 
 static struct usb_serial_driver * const serial_drivers[] = {
-- 
2.23.0


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

* [PATCH 05/11] USB: serial: mos7840: drop redundant urb context check
  2019-11-07 13:28 [PATCH 00/11] USB: serial: mos7840: type detection and clean ups Johan Hovold
                   ` (3 preceding siblings ...)
  2019-11-07 13:28 ` [PATCH 04/11] USB: serial: mos7840: rip out broken interrupt handling Johan Hovold
@ 2019-11-07 13:28 ` Johan Hovold
  2019-11-07 13:28 ` [PATCH 06/11] USB: serial: mos7840: drop paranoid port checks Johan Hovold
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Johan Hovold @ 2019-11-07 13:28 UTC (permalink / raw)
  To: Johan Hovold; +Cc: linux-usb

The bulk-in URB context is set to the driver-data struct at open and is
never cleared so drop the redundant check in the completion callback.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/mos7840.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index 9c5956fbd600..92180687097f 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -500,17 +500,13 @@ static struct usb_serial *mos7840_get_usb_serial(struct usb_serial_port *port,
 
 static void mos7840_bulk_in_callback(struct urb *urb)
 {
+	struct moschip_port *mos7840_port = urb->context;
 	int retval;
 	unsigned char *data;
 	struct usb_serial *serial;
 	struct usb_serial_port *port;
-	struct moschip_port *mos7840_port;
 	int status = urb->status;
 
-	mos7840_port = urb->context;
-	if (!mos7840_port)
-		return;
-
 	if (status) {
 		dev_dbg(&urb->dev->dev, "nonzero read bulk status received: %d\n", status);
 		mos7840_port->read_urb_busy = false;
-- 
2.23.0


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

* [PATCH 06/11] USB: serial: mos7840: drop paranoid port checks
  2019-11-07 13:28 [PATCH 00/11] USB: serial: mos7840: type detection and clean ups Johan Hovold
                   ` (4 preceding siblings ...)
  2019-11-07 13:28 ` [PATCH 05/11] USB: serial: mos7840: drop redundant urb context check Johan Hovold
@ 2019-11-07 13:28 ` Johan Hovold
  2019-11-07 13:29 ` [PATCH 07/11] USB: serial: mos7840: drop paranoid serial checks Johan Hovold
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Johan Hovold @ 2019-11-07 13:28 UTC (permalink / raw)
  To: Johan Hovold; +Cc: linux-usb

Drop the paranoid port structure sanity checks which are confusing at
best.

The driver data port pointer is set at port probe and never cleared,
while USB serial core sets the tty driver data at install and won't call
any driver functions with a NULL port pointer.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/mos7840.c | 92 ++++--------------------------------
 1 file changed, 8 insertions(+), 84 deletions(-)

diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index 92180687097f..dcb1e04a0514 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -446,21 +446,6 @@ static void mos7840_led_activity(struct usb_serial_port *port)
 				jiffies + msecs_to_jiffies(LED_ON_MS));
 }
 
-static int mos7840_port_paranoia_check(struct usb_serial_port *port,
-				       const char *function)
-{
-	if (!port) {
-		pr_debug("%s - port == NULL\n", function);
-		return -1;
-	}
-	if (!port->serial) {
-		pr_debug("%s - port->serial == NULL\n", function);
-		return -1;
-	}
-
-	return 0;
-}
-
 /* Inline functions to check the sanity of a pointer that is passed to us */
 static int mos7840_serial_paranoia_check(struct usb_serial *serial,
 					 const char *function)
@@ -482,7 +467,6 @@ static struct usb_serial *mos7840_get_usb_serial(struct usb_serial_port *port,
 {
 	/* if no port was specified, or it fails a paranoia check */
 	if (!port ||
-	    mos7840_port_paranoia_check(port, function) ||
 	    mos7840_serial_paranoia_check(port->serial, function)) {
 		/* then say that we don't have a valid usb_serial thing,
 		 * which will end up genrating -ENODEV return values */
@@ -501,10 +485,10 @@ static struct usb_serial *mos7840_get_usb_serial(struct usb_serial_port *port,
 static void mos7840_bulk_in_callback(struct urb *urb)
 {
 	struct moschip_port *mos7840_port = urb->context;
+	struct usb_serial_port *port = mos7840_port->port;
 	int retval;
 	unsigned char *data;
 	struct usb_serial *serial;
-	struct usb_serial_port *port;
 	int status = urb->status;
 
 	if (status) {
@@ -513,12 +497,6 @@ static void mos7840_bulk_in_callback(struct urb *urb)
 		return;
 	}
 
-	port = mos7840_port->port;
-	if (mos7840_port_paranoia_check(port, __func__)) {
-		mos7840_port->read_urb_busy = false;
-		return;
-	}
-
 	serial = mos7840_get_usb_serial(port, __func__);
 	if (!serial) {
 		mos7840_port->read_urb_busy = false;
@@ -562,14 +540,12 @@ static void mos7840_bulk_in_callback(struct urb *urb)
 
 static void mos7840_bulk_out_data_callback(struct urb *urb)
 {
-	struct moschip_port *mos7840_port;
-	struct usb_serial_port *port;
+	struct moschip_port *mos7840_port = urb->context;
+	struct usb_serial_port *port = mos7840_port->port;
 	int status = urb->status;
 	unsigned long flags;
 	int i;
 
-	mos7840_port = urb->context;
-	port = mos7840_port->port;
 	spin_lock_irqsave(&mos7840_port->pool_lock, flags);
 	for (i = 0; i < NUM_URBS; i++) {
 		if (urb == mos7840_port->write_urb_pool[i]) {
@@ -584,9 +560,6 @@ static void mos7840_bulk_out_data_callback(struct urb *urb)
 		return;
 	}
 
-	if (mos7840_port_paranoia_check(port, __func__))
-		return;
-
 	if (mos7840_port->open)
 		tty_port_tty_wakeup(&port->port);
 
@@ -605,19 +578,14 @@ static void mos7840_bulk_out_data_callback(struct urb *urb)
 
 static int mos7840_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
+	struct usb_serial *serial = port->serial;
 	int response;
 	int j;
-	struct usb_serial *serial;
 	struct urb *urb;
 	__u16 Data;
 	int status;
 	struct moschip_port *mos7840_port;
 
-	if (mos7840_port_paranoia_check(port, __func__))
-		return -ENODEV;
-
-	serial = port->serial;
-
 	if (mos7840_serial_paranoia_check(serial, __func__))
 		return -ENODEV;
 
@@ -850,9 +818,6 @@ static int mos7840_chars_in_buffer(struct tty_struct *tty)
 	unsigned long flags;
 	struct moschip_port *mos7840_port;
 
-	if (mos7840_port_paranoia_check(port, __func__))
-		return 0;
-
 	mos7840_port = mos7840_get_port_private(port);
 	if (mos7840_port == NULL)
 		return 0;
@@ -882,9 +847,6 @@ static void mos7840_close(struct usb_serial_port *port)
 	int j;
 	__u16 Data;
 
-	if (mos7840_port_paranoia_check(port, __func__))
-		return;
-
 	serial = mos7840_get_usb_serial(port, __func__);
 	if (!serial)
 		return;
@@ -927,9 +889,6 @@ static void mos7840_break(struct tty_struct *tty, int break_state)
 	struct usb_serial *serial;
 	struct moschip_port *mos7840_port;
 
-	if (mos7840_port_paranoia_check(port, __func__))
-		return;
-
 	serial = mos7840_get_usb_serial(port, __func__);
 	if (!serial)
 		return;
@@ -967,9 +926,6 @@ static int mos7840_write_room(struct tty_struct *tty)
 	unsigned long flags;
 	struct moschip_port *mos7840_port;
 
-	if (mos7840_port_paranoia_check(port, __func__))
-		return -1;
-
 	mos7840_port = mos7840_get_port_private(port);
 	if (mos7840_port == NULL)
 		return -1;
@@ -998,6 +954,7 @@ static int mos7840_write_room(struct tty_struct *tty)
 static int mos7840_write(struct tty_struct *tty, struct usb_serial_port *port,
 			 const unsigned char *data, int count)
 {
+	struct usb_serial *serial = port->serial;
 	int status;
 	int i;
 	int bytes_sent = 0;
@@ -1005,15 +962,10 @@ static int mos7840_write(struct tty_struct *tty, struct usb_serial_port *port,
 	unsigned long flags;
 
 	struct moschip_port *mos7840_port;
-	struct usb_serial *serial;
 	struct urb *urb;
 	/* __u16 Data; */
 	const unsigned char *current_position = data;
 
-	if (mos7840_port_paranoia_check(port, __func__))
-		return -1;
-
-	serial = port->serial;
 	if (mos7840_serial_paranoia_check(serial, __func__))
 		return -1;
 
@@ -1104,9 +1056,6 @@ static void mos7840_throttle(struct tty_struct *tty)
 	struct moschip_port *mos7840_port;
 	int status;
 
-	if (mos7840_port_paranoia_check(port, __func__))
-		return;
-
 	mos7840_port = mos7840_get_port_private(port);
 
 	if (mos7840_port == NULL)
@@ -1146,9 +1095,6 @@ static void mos7840_unthrottle(struct tty_struct *tty)
 	int status;
 	struct moschip_port *mos7840_port = mos7840_get_port_private(port);
 
-	if (mos7840_port_paranoia_check(port, __func__))
-		return;
-
 	if (mos7840_port == NULL)
 		return;
 
@@ -1296,18 +1242,11 @@ static int mos7840_calc_baud_rate_divisor(struct usb_serial_port *port,
 static int mos7840_send_cmd_write_baud_rate(struct moschip_port *mos7840_port,
 					    int baudRate)
 {
+	struct usb_serial_port *port = mos7840_port->port;
 	int divisor = 0;
 	int status;
 	__u16 Data;
 	__u16 clk_sel_val;
-	struct usb_serial_port *port;
-
-	if (mos7840_port == NULL)
-		return -1;
-
-	port = mos7840_port->port;
-	if (mos7840_port_paranoia_check(port, __func__))
-		return -1;
 
 	if (mos7840_serial_paranoia_check(port->serial, __func__))
 		return -1;
@@ -1399,6 +1338,7 @@ static int mos7840_send_cmd_write_baud_rate(struct moschip_port *mos7840_port,
 static void mos7840_change_port_settings(struct tty_struct *tty,
 	struct moschip_port *mos7840_port, struct ktermios *old_termios)
 {
+	struct usb_serial_port *port = mos7840_port->port;
 	int baud;
 	unsigned cflag;
 	__u8 lData;
@@ -1406,15 +1346,6 @@ static void mos7840_change_port_settings(struct tty_struct *tty,
 	__u8 lStop;
 	int status;
 	__u16 Data;
-	struct usb_serial_port *port;
-
-	if (mos7840_port == NULL)
-		return;
-
-	port = mos7840_port->port;
-
-	if (mos7840_port_paranoia_check(port, __func__))
-		return;
 
 	if (mos7840_serial_paranoia_check(port->serial, __func__))
 		return;
@@ -1557,14 +1488,10 @@ static void mos7840_set_termios(struct tty_struct *tty,
 				struct usb_serial_port *port,
 				struct ktermios *old_termios)
 {
+	struct usb_serial *serial = port->serial;
 	int status;
-	struct usb_serial *serial;
 	struct moschip_port *mos7840_port;
 
-	if (mos7840_port_paranoia_check(port, __func__))
-		return;
-
-	serial = port->serial;
 
 	if (mos7840_serial_paranoia_check(serial, __func__))
 		return;
@@ -1659,9 +1586,6 @@ static int mos7840_ioctl(struct tty_struct *tty,
 	void __user *argp = (void __user *)arg;
 	struct moschip_port *mos7840_port;
 
-	if (mos7840_port_paranoia_check(port, __func__))
-		return -1;
-
 	mos7840_port = mos7840_get_port_private(port);
 
 	if (mos7840_port == NULL)
-- 
2.23.0


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

* [PATCH 07/11] USB: serial: mos7840: drop paranoid serial checks
  2019-11-07 13:28 [PATCH 00/11] USB: serial: mos7840: type detection and clean ups Johan Hovold
                   ` (5 preceding siblings ...)
  2019-11-07 13:28 ` [PATCH 06/11] USB: serial: mos7840: drop paranoid port checks Johan Hovold
@ 2019-11-07 13:29 ` Johan Hovold
  2019-11-07 13:29 ` [PATCH 08/11] USB: serial: mos7840: drop serial struct accessor Johan Hovold
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Johan Hovold @ 2019-11-07 13:29 UTC (permalink / raw)
  To: Johan Hovold; +Cc: linux-usb

Drop the likewise paranoid serial structure sanity checks.

USB serial core sets the serial pointer in the port structures at
initialisation and it is never cleared, and similar for the serial
structure type.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/mos7840.c | 38 ++----------------------------------
 1 file changed, 2 insertions(+), 36 deletions(-)

diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index dcb1e04a0514..207b88d948a9 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -446,28 +446,11 @@ static void mos7840_led_activity(struct usb_serial_port *port)
 				jiffies + msecs_to_jiffies(LED_ON_MS));
 }
 
-/* Inline functions to check the sanity of a pointer that is passed to us */
-static int mos7840_serial_paranoia_check(struct usb_serial *serial,
-					 const char *function)
-{
-	if (!serial) {
-		pr_debug("%s - serial == NULL\n", function);
-		return -1;
-	}
-	if (!serial->type) {
-		pr_debug("%s - serial->type == NULL!\n", function);
-		return -1;
-	}
-
-	return 0;
-}
-
 static struct usb_serial *mos7840_get_usb_serial(struct usb_serial_port *port,
 						 const char *function)
 {
-	/* if no port was specified, or it fails a paranoia check */
-	if (!port ||
-	    mos7840_serial_paranoia_check(port->serial, function)) {
+	/* if no port was specified */
+	if (!port) {
 		/* then say that we don't have a valid usb_serial thing,
 		 * which will end up genrating -ENODEV return values */
 		return NULL;
@@ -586,9 +569,6 @@ static int mos7840_open(struct tty_struct *tty, struct usb_serial_port *port)
 	int status;
 	struct moschip_port *mos7840_port;
 
-	if (mos7840_serial_paranoia_check(serial, __func__))
-		return -ENODEV;
-
 	mos7840_port = mos7840_get_port_private(port);
 	if (mos7840_port == NULL)
 		return -ENODEV;
@@ -966,9 +946,6 @@ static int mos7840_write(struct tty_struct *tty, struct usb_serial_port *port,
 	/* __u16 Data; */
 	const unsigned char *current_position = data;
 
-	if (mos7840_serial_paranoia_check(serial, __func__))
-		return -1;
-
 	mos7840_port = mos7840_get_port_private(port);
 	if (mos7840_port == NULL)
 		return -1;
@@ -1248,9 +1225,6 @@ static int mos7840_send_cmd_write_baud_rate(struct moschip_port *mos7840_port,
 	__u16 Data;
 	__u16 clk_sel_val;
 
-	if (mos7840_serial_paranoia_check(port->serial, __func__))
-		return -1;
-
 	dev_dbg(&port->dev, "%s - baud = %d\n", __func__, baudRate);
 	/* reset clk_uart_sel in spregOffset */
 	if (baudRate > 115200) {
@@ -1347,9 +1321,6 @@ static void mos7840_change_port_settings(struct tty_struct *tty,
 	int status;
 	__u16 Data;
 
-	if (mos7840_serial_paranoia_check(port->serial, __func__))
-		return;
-
 	if (!mos7840_port->open) {
 		dev_dbg(&port->dev, "%s - port not opened\n", __func__);
 		return;
@@ -1488,14 +1459,9 @@ static void mos7840_set_termios(struct tty_struct *tty,
 				struct usb_serial_port *port,
 				struct ktermios *old_termios)
 {
-	struct usb_serial *serial = port->serial;
 	int status;
 	struct moschip_port *mos7840_port;
 
-
-	if (mos7840_serial_paranoia_check(serial, __func__))
-		return;
-
 	mos7840_port = mos7840_get_port_private(port);
 
 	if (mos7840_port == NULL)
-- 
2.23.0


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

* [PATCH 08/11] USB: serial: mos7840: drop serial struct accessor
  2019-11-07 13:28 [PATCH 00/11] USB: serial: mos7840: type detection and clean ups Johan Hovold
                   ` (6 preceding siblings ...)
  2019-11-07 13:29 ` [PATCH 07/11] USB: serial: mos7840: drop paranoid serial checks Johan Hovold
@ 2019-11-07 13:29 ` Johan Hovold
  2019-11-07 13:29 ` [PATCH 09/11] USB: serial: mos7840: drop port driver data accessors Johan Hovold
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Johan Hovold @ 2019-11-07 13:29 UTC (permalink / raw)
  To: Johan Hovold; +Cc: linux-usb

Drop the helper used to retrieve the serial struct pointer from the port
structure.

Note that this helper was only used when the serial structure was
actually not needed.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/mos7840.c | 30 ------------------------------
 1 file changed, 30 deletions(-)

diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index 207b88d948a9..01787685a4ee 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -446,19 +446,6 @@ static void mos7840_led_activity(struct usb_serial_port *port)
 				jiffies + msecs_to_jiffies(LED_ON_MS));
 }
 
-static struct usb_serial *mos7840_get_usb_serial(struct usb_serial_port *port,
-						 const char *function)
-{
-	/* if no port was specified */
-	if (!port) {
-		/* then say that we don't have a valid usb_serial thing,
-		 * which will end up genrating -ENODEV return values */
-		return NULL;
-	}
-
-	return port->serial;
-}
-
 /*****************************************************************************
  * mos7840_bulk_in_callback
  *	this is the callback function for when we have received data on the
@@ -471,7 +458,6 @@ static void mos7840_bulk_in_callback(struct urb *urb)
 	struct usb_serial_port *port = mos7840_port->port;
 	int retval;
 	unsigned char *data;
-	struct usb_serial *serial;
 	int status = urb->status;
 
 	if (status) {
@@ -480,12 +466,6 @@ static void mos7840_bulk_in_callback(struct urb *urb)
 		return;
 	}
 
-	serial = mos7840_get_usb_serial(port, __func__);
-	if (!serial) {
-		mos7840_port->read_urb_busy = false;
-		return;
-	}
-
 	data = urb->transfer_buffer;
 	usb_serial_debug_data(&port->dev, __func__, urb->actual_length, data);
 
@@ -822,15 +802,10 @@ static int mos7840_chars_in_buffer(struct tty_struct *tty)
 
 static void mos7840_close(struct usb_serial_port *port)
 {
-	struct usb_serial *serial;
 	struct moschip_port *mos7840_port;
 	int j;
 	__u16 Data;
 
-	serial = mos7840_get_usb_serial(port, __func__);
-	if (!serial)
-		return;
-
 	mos7840_port = mos7840_get_port_private(port);
 	if (mos7840_port == NULL)
 		return;
@@ -866,13 +841,8 @@ static void mos7840_break(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	unsigned char data;
-	struct usb_serial *serial;
 	struct moschip_port *mos7840_port;
 
-	serial = mos7840_get_usb_serial(port, __func__);
-	if (!serial)
-		return;
-
 	mos7840_port = mos7840_get_port_private(port);
 
 	if (mos7840_port == NULL)
-- 
2.23.0


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

* [PATCH 09/11] USB: serial: mos7840: drop port driver data accessors
  2019-11-07 13:28 [PATCH 00/11] USB: serial: mos7840: type detection and clean ups Johan Hovold
                   ` (7 preceding siblings ...)
  2019-11-07 13:29 ` [PATCH 08/11] USB: serial: mos7840: drop serial struct accessor Johan Hovold
@ 2019-11-07 13:29 ` Johan Hovold
  2019-11-07 13:29 ` [PATCH 10/11] USB: serial: mos7840: drop read-urb check Johan Hovold
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Johan Hovold @ 2019-11-07 13:29 UTC (permalink / raw)
  To: Johan Hovold; +Cc: linux-usb

Drop the custom port driver data accessors and paranoid sanity checks.
The driver data is not cleared until the driver is unbound.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/mos7840.c | 104 +++++------------------------------
 1 file changed, 13 insertions(+), 91 deletions(-)

diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index 01787685a4ee..11706f2d4145 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -344,26 +344,6 @@ static void mos7840_dump_serial_port(struct usb_serial_port *port,
 
 }
 
-/************************************************************************/
-/************************************************************************/
-/*             I N T E R F A C E   F U N C T I O N S			*/
-/*             I N T E R F A C E   F U N C T I O N S			*/
-/************************************************************************/
-/************************************************************************/
-
-static inline void mos7840_set_port_private(struct usb_serial_port *port,
-					    struct moschip_port *data)
-{
-	usb_set_serial_port_data(port, (void *)data);
-}
-
-static inline struct moschip_port *mos7840_get_port_private(struct
-							    usb_serial_port
-							    *port)
-{
-	return (struct moschip_port *)usb_get_serial_port_data(port);
-}
-
 /************************************************************************/
 /************************************************************************/
 /*            U S B  C A L L B A C K   F U N C T I O N S                */
@@ -541,17 +521,13 @@ static void mos7840_bulk_out_data_callback(struct urb *urb)
 
 static int mos7840_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
+	struct moschip_port *mos7840_port = usb_get_serial_port_data(port);
 	struct usb_serial *serial = port->serial;
 	int response;
 	int j;
 	struct urb *urb;
 	__u16 Data;
 	int status;
-	struct moschip_port *mos7840_port;
-
-	mos7840_port = mos7840_get_port_private(port);
-	if (mos7840_port == NULL)
-		return -ENODEV;
 
 	usb_clear_halt(serial->dev, port->write_urb->pipe);
 	usb_clear_halt(serial->dev, port->read_urb->pipe);
@@ -773,14 +749,10 @@ static int mos7840_open(struct tty_struct *tty, struct usb_serial_port *port)
 static int mos7840_chars_in_buffer(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
+	struct moschip_port *mos7840_port = usb_get_serial_port_data(port);
 	int i;
 	int chars = 0;
 	unsigned long flags;
-	struct moschip_port *mos7840_port;
-
-	mos7840_port = mos7840_get_port_private(port);
-	if (mos7840_port == NULL)
-		return 0;
 
 	spin_lock_irqsave(&mos7840_port->pool_lock, flags);
 	for (i = 0; i < NUM_URBS; ++i) {
@@ -802,14 +774,10 @@ static int mos7840_chars_in_buffer(struct tty_struct *tty)
 
 static void mos7840_close(struct usb_serial_port *port)
 {
-	struct moschip_port *mos7840_port;
+	struct moschip_port *mos7840_port = usb_get_serial_port_data(port);
 	int j;
 	__u16 Data;
 
-	mos7840_port = mos7840_get_port_private(port);
-	if (mos7840_port == NULL)
-		return;
-
 	for (j = 0; j < NUM_URBS; ++j)
 		usb_kill_urb(mos7840_port->write_urb_pool[j]);
 
@@ -840,13 +808,8 @@ static void mos7840_close(struct usb_serial_port *port)
 static void mos7840_break(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
+	struct moschip_port *mos7840_port = usb_get_serial_port_data(port);
 	unsigned char data;
-	struct moschip_port *mos7840_port;
-
-	mos7840_port = mos7840_get_port_private(port);
-
-	if (mos7840_port == NULL)
-		return;
 
 	if (break_state == -1)
 		data = mos7840_port->shadowLCR | LCR_SET_BREAK;
@@ -871,14 +834,10 @@ static void mos7840_break(struct tty_struct *tty, int break_state)
 static int mos7840_write_room(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
+	struct moschip_port *mos7840_port = usb_get_serial_port_data(port);
 	int i;
 	int room = 0;
 	unsigned long flags;
-	struct moschip_port *mos7840_port;
-
-	mos7840_port = mos7840_get_port_private(port);
-	if (mos7840_port == NULL)
-		return -1;
 
 	spin_lock_irqsave(&mos7840_port->pool_lock, flags);
 	for (i = 0; i < NUM_URBS; ++i) {
@@ -904,22 +863,17 @@ static int mos7840_write_room(struct tty_struct *tty)
 static int mos7840_write(struct tty_struct *tty, struct usb_serial_port *port,
 			 const unsigned char *data, int count)
 {
+	struct moschip_port *mos7840_port = usb_get_serial_port_data(port);
 	struct usb_serial *serial = port->serial;
 	int status;
 	int i;
 	int bytes_sent = 0;
 	int transfer_size;
 	unsigned long flags;
-
-	struct moschip_port *mos7840_port;
 	struct urb *urb;
 	/* __u16 Data; */
 	const unsigned char *current_position = data;
 
-	mos7840_port = mos7840_get_port_private(port);
-	if (mos7840_port == NULL)
-		return -1;
-
 	/* try to find a free urb in the list */
 	urb = NULL;
 
@@ -1000,14 +954,9 @@ static int mos7840_write(struct tty_struct *tty, struct usb_serial_port *port,
 static void mos7840_throttle(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
-	struct moschip_port *mos7840_port;
+	struct moschip_port *mos7840_port = usb_get_serial_port_data(port);
 	int status;
 
-	mos7840_port = mos7840_get_port_private(port);
-
-	if (mos7840_port == NULL)
-		return;
-
 	if (!mos7840_port->open) {
 		dev_dbg(&port->dev, "%s", "port not opened\n");
 		return;
@@ -1039,11 +988,8 @@ static void mos7840_throttle(struct tty_struct *tty)
 static void mos7840_unthrottle(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
+	struct moschip_port *mos7840_port = usb_get_serial_port_data(port);
 	int status;
-	struct moschip_port *mos7840_port = mos7840_get_port_private(port);
-
-	if (mos7840_port == NULL)
-		return;
 
 	if (!mos7840_port->open) {
 		dev_dbg(&port->dev, "%s - port not opened\n", __func__);
@@ -1071,15 +1017,10 @@ static void mos7840_unthrottle(struct tty_struct *tty)
 static int mos7840_tiocmget(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
-	struct moschip_port *mos7840_port;
 	unsigned int result;
 	__u16 msr;
 	__u16 mcr;
 	int status;
-	mos7840_port = mos7840_get_port_private(port);
-
-	if (mos7840_port == NULL)
-		return -ENODEV;
 
 	status = mos7840_get_uart_reg(port, MODEM_STATUS_REGISTER, &msr);
 	if (status < 0)
@@ -1104,15 +1045,10 @@ static int mos7840_tiocmset(struct tty_struct *tty,
 			    unsigned int set, unsigned int clear)
 {
 	struct usb_serial_port *port = tty->driver_data;
-	struct moschip_port *mos7840_port;
+	struct moschip_port *mos7840_port = usb_get_serial_port_data(port);
 	unsigned int mcr;
 	int status;
 
-	mos7840_port = mos7840_get_port_private(port);
-
-	if (mos7840_port == NULL)
-		return -ENODEV;
-
 	/* FIXME: What locks the port registers ? */
 	mcr = mos7840_port->shadowMCR;
 	if (clear & TIOCM_RTS)
@@ -1429,13 +1365,8 @@ static void mos7840_set_termios(struct tty_struct *tty,
 				struct usb_serial_port *port,
 				struct ktermios *old_termios)
 {
+	struct moschip_port *mos7840_port = usb_get_serial_port_data(port);
 	int status;
-	struct moschip_port *mos7840_port;
-
-	mos7840_port = mos7840_get_port_private(port);
-
-	if (mos7840_port == NULL)
-		return;
 
 	if (!mos7840_port->open) {
 		dev_dbg(&port->dev, "%s - port not opened\n", __func__);
@@ -1497,7 +1428,7 @@ static int mos7840_get_serial_info(struct tty_struct *tty,
 				   struct serial_struct *ss)
 {
 	struct usb_serial_port *port = tty->driver_data;
-	struct moschip_port *mos7840_port = mos7840_get_port_private(port);
+	struct moschip_port *mos7840_port = usb_get_serial_port_data(port);
 
 	ss->type = PORT_16550A;
 	ss->line = mos7840_port->port->minor;
@@ -1520,12 +1451,6 @@ static int mos7840_ioctl(struct tty_struct *tty,
 {
 	struct usb_serial_port *port = tty->driver_data;
 	void __user *argp = (void __user *)arg;
-	struct moschip_port *mos7840_port;
-
-	mos7840_port = mos7840_get_port_private(port);
-
-	if (mos7840_port == NULL)
-		return -1;
 
 	switch (cmd) {
 		/* return number of bytes available */
@@ -1692,7 +1617,6 @@ static int mos7840_port_probe(struct usb_serial_port *port)
 	 * common to all port */
 
 	mos7840_port->port = port;
-	mos7840_set_port_private(port, mos7840_port);
 	spin_lock_init(&mos7840_port->pool_lock);
 
 	/* minor is not initialised until later by
@@ -1718,7 +1642,7 @@ static int mos7840_port_probe(struct usb_serial_port *port)
 		mos7840_port->DcrRegOffset = 0x16 + 3 * (phy_num - 2);
 	}
 	mos7840_dump_serial_port(port, mos7840_port);
-	mos7840_set_port_private(port, mos7840_port);
+	usb_set_serial_port_data(port, mos7840_port);
 
 	/* enable rx_disable bit in control register */
 	status = mos7840_get_reg_sync(port,
@@ -1859,9 +1783,7 @@ static int mos7840_port_probe(struct usb_serial_port *port)
 
 static int mos7840_port_remove(struct usb_serial_port *port)
 {
-	struct moschip_port *mos7840_port;
-
-	mos7840_port = mos7840_get_port_private(port);
+	struct moschip_port *mos7840_port = usb_get_serial_port_data(port);
 
 	if (mos7840_port->has_led) {
 		/* Turn off LED */
-- 
2.23.0


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

* [PATCH 10/11] USB: serial: mos7840: drop read-urb check
  2019-11-07 13:28 [PATCH 00/11] USB: serial: mos7840: type detection and clean ups Johan Hovold
                   ` (8 preceding siblings ...)
  2019-11-07 13:29 ` [PATCH 09/11] USB: serial: mos7840: drop port driver data accessors Johan Hovold
@ 2019-11-07 13:29 ` Johan Hovold
  2019-11-07 13:29 ` [PATCH 11/11] USB: serial: mos7840: drop port open flag Johan Hovold
  2019-11-11 13:27 ` [PATCH 00/11] USB: serial: mos7840: type detection and clean ups Greg KH
  11 siblings, 0 replies; 14+ messages in thread
From: Johan Hovold @ 2019-11-07 13:29 UTC (permalink / raw)
  To: Johan Hovold; +Cc: linux-usb

Drop read-urb check which is always false from completion the callback.

The driver read-urb pointer is set at every open and is never cleared.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/mos7840.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index 11706f2d4145..f5c08effa3ab 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -457,12 +457,6 @@ static void mos7840_bulk_in_callback(struct urb *urb)
 		dev_dbg(&port->dev, "icount.rx is %d:\n", port->icount.rx);
 	}
 
-	if (!mos7840_port->read_urb) {
-		dev_dbg(&port->dev, "%s", "URB KILLED !!!\n");
-		mos7840_port->read_urb_busy = false;
-		return;
-	}
-
 	if (mos7840_port->has_led)
 		mos7840_led_activity(port);
 
@@ -1377,11 +1371,6 @@ static void mos7840_set_termios(struct tty_struct *tty,
 
 	mos7840_change_port_settings(tty, mos7840_port, old_termios);
 
-	if (!mos7840_port->read_urb) {
-		dev_dbg(&port->dev, "%s", "URB KILLED !!!!!\n");
-		return;
-	}
-
 	if (!mos7840_port->read_urb_busy) {
 		mos7840_port->read_urb_busy = true;
 		status = usb_submit_urb(mos7840_port->read_urb, GFP_KERNEL);
-- 
2.23.0


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

* [PATCH 11/11] USB: serial: mos7840: drop port open flag
  2019-11-07 13:28 [PATCH 00/11] USB: serial: mos7840: type detection and clean ups Johan Hovold
                   ` (9 preceding siblings ...)
  2019-11-07 13:29 ` [PATCH 10/11] USB: serial: mos7840: drop read-urb check Johan Hovold
@ 2019-11-07 13:29 ` Johan Hovold
  2019-11-11 13:27 ` [PATCH 00/11] USB: serial: mos7840: type detection and clean ups Greg KH
  11 siblings, 0 replies; 14+ messages in thread
From: Johan Hovold @ 2019-11-07 13:29 UTC (permalink / raw)
  To: Johan Hovold; +Cc: linux-usb

Drop the redundant port open flag and corresponding checks. USB serial
core will not call any of these driver callbacks for a closed port, and the
write URBs are stopped at close().

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/mos7840.c | 29 +----------------------------
 1 file changed, 1 insertion(+), 28 deletions(-)

diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index f5c08effa3ab..23f91d658cb4 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -198,7 +198,6 @@ struct moschip_port {
 	struct urb *read_urb;	/* read URB for this port */
 	__u8 shadowLCR;		/* last LCR value received */
 	__u8 shadowMCR;		/* last MCR value received */
-	char open;
 	struct usb_serial_port *port;	/* loop back to the owner of this object */
 
 	/* Offsets */
@@ -497,8 +496,7 @@ static void mos7840_bulk_out_data_callback(struct urb *urb)
 		return;
 	}
 
-	if (mos7840_port->open)
-		tty_port_tty_wakeup(&port->port);
+	tty_port_tty_wakeup(&port->port);
 
 }
 
@@ -714,9 +712,6 @@ static int mos7840_open(struct tty_struct *tty, struct usb_serial_port *port)
 	/* initialize our port settings */
 	/* Must set to enable ints! */
 	mos7840_port->shadowMCR = MCR_MASTER_IE;
-	/* send a open port command */
-	mos7840_port->open = 1;
-	/* mos7840_change_port_settings(mos7840_port,old_termios); */
 
 	return 0;
 err:
@@ -791,8 +786,6 @@ static void mos7840_close(struct usb_serial_port *port)
 
 	Data = 0x00;
 	mos7840_set_uart_reg(port, INTERRUPT_ENABLE_REGISTER, Data);
-
-	mos7840_port->open = 0;
 }
 
 /*****************************************************************************
@@ -951,11 +944,6 @@ static void mos7840_throttle(struct tty_struct *tty)
 	struct moschip_port *mos7840_port = usb_get_serial_port_data(port);
 	int status;
 
-	if (!mos7840_port->open) {
-		dev_dbg(&port->dev, "%s", "port not opened\n");
-		return;
-	}
-
 	/* if we are implementing XON/XOFF, send the stop character */
 	if (I_IXOFF(tty)) {
 		unsigned char stop_char = STOP_CHAR(tty);
@@ -985,11 +973,6 @@ static void mos7840_unthrottle(struct tty_struct *tty)
 	struct moschip_port *mos7840_port = usb_get_serial_port_data(port);
 	int status;
 
-	if (!mos7840_port->open) {
-		dev_dbg(&port->dev, "%s - port not opened\n", __func__);
-		return;
-	}
-
 	/* if we are implementing XON/XOFF, send the start character */
 	if (I_IXOFF(tty)) {
 		unsigned char start_char = START_CHAR(tty);
@@ -1221,11 +1204,6 @@ static void mos7840_change_port_settings(struct tty_struct *tty,
 	int status;
 	__u16 Data;
 
-	if (!mos7840_port->open) {
-		dev_dbg(&port->dev, "%s - port not opened\n", __func__);
-		return;
-	}
-
 	lData = LCR_BITS_8;
 	lStop = LCR_STOP_1;
 	lParity = LCR_PAR_NONE;
@@ -1362,11 +1340,6 @@ static void mos7840_set_termios(struct tty_struct *tty,
 	struct moschip_port *mos7840_port = usb_get_serial_port_data(port);
 	int status;
 
-	if (!mos7840_port->open) {
-		dev_dbg(&port->dev, "%s - port not opened\n", __func__);
-		return;
-	}
-
 	/* change the port settings to the new ones specified */
 
 	mos7840_change_port_settings(tty, mos7840_port, old_termios);
-- 
2.23.0


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

* Re: [PATCH 00/11] USB: serial: mos7840: type detection and clean ups
  2019-11-07 13:28 [PATCH 00/11] USB: serial: mos7840: type detection and clean ups Johan Hovold
                   ` (10 preceding siblings ...)
  2019-11-07 13:29 ` [PATCH 11/11] USB: serial: mos7840: drop port open flag Johan Hovold
@ 2019-11-11 13:27 ` Greg KH
  2019-11-12  9:21   ` Johan Hovold
  11 siblings, 1 reply; 14+ messages in thread
From: Greg KH @ 2019-11-11 13:27 UTC (permalink / raw)
  To: Johan Hovold; +Cc: linux-usb

On Thu, Nov 07, 2019 at 02:28:53PM +0100, Johan Hovold wrote:
> The mos7840 device-type detection is fragile and cannot generally be
> relied upon (e.g. as recently reported for Moxa UPort 2210 which was
> detected as a four-port device).
> 
> The first couple of patches adds support for encoding known chip
> features in the device-id table, and documents the underlying
> assumptions for the mcs7810-detection hack.
> 
> Turns out we have a lot of legacy cruft in this driver, and the
> remaining patches rips that out.
> 
> Johan
> 
> 
> Johan Hovold (11):
>   USB: serial: mos7840: clean up device-type handling
>   USB: serial: mos7840: document MCS7810 detection hack
>   USB: serial: mos7840: fix probe error handling
>   USB: serial: mos7840: rip out broken interrupt handling
>   USB: serial: mos7840: drop redundant urb context check
>   USB: serial: mos7840: drop paranoid port checks
>   USB: serial: mos7840: drop paranoid serial checks
>   USB: serial: mos7840: drop serial struct accessor
>   USB: serial: mos7840: drop port driver data accessors
>   USB: serial: mos7840: drop read-urb check
>   USB: serial: mos7840: drop port open flag
> 
>  drivers/usb/serial/mos7840.c | 770 +++++------------------------------
>  1 file changed, 102 insertions(+), 668 deletions(-)

Nice cleanups:

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

* Re: [PATCH 00/11] USB: serial: mos7840: type detection and clean ups
  2019-11-11 13:27 ` [PATCH 00/11] USB: serial: mos7840: type detection and clean ups Greg KH
@ 2019-11-12  9:21   ` Johan Hovold
  0 siblings, 0 replies; 14+ messages in thread
From: Johan Hovold @ 2019-11-12  9:21 UTC (permalink / raw)
  To: Greg KH; +Cc: Johan Hovold, linux-usb

On Mon, Nov 11, 2019 at 02:27:46PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Nov 07, 2019 at 02:28:53PM +0100, Johan Hovold wrote:

> > Johan Hovold (11):
> >   USB: serial: mos7840: clean up device-type handling
> >   USB: serial: mos7840: document MCS7810 detection hack
> >   USB: serial: mos7840: fix probe error handling
> >   USB: serial: mos7840: rip out broken interrupt handling
> >   USB: serial: mos7840: drop redundant urb context check
> >   USB: serial: mos7840: drop paranoid port checks
> >   USB: serial: mos7840: drop paranoid serial checks
> >   USB: serial: mos7840: drop serial struct accessor
> >   USB: serial: mos7840: drop port driver data accessors
> >   USB: serial: mos7840: drop read-urb check
> >   USB: serial: mos7840: drop port open flag
> > 
> >  drivers/usb/serial/mos7840.c | 770 +++++------------------------------
> >  1 file changed, 102 insertions(+), 668 deletions(-)
> 
> Nice cleanups:
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks for the review. All now applied.

Johan

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

end of thread, back to index

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-07 13:28 [PATCH 00/11] USB: serial: mos7840: type detection and clean ups Johan Hovold
2019-11-07 13:28 ` [PATCH 01/11] USB: serial: mos7840: clean up device-type handling Johan Hovold
2019-11-07 13:28 ` [PATCH 02/11] USB: serial: mos7840: document MCS7810 detection hack Johan Hovold
2019-11-07 13:28 ` [PATCH 03/11] USB: serial: mos7840: fix probe error handling Johan Hovold
2019-11-07 13:28 ` [PATCH 04/11] USB: serial: mos7840: rip out broken interrupt handling Johan Hovold
2019-11-07 13:28 ` [PATCH 05/11] USB: serial: mos7840: drop redundant urb context check Johan Hovold
2019-11-07 13:28 ` [PATCH 06/11] USB: serial: mos7840: drop paranoid port checks Johan Hovold
2019-11-07 13:29 ` [PATCH 07/11] USB: serial: mos7840: drop paranoid serial checks Johan Hovold
2019-11-07 13:29 ` [PATCH 08/11] USB: serial: mos7840: drop serial struct accessor Johan Hovold
2019-11-07 13:29 ` [PATCH 09/11] USB: serial: mos7840: drop port driver data accessors Johan Hovold
2019-11-07 13:29 ` [PATCH 10/11] USB: serial: mos7840: drop read-urb check Johan Hovold
2019-11-07 13:29 ` [PATCH 11/11] USB: serial: mos7840: drop port open flag Johan Hovold
2019-11-11 13:27 ` [PATCH 00/11] USB: serial: mos7840: type detection and clean ups Greg KH
2019-11-12  9:21   ` Johan Hovold

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git