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

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

[Yan Xinyu]

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@bupt.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"
 #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);
 
-- 
2.25.1

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

[Johan Hovold]

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@bupt.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

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

[Greg Kroah-Hartman]

On Sun, Jul 24, 2022 at 11:10:22AM +0200, Johan Hovold wrote:
> 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@bupt.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?

I think Yan should write a patch series to unify these and make it
right, instead of just papering over it all.  Also this "../" stuff in a
#include directive is not ok, I wouldn't recommend this change be taken
as-is.

thanks,

greg k-h

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

[Johan Hovold]

On Sun, Jul 24, 2022 at 03:50:52PM +0200, Greg Kroah-Hartman wrote:
> On Sun, Jul 24, 2022 at 11:10:22AM +0200, Johan Hovold wrote:
> > 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@bupt.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?
> 
> I think Yan should write a patch series to unify these and make it
> right, instead of just papering over it all.

Ok, I just fear it will be more work for us since that involves
decisions like whether it should be added to the uapi header, and then
we get into naming, etc. But we're in no rush.

> Also this "../" stuff in a
> #include directive is not ok, I wouldn't recommend this change be taken
> as-is.

That was never an option, but I'd be ok with taking the v2 which added
defines for the constants directly in the driver.

Johan

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

[Greg Kroah-Hartman]

On Sun, Jul 24, 2022 at 04:07:17PM +0200, Johan Hovold wrote:
> On Sun, Jul 24, 2022 at 03:50:52PM +0200, Greg Kroah-Hartman wrote:
> > On Sun, Jul 24, 2022 at 11:10:22AM +0200, Johan Hovold wrote:
> > > 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@bupt.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?
> > 
> > I think Yan should write a patch series to unify these and make it
> > right, instead of just papering over it all.
> 
> Ok, I just fear it will be more work for us since that involves
> decisions like whether it should be added to the uapi header, and then
> we get into naming, etc. But we're in no rush.
> 
> > Also this "../" stuff in a
> > #include directive is not ok, I wouldn't recommend this change be taken
> > as-is.
> 
> That was never an option, but I'd be ok with taking the v2 which added
> defines for the constants directly in the driver.

These are global defines, in a public spec, and they should all just be
in one place in the kernel.  What's wrong with include/uapi/linux/cdc.h
which is where the other ACM defined values are at?

thanks,

greg k-h

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

[Johan Hovold]

On Sun, Jul 24, 2022 at 04:15:55PM +0200, Greg Kroah-Hartman wrote:
> On Sun, Jul 24, 2022 at 04:07:17PM +0200, Johan Hovold wrote:
> > On Sun, Jul 24, 2022 at 03:50:52PM +0200, Greg Kroah-Hartman wrote:

> > > I think Yan should write a patch series to unify these and make it
> > > right, instead of just papering over it all.
> > 
> > Ok, I just fear it will be more work for us since that involves
> > decisions like whether it should be added to the uapi header, and then
> > we get into naming, etc. But we're in no rush.
> > 
> > > Also this "../" stuff in a
> > > #include directive is not ok, I wouldn't recommend this change be taken
> > > as-is.
> > 
> > That was never an option, but I'd be ok with taking the v2 which added
> > defines for the constants directly in the driver.
> 
> These are global defines, in a public spec, and they should all just be
> in one place in the kernel.  What's wrong with include/uapi/linux/cdc.h
> which is where the other ACM defined values are at?

Nothing. We'd just need to figure out how best to name them if they're
going to become UAPI, that's all (e.g. at least use a USB_CDC_ prefix to
match the other defines).

And the in-tree users would need to be updated to match.

And it's not just the control lines. We have the state notification bits
as well. Someone would need to dig out the spec.

So we go from accepting a small clean-up patch to some non-trivial
tree-wide work and review. But sure, bring it on.

Johan