linux-kernel - Re: [PATCH v3] USB: serial: usb_wwan: replace DTR/RTS magic numbers with macros

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yt0MfqQQTwe4ztuN@hovoldconsulting.com>
Date:   Sun, 24 Jul 2022 11:10:22 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Yan Xinyu <sdlyyxy@...t.edu.cn>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] USB: serial: usb_wwan: replace DTR/RTS magic numbers
 with macros

On Fri, Jul 22, 2022 at 04:50:40PM +0800, Yan Xinyu wrote:
> The usb_wwan_send_setup function generates DTR/RTS signals in compliance
> with CDC ACM standard. This patch changes magic numbers in this function
> to equivalent macros.
> 
> Signed-off-by: Yan Xinyu <sdlyyxy@...t.edu.cn>
> ---
> v1->v2:
>  * Fix Signed-off-by name.
> v2->v3:
>  * Use already defined ACM_CTRL_DTR and ACM_CTRL_RTS in drivers/usb/class/cdc-acm.h
> ---
>  drivers/usb/serial/usb_wwan.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
> index dab38b63eaf7..5c8303bd3676 100644
> --- a/drivers/usb/serial/usb_wwan.c
> +++ b/drivers/usb/serial/usb_wwan.c
> @@ -29,8 +29,10 @@
>  #include <linux/bitops.h>
>  #include <linux/uaccess.h>
>  #include <linux/usb.h>
> +#include <linux/usb/cdc.h>
>  #include <linux/usb/serial.h>
>  #include <linux/serial.h>
> +#include "../class/cdc-acm.h"

If we are to use common defines, these would need to be added to
linux/usb/cdc.h first (parts of which are exposed to user space).

Note that we already have at least three copies of these defines in the
tree.

I'm fine with adding another copy for now and not have to deal with with
naming and cross driver updates. What do you think, Greg?

>  #include "usb-wwan.h"
>  
>  /*
> @@ -48,9 +50,9 @@ static int usb_wwan_send_setup(struct usb_serial_port *port)
>  	portdata = usb_get_serial_port_data(port);
>  
>  	if (portdata->dtr_state)
> -		val |= 0x01;
> +		val |= ACM_CTRL_DTR;
>  	if (portdata->rts_state)
> -		val |= 0x02;
> +		val |= ACM_CTRL_RTS;
>  
>  	ifnum = serial->interface->cur_altsetting->desc.bInterfaceNumber;
>  
> @@ -59,8 +61,9 @@ static int usb_wwan_send_setup(struct usb_serial_port *port)
>  		return res;
>  
>  	res = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
> -				0x22, 0x21, val, ifnum, NULL, 0,
> -				USB_CTRL_SET_TIMEOUT);
> +				USB_CDC_REQ_SET_CONTROL_LINE_STATE,
> +				USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> +				val, ifnum, NULL, 0, USB_CTRL_SET_TIMEOUT);
>  
>  	usb_autopm_put_interface(port->serial->interface);

Looks good otherwise.

Johan