Skip to contentSkip to conversation

USB: mxu11x0: fixes and follow ups

PATCH Superseded Mathieu OTHACEHE 2 participants 7 messages Jan 3, 2016

Subsystems (2)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

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.

Mathieu

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

 drivers/usb/serial/mxu11x0.c | 179 +++++++++++++++++++++++++------------------
 1 file changed, 104 insertions(+), 75 deletions(-)
Signature · 1 line
2.6.2
#1 lore raw

PATCH 1/3 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.

---
 drivers/usb/serial/mxu11x0.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
drivers/usb/serial/mxu11x0.c +20 -0
index e3c3f57c..0fe7eab 100644
@@ -328,6 +328,14 @@ static int mxu1_download_firmware(struct usb_serial *serial,
 	return 0;
 }
 
+static void mxu1_release(struct usb_serial *serial)
+{
+	struct mxu1_device *mxdev;
+
+	mxdev = usb_get_serial_data(serial);
+	kfree(mxdev);
+}
+
 static int mxu1_port_probe(struct usb_serial_port *port)
 {
 	struct mxu1_port *mxport;
@@ -368,6 +376,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;
@@ -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.2
maintainer
#2 lore raw

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

Quoted 3 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.

Ouch. I thought I'd vetted the driver for further memleaks but apparently missed the most obvious ones.

Quoted 10 lines
---
 drivers/usb/serial/mxu11x0.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
drivers/usb/serial/mxu11x0.c +0 -0
index e3c3f57c..0fe7eab 100644

drivers/usb/serial/mxu11x0.c static int mxu1_download_firmware(struct usb_serial *serial, · line 337

Quoted 10 lines
drivers/usb/serial/mxu11x0.c +7 -0
@@ -328,6 +328,14 @@ static int mxu1_download_firmware(struct usb_serial *serial,
 	return 0;
 }
 
+static void mxu1_release(struct usb_serial *serial)
+{
+	struct mxu1_device *mxdev;
+
+	mxdev = usb_get_serial_data(serial);
+	kfree(mxdev);
+}

Please place this once after the matching attach (startup) callback.

Quoted 11 lines
drivers/usb/serial/mxu11x0.c +4 -0
+
 static int mxu1_port_probe(struct usb_serial_port *port)
 {
 	struct mxu1_port *mxport;
@@ -368,6 +376,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;

drivers/usb/serial/mxu11x0.c static int mxu1_port_probe(struct usb_serial_port *port) · line 391

Quoted 10 lines
drivers/usb/serial/mxu11x0.c +7 -0
+
+	mxport = usb_get_serial_port_data(port);
+	kfree(mxport);
+
+	return 0;
+}
+
 static int mxu1_startup(struct usb_serial *serial)
 {
 	struct mxu1_device *mxdev;

Thanks,
Johan

#3 lore raw

PATCH 2/3 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.

---
 drivers/usb/serial/mxu11x0.c | 31 +++++++------------------------
 1 file changed, 7 insertions(+), 24 deletions(-)
drivers/usb/serial/mxu11x0.c +7 -24
index 0fe7eab..354fcb5 100644
@@ -823,30 +823,6 @@ static int mxu1_open(struct tty_struct *tty, struct usb_serial_port *port)
 		goto unlink_int_urb;
 	}
 
-	/*
-	 * reset the data toggle on the bulk endpoints to work around bug in
-	 * host controllers where things get out of sync some times
-	 */
-	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 = 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 +842,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.2
maintainer
#4 lore raw

Re: [PATCH 2/3] USB: mxu11x0: clean device control commands

Quoted 13 lines · Mathieu OTHACEHE

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

---
 drivers/usb/serial/mxu11x0.c | 31 +++++++------------------------
 1 file changed, 7 insertions(+), 24 deletions(-)
drivers/usb/serial/mxu11x0.c +0 -0
index 0fe7eab..354fcb5 100644

drivers/usb/serial/mxu11x0.c static int mxu1_open(struct tty_struct *tty, struct usb_serial_port *port) · line 831

Quoted 9 lines
drivers/usb/serial/mxu11x0.c +0 -6
@@ -823,30 +823,6 @@ static int mxu1_open(struct tty_struct *tty, struct usb_serial_port *port)
 		goto unlink_int_urb;
 	}
 
-	/*
-	 * reset the data toggle on the bulk endpoints to work around bug in
-	 * host controllers where things get out of sync some times
-	 */
-	usb_clear_halt(serial->dev, port->write_urb->pipe);
-	usb_clear_halt(serial->dev, port->read_urb->pipe);

This is an unrelated change. You can remove it, but then do so in a separate patch.

drivers/usb/serial/mxu11x0.c static int mxu1_open(struct tty_struct *tty, struct usb_serial_port *port) · line 834

Quoted 3 lines
drivers/usb/serial/mxu11x0.c +0 -3
-
-	if (tty)
-		mxu1_set_termios(tty, port, NULL);

But you should definitely not be removing this.

Quoted 5 lines
drivers/usb/serial/mxu11x0.c +0 -5
-
-	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);

drivers/usb/serial/mxu11x0.c static int mxu1_open(struct tty_struct *tty, struct usb_serial_port *port) · line 849

Quoted 10 lines
drivers/usb/serial/mxu11x0.c +0 -10
-		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;
-	}
-

Also did you not say that your sniffed traffic showed the following sequence OPEN, CONFIG (e.g. purge, set_termios), START?

Johan

#5 lore raw

PATCH 3/3 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.

---
 drivers/usb/serial/mxu11x0.c | 130 ++++++++++++++++++++++++++-----------------
 1 file changed, 78 insertions(+), 52 deletions(-)
drivers/usb/serial/mxu11x0.c +78 -52
index 354fcb5..0392531 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);
@@ -336,16 +337,85 @@ static void mxu1_release(struct usb_serial *serial)
 	kfree(mxdev);
 }
 
-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;
@@ -389,16 +459,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);
@@ -407,42 +467,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 int mxu1_write_byte(struct usb_serial_port *port, u32 addr,
@@ -957,6 +982,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.2
maintainer
#6 lore raw

Re: [PATCH 3/3] USB: mxu11x0: move firmware download and endpoint testing to probe callback

Quoted 70 lines · 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.

---
 drivers/usb/serial/mxu11x0.c | 130 ++++++++++++++++++++++++++-----------------
 1 file changed, 78 insertions(+), 52 deletions(-)
drivers/usb/serial/mxu11x0.c +29 -5
index 354fcb5..0392531 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);
@@ -336,16 +337,85 @@ static void mxu1_release(struct usb_serial *serial)
 	kfree(mxdev);
 }
 
-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;
+		}

drivers/usb/serial/mxu11x0.c static void mxu1_release(struct usb_serial *serial) · line 376

Quoted 10 lines
drivers/usb/serial/mxu11x0.c +10 -0
+
+		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)) {

No need for inner parentheses.

Quoted 18 lines
drivers/usb/serial/mxu11x0.c +17 -0
+
+		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;
 

drivers/usb/serial/mxu11x0.c static void mxu1_release(struct usb_serial *serial) · line 402

Quoted 10 lines
drivers/usb/serial/mxu11x0.c +8 -2
-	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
+		 */

Multi-line comments should be on the following format

	/*
	 * ...
	 */

Looks good otherwise.

Johan