USB: mxu11x0: fixes and follow ups
Subsystems (2)
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
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
@@ -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
Johan Hovold
maintainerRe: [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_probeand attach are never freed. Addport_removeand 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
Signed-off-by Mathieu OTHACEHE <[email protected]>--- drivers/usb/serial/mxu11x0.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)drivers/usb/serial/mxu11x0.c +0
-0
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
PATCH 2/3 USB: mxu11x0: clean device control commands
In reply to Mathieu OTHACEHESending 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
@@ -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
Johan Hovold
maintainerRe: [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.
Signed-off-by Mathieu OTHACEHE <[email protected]>--- drivers/usb/serial/mxu11x0.c | 31 +++++++------------------------ 1 file changed, 7 insertions(+), 24 deletions(-)drivers/usb/serial/mxu11x0.c +0
-0
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
PATCH 3/3 USB: mxu11x0: move firmware download and endpoint testing to probe callback
In reply to Mathieu OTHACEHEMove 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
@@ -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
Johan Hovold
maintainerRe: [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_probecallback 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]>--- drivers/usb/serial/mxu11x0.c | 130 ++++++++++++++++++++++++++----------------- 1 file changed, 78 insertions(+), 52 deletions(-)drivers/usb/serial/mxu11x0.c +29
-5@@ -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