| 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
| ||
|
Message-ID: <37bdd932-07a4-4514-a5cc-b70d48c962a6@rowland.harvard.edu>
Date: Fri, 8 Mar 2024 15:34:04 -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 v2] USB: Use EHCI control transfer pid macros instead of
constant values.
On Fri, Mar 08, 2024 at 09:08: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>
> ---
You found some things that I missed! Good for you. When you resubmit
with a list of changes from the original version, you can add:
Reviewed-by: Alan Stern <stern@...land.harvard.edu>
> drivers/usb/host/ehci-dbg.c | 10 +++++-----
> drivers/usb/host/ehci-q.c | 20 ++++++++------------
> drivers/usb/host/ehci.h | 8 +++++++-
> 3 files changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-dbg.c b/drivers/usb/host/ehci-dbg.c
> index c063fb042926..435001128221 100644
> --- a/drivers/usb/host/ehci-dbg.c
> +++ b/drivers/usb/host/ehci-dbg.c
> @@ -430,13 +430,13 @@ static void qh_lines(struct ehci_hcd *ehci, struct ehci_qh *qh,
> mark = '/';
> }
> switch ((scratch >> 8) & 0x03) {
> - case 0:
> + case PID_CODE_OUT:
> type = "out";
> break;
> - case 1:
> + case PID_CODE_IN:
> type = "in";
> break;
> - case 2:
> + case PID_CODE_SETUP:
> type = "setup";
> break;
> default:
> @@ -602,10 +602,10 @@ static unsigned output_buf_tds_dir(char *buf, struct ehci_hcd *ehci,
> list_for_each_entry(qtd, &qh->qtd_list, qtd_list) {
> temp++;
> switch ((hc32_to_cpu(ehci, qtd->hw_token) >> 8) & 0x03) {
> - case 0:
> + case PID_CODE_OUT:
> type = "out";
> continue;
> - case 1:
> + case PID_CODE_IN:
> type = "in";
> continue;
> }
> diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> index 666f5c4db25a..ba37a9fcab92 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;
> @@ -620,7 +616,7 @@ qh_urb_transaction (
>
> /* for zero length DATA stages, STATUS is always IN */
> if (len == 0)
> - token |= (1 /* "in" */ << 8);
> + token |= (PID_CODE_IN << 8);
> }
>
> /*
> @@ -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);
> @@ -709,7 +705,7 @@ qh_urb_transaction (
>
> if (usb_pipecontrol (urb->pipe)) {
> one_more = 1;
> - token ^= 0x0100; /* "in" <--> "out" */
> + token ^= (PID_CODE_IN << 8); /* "in" <--> "out" */
> token |= QTD_TOGGLE; /* force DATA1 */
> } else if (usb_pipeout(urb->pipe)
> && (urb->transfer_flags & URB_ZERO_PACKET)
> @@ -1203,7 +1199,7 @@ static int ehci_submit_single_step_set_feature(
> /* SETUP pid, and interrupt after SETUP completion */
> qtd_fill(ehci, qtd, urb->setup_dma,
> sizeof(struct usb_ctrlrequest),
> - QTD_IOC | token | (2 /* "setup" */ << 8), 8);
> + QTD_IOC | token | (PID_CODE_SETUP << 8), 8);
>
> submit_async(ehci, urb, &qtd_list, GFP_ATOMIC);
> return 0; /*Return now; we shall come back after 15 seconds*/
> @@ -1216,7 +1212,7 @@ static int ehci_submit_single_step_set_feature(
> token ^= QTD_TOGGLE; /*We need to start IN with DATA-1 Pid-sequence*/
> buf = urb->transfer_dma;
>
> - token |= (1 /* "in" */ << 8); /*This is IN stage*/
> + token |= (PID_CODE_IN << 8); /*This is IN stage*/
>
> maxpacket = usb_endpoint_maxp(&urb->ep->desc);
>
> @@ -1229,7 +1225,7 @@ static int ehci_submit_single_step_set_feature(
> qtd->hw_alt_next = EHCI_LIST_END(ehci);
>
> /* STATUS stage for GetDesc control request */
> - token ^= 0x0100; /* "in" <--> "out" */
> + token ^= (PID_CODE_IN << 8); /* "in" <--> "out" */
> token |= QTD_TOGGLE; /* force DATA1 */
>
> qtd_prev = qtd;
> diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
> index 1441e3400796..d7a3c8d13f6b 100644
> --- a/drivers/usb/host/ehci.h
> +++ b/drivers/usb/host/ehci.h
> @@ -321,10 +321,16 @@ struct ehci_qtd {
> size_t length; /* length of buffer */
> } __aligned(32);
>
> +/* PID Codes that are used here, from EHCI specification, Table 3-16. */
> +#define PID_CODE_OUT 0
> +#define PID_CODE_IN 1
> +#define PID_CODE_SETUP 2
> +
> /* mask NakCnt+T in qh->hw_alt_next */
> #define QTD_MASK(ehci) cpu_to_hc32(ehci, ~0x1f)
>
> -#define IS_SHORT_READ(token) (QTD_LENGTH(token) != 0 && QTD_PID(token) == 1)
> +#define IS_SHORT_READ(token) (QTD_LENGTH(token) != 0 && \
> + QTD_PID(token) == PID_CODE_IN)
>
> /*-------------------------------------------------------------------------*/
>
> --
> 2.25.1
>
Powered by blists - more mailing lists