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

[PATCH v2 0/4] 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.

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

-- 
2.6.4

[PATCH v2 1/4] 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>
---

Changes in v2:
* Move release callback after attach callback

 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..6196073 100644
--- a/drivers/usb/serial/mxu11x0.c
+++ b/drivers/usb/serial/mxu11x0.c
@@ -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,
-- 
2.6.4

[PATCH v2 2/4] 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>
---

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

diff --git a/drivers/usb/serial/mxu11x0.c b/drivers/usb/serial/mxu11x0.c
index 6196073..7ad2727 100644
--- a/drivers/usb/serial/mxu11x0.c
+++ b/drivers/usb/serial/mxu11x0.c
@@ -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) {
-- 
2.6.4

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

[Mathieu OTHACEHE]

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

Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
---
 drivers/usb/serial/mxu11x0.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/usb/serial/mxu11x0.c b/drivers/usb/serial/mxu11x0.c
index 7ad2727..6326cda 100644
--- a/drivers/usb/serial/mxu11x0.c
+++ b/drivers/usb/serial/mxu11x0.c
@@ -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;
-- 
2.6.4

[PATCH v2 4/4] 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>
---

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

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

diff --git a/drivers/usb/serial/mxu11x0.c b/drivers/usb/serial/mxu11x0.c
index 6326cda..b942aeb 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);
@@ -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,
-- 
2.6.4

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

[Johan Hovold]

On Mon, Jan 04, 2016 at 07:49:36PM +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.
> 
> Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>

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

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

[Mathieu OTHACEHE]

On Mon, Jan 25, 2016 at 01:01:59PM +0100, Johan Hovold wrote:
> On Mon, Jan 04, 2016 at 07:49:36PM +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.
> > 
> > Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
> 
> 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

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

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

[Johan Hovold]

On Sat, Jan 30, 2016 at 06:40:30PM +0100, Mathieu OTHACEHE wrote:
> On Mon, Jan 25, 2016 at 01:01:59PM +0100, Johan Hovold wrote:
> > On Mon, Jan 04, 2016 at 07:49:36PM +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.
> > > 
> > > Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
> > 
> > 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
> 
> 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

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

[Mathieu OTHACEHE]

On Sun, Feb 28, 2016 at 01:20:16PM +0100, Johan Hovold wrote:
> On Sat, Jan 30, 2016 at 06:40:30PM +0100, Mathieu OTHACEHE wrote:
> > On Mon, Jan 25, 2016 at 01:01:59PM +0100, Johan Hovold wrote:
> > > On Mon, Jan 04, 2016 at 07:49:36PM +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.
> > > > 
> > > > Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
> > > 
> > > 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
> > 
> > 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?

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

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

[Johan Hovold]

On Sun, Feb 28, 2016 at 05:30:54PM +0100, Mathieu OTHACEHE wrote:
> 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.

> 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