USB: mxu11x0: fixes and follow ups

PATCH In review Mathieu OTHACEHE 2 participants 10 messages Jan 4, 2016 – Feb 29, 2016

Subsystems (2)

USB USB serial

lore

Share & tools

lore

Mathieu OTHACEHE

author

2016-01-04 18:49 UTC #0 lore raw

v2 cover USB: mxu11x0: fixes and follow ups

Hi,

Here are the follow up commits proposed during last Johan review of the new mxu11x0 driver. I also patched a memory leak on usb_serial private data.

In v2, I fixed the trivial issues reported by Johan in commits 1/4 and 4/4. About commit 2/4, I was wrong in last cover-letter message.
The exact command order sniffed on Windows during opening is :

      	      SET_CONFIG (set_termios) -> OPEN_PORT -> START_PORT

To be compliant, commit 2/4 only deletes duplicated OPEN_PORT and START_PORT commands. The new commit 3/4 deletes the duplicated set_termios in open callback.

Thanks,
Mathieu

Mathieu OTHACEHE (4):
USB: mxu11x0: fix memory leak on usb_serial private data
USB: mxu11x0: clean device control commands
USB: mxu11x0: remove duplicated set_termios call
USB: mxu11x0: move firmware download and endpoint testing to probe
callback

 drivers/usb/serial/mxu11x0.c | 171 ++++++++++++++++++++++++++-----------------
 1 file changed, 104 insertions(+), 67 deletions(-)

Signature · 1 line

2.6.4

Mathieu OTHACEHE

author

2016-01-04 18:51 UTC #1 lore raw

PATCH v2 1/4 USB: mxu11x0: fix memory leak on usb_serial private data

On nominal execution, private data allocated on port_probe and attach are never freed. Add port_remove and release callbacks to free them respectively.

Signed-off-by Mathieu OTHACEHE <[email protected]>

---

Changes in v2:
* Move release callback after attach callback

 drivers/usb/serial/mxu11x0.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

drivers/usb/serial/mxu11x0.c +20 -0

index e3c3f57c..6196073 100644
@@ -368,6 +368,16 @@ static int mxu1_port_probe(struct usb_serial_port *port)
 	return 0;
 }
 
+static int mxu1_port_remove(struct usb_serial_port *port)
+{
+	struct mxu1_port *mxport;
+
+	mxport = usb_get_serial_port_data(port);
+	kfree(mxport);
+
+	return 0;
+}
+
 static int mxu1_startup(struct usb_serial *serial)
 {
 	struct mxu1_device *mxdev;

@@ -427,6 +437,14 @@ err_free_mxdev:
 	return err;
 }
 
+static void mxu1_release(struct usb_serial *serial)
+{
+	struct mxu1_device *mxdev;
+
+	mxdev = usb_get_serial_data(serial);
+	kfree(mxdev);
+}
+
 static int mxu1_write_byte(struct usb_serial_port *port, u32 addr,
 			   u8 mask, u8 byte)
 {

@@ -957,7 +975,9 @@ static struct usb_serial_driver mxu11x0_device = {
 	.id_table		= mxu1_idtable,
 	.num_ports		= 1,
 	.port_probe             = mxu1_port_probe,
+	.port_remove            = mxu1_port_remove,
 	.attach			= mxu1_startup,
+	.release                = mxu1_release,
 	.open			= mxu1_open,
 	.close			= mxu1_close,
 	.ioctl			= mxu1_ioctl,

Signature · 1 line

2.6.4

Johan Hovold

maintainer

2016-01-25 12:02 UTC #2 lore raw

Re: [PATCH v2 1/4] USB: mxu11x0: fix memory leak on usb_serial private data

Quoted 5 lines · Mathieu OTHACEHE

On nominal execution, private data allocated on port_probe and attach are never freed. Add port_remove and release callbacks to free them respectively.
Signed-off-by Mathieu OTHACEHE <[email protected]>

I've applied this one for 4.5-rc2 now.

I want to take a closer look at the last three patches and it seems they should wait for 4.6 anyway. I did notice that the vendor driver also sends double START/OPEN commands at open by the way. Perhaps ask Moxa why that is before we remove them?

Thanks,
Johan

Mathieu OTHACEHE

author

2016-01-30 17:40 UTC #3 lore raw

Re: [PATCH v2 1/4] USB: mxu11x0: fix memory leak on usb_serial private data

Quoted 9 lines · Johan Hovold

Quoted 5 lines · Mathieu OTHACEHE
On nominal execution, private data allocated on port_probe and attach are never freed. Add port_remove and release callbacks to free them respectively.
Signed-off-by Mathieu OTHACEHE <[email protected]>
I've applied this one for 4.5-rc2 now.

Quoted 7 lines

I want to take a closer look at the last three patches and it seems they should wait for 4.6 anyway. I did notice that the vendor driver also sends double START/OPEN commands at open by the way. Perhaps ask Moxa why that is before we remove them?
Thanks,
Johan

Hi Johan,

I asked MOXA about this double opening. I also noticed that the mainline driver ti_usb_3410_5052 uses the same double opening pattern. And, MOXA UPORT 11x0 serie is based on TUSB3410 chip of TI.

So, I also emailed TI, and the authors of ti_usb_3410_5052 driver.

I keep you informed,

Thank you

Mathieu

Johan Hovold

maintainer

2016-02-28 12:20 UTC #4 lore raw

Re: [PATCH v2 1/4] USB: mxu11x0: fix memory leak on usb_serial private data

Quoted 18 lines · Mathieu OTHACEHE

Quoted 9 lines · Johan Hovold
Quoted 5 lines · Mathieu OTHACEHE
On nominal execution, private data allocated on port_probe and attach are never freed. Add port_remove and release callbacks to free them respectively.
Signed-off-by Mathieu OTHACEHE <[email protected]>
I've applied this one for 4.5-rc2 now.
Quoted 7 lines
I want to take a closer look at the last three patches and it seems they should wait for 4.6 anyway. I did notice that the vendor driver also sends double START/OPEN commands at open by the way. Perhaps ask Moxa why that is before we remove them?
Thanks,
Johan

Quoted 7 lines

Hi Johan,
I asked MOXA about this double opening. I also noticed that the mainline driver ti_usb_3410_5052 uses the same double opening pattern. And, MOXA UPORT 11x0 serie is based on TUSB3410 chip of TI.
So, I also emailed TI, and the authors of ti_usb_3410_5052 driver.

Wow, this is embarrassing. I only now noticed that the mxu11x0 driver, well at least prior to all your clean-ups, is almost identical to the ti_usb_3410_5052 driver, and here I see you mention that it is indeed based on the same chip.

I wish that this had been made clear from the outset. We don't want two drivers for the same chip if we can avoid it. Instead we should try to merge these changes back to the ti_usb_3410_5052 driver and clean that up instead.

Do you see anything preventing us from using the ti_usb_3410_5052 driver for these Moxa devices?

Thanks,
Johan

Mathieu OTHACEHE

author

2016-02-28 16:31 UTC #5 lore raw

Re: [PATCH v2 1/4] USB: mxu11x0: fix memory leak on usb_serial private data

Quoted 32 lines · Johan Hovold

Quoted 18 lines · Mathieu OTHACEHE
Quoted 9 lines · Johan Hovold
Quoted 5 lines · Mathieu OTHACEHE
On nominal execution, private data allocated on port_probe and attach are never freed. Add port_remove and release callbacks to free them respectively.
Signed-off-by Mathieu OTHACEHE <[email protected]>
I've applied this one for 4.5-rc2 now.
Quoted 7 lines
I want to take a closer look at the last three patches and it seems they should wait for 4.6 anyway. I did notice that the vendor driver also sends double START/OPEN commands at open by the way. Perhaps ask Moxa why that is before we remove them?
Thanks,
Johan
Quoted 7 lines
Hi Johan,
I asked MOXA about this double opening. I also noticed that the mainline driver ti_usb_3410_5052 uses the same double opening pattern. And, MOXA UPORT 11x0 serie is based on TUSB3410 chip of TI.
So, I also emailed TI, and the authors of ti_usb_3410_5052 driver.
Wow, this is embarrassing. I only now noticed that the mxu11x0 driver, well at least prior to all your clean-ups, is almost identical to the ti_usb_3410_5052 driver, and here I see you mention that it is indeed based on the same chip.

Quoted 7 lines

I wish that this had been made clear from the outset. We don't want two drivers for the same chip if we can avoid it. Instead we should try to merge these changes back to the ti_usb_3410_5052 driver and clean that up instead.
Do you see anything preventing us from using the ti_usb_3410_5052 driver for these Moxa devices?

Hi Johan,

No, I don't see any problem to do that. I am testing ti_usb_3410_5052 with support for MOXA 11x0 and almost everything seems fine. Sorry I didn't noticed it before, it would have saved us some time.

So, I could post a patch serie :

1. Removing mxu11x0 driver
2. Patching ti_usb_3410_5052
3. Cleaning up ti_usb_3410_5052 the same as we cleaned-up mxu11x0

Btw, no response of TI or MOXA about the double opening stuff.

Thank you,
Mathieu

Johan Hovold

maintainer

2016-02-29 09:38 UTC #6 lore raw

Re: [PATCH v2 1/4] USB: mxu11x0: fix memory leak on usb_serial private data

Quoted 7 lines · Mathieu OTHACEHE

On Sun, Feb 28, 2016 at 01:20:16PM +0100, Johan Hovold wrote:
So, I could post a patch serie :
1. Removing mxu11x0 driver
2. Patching ti_usb_3410_5052
3. Cleaning up ti_usb_3410_5052 the same as we cleaned-up mxu11x0

Please do. If we could bring ti_usb_3410_5052 to the same state as mxu11x0 is today that's be great.

Quoted 1 line

Btw, no response of TI or MOXA about the double opening stuff.

Never mind that for now. Moxa apparently just copied the TI driver and ran search and replace on the symbol prefixes. I assume they won't have an answer.

Thanks,
Johan

Mathieu OTHACEHE

author

2016-01-04 18:50 UTC #7 lore raw

PATCH v2 2/4 USB: mxu11x0: clean device control commands

In reply to Mathieu OTHACEHE

Sending OPEN and START commands twice is not necessary for this driver. Also send STOP command at close.

Signed-off-by Mathieu OTHACEHE <[email protected]>

---

Changes in v2:
* Only remove OPEN_PORT and START_PORT in open callback.

 drivers/usb/serial/mxu11x0.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

drivers/usb/serial/mxu11x0.c +7 ~~-14~~

index 6196073..7ad2727 100644
@@ -833,20 +833,6 @@ static int mxu1_open(struct tty_struct *tty, struct usb_serial_port *port)
 	if (tty)
 		mxu1_set_termios(tty, port, NULL);
 
-	status = mxu1_send_ctrl_urb(serial, MXU1_OPEN_PORT,
-				    open_settings, MXU1_UART1_PORT);
-	if (status) {
-		dev_err(&port->dev, "cannot send open command: %d\n", status);
-		goto unlink_int_urb;
-	}
-
-	status = mxu1_send_ctrl_urb(serial, MXU1_START_PORT,
-				    0, MXU1_UART1_PORT);
-	if (status) {
-		dev_err(&port->dev, "cannot send start command: %d\n", status);
-		goto unlink_int_urb;
-	}
-
 	status = usb_serial_generic_open(tty, port);
 	if (status)
 		goto unlink_int_urb;

@@ -866,6 +852,13 @@ static void mxu1_close(struct usb_serial_port *port)
 	usb_serial_generic_close(port);
 	usb_kill_urb(port->interrupt_in_urb);
 
+	status = mxu1_send_ctrl_urb(port->serial, MXU1_STOP_PORT,
+				    0, MXU1_UART1_PORT);
+	if (status) {
+		dev_err(&port->dev, "failed to send stop port command: %d\n",
+			status);
+	}
+
 	status = mxu1_send_ctrl_urb(port->serial, MXU1_CLOSE_PORT,
 				    0, MXU1_UART1_PORT);
 	if (status) {

Signature · 1 line

2.6.4

Mathieu OTHACEHE

author

2016-01-04 18:50 UTC #8 lore raw

PATCH v2 3/4 USB: mxu11x0: remove duplicated set_termios call

In reply to Mathieu OTHACEHE

The function mxu1_set_termios is called twice in open callback. Only the first call is necessary.

Signed-off-by Mathieu OTHACEHE <[email protected]>

---
 drivers/usb/serial/mxu11x0.c | 3 ---
 1 file changed, 3 deletions(-)

drivers/usb/serial/mxu11x0.c +0 -3

index 7ad2727..6326cda 100644
@@ -830,9 +830,6 @@ static int mxu1_open(struct tty_struct *tty, struct usb_serial_port *port)
 	usb_clear_halt(serial->dev, port->write_urb->pipe);
 	usb_clear_halt(serial->dev, port->read_urb->pipe);
 
-	if (tty)
-		mxu1_set_termios(tty, port, NULL);
-
 	status = usb_serial_generic_open(tty, port);
 	if (status)
 		goto unlink_int_urb;

Signature · 1 line

2.6.4

Mathieu OTHACEHE

author

2016-01-04 18:50 UTC #9 lore raw

PATCH v2 4/4 USB: mxu11x0: move firmware download and endpoint testing to probe callback

In reply to Mathieu OTHACEHE

Move interrupt in endpoint test and firmware download to a new probe callback. This avoids unnecessary memory allocations done by core before port_probe callback is called.

If the device has to be reseted (firmware downloaded) or if the interface is incorrect (no interrupt in endpoint), the probe fails by returning ENODEV.

Signed-off-by Mathieu OTHACEHE <[email protected]>

---

Changes in v2:
* Remove useless paranthesis
* Fix multi-line comment

 drivers/usb/serial/mxu11x0.c | 131 ++++++++++++++++++++++++++-----------------
 1 file changed, 79 insertions(+), 52 deletions(-)

drivers/usb/serial/mxu11x0.c +79 ~~-52~~

index 6326cda..b942aeb 100644
@@ -272,7 +272,8 @@ static int mxu1_send_ctrl_urb(struct usb_serial *serial,
 }
 
 static int mxu1_download_firmware(struct usb_serial *serial,
-				  const struct firmware *fw_p)
+				  const struct firmware *fw_p,
+				  struct usb_endpoint_descriptor *endpoint)
 {
 	int status = 0;
 	int buffer_size;

@@ -285,7 +286,7 @@ static int mxu1_download_firmware(struct usb_serial *serial,
 	struct mxu1_firmware_header *header;
 	unsigned int pipe;
 
-	pipe = usb_sndbulkpipe(dev, serial->port[0]->bulk_out_endpointAddress);
+	pipe = usb_sndbulkpipe(dev, endpoint->bEndpointAddress);
 
 	buffer_size = fw_p->size + sizeof(*header);
 	buffer = kmalloc(buffer_size, GFP_KERNEL);

@@ -328,16 +329,86 @@ static int mxu1_download_firmware(struct usb_serial *serial,
 	return 0;
 }
 
-static int mxu1_port_probe(struct usb_serial_port *port)
+static int mxu1_probe(struct usb_serial *serial, const struct usb_device_id *id)
 {
-	struct mxu1_port *mxport;
-	struct mxu1_device *mxdev;
+	struct usb_host_interface *cur_altsetting;
+	char fw_name[32];
+	const struct firmware *fw_p = NULL;
+	struct usb_device *dev = serial->dev;
+	u16 model;
+	int err;
+	struct usb_endpoint_descriptor *endpoint, *interrupt_in, *bulk_out;
+	int i;
+
+	dev_dbg(&serial->interface->dev, "%s - product 0x%04X, num configurations %d, configuration value %d\n",
+		__func__, le16_to_cpu(dev->descriptor.idProduct),
+		dev->descriptor.bNumConfigurations,
+		dev->actconfig->desc.bConfigurationValue);
+
+	cur_altsetting = serial->interface->cur_altsetting;
+	interrupt_in = NULL;
+	bulk_out = NULL;
+
+	for (i = 0; i < cur_altsetting->desc.bNumEndpoints; i++) {
+		endpoint = &cur_altsetting->endpoint[i].desc;
+		if (usb_endpoint_is_bulk_out(endpoint)) {
+			dev_dbg(&serial->interface->dev,
+				"found bulk out on endpoint %d\n", i);
+			bulk_out = endpoint;
+		}
+
+		if (usb_endpoint_is_int_in(endpoint)) {
+			dev_dbg(&serial->interface->dev,
+				"found interrupt in on endpoint %d\n", i);
+			interrupt_in = endpoint;
+		}
+	}
+
+	/* if we have only 1 bulk out endpoint, download firmware */
+	if (bulk_out && cur_altsetting->desc.bNumEndpoints == 1) {
+
+		model = le16_to_cpu(dev->descriptor.idProduct);
+		snprintf(fw_name,
+			 sizeof(fw_name),
+			 "moxa/moxa-%04x.fw",
+			 model);
+
+		err = request_firmware(&fw_p, fw_name, &serial->interface->dev);
+		if (err) {
+			dev_err(&serial->interface->dev, "failed to request firmware: %d\n",
+				err);
+			return -ENODEV;
+		}
+
+		err = mxu1_download_firmware(serial, fw_p, bulk_out);
+		if (err)
+			goto err_release_firmware;
 
-	if (!port->interrupt_in_urb) {
-		dev_err(&port->dev, "no interrupt urb\n");
+		/* device is being reset */
+		err = -ENODEV;
+		goto err_release_firmware;
+
+	} else if (!interrupt_in) {
+		/*
+		 * firmware is already loaded but there is
+		 * no interrupt in endpoint available
+		 */
+		dev_err(&serial->interface->dev, "no interrupt endpoint\n");
 		return -ENODEV;
 	}
 
+	return 0;
+
+err_release_firmware:
+	release_firmware(fw_p);
+	return err;
+}
+
+static int mxu1_port_probe(struct usb_serial_port *port)
+{
+	struct mxu1_port *mxport;
+	struct mxu1_device *mxdev;
+
 	mxport = kzalloc(sizeof(struct mxu1_port), GFP_KERNEL);
 	if (!mxport)
 		return -ENOMEM;

@@ -381,16 +452,6 @@ static int mxu1_port_remove(struct usb_serial_port *port)
 static int mxu1_startup(struct usb_serial *serial)
 {
 	struct mxu1_device *mxdev;
-	struct usb_device *dev = serial->dev;
-	struct usb_host_interface *cur_altsetting;
-	char fw_name[32];
-	const struct firmware *fw_p = NULL;
-	int err;
-
-	dev_dbg(&serial->interface->dev, "%s - product 0x%04X, num configurations %d, configuration value %d\n",
-		__func__, le16_to_cpu(dev->descriptor.idProduct),
-		dev->descriptor.bNumConfigurations,
-		dev->actconfig->desc.bConfigurationValue);
 
 	/* create device structure */
 	mxdev = kzalloc(sizeof(struct mxu1_device), GFP_KERNEL);

@@ -399,42 +460,7 @@ static int mxu1_startup(struct usb_serial *serial)
 
 	usb_set_serial_data(serial, mxdev);
 
-	mxdev->mxd_model = le16_to_cpu(dev->descriptor.idProduct);
-
-	cur_altsetting = serial->interface->cur_altsetting;
-
-	/* if we have only 1 configuration, download firmware */
-	if (cur_altsetting->desc.bNumEndpoints == 1) {
-
-		snprintf(fw_name,
-			 sizeof(fw_name),
-			 "moxa/moxa-%04x.fw",
-			 mxdev->mxd_model);
-
-		err = request_firmware(&fw_p, fw_name, &serial->interface->dev);
-		if (err) {
-			dev_err(&serial->interface->dev, "failed to request firmware: %d\n",
-				err);
-			goto err_free_mxdev;
-		}
-
-		err = mxu1_download_firmware(serial, fw_p);
-		if (err)
-			goto err_release_firmware;
-
-		/* device is being reset */
-		err = -ENODEV;
-		goto err_release_firmware;
-	}
-
 	return 0;
-
-err_release_firmware:
-	release_firmware(fw_p);
-err_free_mxdev:
-	kfree(mxdev);
-
-	return err;
 }
 
 static void mxu1_release(struct usb_serial *serial)

@@ -964,6 +990,7 @@ static struct usb_serial_driver mxu11x0_device = {
 	.description		= "MOXA UPort 11x0",
 	.id_table		= mxu1_idtable,
 	.num_ports		= 1,
+	.probe                  = mxu1_probe,
 	.port_probe             = mxu1_port_probe,
 	.port_remove            = mxu1_port_remove,
 	.attach			= mxu1_startup,

Signature · 1 line

2.6.4