[PATCH 0/3] USB: mxu11x0: fixes and follow ups

[PATCH 0/3] USB: mxu11x0: fixes and follow ups

[Mathieu OTHACEHE]

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

-- 
2.6.2

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

[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 <m.othacehe@gmail.com>
---
 drivers/usb/serial/mxu11x0.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/usb/serial/mxu11x0.c b/drivers/usb/serial/mxu11x0.c
index e3c3f57c..0fe7eab 100644
--- a/drivers/usb/serial/mxu11x0.c
+++ b/drivers/usb/serial/mxu11x0.c
@@ -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,
-- 
2.6.2

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

[Johan Hovold]

On Sun, Jan 03, 2016 at 03:25:59PM +0100, Mathieu OTHACEHE wrote:
> 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.

> Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
> ---
>  drivers/usb/serial/mxu11x0.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/usb/serial/mxu11x0.c b/drivers/usb/serial/mxu11x0.c
> index e3c3f57c..0fe7eab 100644
> --- a/drivers/usb/serial/mxu11x0.c
> +++ b/drivers/usb/serial/mxu11x0.c
> @@ -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.

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

Thanks,
Johan

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

[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 <m.othacehe@gmail.com>
---
 drivers/usb/serial/mxu11x0.c | 31 +++++++------------------------
 1 file changed, 7 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/serial/mxu11x0.c b/drivers/usb/serial/mxu11x0.c
index 0fe7eab..354fcb5 100644
--- a/drivers/usb/serial/mxu11x0.c
+++ b/drivers/usb/serial/mxu11x0.c
@@ -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) {
-- 
2.6.2

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

[Johan Hovold]

On Sun, Jan 03, 2016 at 03:26:00PM +0100, Mathieu OTHACEHE wrote:
> Sending OPEN and START commands twice is not necessary for this driver.
> Also send STOP command at close.
> 
> Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
> ---
>  drivers/usb/serial/mxu11x0.c | 31 +++++++------------------------
>  1 file changed, 7 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/usb/serial/mxu11x0.c b/drivers/usb/serial/mxu11x0.c
> index 0fe7eab..354fcb5 100644
> --- a/drivers/usb/serial/mxu11x0.c
> +++ b/drivers/usb/serial/mxu11x0.c
> @@ -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.

> -
> -	if (tty)
> -		mxu1_set_termios(tty, port, NULL);

But you should definitely not be removing this.

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

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

Johan

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

[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 <m.othacehe@gmail.com>
---
 drivers/usb/serial/mxu11x0.c | 130 ++++++++++++++++++++++++++-----------------
 1 file changed, 78 insertions(+), 52 deletions(-)

diff --git a/drivers/usb/serial/mxu11x0.c b/drivers/usb/serial/mxu11x0.c
index 354fcb5..0392531 100644
--- a/drivers/usb/serial/mxu11x0.c
+++ b/drivers/usb/serial/mxu11x0.c
@@ -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,
-- 
2.6.2

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

[Johan Hovold]

On Sun, Jan 03, 2016 at 03:26:01PM +0100, Mathieu OTHACEHE wrote:
> 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 <m.othacehe@gmail.com>
> ---
>  drivers/usb/serial/mxu11x0.c | 130 ++++++++++++++++++++++++++-----------------
>  1 file changed, 78 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/usb/serial/mxu11x0.c b/drivers/usb/serial/mxu11x0.c
> index 354fcb5..0392531 100644
> --- a/drivers/usb/serial/mxu11x0.c
> +++ b/drivers/usb/serial/mxu11x0.c
> @@ -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)) {

No need for inner parentheses.

> +
> +		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
> +		 */

Multi-line comments should be on the following format

	/*
	 * ...
	 */

Looks good otherwise.

Johan