* [PATCH] USB: Use EHCI control transfer pid macros instead of constant values. @ 2024-03-07 17:31 Dingyan Li 2024-03-07 18:42 ` Alan Stern 0 siblings, 1 reply; 9+ messages in thread From: Dingyan Li @ 2024-03-07 17:31 UTC (permalink / raw) To: stern, gregkh; +Cc: linux-usb, linux-kernel Macros with good names offer better readability. Besides, also move the definition to ehci.h. Signed-off-by: Dingyan Li <18500469033@163.com> --- 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; @@ -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); diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h index 1441e3400796..dafa6628e134 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] USB: Use EHCI control transfer pid macros instead of constant values. 2024-03-07 17:31 [PATCH] USB: Use EHCI control transfer pid macros instead of constant values Dingyan Li @ 2024-03-07 18:42 ` Alan Stern 2024-03-08 1:08 ` [PATCH v2] " Dingyan Li 0 siblings, 1 reply; 9+ messages in thread From: Alan Stern @ 2024-03-07 18:42 UTC (permalink / raw) To: Dingyan Li; +Cc: gregkh, linux-usb, linux-kernel 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@163.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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] USB: Use EHCI control transfer pid macros instead of constant values. 2024-03-07 18:42 ` Alan Stern @ 2024-03-08 1:08 ` Dingyan Li 2024-03-08 7:55 ` Greg KH 2024-03-08 20:34 ` Alan Stern 0 siblings, 2 replies; 9+ messages in thread From: Dingyan Li @ 2024-03-08 1:08 UTC (permalink / raw) To: stern; +Cc: gregkh, linux-usb, linux-kernel Macros with good names offer better readability. Besides, also move the definition to ehci.h. Signed-off-by: Dingyan Li <18500469033@163.com> --- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] USB: Use EHCI control transfer pid macros instead of constant values. 2024-03-08 1:08 ` [PATCH v2] " Dingyan Li @ 2024-03-08 7:55 ` Greg KH 2024-03-08 20:34 ` Alan Stern 1 sibling, 0 replies; 9+ messages in thread From: Greg KH @ 2024-03-08 7:55 UTC (permalink / raw) To: Dingyan Li; +Cc: stern, linux-usb, linux-kernel 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@163.com> > --- > 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 > > Hi, This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree. You are receiving this message because of the following common error(s) as indicated below: - This looks like a new version of a previously submitted patch, but you did not list below the --- line any changes from the previous version. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/process/submitting-patches.rst for what needs to be done here to properly describe this. If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers. thanks, greg k-h's patch email bot ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] USB: Use EHCI control transfer pid macros instead of constant values. 2024-03-08 1:08 ` [PATCH v2] " Dingyan Li 2024-03-08 7:55 ` Greg KH @ 2024-03-08 20:34 ` Alan Stern 2024-03-09 3:37 ` Dingyan Li 1 sibling, 1 reply; 9+ messages in thread From: Alan Stern @ 2024-03-08 20:34 UTC (permalink / raw) To: Dingyan Li; +Cc: gregkh, linux-usb, linux-kernel 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@163.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@rowland.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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] USB: Use EHCI control transfer pid macros instead of constant values. 2024-03-08 20:34 ` Alan Stern @ 2024-03-09 3:37 ` Dingyan Li 2024-03-09 7:14 ` Greg KH 0 siblings, 1 reply; 9+ messages in thread From: Dingyan Li @ 2024-03-09 3:37 UTC (permalink / raw) To: stern; +Cc: gregkh, linux-usb, linux-kernel Macros with good names offer better readability. Besides, also move the definition to ehci.h. Signed-off-by: Dingyan Li <18500469033@163.com> Reviewed-by: Alan Stern <stern@rowland.harvard.edu> --- V1 -> V2: Replacement in more places where Alan pointed out. 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] USB: Use EHCI control transfer pid macros instead of constant values. 2024-03-09 3:37 ` Dingyan Li @ 2024-03-09 7:14 ` Greg KH 2024-03-09 9:18 ` [PATCH v3] " Dingyan Li 2024-03-16 5:35 ` Re:Re: [PATCH v2] " Dingyan Li 0 siblings, 2 replies; 9+ messages in thread From: Greg KH @ 2024-03-09 7:14 UTC (permalink / raw) To: Dingyan Li; +Cc: stern, linux-usb, linux-kernel On Sat, Mar 09, 2024 at 11:37:09AM +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@163.com> > Reviewed-by: Alan Stern <stern@rowland.harvard.edu> > --- > V1 -> V2: Replacement in more places where Alan pointed out. > This should be v3, as you have added Alan's reviewed-by, and added the proper versioning information. Please resend it as such as our tools can't dig it out from the end of the thread like this :( thanks, greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] USB: Use EHCI control transfer pid macros instead of constant values. 2024-03-09 7:14 ` Greg KH @ 2024-03-09 9:18 ` Dingyan Li 2024-03-16 5:35 ` Re:Re: [PATCH v2] " Dingyan Li 1 sibling, 0 replies; 9+ messages in thread From: Dingyan Li @ 2024-03-09 9:18 UTC (permalink / raw) To: stern, gregkh; +Cc: linux-usb, linux-kernel Macros with good names offer better readability. Besides, also move the definition to ehci.h. Signed-off-by: Dingyan Li <18500469033@163.com> Reviewed-by: Alan Stern <stern@rowland.harvard.edu> --- V1 -> V2: Replacement in more places where Alan pointed out. V2 -> V3: Add Reviewed-by for Alan 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re:Re: [PATCH v2] USB: Use EHCI control transfer pid macros instead of constant values. 2024-03-09 7:14 ` Greg KH 2024-03-09 9:18 ` [PATCH v3] " Dingyan Li @ 2024-03-16 5:35 ` Dingyan Li 1 sibling, 0 replies; 9+ messages in thread From: Dingyan Li @ 2024-03-16 5:35 UTC (permalink / raw) To: Greg KH; +Cc: stern, linux-usb, linux-kernel At 2024-03-09 15:14:48, "Greg KH" <gregkh@linuxfoundation.org> wrote: >On Sat, Mar 09, 2024 at 11:37:09AM +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@163.com> >> Reviewed-by: Alan Stern <stern@rowland.harvard.edu> >> --- >> V1 -> V2: Replacement in more places where Alan pointed out. >> > >This should be v3, as you have added Alan's reviewed-by, and added the >proper versioning information. Please resend it as such as our tools >can't dig it out from the end of the thread like this :( > >thanks, > >greg k-h Hi Greg, Sorry that I'm not quite familiar with the process. Since I haven't got any feedback for a while after sending the v3 patch. Do that mean the patch is accepted? Or anything else I still need to improve? Regards, Dingyan ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-03-16 5:51 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-03-07 17:31 [PATCH] USB: Use EHCI control transfer pid macros instead of constant values Dingyan Li 2024-03-07 18:42 ` Alan Stern 2024-03-08 1:08 ` [PATCH v2] " Dingyan Li 2024-03-08 7:55 ` Greg KH 2024-03-08 20:34 ` Alan Stern 2024-03-09 3:37 ` Dingyan Li 2024-03-09 7:14 ` Greg KH 2024-03-09 9:18 ` [PATCH v3] " Dingyan Li 2024-03-16 5:35 ` Re:Re: [PATCH v2] " Dingyan Li
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox