linux-kernel - Re: [PATCH] USB: Use EHCI control transfer pid macros instead of constant values.

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: <1e7f57d6-a4c1-4a3d-8cff-f966c89a8140@rowland.harvard.edu>
Date: Thu, 7 Mar 2024 13:42:55 -0500
From: Alan Stern <stern@...land.harvard.edu>
To: Dingyan Li <18500469033@....com>
Cc: gregkh@...uxfoundation.org, linux-usb@...r.kernel.org,
  linux-kernel@...r.kernel.org
Subject: Re: [PATCH] USB: Use EHCI control transfer pid macros instead of
 constant values.

On Fri, Mar 08, 2024 at 01:31:59AM +0800, Dingyan Li wrote:
> Macros with good names offer better readability. Besides, also move
> the definition to ehci.h.
> 
> Signed-off-by: Dingyan Li <18500469033@....com>
> ---

Good idea, but you missed a few spots.

>  drivers/usb/host/ehci-q.c | 10 +++-------
>  drivers/usb/host/ehci.h   |  8 +++++++-
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> index 666f5c4db25a..51b46001e344 100644
> --- a/drivers/usb/host/ehci-q.c
> +++ b/drivers/usb/host/ehci-q.c
> @@ -27,10 +27,6 @@
>  
>  /*-------------------------------------------------------------------------*/
>  
> -/* PID Codes that are used here, from EHCI specification, Table 3-16. */
> -#define PID_CODE_IN    1
> -#define PID_CODE_SETUP 2
> -
>  /* fill a qtd, returning how much of the buffer we were able to queue up */
>  
>  static unsigned int
> @@ -230,7 +226,7 @@ static int qtd_copy_status (
>  			/* fs/ls interrupt xfer missed the complete-split */
>  			status = -EPROTO;
>  		} else if (token & QTD_STS_DBE) {
> -			status = (QTD_PID (token) == 1) /* IN ? */
> +			status = (QTD_PID(token) == PID_CODE_IN) /* IN ? */
>  				? -ENOSR  /* hc couldn't read data */
>  				: -ECOMM; /* hc couldn't write data */
>  		} else if (token & QTD_STS_XACT) {
> @@ -606,7 +602,7 @@ qh_urb_transaction (
>  		/* SETUP pid */
>  		qtd_fill(ehci, qtd, urb->setup_dma,
>  				sizeof (struct usb_ctrlrequest),
> -				token | (2 /* "setup" */ << 8), 8);
> +				token | (PID_CODE_SETUP << 8), 8);
>  
>  		/* ... and always at least one more pid */
>  		token ^= QTD_TOGGLE;

There is an occurrence on line 623.

> @@ -642,7 +638,7 @@ qh_urb_transaction (
>  	}
>  
>  	if (is_input)
> -		token |= (1 /* "in" */ << 8);
> +		token |= (PID_CODE_IN << 8);
>  	/* else it's already initted to "out" pid (0 << 8) */
>  
>  	maxpacket = usb_endpoint_maxp(&urb->ep->desc);

You could use PID_CODE_IN on lines 712 and 1232.

There are occurrences on lines 1206, 1219.

Also, there's a bunch of "switch" cases near line 433 in ehci-dbg.c.

Alan Stern