| Message ID | 20201104064703.15123-1-himadrispandya@gmail.com |
|---|---|
| Headers | show |
| Series | usb: serial: avoid using usb_control_msg() directly | expand |
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. > > Himadri Pandya (15): > usb: serial: ark3116: use usb_control_msg_recv() and > usb_control_msg_send() > 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() For the whole series: Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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
On Fri, Dec 4, 2020 at 2:38 PM Johan Hovold <johan@kernel.org> 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
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. Himadri Pandya (15): usb: serial: ark3116: use usb_control_msg_recv() and usb_control_msg_send() 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(-)