| 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: <Pine.LNX.4.44L0.1811141338100.1637-100000@iolanthe.rowland.org>
Date: Wed, 14 Nov 2018 13:47:04 -0500 (EST)
From: Alan Stern <stern@...land.harvard.edu>
To: Ben Dooks <ben-linux@...ff.org>
cc: gregkh@...uxfoundation.org, <linux-usb@...r.kernel.org>,
<linux-kernel@...ts.codethink.co.uk>,
<linux-kernel@...r.kernel.org>,
Ben Dooks <ben.dooks@...ethink.co.uk>
Subject: Re: [PATCH] USB: host: ehci: allow tine of highspeed nak-count
On Wed, 14 Nov 2018, Ben Dooks wrote:
> From: Ben Dooks <ben.dooks@...ethink.co.uk>
>
> At least some systems benefit with less scheduling if the NAK count
> value is set higher than the default 4. For instance a Tegra3 with
> an SMSC9512 showed less interrupt load when this was changed to 14.
Interesting. Why do you think this is? In theory, increasing the NAK
count RL value should cause a higher memory bus load and perhaps a
slight rise in the interrupt load (transfers will complete a little
more quickly because the controller tries harder to poll the endpoints
and see if they are ready).
> To allow the changing of this value, add a sysfs node to each of
> the controllers to allow run-time changing.
>
> Signed-off-by: Ben Dooks <ben.dooks@...ethink.co.uk>
The patch looks okay to me.
Acked-by: Alan Stern <stern@...land.harvard.edu>
Alan Stern
> ---
> drivers/usb/host/ehci-hcd.c | 1 +
> drivers/usb/host/ehci-q.c | 4 +--
> drivers/usb/host/ehci-sysfs.c | 52 +++++++++++++++++++++++++++++++++--
> drivers/usb/host/ehci.h | 1 +
> 4 files changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 8608ac513fb7..799262951f41 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -526,6 +526,7 @@ static int ehci_init(struct usb_hcd *hcd)
> hw->hw_qtd_next = EHCI_LIST_END(ehci);
> ehci->async->qh_state = QH_STATE_LINKED;
> hw->hw_alt_next = QTD_NEXT(ehci, ehci->async->dummy->qtd_dma);
> + ehci->nak_tune_hs = EHCI_TUNE_RL_HS;
>
> /* clear interrupt enables, set irq latency */
> if (log2_irq_thresh < 0 || log2_irq_thresh > 6)
> diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> index 327630405695..ccb754893b5a 100644
> --- a/drivers/usb/host/ehci-q.c
> +++ b/drivers/usb/host/ehci-q.c
> @@ -898,12 +898,12 @@ qh_make (
> case USB_SPEED_HIGH: /* no TT involved */
> info1 |= QH_HIGH_SPEED;
> if (type == PIPE_CONTROL) {
> - info1 |= (EHCI_TUNE_RL_HS << 28);
> + info1 |= ehci->nak_tune_hs << 28;
> info1 |= 64 << 16; /* usb2 fixed maxpacket */
> info1 |= QH_TOGGLE_CTL; /* toggle from qtd */
> info2 |= (EHCI_TUNE_MULT_HS << 30);
> } else if (type == PIPE_BULK) {
> - info1 |= (EHCI_TUNE_RL_HS << 28);
> + info1 |= ehci->nak_tune_hs << 28;
> /* The USB spec says that high speed bulk endpoints
> * always use 512 byte maxpacket. But some device
> * vendors decided to ignore that, and MSFT is happy
> diff --git a/drivers/usb/host/ehci-sysfs.c b/drivers/usb/host/ehci-sysfs.c
> index 8f75cb7b197c..d710d35282a6 100644
> --- a/drivers/usb/host/ehci-sysfs.c
> +++ b/drivers/usb/host/ehci-sysfs.c
> @@ -145,19 +145,66 @@ static ssize_t uframe_periodic_max_store(struct device *dev,
> }
> static DEVICE_ATTR_RW(uframe_periodic_max);
>
> +static ssize_t nak_tune_hs_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct ehci_hcd *ehci;
> +
> + ehci = hcd_to_ehci(dev_get_drvdata(dev));
> + return scnprintf(buf, PAGE_SIZE, "%d\n", ehci->nak_tune_hs);
> +}
> +
> +static ssize_t nak_tune_hs_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct ehci_hcd *ehci;
> + unsigned val;
> + unsigned long flags;
> +
> + ehci = hcd_to_ehci(dev_get_drvdata(dev));
> +
> + if (kstrtouint(buf, 0, &val) < 0)
> + return -EINVAL;
> +
> + if (val >= 15) {
> + ehci_info(ehci, "invalid value for nak_tune_hs (%d)\n", val);
> + return -EINVAL;
> + }
> +
> + spin_lock_irqsave (&ehci->lock, flags);
> + ehci->nak_tune_hs = val;
> + spin_unlock_irqrestore (&ehci->lock, flags);
> + return count;
> +}
> +
> +static DEVICE_ATTR_RW(nak_tune_hs);
>
> static inline int create_sysfs_files(struct ehci_hcd *ehci)
> {
> struct device *controller = ehci_to_hcd(ehci)->self.controller;
> int i = 0;
>
> + i = device_create_file(controller, &dev_attr_nak_tune_hs);
> + if (i)
> + goto out;
> +
> + i = device_create_file(controller, &dev_attr_uframe_periodic_max);
> + if (i)
> + goto out_nak;
> +
> /* with integrated TT there is no companion! */
> if (!ehci_is_TDI(ehci))
> i = device_create_file(controller, &dev_attr_companion);
> if (i)
> - goto out;
> + goto out_all;
>
> - i = device_create_file(controller, &dev_attr_uframe_periodic_max);
> + return 0;
> +out_all:
> + device_remove_file(controller, &dev_attr_uframe_periodic_max);
> +out_nak:
> + device_remove_file(controller, &dev_attr_nak_tune_hs);
> out:
> return i;
> }
> @@ -170,5 +217,6 @@ static inline void remove_sysfs_files(struct ehci_hcd *ehci)
> if (!ehci_is_TDI(ehci))
> device_remove_file(controller, &dev_attr_companion);
>
> + device_remove_file(controller, &dev_attr_nak_tune_hs);
> device_remove_file(controller, &dev_attr_uframe_periodic_max);
> }
> diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
> index c8e9a48e1d51..1fb6f1ad8128 100644
> --- a/drivers/usb/host/ehci.h
> +++ b/drivers/usb/host/ehci.h
> @@ -154,6 +154,7 @@ struct ehci_hcd { /* one per controller */
> dma_addr_t periodic_dma;
> struct list_head intr_qh_list;
> unsigned i_thresh; /* uframes HC might cache */
> + u32 nak_tune_hs;
>
> union ehci_shadow *pshadow; /* mirror hw periodic table */
> struct list_head intr_unlink_wait;
>
Powered by blists - more mailing lists