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

On Fri, Dec 4, 2020 at 2:38 PM Johan Hovold [off-list ref] wrote:

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:

quoted

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.

quoted

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

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

Hi Johan,

Thanks for reviewing this series and sorry for the late reply. I'll
soon send a v2 according to your comments.

Best regards,
Himadri

quoted

  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

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees