| Message ID | 20201104064703.15123-6-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:53PM +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/cypress_m8.c | 38 +++++++++++++++++---------------- > 1 file changed, 20 insertions(+), 18 deletions(-) > > diff --git a/drivers/usb/serial/cypress_m8.c b/drivers/usb/serial/cypress_m8.c > index cc028601c388..4d66cab8eece 100644 > --- a/drivers/usb/serial/cypress_m8.c > +++ b/drivers/usb/serial/cypress_m8.c > @@ -341,20 +341,21 @@ static int cypress_serial_control(struct tty_struct *tty, > feature_buffer[4]); > > do { > - retval = usb_control_msg(port->serial->dev, > - usb_sndctrlpipe(port->serial->dev, 0), > - HID_REQ_SET_REPORT, > - USB_DIR_OUT | USB_RECIP_INTERFACE | USB_TYPE_CLASS, > - 0x0300, 0, feature_buffer, > - feature_len, 500); > + retval = usb_control_msg_send(port->serial->dev, 0, > + HID_REQ_SET_REPORT, > + USB_DIR_OUT | > + USB_RECIP_INTERFACE | > + USB_TYPE_CLASS, 0x0300, > + 0, feature_buffer, > + feature_len, 500, > + GFP_KERNEL); > > if (tries++ >= 3) > break; > > - } while (retval != feature_len && > - retval != -ENODEV); > + } while (retval != -ENODEV); Here you actually break the logic; after this change the driver will always retry as the test for success is removed. > > - if (retval != feature_len) { > + if (retval) { > dev_err(dev, "%s - failed sending serial line settings - %d\n", > __func__, retval); > cypress_set_dead(port); > @@ -379,19 +380,20 @@ static int cypress_serial_control(struct tty_struct *tty, > } > dev_dbg(dev, "%s - retrieving serial line settings\n", __func__); > do { > - retval = usb_control_msg(port->serial->dev, > - usb_rcvctrlpipe(port->serial->dev, 0), > - HID_REQ_GET_REPORT, > - USB_DIR_IN | USB_RECIP_INTERFACE | USB_TYPE_CLASS, > - 0x0300, 0, feature_buffer, > - feature_len, 500); > + retval = usb_control_msg_recv(port->serial->dev, 0, > + HID_REQ_GET_REPORT, > + USB_DIR_IN | > + USB_RECIP_INTERFACE | > + USB_TYPE_CLASS, 0x0300, > + 0, feature_buffer, > + feature_len, 500, > + GFP_KERNEL); > > if (tries++ >= 3) > break; > - } while (retval != feature_len > - && retval != -ENODEV); > + } while (retval != -ENODEV); Same here. > > - if (retval != feature_len) { > + if (retval) { > dev_err(dev, "%s - failed to retrieve serial line settings - %d\n", > __func__, retval); > cypress_set_dead(port); As the driver is already using a DMA-able buffer and is reusing it for every retry as well, this change is now introducing redundant memdup + memcpy for each call. I suggest just dropping this one. Johan
diff --git a/drivers/usb/serial/cypress_m8.c b/drivers/usb/serial/cypress_m8.c index cc028601c388..4d66cab8eece 100644 --- a/drivers/usb/serial/cypress_m8.c +++ b/drivers/usb/serial/cypress_m8.c @@ -341,20 +341,21 @@ static int cypress_serial_control(struct tty_struct *tty, feature_buffer[4]); do { - retval = usb_control_msg(port->serial->dev, - usb_sndctrlpipe(port->serial->dev, 0), - HID_REQ_SET_REPORT, - USB_DIR_OUT | USB_RECIP_INTERFACE | USB_TYPE_CLASS, - 0x0300, 0, feature_buffer, - feature_len, 500); + retval = usb_control_msg_send(port->serial->dev, 0, + HID_REQ_SET_REPORT, + USB_DIR_OUT | + USB_RECIP_INTERFACE | + USB_TYPE_CLASS, 0x0300, + 0, feature_buffer, + feature_len, 500, + GFP_KERNEL); if (tries++ >= 3) break; - } while (retval != feature_len && - retval != -ENODEV); + } while (retval != -ENODEV); - if (retval != feature_len) { + if (retval) { dev_err(dev, "%s - failed sending serial line settings - %d\n", __func__, retval); cypress_set_dead(port); @@ -379,19 +380,20 @@ static int cypress_serial_control(struct tty_struct *tty, } dev_dbg(dev, "%s - retrieving serial line settings\n", __func__); do { - retval = usb_control_msg(port->serial->dev, - usb_rcvctrlpipe(port->serial->dev, 0), - HID_REQ_GET_REPORT, - USB_DIR_IN | USB_RECIP_INTERFACE | USB_TYPE_CLASS, - 0x0300, 0, feature_buffer, - feature_len, 500); + retval = usb_control_msg_recv(port->serial->dev, 0, + HID_REQ_GET_REPORT, + USB_DIR_IN | + USB_RECIP_INTERFACE | + USB_TYPE_CLASS, 0x0300, + 0, feature_buffer, + feature_len, 500, + GFP_KERNEL); if (tries++ >= 3) break; - } while (retval != feature_len - && retval != -ENODEV); + } while (retval != -ENODEV); - if (retval != feature_len) { + if (retval) { dev_err(dev, "%s - failed to retrieve serial line settings - %d\n", __func__, retval); cypress_set_dead(port);
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/cypress_m8.c | 38 +++++++++++++++++---------------- 1 file changed, 20 insertions(+), 18 deletions(-)