| Message ID | 20200923090519.361-5-himadrispandya@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: usb: avoid using usb_control_msg() directly | expand |
On Wed, Sep 23, 2020 at 02:35:19PM +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/net/usb/rndis_host.c | 44 ++++++++++++++---------------------- > 1 file changed, 17 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/usb/rndis_host.c b/drivers/net/usb/rndis_host.c > index 6fa7a009a24a..30fc4a7183d3 100644 > --- a/drivers/net/usb/rndis_host.c > +++ b/drivers/net/usb/rndis_host.c > @@ -113,14 +113,13 @@ int rndis_command(struct usbnet *dev, struct rndis_msg_hdr *buf, int buflen) > buf->request_id = (__force __le32) xid; > } > master_ifnum = info->control->cur_altsetting->desc.bInterfaceNumber; > - retval = usb_control_msg(dev->udev, > - usb_sndctrlpipe(dev->udev, 0), > - USB_CDC_SEND_ENCAPSULATED_COMMAND, > - USB_TYPE_CLASS | USB_RECIP_INTERFACE, > - 0, master_ifnum, > - buf, le32_to_cpu(buf->msg_len), > - RNDIS_CONTROL_TIMEOUT_MS); > - if (unlikely(retval < 0 || xid == 0)) > + retval = usb_control_msg_send(dev->udev, 0, > + USB_CDC_SEND_ENCAPSULATED_COMMAND, > + USB_TYPE_CLASS | USB_RECIP_INTERFACE, > + 0, master_ifnum, buf, > + le32_to_cpu(buf->msg_len), > + RNDIS_CONTROL_TIMEOUT_MS); > + if (unlikely(xid == 0)) > return retval; > > /* Some devices don't respond on the control channel until > @@ -139,14 +138,11 @@ int rndis_command(struct usbnet *dev, struct rndis_msg_hdr *buf, int buflen) > /* Poll the control channel; the request probably completed immediately */ > rsp = le32_to_cpu(buf->msg_type) | RNDIS_MSG_COMPLETION; > for (count = 0; count < 10; count++) { > - memset(buf, 0, CONTROL_BUFFER_SIZE); > - retval = usb_control_msg(dev->udev, > - usb_rcvctrlpipe(dev->udev, 0), > - USB_CDC_GET_ENCAPSULATED_RESPONSE, > - USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE, > - 0, master_ifnum, > - buf, buflen, > - RNDIS_CONTROL_TIMEOUT_MS); > + retval = usb_control_msg_recv(dev->udev, 0, > + USB_CDC_GET_ENCAPSULATED_RESPONSE, > + USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE, > + 0, master_ifnum, buf, buflen, > + RNDIS_CONTROL_TIMEOUT_MS); > if (likely(retval >= 8)) { retval here is never going to be positive, right? So I don't think this patch is correct :( > msg_type = le32_to_cpu(buf->msg_type); > msg_len = le32_to_cpu(buf->msg_len); > @@ -178,17 +174,11 @@ int rndis_command(struct usbnet *dev, struct rndis_msg_hdr *buf, int buflen) > msg->msg_type = cpu_to_le32(RNDIS_MSG_KEEPALIVE_C); > msg->msg_len = cpu_to_le32(sizeof *msg); > msg->status = cpu_to_le32(RNDIS_STATUS_SUCCESS); > - retval = usb_control_msg(dev->udev, > - usb_sndctrlpipe(dev->udev, 0), > - USB_CDC_SEND_ENCAPSULATED_COMMAND, > - USB_TYPE_CLASS | USB_RECIP_INTERFACE, > - 0, master_ifnum, > - msg, sizeof *msg, > - RNDIS_CONTROL_TIMEOUT_MS); > - if (unlikely(retval < 0)) > - dev_dbg(&info->control->dev, > - "rndis keepalive err %d\n", > - retval); > + retval = usb_control_msg_send(dev->udev, 0, > + USB_CDC_SEND_ENCAPSULATED_COMMAND, > + USB_TYPE_CLASS | USB_RECIP_INTERFACE, > + 0, master_ifnum, msg, sizeof(*msg), > + RNDIS_CONTROL_TIMEOUT_MS); You lost the error message that the previous call had if something went wrong. Don't know if it's really needed, but there's no reason to remove it here. thanks, greg k-h
On Wed, Sep 23, 2020 at 3:52 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Wed, Sep 23, 2020 at 02:35:19PM +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/net/usb/rndis_host.c | 44 ++++++++++++++---------------------- > > 1 file changed, 17 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/net/usb/rndis_host.c b/drivers/net/usb/rndis_host.c > > index 6fa7a009a24a..30fc4a7183d3 100644 > > --- a/drivers/net/usb/rndis_host.c > > +++ b/drivers/net/usb/rndis_host.c > > @@ -113,14 +113,13 @@ int rndis_command(struct usbnet *dev, struct rndis_msg_hdr *buf, int buflen) > > buf->request_id = (__force __le32) xid; > > } > > master_ifnum = info->control->cur_altsetting->desc.bInterfaceNumber; > > - retval = usb_control_msg(dev->udev, > > - usb_sndctrlpipe(dev->udev, 0), > > - USB_CDC_SEND_ENCAPSULATED_COMMAND, > > - USB_TYPE_CLASS | USB_RECIP_INTERFACE, > > - 0, master_ifnum, > > - buf, le32_to_cpu(buf->msg_len), > > - RNDIS_CONTROL_TIMEOUT_MS); > > - if (unlikely(retval < 0 || xid == 0)) > > + retval = usb_control_msg_send(dev->udev, 0, > > + USB_CDC_SEND_ENCAPSULATED_COMMAND, > > + USB_TYPE_CLASS | USB_RECIP_INTERFACE, > > + 0, master_ifnum, buf, > > + le32_to_cpu(buf->msg_len), > > + RNDIS_CONTROL_TIMEOUT_MS); > > + if (unlikely(xid == 0)) > > return retval; > > > > /* Some devices don't respond on the control channel until > > @@ -139,14 +138,11 @@ int rndis_command(struct usbnet *dev, struct rndis_msg_hdr *buf, int buflen) > > /* Poll the control channel; the request probably completed immediately */ > > rsp = le32_to_cpu(buf->msg_type) | RNDIS_MSG_COMPLETION; > > for (count = 0; count < 10; count++) { > > - memset(buf, 0, CONTROL_BUFFER_SIZE); > > - retval = usb_control_msg(dev->udev, > > - usb_rcvctrlpipe(dev->udev, 0), > > - USB_CDC_GET_ENCAPSULATED_RESPONSE, > > - USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE, > > - 0, master_ifnum, > > - buf, buflen, > > - RNDIS_CONTROL_TIMEOUT_MS); > > + retval = usb_control_msg_recv(dev->udev, 0, > > + USB_CDC_GET_ENCAPSULATED_RESPONSE, > > + USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE, > > + 0, master_ifnum, buf, buflen, > > + RNDIS_CONTROL_TIMEOUT_MS); > > if (likely(retval >= 8)) { > > retval here is never going to be positive, right? So I don't think this > patch is correct :( > Yes :(. > > msg_type = le32_to_cpu(buf->msg_type); > > msg_len = le32_to_cpu(buf->msg_len); > > @@ -178,17 +174,11 @@ int rndis_command(struct usbnet *dev, struct rndis_msg_hdr *buf, int buflen) > > msg->msg_type = cpu_to_le32(RNDIS_MSG_KEEPALIVE_C); > > msg->msg_len = cpu_to_le32(sizeof *msg); > > msg->status = cpu_to_le32(RNDIS_STATUS_SUCCESS); > > - retval = usb_control_msg(dev->udev, > > - usb_sndctrlpipe(dev->udev, 0), > > - USB_CDC_SEND_ENCAPSULATED_COMMAND, > > - USB_TYPE_CLASS | USB_RECIP_INTERFACE, > > - 0, master_ifnum, > > - msg, sizeof *msg, > > - RNDIS_CONTROL_TIMEOUT_MS); > > - if (unlikely(retval < 0)) > > - dev_dbg(&info->control->dev, > > - "rndis keepalive err %d\n", > > - retval); > > + retval = usb_control_msg_send(dev->udev, 0, > > + USB_CDC_SEND_ENCAPSULATED_COMMAND, > > + USB_TYPE_CLASS | USB_RECIP_INTERFACE, > > + 0, master_ifnum, msg, sizeof(*msg), > > + RNDIS_CONTROL_TIMEOUT_MS); > > You lost the error message that the previous call had if something went > wrong. Don't know if it's really needed, but there's no reason to > remove it here. > The wrapper returns the error so thought that might work instead. But yes, the old msg is better. Anyways, this series is dropped :). Thanks for the review, Himadri > thanks, > > greg k-h
diff --git a/drivers/net/usb/rndis_host.c b/drivers/net/usb/rndis_host.c index 6fa7a009a24a..30fc4a7183d3 100644 --- a/drivers/net/usb/rndis_host.c +++ b/drivers/net/usb/rndis_host.c @@ -113,14 +113,13 @@ int rndis_command(struct usbnet *dev, struct rndis_msg_hdr *buf, int buflen) buf->request_id = (__force __le32) xid; } master_ifnum = info->control->cur_altsetting->desc.bInterfaceNumber; - retval = usb_control_msg(dev->udev, - usb_sndctrlpipe(dev->udev, 0), - USB_CDC_SEND_ENCAPSULATED_COMMAND, - USB_TYPE_CLASS | USB_RECIP_INTERFACE, - 0, master_ifnum, - buf, le32_to_cpu(buf->msg_len), - RNDIS_CONTROL_TIMEOUT_MS); - if (unlikely(retval < 0 || xid == 0)) + retval = usb_control_msg_send(dev->udev, 0, + USB_CDC_SEND_ENCAPSULATED_COMMAND, + USB_TYPE_CLASS | USB_RECIP_INTERFACE, + 0, master_ifnum, buf, + le32_to_cpu(buf->msg_len), + RNDIS_CONTROL_TIMEOUT_MS); + if (unlikely(xid == 0)) return retval; /* Some devices don't respond on the control channel until @@ -139,14 +138,11 @@ int rndis_command(struct usbnet *dev, struct rndis_msg_hdr *buf, int buflen) /* Poll the control channel; the request probably completed immediately */ rsp = le32_to_cpu(buf->msg_type) | RNDIS_MSG_COMPLETION; for (count = 0; count < 10; count++) { - memset(buf, 0, CONTROL_BUFFER_SIZE); - retval = usb_control_msg(dev->udev, - usb_rcvctrlpipe(dev->udev, 0), - USB_CDC_GET_ENCAPSULATED_RESPONSE, - USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE, - 0, master_ifnum, - buf, buflen, - RNDIS_CONTROL_TIMEOUT_MS); + retval = usb_control_msg_recv(dev->udev, 0, + USB_CDC_GET_ENCAPSULATED_RESPONSE, + USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE, + 0, master_ifnum, buf, buflen, + RNDIS_CONTROL_TIMEOUT_MS); if (likely(retval >= 8)) { msg_type = le32_to_cpu(buf->msg_type); msg_len = le32_to_cpu(buf->msg_len); @@ -178,17 +174,11 @@ int rndis_command(struct usbnet *dev, struct rndis_msg_hdr *buf, int buflen) msg->msg_type = cpu_to_le32(RNDIS_MSG_KEEPALIVE_C); msg->msg_len = cpu_to_le32(sizeof *msg); msg->status = cpu_to_le32(RNDIS_STATUS_SUCCESS); - retval = usb_control_msg(dev->udev, - usb_sndctrlpipe(dev->udev, 0), - USB_CDC_SEND_ENCAPSULATED_COMMAND, - USB_TYPE_CLASS | USB_RECIP_INTERFACE, - 0, master_ifnum, - msg, sizeof *msg, - RNDIS_CONTROL_TIMEOUT_MS); - if (unlikely(retval < 0)) - dev_dbg(&info->control->dev, - "rndis keepalive err %d\n", - retval); + retval = usb_control_msg_send(dev->udev, 0, + USB_CDC_SEND_ENCAPSULATED_COMMAND, + USB_TYPE_CLASS | USB_RECIP_INTERFACE, + 0, master_ifnum, msg, sizeof(*msg), + RNDIS_CONTROL_TIMEOUT_MS); } break; default:
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/net/usb/rndis_host.c | 44 ++++++++++++++---------------------- 1 file changed, 17 insertions(+), 27 deletions(-)