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

[Himadri Pandya]

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