Current implementation to convert urb pipe number to struct
usb_host_endpoint in rquest_single_step_set_feature_urb is a little
messy.
Use usb_pipe_endpoint helper function to get the endpoint instead.
Signed-off-by: Khalid Masum <[email protected]>
---
drivers/usb/core/hcd.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 94b305bbd621..107e29d5d3ae 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2165,8 +2165,7 @@ static struct urb *request_single_step_set_feature_urb(
return NULL;
urb->pipe = usb_rcvctrlpipe(udev, 0);
- ep = (usb_pipein(urb->pipe) ? udev->ep_in : udev->ep_out)
- [usb_pipeendpoint(urb->pipe)];
+ ep = usb_pipe_endpoint(udev, urb->pipe);
if (!ep) {
usb_free_urb(urb);
return NULL;
--
2.37.1
On Wed, Aug 24, 2022 at 07:07:02PM +0600, Khalid Masum wrote:
> Current implementation to convert urb pipe number to struct
> usb_host_endpoint in rquest_single_step_set_feature_urb is a little
> messy.
>
> Use usb_pipe_endpoint helper function to get the endpoint instead.
>
> Signed-off-by: Khalid Masum <[email protected]>
> ---
> drivers/usb/core/hcd.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 94b305bbd621..107e29d5d3ae 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2165,8 +2165,7 @@ static struct urb *request_single_step_set_feature_urb(
> return NULL;
>
> urb->pipe = usb_rcvctrlpipe(udev, 0);
> - ep = (usb_pipein(urb->pipe) ? udev->ep_in : udev->ep_out)
> - [usb_pipeendpoint(urb->pipe)];
> + ep = usb_pipe_endpoint(udev, urb->pipe);
> if (!ep) {
> usb_free_urb(urb);
> return NULL;
Even this is awkward. It's silly to look up the endpoint in a table
when you already know that it is endpoint 0. Just do:
ep = &udev->ep0;
with no need to check for NULL.
Alan Stern
On Wed, Aug 24, 2022 at 8:40 PM Alan Stern <[email protected]> wrote:
>
> On Wed, Aug 24, 2022 at 07:07:02PM +0600, Khalid Masum wrote:
>
> ep = &udev->ep0;
>
> with no need to check for NULL.
Thanks, I shall send a v2 with this change.
> Alan Stern
-- Khalid Masum
On Wed, Aug 24, 2022 at 8:40 PM Alan Stern <[email protected]> wrote:
>
>
> Even this is awkward. It's silly to look up the endpoint in a table
> when you already know that it is endpoint 0. Just do:
>
> ep = &udev->ep0;
>
> with no need to check for NULL.
After further checking, I realized that usb_device udev is created by
ehset_single_step_set_feature and depends on usb_hcd and port. So I do
not get why the endpoint is at udev->ep0. Can you help me with this?
-- Khalid Masum
On Wed, Aug 24, 2022 at 11:57:25PM +0600, Khalid Masum wrote:
> On Wed, Aug 24, 2022 at 8:40 PM Alan Stern <[email protected]> wrote:
> >
> >
> > Even this is awkward. It's silly to look up the endpoint in a table
> > when you already know that it is endpoint 0. Just do:
> >
> > ep = &udev->ep0;
> >
> > with no need to check for NULL.
>
> After further checking, I realized that usb_device udev is created by
> ehset_single_step_set_feature and depends on usb_hcd and port. So I do
> not get why the endpoint is at udev->ep0. Can you help me with this?
udev is not a usb_device structure; it is a _pointer_ to a usb_device
structure. The pointer is created when ehset_single_step_set_feature()
calls usb_hub_find_child(), but the structure itself gets created long
before that, when the USB device is first detected and initialized.
You can see this happening where hub_port_connect() calls
usb_alloc_dev(). The ep0 field is filled in by hub_port_init() and the
routines it calls.
Alan Stern
On Thu, Aug 25, 2022 at 1:24 AM Alan Stern <[email protected]> wrote:
>
> udev is not a usb_device structure; it is a _pointer_ to a usb_device
> structure. The pointer is created when ehset_single_step_set_feature()
> calls usb_hub_find_child(), but the structure itself gets created long
> before that, when the USB device is first detected and initialized.
>
> You can see this happening where hub_port_connect() calls
> usb_alloc_dev(). The ep0 field is filled in by hub_port_init() and the
> routines it calls.
>
Thanks for clarifying this. That helps a lot.
-- Khalid Masum