| 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: <eb302fcf-83b1-bed9-f2d3-201dc767a30b@norrbonn.se>
Date: Sat, 27 Apr 2019 07:01:17 +0200
From: Jonas Bonn <jonas@...rbonn.se>
To: linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Cc: Cristian Birsan <cristian.birsan@...rochip.com>,
Felipe Balbi <balbi@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Nicolas Ferre <nicolas.ferre@...rochip.com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Ludovic Desroches <ludovic.desroches@...rochip.com>,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/3] usb: gadget: atmel: support USB suspend
Ping. Any feedback on this at all? It's been over two months without a
single comment.
Thanks,
Jonas
On 20/02/2019 13:20, Jonas Bonn wrote:
> This patch adds support for USB suspend to the Atmel UDC.
>
> When suspended, the UDC clock can be stopped, resulting in some power
> savings. The "wake up" interrupt will fire irregardless of whether the
> clock is running or not, allowing the UDC clock to be restarted when the
> USB master wants to wake the device again.
>
> The IRQ state of this device is somewhat fiddly. The "wake up" IRQ
> seems to actually be a "bus activity" indicator; the IRQ is almost
> continuously asserted so enabling this IRQ should only be done after a
> suspend when the wake IRQ becomes relevant. Similarly, the "suspend"
> IRQ detects "bus inactivity" and may therefore fire together with a
> "wake" if the two types of activity coincide during the period between
> two IRQ handler invocations; therefore, it's important to ignore the
> "suspend" IRQ while waiting for a wake-up.
>
> This has been tested on a SAMA5D2 board.
>
> Signed-off-by: Jonas Bonn <jonas@...rbonn.se>
> CC: Cristian Birsan <cristian.birsan@...rochip.com>
> CC: Felipe Balbi <balbi@...nel.org>
> CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> CC: Nicolas Ferre <nicolas.ferre@...rochip.com>
> CC: Alexandre Belloni <alexandre.belloni@...tlin.com>
> CC: Ludovic Desroches <ludovic.desroches@...rochip.com>
> CC: linux-arm-kernel@...ts.infradead.org
> CC: linux-usb@...r.kernel.org
> ---
> drivers/usb/gadget/udc/atmel_usba_udc.c | 55 ++++++++++++++++++++++---
> drivers/usb/gadget/udc/atmel_usba_udc.h | 1 +
> 2 files changed, 50 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index 9d18fdddd9b2..740cb9308a86 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -1703,6 +1703,9 @@ static void usba_dma_irq(struct usba_udc *udc, struct usba_ep *ep)
> }
> }
>
> +static int start_clock(struct usba_udc *udc);
> +static void stop_clock(struct usba_udc *udc);
> +
> static irqreturn_t usba_udc_irq(int irq, void *devid)
> {
> struct usba_udc *udc = devid;
> @@ -1720,10 +1723,13 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
> DBG(DBG_INT, "irq, status=%#08x\n", status);
>
> if (status & USBA_DET_SUSPEND) {
> - toggle_bias(udc, 0);
> - usba_writel(udc, INT_CLR, USBA_DET_SUSPEND);
> + usba_writel(udc, INT_CLR, USBA_DET_SUSPEND|USBA_WAKE_UP);
> usba_int_enb_set(udc, USBA_WAKE_UP);
> + usba_int_enb_clear(udc, USBA_DET_SUSPEND);
> + udc->suspended = true;
> + toggle_bias(udc, 0);
> udc->bias_pulse_needed = true;
> + stop_clock(udc);
> DBG(DBG_BUS, "Suspend detected\n");
> if (udc->gadget.speed != USB_SPEED_UNKNOWN
> && udc->driver && udc->driver->suspend) {
> @@ -1734,14 +1740,17 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
> }
>
> if (status & USBA_WAKE_UP) {
> + start_clock(udc);
> toggle_bias(udc, 1);
> usba_writel(udc, INT_CLR, USBA_WAKE_UP);
> - usba_int_enb_clear(udc, USBA_WAKE_UP);
> DBG(DBG_BUS, "Wake Up CPU detected\n");
> }
>
> if (status & USBA_END_OF_RESUME) {
> + udc->suspended = false;
> usba_writel(udc, INT_CLR, USBA_END_OF_RESUME);
> + usba_int_enb_clear(udc, USBA_WAKE_UP);
> + usba_int_enb_set(udc, USBA_DET_SUSPEND);
> generate_bias_pulse(udc);
> DBG(DBG_BUS, "Resume detected\n");
> if (udc->gadget.speed != USB_SPEED_UNKNOWN
> @@ -1756,6 +1765,8 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
> if (dma_status) {
> int i;
>
> + usba_int_enb_set(udc, USBA_DET_SUSPEND);
> +
> for (i = 1; i <= USBA_NR_DMAS; i++)
> if (dma_status & (1 << i))
> usba_dma_irq(udc, &udc->usba_ep[i]);
> @@ -1765,6 +1776,8 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
> if (ep_status) {
> int i;
>
> + usba_int_enb_set(udc, USBA_DET_SUSPEND);
> +
> for (i = 0; i < udc->num_ep; i++)
> if (ep_status & (1 << i)) {
> if (ep_is_control(&udc->usba_ep[i]))
> @@ -1778,7 +1791,9 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
> struct usba_ep *ep0, *ep;
> int i, n;
>
> - usba_writel(udc, INT_CLR, USBA_END_OF_RESET);
> + usba_writel(udc, INT_CLR,
> + USBA_END_OF_RESET|USBA_END_OF_RESUME
> + |USBA_DET_SUSPEND|USBA_WAKE_UP);
> generate_bias_pulse(udc);
> reset_all_endpoints(udc);
>
> @@ -1805,6 +1820,11 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
> | USBA_BF(BK_NUMBER, USBA_BK_NUMBER_ONE)));
> usba_ep_writel(ep0, CTL_ENB,
> USBA_EPT_ENABLE | USBA_RX_SETUP);
> +
> + /* If we get reset while suspended... */
> + udc->suspended = false;
> + usba_int_enb_clear(udc, USBA_WAKE_UP);
> +
> usba_int_enb_set(udc, USBA_BF(EPT_INT, 1) |
> USBA_DET_SUSPEND | USBA_END_OF_RESUME);
>
> @@ -1872,9 +1892,19 @@ static int usba_start(struct usba_udc *udc)
> if (ret)
> return ret;
>
> + if (udc->suspended)
> + return 0;
> +
> spin_lock_irqsave(&udc->lock, flags);
> toggle_bias(udc, 1);
> usba_writel(udc, CTRL, USBA_ENABLE_MASK);
> + /* Clear all requested and pending interrupts... */
> + usba_writel(udc, INT_ENB, 0);
> + udc->int_enb_cache = 0;
> + usba_writel(udc, INT_CLR,
> + USBA_END_OF_RESET|USBA_END_OF_RESUME
> + |USBA_DET_SUSPEND|USBA_WAKE_UP);
> + /* ...and enable just 'reset' IRQ to get us started */
> usba_int_enb_set(udc, USBA_END_OF_RESET);
> spin_unlock_irqrestore(&udc->lock, flags);
>
> @@ -1885,6 +1915,9 @@ static void usba_stop(struct usba_udc *udc)
> {
> unsigned long flags;
>
> + if (udc->suspended)
> + return;
> +
> spin_lock_irqsave(&udc->lock, flags);
> udc->gadget.speed = USB_SPEED_UNKNOWN;
> reset_all_endpoints(udc);
> @@ -1912,6 +1945,7 @@ static irqreturn_t usba_vbus_irq_thread(int irq, void *devid)
> if (vbus) {
> usba_start(udc);
> } else {
> + udc->suspended = false;
> usba_stop(udc);
>
> if (udc->driver->disconnect)
> @@ -1975,6 +2009,7 @@ static int atmel_usba_stop(struct usb_gadget *gadget)
> if (fifo_mode == 0)
> udc->configured_ep = 1;
>
> + udc->suspended = false;
> usba_stop(udc);
>
> udc->driver = NULL;
> @@ -2288,6 +2323,7 @@ static int usba_udc_suspend(struct device *dev)
> mutex_lock(&udc->vbus_mutex);
>
> if (!device_may_wakeup(dev)) {
> + udc->suspended = false;
> usba_stop(udc);
> goto out;
> }
> @@ -2297,10 +2333,13 @@ static int usba_udc_suspend(struct device *dev)
> * to request vbus irq, assuming always on.
> */
> if (udc->vbus_pin) {
> + /* FIXME: right to stop here...??? */
> usba_stop(udc);
> enable_irq_wake(gpiod_to_irq(udc->vbus_pin));
> }
>
> + enable_irq_wake(udc->irq);
> +
> out:
> mutex_unlock(&udc->vbus_mutex);
> return 0;
> @@ -2314,8 +2353,12 @@ static int usba_udc_resume(struct device *dev)
> if (!udc->driver)
> return 0;
>
> - if (device_may_wakeup(dev) && udc->vbus_pin)
> - disable_irq_wake(gpiod_to_irq(udc->vbus_pin));
> + if (device_may_wakeup(dev)) {
> + if (udc->vbus_pin)
> + disable_irq_wake(gpiod_to_irq(udc->vbus_pin));
> +
> + disable_irq_wake(udc->irq);
> + }
>
> /* If Vbus is present, enable the controller and wait for reset */
> mutex_lock(&udc->vbus_mutex);
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h
> index 58c96730e32e..24e6fbd3bb99 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.h
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
> @@ -331,6 +331,7 @@ struct usba_udc {
> struct usba_ep *usba_ep;
> bool bias_pulse_needed;
> bool clocked;
> + bool suspended;
>
> u16 devstatus;
>
>
Powered by blists - more mailing lists