linux-kernel - Re: [PATCH v11 1/2] usb: Add devaddr in struct usb

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] [day] [month] [year] [list]
Message-ID: <e56146a9-9a88-b257-c3ea-1dcf307df830@nvidia.com>
Date:   Wed, 5 Jun 2019 17:48:02 +0800
From:   Jim Lin <jilin@...dia.com>
To:     <gregkh@...uxfoundation.org>, <mathias.nyman@...el.com>,
        <stern@...land.harvard.edu>, <kai.heng.feng@...onical.com>,
        <drinkcat@...omium.org>, <Thinh.Nguyen@...opsys.com>,
        <nsaenzjulienne@...e.de>, <jflat@...omium.org>, <malat@...ian.org>
CC:     <linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v11 1/2] usb: Add devaddr in struct usb_device


On 2019年06月03日 18:53, Jim Lin wrote:
> The Clear_TT_Buffer request sent to the hub includes the address of
> the LS/FS child device in wValue field. usb_hub_clear_tt_buffer()
> uses udev->devnum to set the address wValue. This won't work for
> devices connected to xHC.
>
> For other host controllers udev->devnum is the same as the address of
> the usb device, chosen and set by usb core. With xHC the controller
> hardware assigns the address, and won't be the same as devnum.
>
> Here we add devaddr in "struct usb_device" for
> usb_hub_clear_tt_buffer() to use.
>
> Signed-off-by: Jim Lin <jilin@...dia.com>
> ---
> v2: xhci_clear_tt_buffer_complete: add static, shorter indentation
>      , remove its claiming in xhci.h
> v3: Add description for clearing_tt (xhci.h)
> v4: Remove clearing_tt flag because hub_tt_work has hub->tt.lock
>      to protect for Clear_TT_Buffer to be run serially.
>      Remove xhci_clear_tt_buffer_complete as it's not necessary.
>      Same reason as the above.
>      Extend usb_hub_clear_tt_buffer parameter
> v5: Not extending usb_hub_clear_tt_buffer parameter
>      Add description.
> v6: Remove unused parameter slot_id from xhci_clear_hub_tt_buffer
> v7: Add devaddr field in "struct usb_device"
> v8: split as two patches, change type from int to u8 for devaddr.
> v9: Use pahole to find place to put devaddr in struct usb_device.
>      Remove space between type cast and variable.
>      hub.c changed from v8
>      clear->devinfo |= (u16) (udev->devaddr << 4);
>      to
>      clear->devinfo |= ((u16)udev->devaddr) << 4;
>      to solve a problem if devaddr is larger than 16.
> v10 Initialize devaddr in xhci_setup_device()
>      Move devaddr to be below "u8 level"
> v11 Add xhci.c "slot_ctx = ..."
>
>   drivers/usb/core/hub.c  | 4 +++-
>   drivers/usb/host/xhci.c | 2 ++
>   include/linux/usb.h     | 2 ++
>   3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 572e8c26a129..82cc3766cb23 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -873,7 +873,7 @@ int usb_hub_clear_tt_buffer(struct urb *urb)
>   	/* info that CLEAR_TT_BUFFER needs */
>   	clear->tt = tt->multi ? udev->ttport : 1;
>   	clear->devinfo = usb_pipeendpoint (pipe);
> -	clear->devinfo |= udev->devnum << 4;
> +	clear->devinfo |= ((u16)udev->devaddr) << 4;
>   	clear->devinfo |= usb_pipecontrol(pipe)
>   			? (USB_ENDPOINT_XFER_CONTROL << 11)
>   			: (USB_ENDPOINT_XFER_BULK << 11);
> @@ -2125,6 +2125,8 @@ static void update_devnum(struct usb_device *udev, int devnum)
>   	/* The address for a WUSB device is managed by wusbcore. */
>   	if (!udev->wusb)
>   		udev->devnum = devnum;
> +	if (!udev->devaddr)
> +		udev->devaddr = (u8)devnum;
>   }
>   
>   static void hub_free_dev(struct usb_device *udev)
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 20db378a6012..4f92643e3a4c 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -4125,6 +4125,8 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
>   	/* Zero the input context control for later use */
>   	ctrl_ctx->add_flags = 0;
>   	ctrl_ctx->drop_flags = 0;
> +	slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->out_ctx);
> +	udev->devaddr = (u8)(le32_to_cpu(slot_ctx->dev_state) & DEV_ADDR_MASK);
>   
>   	xhci_dbg_trace(xhci, trace_xhci_dbg_address,
>   		       "Internal device address = %d",
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index ae82d9d1112b..83d35d993e8c 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -578,6 +578,7 @@ struct usb3_lpm_parameters {
>    * @bus_mA: Current available from the bus
>    * @portnum: parent port number (origin 1)
>    * @level: number of USB hub ancestors
> + * @devaddr: device address, XHCI: assigned by HW, others: same as devnum
>    * @can_submit: URBs may be submitted
>    * @persist_enabled:  USB_PERSIST enabled for this device
>    * @have_langid: whether string_langid is valid
> @@ -661,6 +662,7 @@ struct usb_device {
>   	unsigned short bus_mA;
>   	u8 portnum;
>   	u8 level;
> +	u8 devaddr;
>   
>   	unsigned can_submit:1;
>   	unsigned persist_enabled:1;

- May I get patch v11 1/2 acked or reviewed by Alan?

Did I not do this already?  Oh well, in any case:

Acked-by: Alan Stern <stern@...land.harvard.edu>

Alan Stern

--nvpublic