[Linux-kernel-mentees] [PATCH 00/15] usb: serial: avoid using usb_control_msg() directly

Johan Hovold johan at kernel.org
Fri Dec 4 09:09:29 UTC 2020


Hi Himadri,

and sorry about the late feedback on this one. I'm still trying to dig
myself out of some backlog.

On Wed, Nov 04, 2020 at 12:16:48PM +0530, Himadri Pandya wrote:
> There are many usages of usb_control_msg() that can use the new wrapper
> functions usb_contro_msg_send() & usb_control_msg_recv() for better
> error checks on short reads and writes. Hence use them whenever possible
> and avoid using usb_control_msg() directly.

Replacing working code with shiny new helpers unfortunately often ends
up introducing new bugs and I'm afraid there are a few examples of that
in this series as well.

I'll comment on the patches individually, but my impression is that we
should primarily use these helpers to replace code which allocates a
temporary buffer for each transfer as otherwise there's no clear gain
from using them.

Some of your patches contains unrelated changes which needs to go in
separate patches if they're to be included at all.

Please also try to write dedicated commit messages rater than reusing
more or less the same stock message since not everything in these
messages apply to each patch. You never mention that these helpers
nicely hides the allocation of temporary transfer buffers in some cases
for examples. In other places they instead introduce additional
allocations which at least should have been highlighted.

> Himadri Pandya (15):
>   usb: serial: ark3116: use usb_control_msg_recv() and
>     usb_control_msg_send()

Nit: please also use an uppercase "USB" prefix.

>   usb: serial: belkin_sa: use usb_control_msg_send()
>   usb: serial: ch314: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: cp210x: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: cypress_m8: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: f81232: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: f81534: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: ftdi_sio: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: io_edgeport: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: io_ti: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: ipaq: use usb_control_msg_send()
>   usb: serial: ipw: use usb_control_msg_send()
>   usb: serial: iuu_phoenix: use usb_control_msg_send()
>   usb: serial: keyspan_pda: use usb_control_msg_recv() and
>     usb_control_msg_send()
>   usb: serial: kl5kusb105: use usb_control_msg_recv() and
>     usb_control_msg_send()
> 
>  drivers/usb/serial/ark3116.c     |  29 +----
>  drivers/usb/serial/belkin_sa.c   |  35 +++---
>  drivers/usb/serial/ch341.c       |  45 +++-----
>  drivers/usb/serial/cp210x.c      | 148 +++++++------------------
>  drivers/usb/serial/cypress_m8.c  |  38 ++++---
>  drivers/usb/serial/f81232.c      |  88 +++------------
>  drivers/usb/serial/f81534.c      |  63 +++--------
>  drivers/usb/serial/ftdi_sio.c    | 182 +++++++++++++------------------
>  drivers/usb/serial/io_edgeport.c |  73 +++++--------
>  drivers/usb/serial/io_ti.c       |  28 ++---
>  drivers/usb/serial/ipaq.c        |   9 +-
>  drivers/usb/serial/ipw.c         | 107 ++++++------------
>  drivers/usb/serial/iuu_phoenix.c |   5 +-
>  drivers/usb/serial/keyspan_pda.c | 172 ++++++++++++-----------------
>  drivers/usb/serial/kl5kusb105.c  |  94 ++++++++--------
>  15 files changed, 406 insertions(+), 710 deletions(-)

Johan


More information about the Linux-kernel-mentees mailing list