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

From: Himadri Pandya
Date: Thu Dec 24 2020 - 05:03:06 EST


On Fri, Dec 4, 2020 at 2:38 PM Johan Hovold <johan@xxxxxxxxxx> 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:
> > 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.

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

>
> > 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