| Message ID | 20201104064703.15123-8-himadrispandya@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: serial: avoid using usb_control_msg() directly | expand |
On Wed, Nov 04, 2020 at 12:16:55PM +0530, Himadri Pandya wrote: > The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps > usb_control_msg() with proper error check. Hence use the wrappers > instead of calling usb_control_msg() directly. > > Signed-off-by: Himadri Pandya <himadrispandya@gmail.com> > --- > drivers/usb/serial/f81534.c | 63 +++++++++++-------------------------- > 1 file changed, 18 insertions(+), 45 deletions(-) > > diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c > index 5661fd03e545..23eb17a2c052 100644 > --- a/drivers/usb/serial/f81534.c > +++ b/drivers/usb/serial/f81534.c > @@ -217,38 +217,26 @@ static int f81534_set_register(struct usb_serial *serial, u16 reg, u8 data) > struct usb_device *dev = serial->dev; > size_t count = F81534_USB_MAX_RETRY; > int status; > - u8 *tmp; > - > - tmp = kmalloc(sizeof(u8), GFP_KERNEL); > - if (!tmp) > - return -ENOMEM; > - > - *tmp = data; > > /* > * Our device maybe not reply when heavily loading, We'll retry for > * F81534_USB_MAX_RETRY times. > */ > while (count--) { > - status = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), > - F81534_SET_GET_REGISTER, > - USB_TYPE_VENDOR | USB_DIR_OUT, > - reg, 0, tmp, sizeof(u8), > - F81534_USB_TIMEOUT); > - if (status > 0) { > - status = 0; > - break; > - } else if (status == 0) { > - status = -EIO; > + status = usb_control_msg_send(dev, 0, F81534_SET_GET_REGISTER, > + USB_TYPE_VENDOR | USB_DIR_OUT, > + reg, 0, &data, sizeof(u8), > + F81534_USB_TIMEOUT, GFP_KERNEL); > + if (status) { > + /* Try again */ > + continue; > } > } Here too this change breaks the logic and the control transfer is now repeated also after successful transfer (ten times!). This change would also introduce an additional malloc + memcpy for every retry. As this is a function that is used often and the comment suggest that having to retry isn't that rare, I suggest dropping this patch as well. Johan
diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c index 5661fd03e545..23eb17a2c052 100644 --- a/drivers/usb/serial/f81534.c +++ b/drivers/usb/serial/f81534.c @@ -217,38 +217,26 @@ static int f81534_set_register(struct usb_serial *serial, u16 reg, u8 data) struct usb_device *dev = serial->dev; size_t count = F81534_USB_MAX_RETRY; int status; - u8 *tmp; - - tmp = kmalloc(sizeof(u8), GFP_KERNEL); - if (!tmp) - return -ENOMEM; - - *tmp = data; /* * Our device maybe not reply when heavily loading, We'll retry for * F81534_USB_MAX_RETRY times. */ while (count--) { - status = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), - F81534_SET_GET_REGISTER, - USB_TYPE_VENDOR | USB_DIR_OUT, - reg, 0, tmp, sizeof(u8), - F81534_USB_TIMEOUT); - if (status > 0) { - status = 0; - break; - } else if (status == 0) { - status = -EIO; + status = usb_control_msg_send(dev, 0, F81534_SET_GET_REGISTER, + USB_TYPE_VENDOR | USB_DIR_OUT, + reg, 0, &data, sizeof(u8), + F81534_USB_TIMEOUT, GFP_KERNEL); + if (status) { + /* Try again */ + continue; } } - if (status < 0) { + if (status) dev_err(&interface->dev, "%s: reg: %x data: %x failed: %d\n", - __func__, reg, data, status); - } + __func__, reg, data, status); - kfree(tmp); return status; } @@ -258,40 +246,25 @@ static int f81534_get_register(struct usb_serial *serial, u16 reg, u8 *data) struct usb_device *dev = serial->dev; size_t count = F81534_USB_MAX_RETRY; int status; - u8 *tmp; - - tmp = kmalloc(sizeof(u8), GFP_KERNEL); - if (!tmp) - return -ENOMEM; /* * Our device maybe not reply when heavily loading, We'll retry for * F81534_USB_MAX_RETRY times. */ while (count--) { - status = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), - F81534_SET_GET_REGISTER, - USB_TYPE_VENDOR | USB_DIR_IN, - reg, 0, tmp, sizeof(u8), - F81534_USB_TIMEOUT); - if (status > 0) { - status = 0; - break; - } else if (status == 0) { - status = -EIO; + status = usb_control_msg_recv(dev, 0, F81534_SET_GET_REGISTER, + USB_TYPE_VENDOR | USB_DIR_IN, reg, + 0, data, sizeof(u8), + F81534_USB_TIMEOUT, GFP_KERNEL); + if (status) { + /* Try again */ + continue; } } - if (status < 0) { + if (status) dev_err(&interface->dev, "%s: reg: %x failed: %d\n", __func__, - reg, status); - goto end; - } - - *data = *tmp; - -end: - kfree(tmp); + reg, status); return status; }
The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps usb_control_msg() with proper error check. Hence use the wrappers instead of calling usb_control_msg() directly. Signed-off-by: Himadri Pandya <himadrispandya@gmail.com> --- drivers/usb/serial/f81534.c | 63 +++++++++++-------------------------- 1 file changed, 18 insertions(+), 45 deletions(-)