Re: [PATCH] usb/hcd: Fix a NULL vs IS_ERR() bug in usb_hcd_setup_local_mem()

2019-06-11 Thread Sebastian Andrzej Siewior 
On 2019-06-07 16:57:09 [+0300], Dan Carpenter wrote:
> The devm_memremap() function doesn't return NULL, it returns error
> pointers.
> 
> Fixes: b0310c2f09bb ("USB: use genalloc for USB HCs with local memory")
> Signed-off-by: Dan Carpenter 

Acked-by: Sebastian Andrzej Siewior 

Sebastian

Re: [PATCH v2] USB: hso: Fix OOB memory access in hso_probe/hso_get_config_data

2018-12-10 Thread Sebastian Andrzej Siewior 
On 2018-12-10 09:04:43 [+0100], Greg KH wrote:
> From: Hui Peng 
> 
> The function hso_probe reads if_num from the USB device (as an u8) and uses
> it without a length check to index an array, resulting in an OOB memory read
> in hso_probe or hso_get_config_data. Added a length check for both locations
> and updated hso_probe to bail on error.

The function hso_probe reads if_num from the USB device (as an u8) and uses
it without a length check to index an array, resulting in an OOB memory read
in hso_probe or hso_get_config_data.

Add a length check for both locations and update hso_probe to bail on
error.

> This issue has been assigned CVE-2018-19985.
>
> Reported-by: Hui Peng 
> Reported-by: Mathias Payer 
> Signed-off-by: Hui Peng 
> Signed-off-by: Mathias Payer 
> Signed-off-by: Greg Kroah-Hartman 

Reviewed-by: Sebastian Andrzej Siewior 

Sebastian

[PATCH] Revert "usb: cdc-wdm: Fix a sleep-in-atomic-context bug in service_outstanding_interrupt()"

2018-09-11 Thread Sebastian Andrzej Siewior 
This reverts commit 6e22e3af7bb3a7b9dc53cb4687659f6e63fca427.

The bug the patch describes to, has been already fixed in commit
2df6948428542 ("USB: cdc-wdm: don't enable interrupts in USB-giveback")
so need to this, revert it.

Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/class/cdc-wdm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 656d247819c9d..bec581fb7c636 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -460,7 +460,7 @@ static int service_outstanding_interrupt(struct wdm_device 
*desc)
 
set_bit(WDM_RESPONDING, &desc->flags);
spin_unlock_irq(&desc->iuspin);
-   rv = usb_submit_urb(desc->response, GFP_ATOMIC);
+   rv = usb_submit_urb(desc->response, GFP_KERNEL);
spin_lock_irq(&desc->iuspin);
if (rv) {
dev_err(&desc->intf->dev,
-- 
2.19.0.rc2

Re: [PATCH v2] usb: cdc-wdm: Fix a sleep-in-atomic-context bug in service_outstanding_interrupt()

2018-09-11 Thread Sebastian Andrzej Siewior 
On 2018-09-01 16:12:10 [+0800], Jia-Ju Bai wrote:
> wdm_in_callback() is a completion handler function for the USB driver.
> So it should not sleep. But it calls service_outstanding_interrupt(), 
> which calls usb_submit_urb() with GFP_KERNEL. 

At which point does wdm_in_callback() invoke
service_outstanding_interrupt()? I don't see it. I see one invocation
from wdm_read() and another from service_interrupt_work().

Also, if that would be the case, then spin_unlock_irq() in an USB
completion handler (which might run in IRQ context with interrupts
disabled) would be wrong.

> To fix this bug, GFP_KERNEL is replaced with GFP_ATOMIC.
> 
> This bug is found by my static analysis tool DSAC.
> 
> Signed-off-by: Jia-Ju Bai 
> ---
> v2:
>  * Add more description.
> ---
>  drivers/usb/class/cdc-wdm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> index a0d284ef3f40..632a2bfabc08 100644
> --- a/drivers/usb/class/cdc-wdm.c
> +++ b/drivers/usb/class/cdc-wdm.c
> @@ -458,7 +458,7 @@ static int service_outstanding_interrupt(struct 
> wdm_device *desc)
>  
>   set_bit(WDM_RESPONDING, &desc->flags);
>   spin_unlock_irq(&desc->iuspin);
> - rv = usb_submit_urb(desc->response, GFP_KERNEL);
> + rv = usb_submit_urb(desc->response, GFP_ATOMIC);
>   spin_lock_irq(&desc->iuspin);
>   if (rv) {
>   dev_err(&desc->intf->dev,

Sebastian

Re: [PATCH 0/3] media: use irqsave() in USB's complete callback + remove local_irq_save

2018-09-10 Thread Sebastian Andrzej Siewior 
On 2018-09-10 06:25:57 [-0300], Mauro Carvalho Chehab wrote:
> Em Mon, 10 Sep 2018 11:19:57 +0200
> Sebastian Andrzej Siewior  escreveu:
> 
> > I've been looking at my queue and compared to v4.19-rc3. As it turns
> > out, everything was merged except for
> > 
> > media: em28xx-audio: use irqsave() in USB's complete
> > media: tm6000: use irqsave() in USB's complete callback
> > 
> > I haven't seen any reply to those two patches (like asking for changes)
> > so I assume that those two just fell through the cracks.
> > 
> > The last one is the final removal of the local_irq_save() statement once
> > all drivers were audited & fixed.
> 
> I suspect that it is better to merge it via sound tree, due to
> patch 3/3.

Sound? Sound like alsa? Or did you mean USB?

> So, for patches 1 and 2:
> 
> Acked-by: Mauro Carvalho Chehab 

Thank you. It would be nice if this would hit Linus in this cycle :)

> Thanks,
> Mauro

Sebastian

[PATCH 3/3] usb: core: remove local_irq_save() around ->complete() handler

2018-09-10 Thread Sebastian Andrzej Siewior 
The core disabled interrupts before invocation the ->complete handler
because the handler might have expected that interrupts are disabled.

All handlers were audited and use proper locking now. With it, the core
code no longer needs to disable interrupts before invoking the
->complete handler.
Remove local_irq_save() statement before invoking the ->complete
handler.

Cc: Alan Stern 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/core/hcd.c | 13 -
 1 file changed, 13 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 1c21955fe7c00..f985d2303095c 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1755,20 +1755,7 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
 
/* pass ownership to the completion handler */
urb->status = status;
-
-   /*
-* We disable local IRQs here avoid possible deadlock because
-* drivers may call spin_lock() to hold lock which might be
-* acquired in one hard interrupt handler.
-*
-* The local_irq_save()/local_irq_restore() around complete()
-* will be removed if current USB drivers have been cleaned up
-* and no one may trigger the above deadlock situation when
-* running complete() in tasklet.
-*/
-   local_irq_save(flags);
urb->complete(urb);
-   local_irq_restore(flags);
 
usb_anchor_resume_wakeups(anchor);
atomic_dec(&urb->use_count);
-- 
2.19.0.rc2

[PATCH 2/3] media: tm6000: use irqsave() in USB's complete callback

2018-09-10 Thread Sebastian Andrzej Siewior 
The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Mauro Carvalho Chehab 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/media/usb/tm6000/tm6000-video.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/tm6000/tm6000-video.c 
b/drivers/media/usb/tm6000/tm6000-video.c
index 96055de6e8ce2..7d268f2404e19 100644
--- a/drivers/media/usb/tm6000/tm6000-video.c
+++ b/drivers/media/usb/tm6000/tm6000-video.c
@@ -419,6 +419,7 @@ static void tm6000_irq_callback(struct urb *urb)
 {
struct tm6000_dmaqueue  *dma_q = urb->context;
struct tm6000_core *dev = container_of(dma_q, struct tm6000_core, vidq);
+   unsigned long flags;
int i;
 
switch (urb->status) {
@@ -436,9 +437,9 @@ static void tm6000_irq_callback(struct urb *urb)
break;
}
 
-   spin_lock(&dev->slock);
+   spin_lock_irqsave(&dev->slock, flags);
tm6000_isoc_copy(urb);
-   spin_unlock(&dev->slock);
+   spin_unlock_irqrestore(&dev->slock, flags);
 
/* Reset urb buffers */
for (i = 0; i < urb->number_of_packets; i++) {
-- 
2.19.0.rc2

[PATCH 0/3] media: use irqsave() in USB's complete callback + remove local_irq_save

2018-09-10 Thread Sebastian Andrzej Siewior 
I've been looking at my queue and compared to v4.19-rc3. As it turns
out, everything was merged except for

media: em28xx-audio: use irqsave() in USB's complete
media: tm6000: use irqsave() in USB's complete callback

I haven't seen any reply to those two patches (like asking for changes)
so I assume that those two just fell through the cracks.

The last one is the final removal of the local_irq_save() statement once
all drivers were audited & fixed.

Sebastian

[PATCH 1/3] media: em28xx-audio: use irqsave() in USB's complete callback

2018-09-10 Thread Sebastian Andrzej Siewior 
The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Mauro Carvalho Chehab 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/media/usb/em28xx/em28xx-audio.c | 5 +++--
 drivers/media/usb/em28xx/em28xx-core.c  | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-audio.c 
b/drivers/media/usb/em28xx/em28xx-audio.c
index 8e799ae1df692..67481fc824458 100644
--- a/drivers/media/usb/em28xx/em28xx-audio.c
+++ b/drivers/media/usb/em28xx/em28xx-audio.c
@@ -116,6 +116,7 @@ static void em28xx_audio_isocirq(struct urb *urb)
stride = runtime->frame_bits >> 3;
 
for (i = 0; i < urb->number_of_packets; i++) {
+   unsigned long flags;
int length =
urb->iso_frame_desc[i].actual_length / stride;
cp = (unsigned char *)urb->transfer_buffer +
@@ -137,7 +138,7 @@ static void em28xx_audio_isocirq(struct urb *urb)
   length * stride);
}
 
-   snd_pcm_stream_lock(substream);
+   snd_pcm_stream_lock_irqsave(substream, flags);
 
dev->adev.hwptr_done_capture += length;
if (dev->adev.hwptr_done_capture >=
@@ -153,7 +154,7 @@ static void em28xx_audio_isocirq(struct urb *urb)
period_elapsed = 1;
}
 
-   snd_pcm_stream_unlock(substream);
+   snd_pcm_stream_unlock_irqrestore(substream, flags);
}
if (period_elapsed)
snd_pcm_period_elapsed(substream);
diff --git a/drivers/media/usb/em28xx/em28xx-core.c 
b/drivers/media/usb/em28xx/em28xx-core.c
index 5657f8710ca6b..2b8c84a5c9a8e 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -777,6 +777,7 @@ EXPORT_SYMBOL_GPL(em28xx_set_mode);
 static void em28xx_irq_callback(struct urb *urb)
 {
struct em28xx *dev = urb->context;
+   unsigned long flags;
int i;
 
switch (urb->status) {
@@ -793,9 +794,9 @@ static void em28xx_irq_callback(struct urb *urb)
}
 
/* Copy data from URB */
-   spin_lock(&dev->slock);
+   spin_lock_irqsave(&dev->slock, flags);
dev->usb_ctl.urb_data_copy(dev, urb);
-   spin_unlock(&dev->slock);
+   spin_unlock_irqrestore(&dev->slock, flags);
 
/* Reset urb buffers */
for (i = 0; i < urb->number_of_packets; i++) {
-- 
2.19.0.rc2

[PATCH 07/12 v2] usb: usbtest: use irqsave() in USB's complete callback

2018-07-01 Thread Sebastian Andrzej Siewior 
The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Greg Kroah-Hartman 
Acked-by: Alan Stern 
Signed-off-by: Sebastian Andrzej Siewior 
---
v1…v2: don't enable interrupts while releasing the lock around unlink()
   in ctrl_complete()

 drivers/usb/misc/usbtest.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 9e1142b8b91b..c7f82310e73e 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -1082,11 +1082,12 @@ static void ctrl_complete(struct urb *urb)
struct usb_ctrlrequest  *reqp;
struct subcase  *subcase;
int status = urb->status;
+   unsigned long   flags;
 
reqp = (struct usb_ctrlrequest *)urb->setup_packet;
subcase = container_of(reqp, struct subcase, setup);
 
-   spin_lock(&ctx->lock);
+   spin_lock_irqsave(&ctx->lock, flags);
ctx->count--;
ctx->pending--;
 
@@ -1185,7 +1186,7 @@ static void ctrl_complete(struct urb *urb)
/* signal completion when nothing's queued */
if (ctx->pending == 0)
complete(&ctx->complete);
-   spin_unlock(&ctx->lock);
+   spin_unlock_irqrestore(&ctx->lock, flags);
 }
 
 static int
@@ -1917,8 +1918,9 @@ struct transfer_context {
 static void complicated_callback(struct urb *urb)
 {
struct transfer_context *ctx = urb->context;
+   unsigned long flags;
 
-   spin_lock(&ctx->lock);
+   spin_lock_irqsave(&ctx->lock, flags);
ctx->count--;
 
ctx->packet_count += urb->number_of_packets;
@@ -1958,7 +1960,7 @@ static void complicated_callback(struct urb *urb)
complete(&ctx->done);
}
 done:
-   spin_unlock(&ctx->lock);
+   spin_unlock_irqrestore(&ctx->lock, flags);
 }
 
 static struct urb *iso_alloc_urb(
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Bluetooth: btusb: use irqsave() in URB's complete callback

2018-07-01 Thread Sebastian Andrzej Siewior 
On 2018-06-21 11:34:15 [-0400], Alan Stern wrote:
> On Thu, 21 Jun 2018, Sebastian Andrzej Siewior wrote:
> 
> > On 2018-06-21 14:43:41 [+0200], Marcel Holtmann wrote:
> > > can I get an ACK from someone ensuring that this is the direction we are 
> > > going with the USB host controllers?
> > +Alan.
> > 
> > EHCI completes in BH since v3.12-rc1. In order to get rid of that
> > local_irq_save() in USB core code I need to make sure that the USB
> > device driver(s) use irqsave primitives. See
> >   
> > https://lkml.kernel.org/r/pine.lnx.4.44l0.1806011629140.1404-100...@iolanthe.rowland.org
> 
> Hi, Marcel!
Hi Marcel,

> Yes, Sebastian is right.  We are aiming to make it possible for the USB 
> core to invoke URB completion handlers with interrupts enabled, in 
> order to reduce latency (since USB interrupt processing can take a 
> fairly long time).  And of course, this means completion handlers have 
> to work correctly regardless of whether interrupts are enabled or 
> disabled.

I don't see this patch in linux-next. Do you still need some kind of
confirmation or has this been resolved?

> Currently ehci-hcd supports this possibility.  Other host controller 
> drivers may follow along; I'd like to see xhci-hcd do this too.
> 
> Alan Stern

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/12] usb: usbtest: use irqsave() in USB's complete callback

2018-06-26 Thread Sebastian Andrzej Siewior 
On 2018-06-25 11:15:06 [-0400], Alan Stern wrote:
> > @@ -1152,9 +1153,9 @@ static void ctrl_complete(struct urb *urb)
> >  
> > if (u == urb || !u->dev)
> > continue;
> > -   spin_unlock(&ctx->lock);
> > +   spin_unlock_irqrestore(&ctx->lock, flags);
> > status = usb_unlink_urb(u);
> > -   spin_lock(&ctx->lock);
> > +   spin_lock_irqsave(&ctx->lock, flags);
> 
> My feeling tends to be that when a lock is temporarily dropped to take 
> a single action like this, there's no need to restore the 
> interrupt-enable flag.  These two calls can remain 
> spin_unlock()/spin_lock().

okay, let me redo this part.

> Aside from this:
> 
> Acked-by: Alan Stern 
> 
> Alan Stern

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 07/12] usb: usbtest: use irqsave() in USB's complete callback

2018-06-24 Thread Sebastian Andrzej Siewior 
The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Greg Kroah-Hartman 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/misc/usbtest.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 9e1142b8b91b..3b56fb34906c 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -1082,11 +1082,12 @@ static void ctrl_complete(struct urb *urb)
struct usb_ctrlrequest  *reqp;
struct subcase  *subcase;
int status = urb->status;
+   unsigned long   flags;
 
reqp = (struct usb_ctrlrequest *)urb->setup_packet;
subcase = container_of(reqp, struct subcase, setup);
 
-   spin_lock(&ctx->lock);
+   spin_lock_irqsave(&ctx->lock, flags);
ctx->count--;
ctx->pending--;
 
@@ -1152,9 +1153,9 @@ static void ctrl_complete(struct urb *urb)
 
if (u == urb || !u->dev)
continue;
-   spin_unlock(&ctx->lock);
+   spin_unlock_irqrestore(&ctx->lock, flags);
status = usb_unlink_urb(u);
-   spin_lock(&ctx->lock);
+   spin_lock_irqsave(&ctx->lock, flags);
switch (status) {
case -EINPROGRESS:
case -EBUSY:
@@ -1185,7 +1186,7 @@ static void ctrl_complete(struct urb *urb)
/* signal completion when nothing's queued */
if (ctx->pending == 0)
complete(&ctx->complete);
-   spin_unlock(&ctx->lock);
+   spin_unlock_irqrestore(&ctx->lock, flags);
 }
 
 static int
@@ -1917,8 +1918,9 @@ struct transfer_context {
 static void complicated_callback(struct urb *urb)
 {
struct transfer_context *ctx = urb->context;
+   unsigned long flags;
 
-   spin_lock(&ctx->lock);
+   spin_lock_irqsave(&ctx->lock, flags);
ctx->count--;
 
ctx->packet_count += urb->number_of_packets;
@@ -1958,7 +1960,7 @@ static void complicated_callback(struct urb *urb)
complete(&ctx->done);
}
 done:
-   spin_unlock(&ctx->lock);
+   spin_unlock_irqrestore(&ctx->lock, flags);
 }
 
 static struct urb *iso_alloc_urb(
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 06/12] usb: usb-skeleton: use irqsave() in USB's complete callback

2018-06-24 Thread Sebastian Andrzej Siewior 
From: John Ogness 

The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Greg Kroah-Hartman 
Signed-off-by: John Ogness 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/usb-skeleton.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index c3ddd0f1f449..f101347e3ea3 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -159,10 +159,11 @@ static int skel_flush(struct file *file, fl_owner_t id)
 static void skel_read_bulk_callback(struct urb *urb)
 {
struct usb_skel *dev;
+   unsigned long flags;
 
dev = urb->context;
 
-   spin_lock(&dev->err_lock);
+   spin_lock_irqsave(&dev->err_lock, flags);
/* sync/async unlink faults aren't errors */
if (urb->status) {
if (!(urb->status == -ENOENT ||
@@ -177,7 +178,7 @@ static void skel_read_bulk_callback(struct urb *urb)
dev->bulk_in_filled = urb->actual_length;
}
dev->ongoing_read = 0;
-   spin_unlock(&dev->err_lock);
+   spin_unlock_irqrestore(&dev->err_lock, flags);
 
wake_up_interruptible(&dev->bulk_in_wait);
 }
@@ -331,6 +332,7 @@ static ssize_t skel_read(struct file *file, char *buffer, 
size_t count,
 static void skel_write_bulk_callback(struct urb *urb)
 {
struct usb_skel *dev;
+   unsigned long flags;
 
dev = urb->context;
 
@@ -343,9 +345,9 @@ static void skel_write_bulk_callback(struct urb *urb)
"%s - nonzero write bulk status received: %d\n",
__func__, urb->status);
 
-   spin_lock(&dev->err_lock);
+   spin_lock_irqsave(&dev->err_lock, flags);
dev->errors = urb->status;
-   spin_unlock(&dev->err_lock);
+   spin_unlock_irqrestore(&dev->err_lock, flags);
}
 
/* free up our allocated buffer */
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 09/12] usb: adutux: use irqsave() in USB's complete callback

2018-06-24 Thread Sebastian Andrzej Siewior 
The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Greg Kroah-Hartman 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/misc/adutux.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index b3160afe0458..9465fb95d70a 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -155,11 +155,12 @@ static void adu_interrupt_in_callback(struct urb *urb)
 {
struct adu_device *dev = urb->context;
int status = urb->status;
+   unsigned long flags;
 
adu_debug_data(&dev->udev->dev, __func__,
   urb->actual_length, urb->transfer_buffer);
 
-   spin_lock(&dev->buflock);
+   spin_lock_irqsave(&dev->buflock, flags);
 
if (status != 0) {
if ((status != -ENOENT) && (status != -ECONNRESET) &&
@@ -190,7 +191,7 @@ static void adu_interrupt_in_callback(struct urb *urb)
 
 exit:
dev->read_urb_finished = 1;
-   spin_unlock(&dev->buflock);
+   spin_unlock_irqrestore(&dev->buflock, flags);
/* always wake up so we recover from errors */
wake_up_interruptible(&dev->read_wait);
 }
@@ -199,6 +200,7 @@ static void adu_interrupt_out_callback(struct urb *urb)
 {
struct adu_device *dev = urb->context;
int status = urb->status;
+   unsigned long flags;
 
adu_debug_data(&dev->udev->dev, __func__,
   urb->actual_length, urb->transfer_buffer);
@@ -213,10 +215,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
return;
}
 
-   spin_lock(&dev->buflock);
+   spin_lock_irqsave(&dev->buflock, flags);
dev->out_urb_finished = 1;
wake_up(&dev->write_wait);
-   spin_unlock(&dev->buflock);
+   spin_unlock_irqrestore(&dev->buflock, flags);
 }
 
 static int adu_open(struct inode *inode, struct file *file)
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 08/12] usb: wusbcore: remove excessive irqsave

2018-06-24 Thread Sebastian Andrzej Siewior 
From: John Ogness 

wa_urb_dequeue() locks multiple spinlocks while disabling interrupts:

   spin_lock_irqsave(&lock1, flags);
   spin_lock_irqsave(&lock2, flags2);

Obviously there is no need for the second irqsave and "flags2" variable
since interrupts are disabled at that point. Remove the second irqsave:

   spin_lock_irqsave(&lock1, flags);
   spin_lock(&lock2);

and eliminate the flags2 variable.

Cc: Greg Kroah-Hartman 
Signed-off-by: John Ogness 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/wusbcore/wa-xfer.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/wusbcore/wa-xfer.c b/drivers/usb/wusbcore/wa-xfer.c
index 7fca4e7e556d..01f2f21830c0 100644
--- a/drivers/usb/wusbcore/wa-xfer.c
+++ b/drivers/usb/wusbcore/wa-xfer.c
@@ -1918,7 +1918,7 @@ EXPORT_SYMBOL_GPL(wa_urb_enqueue);
  */
 int wa_urb_dequeue(struct wahc *wa, struct urb *urb, int status)
 {
-   unsigned long flags, flags2;
+   unsigned long flags;
struct wa_xfer *xfer;
struct wa_seg *seg;
struct wa_rpipe *rpipe;
@@ -1964,10 +1964,10 @@ int wa_urb_dequeue(struct wahc *wa, struct urb *urb, 
int status)
goto out_unlock;
}
/* Check the delayed list -> if there, release and complete */
-   spin_lock_irqsave(&wa->xfer_list_lock, flags2);
+   spin_lock(&wa->xfer_list_lock);
if (!list_empty(&xfer->list_node) && xfer->seg == NULL)
goto dequeue_delayed;
-   spin_unlock_irqrestore(&wa->xfer_list_lock, flags2);
+   spin_unlock(&wa->xfer_list_lock);
if (xfer->seg == NULL)  /* still hasn't reached */
goto out_unlock;/* setup(), enqueue_b() completes */
/* Ok, the xfer is in flight already, it's been setup and submitted.*/
@@ -2054,7 +2054,7 @@ int wa_urb_dequeue(struct wahc *wa, struct urb *urb, int 
status)
 
 dequeue_delayed:
list_del_init(&xfer->list_node);
-   spin_unlock_irqrestore(&wa->xfer_list_lock, flags2);
+   spin_unlock(&wa->xfer_list_lock);
xfer->result = urb->status;
spin_unlock_irqrestore(&xfer->lock, flags);
wa_xfer_giveback(xfer);
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 12/12] usb: legousbtower: use irqsave() in USB's complete callback

2018-06-24 Thread Sebastian Andrzej Siewior 
The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Juergen Stuber 
Cc: Greg Kroah-Hartman 
Cc: legousb-de...@lists.sourceforge.net
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/misc/legousbtower.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index bf47bd8bc76f..006cf13b2199 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -722,6 +722,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
struct lego_usb_tower *dev = urb->context;
int status = urb->status;
int retval;
+   unsigned long flags;
 
lego_usb_tower_debug_data(&dev->udev->dev, __func__,
  urb->actual_length, urb->transfer_buffer);
@@ -740,7 +741,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
}
 
if (urb->actual_length > 0) {
-   spin_lock (&dev->read_buffer_lock);
+   spin_lock_irqsave(&dev->read_buffer_lock, flags);
if (dev->read_buffer_length + urb->actual_length < 
read_buffer_size) {
memcpy (dev->read_buffer + dev->read_buffer_length,
dev->interrupt_in_buffer,
@@ -753,7 +754,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
pr_warn("read_buffer overflow, %d bytes dropped\n",
urb->actual_length);
}
-   spin_unlock (&dev->read_buffer_lock);
+   spin_unlock_irqrestore(&dev->read_buffer_lock, flags);
}
 
 resubmit:
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 11/12] usb: ldusb: use irqsave() in USB's complete callback

2018-06-24 Thread Sebastian Andrzej Siewior 
The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Greg Kroah-Hartman 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/misc/ldusb.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/misc/ldusb.c b/drivers/usb/misc/ldusb.c
index c2e255f02a72..006762b72ff5 100644
--- a/drivers/usb/misc/ldusb.c
+++ b/drivers/usb/misc/ldusb.c
@@ -225,6 +225,7 @@ static void ld_usb_interrupt_in_callback(struct urb *urb)
size_t *actual_buffer;
unsigned int next_ring_head;
int status = urb->status;
+   unsigned long flags;
int retval;
 
if (status) {
@@ -236,12 +237,12 @@ static void ld_usb_interrupt_in_callback(struct urb *urb)
dev_dbg(&dev->intf->dev,
"%s: nonzero status received: %d\n", __func__,
status);
-   spin_lock(&dev->rbsl);
+   spin_lock_irqsave(&dev->rbsl, flags);
goto resubmit; /* maybe we can recover */
}
}
 
-   spin_lock(&dev->rbsl);
+   spin_lock_irqsave(&dev->rbsl, flags);
if (urb->actual_length > 0) {
next_ring_head = (dev->ring_head+1) % ring_buffer_size;
if (next_ring_head != dev->ring_tail) {
@@ -270,7 +271,7 @@ static void ld_usb_interrupt_in_callback(struct urb *urb)
dev->buffer_overflow = 1;
}
}
-   spin_unlock(&dev->rbsl);
+   spin_unlock_irqrestore(&dev->rbsl, flags);
 exit:
dev->interrupt_in_done = 1;
wake_up_interruptible(&dev->read_wait);
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/12] usb: cdc-wdm: use irqsave() in USB's complete callback

2018-06-24 Thread Sebastian Andrzej Siewior 
The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Greg Kroah-Hartman 
Cc: Oliver Neukum 
Cc: "Bjørn Mork" 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/class/cdc-wdm.c | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 203bbd378858..bec581fb7c63 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -142,10 +142,12 @@ static struct wdm_device *wdm_find_device_by_minor(int 
minor)
 static void wdm_out_callback(struct urb *urb)
 {
struct wdm_device *desc;
+   unsigned long flags;
+
desc = urb->context;
-   spin_lock(&desc->iuspin);
+   spin_lock_irqsave(&desc->iuspin, flags);
desc->werr = urb->status;
-   spin_unlock(&desc->iuspin);
+   spin_unlock_irqrestore(&desc->iuspin, flags);
kfree(desc->outbuf);
desc->outbuf = NULL;
clear_bit(WDM_IN_USE, &desc->flags);
@@ -154,11 +156,12 @@ static void wdm_out_callback(struct urb *urb)
 
 static void wdm_in_callback(struct urb *urb)
 {
+   unsigned long flags;
struct wdm_device *desc = urb->context;
int status = urb->status;
int length = urb->actual_length;
 
-   spin_lock(&desc->iuspin);
+   spin_lock_irqsave(&desc->iuspin, flags);
clear_bit(WDM_RESPONDING, &desc->flags);
 
if (status) {
@@ -220,11 +223,12 @@ static void wdm_in_callback(struct urb *urb)
set_bit(WDM_READ, &desc->flags);
wake_up(&desc->wait);
}
-   spin_unlock(&desc->iuspin);
+   spin_unlock_irqrestore(&desc->iuspin, flags);
 }
 
 static void wdm_int_callback(struct urb *urb)
 {
+   unsigned long flags;
int rv = 0;
int responding;
int status = urb->status;
@@ -284,7 +288,7 @@ static void wdm_int_callback(struct urb *urb)
goto exit;
}
 
-   spin_lock(&desc->iuspin);
+   spin_lock_irqsave(&desc->iuspin, flags);
responding = test_and_set_bit(WDM_RESPONDING, &desc->flags);
if (!desc->resp_count++ && !responding
&& !test_bit(WDM_DISCONNECTING, &desc->flags)
@@ -292,7 +296,7 @@ static void wdm_int_callback(struct urb *urb)
rv = usb_submit_urb(desc->response, GFP_ATOMIC);
dev_dbg(&desc->intf->dev, "submit response URB %d\n", rv);
}
-   spin_unlock(&desc->iuspin);
+   spin_unlock_irqrestore(&desc->iuspin, flags);
if (rv < 0) {
clear_bit(WDM_RESPONDING, &desc->flags);
if (rv == -EPERM)
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 10/12] usb: iowarrior: remove intr_idx_lock

2018-06-24 Thread Sebastian Andrzej Siewior 
The intr_idx_lock lock is acquired only in the completion callback of
the ->int_in_urb (iowarrior_callback()). There is only one URB that is
scheduled / completed so there can't be more than one user of the lock.
The comment says that it protects ->intr_idx and the callback is the
only place in driver that writes to it.

Remove the intr_idx_lock lock because it is superfluous.

Cc: Greg Kroah-Hartman 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/misc/iowarrior.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
index 8d33187ce2af..c2991b8a65ce 100644
--- a/drivers/usb/misc/iowarrior.c
+++ b/drivers/usb/misc/iowarrior.c
@@ -81,7 +81,6 @@ struct iowarrior {
atomic_t write_busy;/* number of write-urbs submitted */
atomic_t read_idx;
atomic_t intr_idx;
-   spinlock_t intr_idx_lock;   /* protects intr_idx */
atomic_t overflow_flag; /* signals an index 'rollover' */
int present;/* this is 1 as long as the device is 
connected */
int opened; /* this is 1 if the device is currently 
open */
@@ -166,7 +165,6 @@ static void iowarrior_callback(struct urb *urb)
goto exit;
}
 
-   spin_lock(&dev->intr_idx_lock);
intr_idx = atomic_read(&dev->intr_idx);
/* aux_idx become previous intr_idx */
aux_idx = (intr_idx == 0) ? (MAX_INTERRUPT_BUFFER - 1) : (intr_idx - 1);
@@ -181,7 +179,6 @@ static void iowarrior_callback(struct urb *urb)
(dev->read_queue + offset, urb->transfer_buffer,
 dev->report_size)) {
/* equal values on interface 0 will be ignored */
-   spin_unlock(&dev->intr_idx_lock);
goto exit;
}
}
@@ -202,7 +199,6 @@ static void iowarrior_callback(struct urb *urb)
*(dev->read_queue + offset + (dev->report_size)) = dev->serial_number++;
 
atomic_set(&dev->intr_idx, aux_idx);
-   spin_unlock(&dev->intr_idx_lock);
/* tell the blocking read about the new data */
wake_up_interruptible(&dev->read_wait);
 
@@ -762,7 +758,6 @@ static int iowarrior_probe(struct usb_interface *interface,
 
atomic_set(&dev->intr_idx, 0);
atomic_set(&dev->read_idx, 0);
-   spin_lock_init(&dev->intr_idx_lock);
atomic_set(&dev->overflow_flag, 0);
init_waitqueue_head(&dev->read_wait);
atomic_set(&dev->write_busy, 0);
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 05/12] usb: usblp: use irqsave() in USB's complete callback

2018-06-24 Thread Sebastian Andrzej Siewior 
The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Pete Zaitcev 
Cc: Greg Kroah-Hartman 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/class/usblp.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c
index d058d7a31e7c..407a7a6198a2 100644
--- a/drivers/usb/class/usblp.c
+++ b/drivers/usb/class/usblp.c
@@ -292,6 +292,7 @@ static void usblp_bulk_read(struct urb *urb)
 {
struct usblp *usblp = urb->context;
int status = urb->status;
+   unsigned long flags;
 
if (usblp->present && usblp->used) {
if (status)
@@ -299,14 +300,14 @@ static void usblp_bulk_read(struct urb *urb)
"nonzero read bulk status received: %d\n",
usblp->minor, status);
}
-   spin_lock(&usblp->lock);
+   spin_lock_irqsave(&usblp->lock, flags);
if (status < 0)
usblp->rstatus = status;
else
usblp->rstatus = urb->actual_length;
usblp->rcomplete = 1;
wake_up(&usblp->rwait);
-   spin_unlock(&usblp->lock);
+   spin_unlock_irqrestore(&usblp->lock, flags);
 
usb_free_urb(urb);
 }
@@ -315,6 +316,7 @@ static void usblp_bulk_write(struct urb *urb)
 {
struct usblp *usblp = urb->context;
int status = urb->status;
+   unsigned long flags;
 
if (usblp->present && usblp->used) {
if (status)
@@ -322,7 +324,7 @@ static void usblp_bulk_write(struct urb *urb)
"nonzero write bulk status received: %d\n",
usblp->minor, status);
}
-   spin_lock(&usblp->lock);
+   spin_lock_irqsave(&usblp->lock, flags);
if (status < 0)
usblp->wstatus = status;
else
@@ -330,7 +332,7 @@ static void usblp_bulk_write(struct urb *urb)
usblp->no_paper = 0;
usblp->wcomplete = 1;
wake_up(&usblp->wwait);
-   spin_unlock(&usblp->lock);
+   spin_unlock_irqrestore(&usblp->lock, flags);
 
usb_free_urb(urb);
 }
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 01/12] usb: cdc-acm: use irqsave() in USB's complete callback

2018-06-24 Thread Sebastian Andrzej Siewior 
The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Oliver Neukum 
Cc: Greg Kroah-Hartman 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/class/cdc-acm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 7b366a6c0b49..682557d6f339 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -276,6 +276,7 @@ static void acm_process_notification(struct acm *acm, 
unsigned char *buf)
 {
int newctrl;
int difference;
+   unsigned long flags;
struct usb_cdc_notification *dr = (struct usb_cdc_notification *)buf;
unsigned char *data = buf + sizeof(struct usb_cdc_notification);
 
@@ -303,7 +304,7 @@ static void acm_process_notification(struct acm *acm, 
unsigned char *buf)
}
 
difference = acm->ctrlin ^ newctrl;
-   spin_lock(&acm->read_lock);
+   spin_lock_irqsave(&acm->read_lock, flags);
acm->ctrlin = newctrl;
acm->oldcount = acm->iocount;
 
@@ -321,7 +322,7 @@ static void acm_process_notification(struct acm *acm, 
unsigned char *buf)
acm->iocount.parity++;
if (difference & ACM_CTRL_OVERRUN)
acm->iocount.overrun++;
-   spin_unlock(&acm->read_lock);
+   spin_unlock_irqrestore(&acm->read_lock, flags);
 
if (difference)
wake_up_all(&acm->wioctl);
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 04/12] usb: usbfs: use irqsave() in USB's complete callback

2018-06-24 Thread Sebastian Andrzej Siewior 
The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Greg Kroah-Hartman 
Cc: Alan Stern 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/core/devio.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 476dcc5f2da3..cea35fa4e7a3 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -585,9 +585,10 @@ static void async_completed(struct urb *urb)
struct siginfo sinfo;
struct pid *pid = NULL;
const struct cred *cred = NULL;
+   unsigned long flags;
int signr;
 
-   spin_lock(&ps->lock);
+   spin_lock_irqsave(&ps->lock, flags);
list_move_tail(&as->asynclist, &ps->async_completed);
as->status = urb->status;
signr = as->signr;
@@ -611,7 +612,7 @@ static void async_completed(struct urb *urb)
cancel_bulk_urbs(ps, as->bulk_addr);
 
wake_up(&ps->wait);
-   spin_unlock(&ps->lock);
+   spin_unlock_irqrestore(&ps->lock, flags);
 
if (signr) {
kill_pid_info_as_cred(sinfo.si_signo, &sinfo, pid, cred);
@@ -1702,6 +1703,7 @@ static int proc_do_submiturb(struct usb_dev_state *ps, 
struct usbdevfs_urb *uurb
 
as->urb->context = as;
as->urb->complete = async_completed;
+
for (totlen = u = 0; u < number_of_packets; u++) {
as->urb->iso_frame_desc[u].offset = totlen;
as->urb->iso_frame_desc[u].length = isopkt[u].length;
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 00/12] usb: Use irqsave in USB's complete callback

2018-06-24 Thread Sebastian Andrzej Siewior 
This is about using _irqsave() primitives in the completion callback in
order to get rid of local_irq_save() in __usb_hcd_giveback_urb(). This is the
last series.

Sebastian

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 03/12] usb: core: use irqsave() in sg_complete() complete callback

2018-06-24 Thread Sebastian Andrzej Siewior 
The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Greg Kroah-Hartman 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/core/message.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 1a15392326fc..228672f2c4a1 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -269,10 +269,11 @@ static void sg_clean(struct usb_sg_request *io)
 
 static void sg_complete(struct urb *urb)
 {
+   unsigned long flags;
struct usb_sg_request *io = urb->context;
int status = urb->status;
 
-   spin_lock(&io->lock);
+   spin_lock_irqsave(&io->lock, flags);
 
/* In 2.5 we require hcds' endpoint queues not to progress after fault
 * reports, until the completion callback (this!) returns.  That lets
@@ -306,7 +307,7 @@ static void sg_complete(struct urb *urb)
 * unlink pending urbs so they won't rx/tx bad data.
 * careful: unlink can sometimes be synchronous...
 */
-   spin_unlock(&io->lock);
+   spin_unlock_irqrestore(&io->lock, flags);
for (i = 0, found = 0; i < io->entries; i++) {
if (!io->urbs[i])
continue;
@@ -323,7 +324,7 @@ static void sg_complete(struct urb *urb)
} else if (urb == io->urbs[i])
found = 1;
}
-   spin_lock(&io->lock);
+   spin_lock_irqsave(&io->lock, flags);
}
 
/* on the last completion, signal usb_sg_wait() */
@@ -332,7 +333,7 @@ static void sg_complete(struct urb *urb)
if (!io->count)
complete(&io->complete);
 
-   spin_unlock(&io->lock);
+   spin_unlock_irqrestore(&io->lock, flags);
 }
 
 
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 10/12] usb: serial: symbolserial: use irqsave() in USB's complete callback

2018-06-23 Thread Sebastian Andrzej Siewior 
From: John Ogness 

The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Johan Hovold 
Cc: Greg Kroah-Hartman 
Signed-off-by: John Ogness 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/serial/symbolserial.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/symbolserial.c 
b/drivers/usb/serial/symbolserial.c
index cd2f8dc8b58c..6ca24e86f686 100644
--- a/drivers/usb/serial/symbolserial.c
+++ b/drivers/usb/serial/symbolserial.c
@@ -35,6 +35,7 @@ static void symbol_int_callback(struct urb *urb)
struct symbol_private *priv = usb_get_serial_port_data(port);
unsigned char *data = urb->transfer_buffer;
int status = urb->status;
+   unsigned long flags;
int result;
int data_length;
 
@@ -73,7 +74,7 @@ static void symbol_int_callback(struct urb *urb)
}
 
 exit:
-   spin_lock(&priv->lock);
+   spin_lock_irqsave(&priv->lock, flags);
 
/* Continue trying to always read if we should */
if (!priv->throttled) {
@@ -84,7 +85,7 @@ static void symbol_int_callback(struct urb *urb)
__func__, result);
} else
priv->actually_throttled = true;
-   spin_unlock(&priv->lock);
+   spin_unlock_irqrestore(&priv->lock, flags);
 }
 
 static int symbol_open(struct tty_struct *tty, struct usb_serial_port *port)
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 06/12] usb: serial: mos7840: use irqsave() in USB's complete callback

2018-06-23 Thread Sebastian Andrzej Siewior 
From: John Ogness 

The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Johan Hovold 
Cc: Greg Kroah-Hartman 
Signed-off-by: John Ogness 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/serial/mos7840.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index fdceb46d9fc6..4efffbbef5ae 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -802,18 +802,19 @@ static void mos7840_bulk_out_data_callback(struct urb 
*urb)
struct moschip_port *mos7840_port;
struct usb_serial_port *port;
int status = urb->status;
+   unsigned long flags;
int i;
 
mos7840_port = urb->context;
port = mos7840_port->port;
-   spin_lock(&mos7840_port->pool_lock);
+   spin_lock_irqsave(&mos7840_port->pool_lock, flags);
for (i = 0; i < NUM_URBS; i++) {
if (urb == mos7840_port->write_urb_pool[i]) {
mos7840_port->busy[i] = 0;
break;
}
}
-   spin_unlock(&mos7840_port->pool_lock);
+   spin_unlock_irqrestore(&mos7840_port->pool_lock, flags);
 
if (status) {
dev_dbg(&port->dev, "nonzero write bulk status received:%d\n", 
status);
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 08/12] usb: serial: sierra: disable irq's for portdata lock

2018-06-23 Thread Sebastian Andrzej Siewior 
From: John Ogness 

The portdata spinlock can be taken in interrupt context (via
sierra_outdat_callback()).
Disable interrupts when taking the portdata spinlock.

Cc: Johan Hovold 
Cc: Greg Kroah-Hartman 
Signed-off-by: John Ogness 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/serial/sierra.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/sierra.c b/drivers/usb/serial/sierra.c
index d189f953c891..55956a638f5b 100644
--- a/drivers/usb/serial/sierra.c
+++ b/drivers/usb/serial/sierra.c
@@ -770,9 +770,9 @@ static void sierra_close(struct usb_serial_port *port)
kfree(urb->transfer_buffer);
usb_free_urb(urb);
usb_autopm_put_interface_async(serial->interface);
-   spin_lock(&portdata->lock);
+   spin_lock_irq(&portdata->lock);
portdata->outstanding_urbs--;
-   spin_unlock(&portdata->lock);
+   spin_unlock_irq(&portdata->lock);
}
 
sierra_stop_rx_urbs(port);
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 07/12] usb: serial: quatech2: use irqsave() in USB's complete callback

2018-06-23 Thread Sebastian Andrzej Siewior 
From: John Ogness 

The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Johan Hovold 
Cc: Greg Kroah-Hartman 
Signed-off-by: John Ogness 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/serial/quatech2.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/quatech2.c b/drivers/usb/serial/quatech2.c
index 958e12e1e7c7..f7962982e204 100644
--- a/drivers/usb/serial/quatech2.c
+++ b/drivers/usb/serial/quatech2.c
@@ -621,16 +621,17 @@ static void qt2_write_bulk_callback(struct urb *urb)
 {
struct usb_serial_port *port;
struct qt2_port_private *port_priv;
+   unsigned long flags;
 
port = urb->context;
port_priv = usb_get_serial_port_data(port);
 
-   spin_lock(&port_priv->urb_lock);
+   spin_lock_irqsave(&port_priv->urb_lock, flags);
 
port_priv->urb_in_use = false;
usb_serial_port_softint(port);
 
-   spin_unlock(&port_priv->urb_lock);
+   spin_unlock_irqrestore(&port_priv->urb_lock, flags);
 
 }
 
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 09/12] usb: serial: sierra: use irqsave() in USB's complete callback

2018-06-23 Thread Sebastian Andrzej Siewior 
From: John Ogness 

The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Johan Hovold 
Cc: Greg Kroah-Hartman 
Signed-off-by: John Ogness 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/serial/sierra.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/serial/sierra.c b/drivers/usb/serial/sierra.c
index 55956a638f5b..a43263a0edd8 100644
--- a/drivers/usb/serial/sierra.c
+++ b/drivers/usb/serial/sierra.c
@@ -409,6 +409,7 @@ static void sierra_outdat_callback(struct urb *urb)
struct sierra_port_private *portdata = usb_get_serial_port_data(port);
struct sierra_intf_private *intfdata;
int status = urb->status;
+   unsigned long flags;
 
intfdata = usb_get_serial_data(port->serial);
 
@@ -419,12 +420,12 @@ static void sierra_outdat_callback(struct urb *urb)
dev_dbg(&port->dev, "%s - nonzero write bulk status "
"received: %d\n", __func__, status);
 
-   spin_lock(&portdata->lock);
+   spin_lock_irqsave(&portdata->lock, flags);
--portdata->outstanding_urbs;
-   spin_unlock(&portdata->lock);
-   spin_lock(&intfdata->susp_lock);
+   spin_unlock_irqrestore(&portdata->lock, flags);
+   spin_lock_irqsave(&intfdata->susp_lock, flags);
--intfdata->in_flight;
-   spin_unlock(&intfdata->susp_lock);
+   spin_unlock_irqrestore(&intfdata->susp_lock, flags);
 
usb_serial_port_softint(port);
 }
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 12/12] usb: serial: usb_wwan: use irqsave() in USB's complete callback

2018-06-23 Thread Sebastian Andrzej Siewior 
From: John Ogness 

The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Johan Hovold 
Cc: Greg Kroah-Hartman 
Signed-off-by: John Ogness 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/serial/usb_wwan.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
index 107e64c42e94..912472f26e4f 100644
--- a/drivers/usb/serial/usb_wwan.c
+++ b/drivers/usb/serial/usb_wwan.c
@@ -326,6 +326,7 @@ static void usb_wwan_outdat_callback(struct urb *urb)
struct usb_serial_port *port;
struct usb_wwan_port_private *portdata;
struct usb_wwan_intf_private *intfdata;
+   unsigned long flags;
int i;
 
port = urb->context;
@@ -334,9 +335,9 @@ static void usb_wwan_outdat_callback(struct urb *urb)
usb_serial_port_softint(port);
usb_autopm_put_interface_async(port->serial->interface);
portdata = usb_get_serial_port_data(port);
-   spin_lock(&intfdata->susp_lock);
+   spin_lock_irqsave(&intfdata->susp_lock, flags);
intfdata->in_flight--;
-   spin_unlock(&intfdata->susp_lock);
+   spin_unlock_irqrestore(&intfdata->susp_lock, flags);
 
for (i = 0; i < N_OUT_URB; ++i) {
if (portdata->out_urbs[i] == urb) {
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 11/12] usb: serial: ti_usb_3410_5052: use irqsave() in USB's complete callback

2018-06-23 Thread Sebastian Andrzej Siewior 
From: John Ogness 

The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Johan Hovold 
Cc: Greg Kroah-Hartman 
Signed-off-by: John Ogness 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/serial/ti_usb_3410_5052.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c 
b/drivers/usb/serial/ti_usb_3410_5052.c
index 6b22857f6e52..3010878f7f8e 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -1215,6 +1215,7 @@ static void ti_bulk_in_callback(struct urb *urb)
struct usb_serial_port *port = tport->tp_port;
struct device *dev = &urb->dev->dev;
int status = urb->status;
+   unsigned long flags;
int retval = 0;
 
switch (status) {
@@ -1247,20 +1248,20 @@ static void ti_bulk_in_callback(struct urb *urb)
__func__);
else
ti_recv(port, urb->transfer_buffer, urb->actual_length);
-   spin_lock(&tport->tp_lock);
+   spin_lock_irqsave(&tport->tp_lock, flags);
port->icount.rx += urb->actual_length;
-   spin_unlock(&tport->tp_lock);
+   spin_unlock_irqrestore(&tport->tp_lock, flags);
}
 
 exit:
/* continue to read unless stopping */
-   spin_lock(&tport->tp_lock);
+   spin_lock_irqsave(&tport->tp_lock, flags);
if (tport->tp_read_urb_state == TI_READ_URB_RUNNING)
retval = usb_submit_urb(urb, GFP_ATOMIC);
else if (tport->tp_read_urb_state == TI_READ_URB_STOPPING)
tport->tp_read_urb_state = TI_READ_URB_STOPPED;
 
-   spin_unlock(&tport->tp_lock);
+   spin_unlock_irqrestore(&tport->tp_lock, flags);
if (retval)
dev_err(dev, "%s - resubmit read urb failed, %d\n",
__func__, retval);
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 01/12] usb: serial: cyberjack: use irqsave() in USB's complete callback

2018-06-23 Thread Sebastian Andrzej Siewior 
From: John Ogness 

The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Johan Hovold 
Cc: Greg Kroah-Hartman 
Signed-off-by: John Ogness 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/serial/cyberjack.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/serial/cyberjack.c b/drivers/usb/serial/cyberjack.c
index dc67a2eb98d7..ebd76ab07b72 100644
--- a/drivers/usb/serial/cyberjack.c
+++ b/drivers/usb/serial/cyberjack.c
@@ -255,6 +255,7 @@ static void cyberjack_read_int_callback(struct urb *urb)
struct device *dev = &port->dev;
unsigned char *data = urb->transfer_buffer;
int status = urb->status;
+   unsigned long flags;
int result;
 
/* the urb might have been killed. */
@@ -270,13 +271,13 @@ static void cyberjack_read_int_callback(struct urb *urb)
/* This is a announcement of coming bulk_ins. */
unsigned short size = ((unsigned short)data[3]<<8)+data[2]+3;
 
-   spin_lock(&priv->lock);
+   spin_lock_irqsave(&priv->lock, flags);
 
old_rdtodo = priv->rdtodo;
 
if (old_rdtodo > SHRT_MAX - size) {
dev_dbg(dev, "To many bulk_in urbs to do.\n");
-   spin_unlock(&priv->lock);
+   spin_unlock_irqrestore(&priv->lock, flags);
goto resubmit;
}
 
@@ -285,7 +286,7 @@ static void cyberjack_read_int_callback(struct urb *urb)
 
dev_dbg(dev, "%s - rdtodo: %d\n", __func__, priv->rdtodo);
 
-   spin_unlock(&priv->lock);
+   spin_unlock_irqrestore(&priv->lock, flags);
 
if (!old_rdtodo) {
result = usb_submit_urb(port->read_urb, GFP_ATOMIC);
@@ -309,6 +310,7 @@ static void cyberjack_read_bulk_callback(struct urb *urb)
struct cyberjack_private *priv = usb_get_serial_port_data(port);
struct device *dev = &port->dev;
unsigned char *data = urb->transfer_buffer;
+   unsigned long flags;
short todo;
int result;
int status = urb->status;
@@ -325,7 +327,7 @@ static void cyberjack_read_bulk_callback(struct urb *urb)
tty_flip_buffer_push(&port->port);
}
 
-   spin_lock(&priv->lock);
+   spin_lock_irqsave(&priv->lock, flags);
 
/* Reduce urbs to do by one. */
priv->rdtodo -= urb->actual_length;
@@ -334,7 +336,7 @@ static void cyberjack_read_bulk_callback(struct urb *urb)
priv->rdtodo = 0;
todo = priv->rdtodo;
 
-   spin_unlock(&priv->lock);
+   spin_unlock_irqrestore(&priv->lock, flags);
 
dev_dbg(dev, "%s - rdtodo: %d\n", __func__, todo);
 
@@ -354,6 +356,7 @@ static void cyberjack_write_bulk_callback(struct urb *urb)
struct cyberjack_private *priv = usb_get_serial_port_data(port);
struct device *dev = &port->dev;
int status = urb->status;
+   unsigned long flags;
 
set_bit(0, &port->write_urbs_free);
if (status) {
@@ -362,7 +365,7 @@ static void cyberjack_write_bulk_callback(struct urb *urb)
return;
}
 
-   spin_lock(&priv->lock);
+   spin_lock_irqsave(&priv->lock, flags);
 
/* only do something if we have more data to send */
if (priv->wrfilled) {
@@ -406,7 +409,7 @@ static void cyberjack_write_bulk_callback(struct urb *urb)
}
 
 exit:
-   spin_unlock(&priv->lock);
+   spin_unlock_irqrestore(&priv->lock, flags);
usb_serial_port_softint(port);
 }
 
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 03/12] usb: serial: io_edgeport: use irqsave() in USB's complete callback

2018-06-23 Thread Sebastian Andrzej Siewior 
From: John Ogness 

The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Johan Hovold 
Cc: Greg Kroah-Hartman 
Signed-off-by: John Ogness 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/serial/io_edgeport.c | 17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
index 17283f4b4779..97c69d373ca6 100644
--- a/drivers/usb/serial/io_edgeport.c
+++ b/drivers/usb/serial/io_edgeport.c
@@ -648,6 +648,7 @@ static void edge_interrupt_callback(struct urb *urb)
struct usb_serial_port *port;
unsigned char *data = urb->transfer_buffer;
int length = urb->actual_length;
+   unsigned long flags;
int bytes_avail;
int position;
int txCredits;
@@ -679,7 +680,7 @@ static void edge_interrupt_callback(struct urb *urb)
if (length > 1) {
bytes_avail = data[0] | (data[1] << 8);
if (bytes_avail) {
-   spin_lock(&edge_serial->es_lock);
+   spin_lock_irqsave(&edge_serial->es_lock, flags);
edge_serial->rxBytesAvail += bytes_avail;
dev_dbg(dev,
"%s - bytes_avail=%d, rxBytesAvail=%d, 
read_in_progress=%d\n",
@@ -702,7 +703,8 @@ static void edge_interrupt_callback(struct urb *urb)
edge_serial->read_in_progress = 
false;
}
}
-   spin_unlock(&edge_serial->es_lock);
+   spin_unlock_irqrestore(&edge_serial->es_lock,
+  flags);
}
}
/* grab the txcredits for the ports if available */
@@ -715,9 +717,11 @@ static void edge_interrupt_callback(struct urb *urb)
port = edge_serial->serial->port[portNumber];
edge_port = usb_get_serial_port_data(port);
if (edge_port->open) {
-   spin_lock(&edge_port->ep_lock);
+   spin_lock_irqsave(&edge_port->ep_lock,
+ flags);
edge_port->txCredits += txCredits;
-   spin_unlock(&edge_port->ep_lock);
+   
spin_unlock_irqrestore(&edge_port->ep_lock,
+  flags);
dev_dbg(dev, "%s - txcredits for port%d 
= %d\n",
__func__, portNumber,
edge_port->txCredits);
@@ -758,6 +762,7 @@ static void edge_bulk_in_callback(struct urb *urb)
int retval;
__u16   raw_data_length;
int status = urb->status;
+   unsigned long flags;
 
if (status) {
dev_dbg(&urb->dev->dev, "%s - nonzero read bulk status 
received: %d\n",
@@ -777,7 +782,7 @@ static void edge_bulk_in_callback(struct urb *urb)
 
usb_serial_debug_data(dev, __func__, raw_data_length, data);
 
-   spin_lock(&edge_serial->es_lock);
+   spin_lock_irqsave(&edge_serial->es_lock, flags);
 
/* decrement our rxBytes available by the number that we just got */
edge_serial->rxBytesAvail -= raw_data_length;
@@ -801,7 +806,7 @@ static void edge_bulk_in_callback(struct urb *urb)
edge_serial->read_in_progress = false;
}
 
-   spin_unlock(&edge_serial->es_lock);
+   spin_unlock_irqrestore(&edge_serial->es_lock, flags);
 }
 
 
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 00/12] usb: serial: Use irqsave in USB's complete callback

2018-06-23 Thread Sebastian Andrzej Siewior 
This is about using _irqsave() primitives in the completion callback in
order to get rid of local_irq_save() in __usb_hcd_giveback_urb().

Sebastian 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/12] usb: serial: digi_acceleport: use irqsave() in USB's complete callback

2018-06-23 Thread Sebastian Andrzej Siewior 
From: John Ogness 

The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Johan Hovold 
Cc: Greg Kroah-Hartman 
Signed-off-by: John Ogness 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/serial/digi_acceleport.c | 23 +--
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/serial/digi_acceleport.c 
b/drivers/usb/serial/digi_acceleport.c
index b0526786fb02..ae512fed08af 100644
--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -984,6 +984,7 @@ static void digi_write_bulk_callback(struct urb *urb)
struct usb_serial *serial;
struct digi_port *priv;
struct digi_serial *serial_priv;
+   unsigned long flags;
int ret = 0;
int status = urb->status;
 
@@ -1004,15 +1005,15 @@ static void digi_write_bulk_callback(struct urb *urb)
/* handle oob callback */
if (priv->dp_port_num == serial_priv->ds_oob_port_num) {
dev_dbg(&port->dev, "digi_write_bulk_callback: oob callback\n");
-   spin_lock(&priv->dp_port_lock);
+   spin_lock_irqsave(&priv->dp_port_lock, flags);
priv->dp_write_urb_in_use = 0;
wake_up_interruptible(&port->write_wait);
-   spin_unlock(&priv->dp_port_lock);
+   spin_unlock_irqrestore(&priv->dp_port_lock, flags);
return;
}
 
/* try to send any buffered data on this port */
-   spin_lock(&priv->dp_port_lock);
+   spin_lock_irqsave(&priv->dp_port_lock, flags);
priv->dp_write_urb_in_use = 0;
if (priv->dp_out_buf_len > 0) {
*((unsigned char *)(port->write_urb->transfer_buffer))
@@ -1035,7 +1036,7 @@ static void digi_write_bulk_callback(struct urb *urb)
/* lost the race in write_chan(). */
schedule_work(&priv->dp_wakeup_work);
 
-   spin_unlock(&priv->dp_port_lock);
+   spin_unlock_irqrestore(&priv->dp_port_lock, flags);
if (ret && ret != -EPERM)
dev_err_console(port,
"%s: usb_submit_urb failed, ret=%d, port=%d\n",
@@ -1381,6 +1382,7 @@ static int digi_read_inb_callback(struct urb *urb)
struct usb_serial_port *port = urb->context;
struct digi_port *priv = usb_get_serial_port_data(port);
unsigned char *buf = urb->transfer_buffer;
+   unsigned long flags;
int opcode;
int len;
int port_status;
@@ -1407,7 +1409,7 @@ static int digi_read_inb_callback(struct urb *urb)
return -1;
}
 
-   spin_lock(&priv->dp_port_lock);
+   spin_lock_irqsave(&priv->dp_port_lock, flags);
 
/* check for throttle; if set, do not resubmit read urb */
/* indicate the read chain needs to be restarted on unthrottle */
@@ -1444,7 +1446,7 @@ static int digi_read_inb_callback(struct urb *urb)
tty_flip_buffer_push(&port->port);
}
}
-   spin_unlock(&priv->dp_port_lock);
+   spin_unlock_irqrestore(&priv->dp_port_lock, flags);
 
if (opcode == DIGI_CMD_RECEIVE_DISABLE)
dev_dbg(&port->dev, "%s: got RECEIVE_DISABLE\n", __func__);
@@ -1474,6 +1476,7 @@ static int digi_read_oob_callback(struct urb *urb)
struct digi_port *priv = usb_get_serial_port_data(port);
unsigned char *buf = urb->transfer_buffer;
int opcode, line, status, val;
+   unsigned long flags;
int i;
unsigned int rts;
 
@@ -1506,7 +1509,7 @@ static int digi_read_oob_callback(struct urb *urb)
rts = C_CRTSCTS(tty);
 
if (tty && opcode == DIGI_CMD_READ_INPUT_SIGNALS) {
-   spin_lock(&priv->dp_port_lock);
+   spin_lock_irqsave(&priv->dp_port_lock, flags);
/* convert from digi flags to termiox flags */
if (val & DIGI_READ_INPUT_SIGNALS_CTS) {
priv->dp_modem_signals |= TIOCM_CTS;
@@ -1530,12 +1533,12 @@ static int digi_read_oob_callback(struct urb *urb)
else
priv->dp_modem_signals &= ~TIOCM_CD;
 
-   spin_unlock(&priv->dp_port_lock);
+   spin_unlock_irqrestore(&priv->dp_port_lock, flags);
} else if (opcode == DIGI_CMD_TRAN

[PATCH 05/12] usb: serial: mos7720: use irqsave() in USB's complete callback

2018-06-23 Thread Sebastian Andrzej Siewior 
From: John Ogness 

The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Johan Hovold 
Cc: Greg Kroah-Hartman 
Signed-off-by: John Ogness 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/serial/mos7720.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/mos7720.c b/drivers/usb/serial/mos7720.c
index bd57630e67e2..8f11e759ad61 100644
--- a/drivers/usb/serial/mos7720.c
+++ b/drivers/usb/serial/mos7720.c
@@ -340,14 +340,15 @@ static void async_complete(struct urb *urb)
 {
struct urbtracker *urbtrack = urb->context;
int status = urb->status;
+   unsigned long flags;
 
if (unlikely(status))
dev_dbg(&urb->dev->dev, "%s - nonzero urb status received: 
%d\n", __func__, status);
 
/* remove the urbtracker from the active_urbs list */
-   spin_lock(&urbtrack->mos_parport->listlock);
+   spin_lock_irqsave(&urbtrack->mos_parport->listlock, flags);
list_del(&urbtrack->urblist_entry);
-   spin_unlock(&urbtrack->mos_parport->listlock);
+   spin_unlock_irqrestore(&urbtrack->mos_parport->listlock, flags);
kref_put(&urbtrack->ref_count, destroy_urbtracker);
 }
 
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 04/12] usb: serial: io_ti: use irqsave() in USB's complete callback

2018-06-23 Thread Sebastian Andrzej Siewior 
From: John Ogness 

The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Johan Hovold 
Cc: Greg Kroah-Hartman 
Signed-off-by: John Ogness 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/serial/io_ti.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
index 0fbadb37c104..6d1d6efa3055 100644
--- a/drivers/usb/serial/io_ti.c
+++ b/drivers/usb/serial/io_ti.c
@@ -1729,6 +1729,7 @@ static void edge_bulk_in_callback(struct urb *urb)
struct edgeport_port *edge_port = urb->context;
struct device *dev = &edge_port->port->dev;
unsigned char *data = urb->transfer_buffer;
+   unsigned long flags;
int retval = 0;
int port_number;
int status = urb->status;
@@ -1780,13 +1781,13 @@ static void edge_bulk_in_callback(struct urb *urb)
 
 exit:
/* continue read unless stopped */
-   spin_lock(&edge_port->ep_lock);
+   spin_lock_irqsave(&edge_port->ep_lock, flags);
if (edge_port->ep_read_urb_state == EDGE_READ_URB_RUNNING)
retval = usb_submit_urb(urb, GFP_ATOMIC);
else if (edge_port->ep_read_urb_state == EDGE_READ_URB_STOPPING)
edge_port->ep_read_urb_state = EDGE_READ_URB_STOPPED;
 
-   spin_unlock(&edge_port->ep_lock);
+   spin_unlock_irqrestore(&edge_port->ep_lock, flags);
if (retval)
dev_err(dev, "%s - usb_submit_urb failed with result %d\n", 
__func__, retval);
 }
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/9] ALSA: usb: caiaq: use usb_fill_int_urb()

2018-06-22 Thread Sebastian Andrzej Siewior 
On 2018-06-22 12:01:13 [+0200], Daniel Mack wrote:
> Hmm, there is no such thing as usb_fill_iso_urb() in my tree, are you going
> to add that?

Yes.

> The only part that needs attention is the interval parameter, which is
> passed to usb_fill_int_urb() as 1 now, and hence urb->interval will also be
> 1, just like the open-coded version before. Unless I miss anything, your
> conversion should work, but I haven't tested it yet.

It should work in most cases. The point is that the argument expects
bInterval (from the endpoint) which has a different encoding on FS vs
HS/SS for INTR endpoints but not for ISOC endpoints and I got this wrong
initially.

> But I agree the function name is misleading, so we should probably get a
> usb_fill_iso_urb() and use it where appropriate. AFAICS, it will be
> identical to usb_fill_bulk_urb(), as the endpoint type is encoded in the
> pipe. Maybe it's worth adding a check?

What check?
it should be identical to INTR without the speed check (always the HS
version should be used). I need to check if it makes sense to extend the
parameters to cover ->number_of_packets and so on.
Any way, I plan to first RFC the function, land it upstream and then
convert the users.

> Thanks,
> Daniel

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/9] ALSA: usb: caiaq: use usb_fill_int_urb()

2018-06-21 Thread Sebastian Andrzej Siewior 
On 2018-06-21 22:19:32 [+0200], Daniel Mack wrote:
> On Tuesday, June 19, 2018 11:55 PM, Sebastian Andrzej Siewior wrote:
> > Using usb_fill_int_urb() helps to find code which initializes an
> > URB. A grep for members of the struct (like ->complete) reveal lots
> > of other things, too.
> 
> Acked-by: Daniel Mack 

nope, please don't.
Takashi, please ignore the usb_fill_.* patches. I will be doing another
spin with usb_fill_iso_urb() instead.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Bluetooth: btusb: use irqsave() in URB's complete callback

2018-06-21 Thread Sebastian Andrzej Siewior 
On 2018-06-21 14:43:41 [+0200], Marcel Holtmann wrote:
> Hi Sebastian,
Hi Marcel,

> > The USB completion callback does not disable interrupts while acquiring
> > the ->lock. We want to remove the local_irq_disable() invocation from
> > __usb_hcd_giveback_urb() and therefore it is required for the callback
> > handler to disable the interrupts while acquiring the lock.
> > The callback may be invoked either in IRQ or BH context depending on the
> > USB host controller.
> > Use the _irqsave variant of the locking primitives.
> > 
> > Cc: Marcel Holtmann 
> > Cc: Johan Hedberg 
> > Cc: linux-blueto...@vger.kernel.org
> > Signed-off-by: Sebastian Andrzej Siewior 
> > ---
> > drivers/bluetooth/btusb.c | 20 
> > 1 file changed, 12 insertions(+), 8 deletions(-)
> 
> can I get an ACK from someone ensuring that this is the direction we are 
> going with the USB host controllers?
+Alan.

EHCI completes in BH since v3.12-rc1. In order to get rid of that
local_irq_save() in USB core code I need to make sure that the USB
device driver(s) use irqsave primitives. See
  
https://lkml.kernel.org/r/pine.lnx.4.44l0.1806011629140.1404-100...@iolanthe.rowland.org

> Regards
> 
> Marcel

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()

2018-06-20 Thread Sebastian Andrzej Siewior 
On 2018-06-20 17:38:44 [+0200], To Takashi Iwai wrote:
> okay, as you wish:

I'm sorry, I compiled one patch and send the other. Here is the fixed
one.

> > thanks,
> > 
> > Takashi
> 
--- >8
Subject: [PATCH 8/9 v4] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()

Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.

The "&& !(*purb)->transfer_buffer" check has been removed because the
URB has been freshly allocated a few lines above so ->transfer_buffer
has to be NULL here.
The `dev' and `transfer_size' assignments have been moved from
usX2Y_urbs_start() to usX2Y_urbs_allocate() because they don't change
overtime.
The multiplication via check_mul_overflow() has been extracted from
kmalloc_array() to avoid two multiplication (one with overflow check and
one without for the length argument). This requires to change the type
`maxpacksize' to int so there is only one type involved in the
multiplication.

Cc: Jaroslav Kysela 
Cc: Takashi Iwai 
Signed-off-by: Sebastian Andrzej Siewior 
---
 sound/usb/usx2y/usbusx2yaudio.c | 23 +++
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
index 2b833054e3b0..de3a21444db3 100644
--- a/sound/usb/usx2y/usbusx2yaudio.c
+++ b/sound/usb/usx2y/usbusx2yaudio.c
@@ -425,6 +425,9 @@ static int usX2Y_urbs_allocate(struct snd_usX2Y_substream 
*subs)
/* allocate and initialize data urbs */
for (i = 0; i < NRURBS; i++) {
struct urb **purb = subs->urb + i;
+   void *buf = NULL;
+   int len = 0;
+
if (*purb) {
usb_kill_urb(*purb);
continue;
@@ -434,22 +437,20 @@ static int usX2Y_urbs_allocate(struct snd_usX2Y_substream 
*subs)
usX2Y_urbs_release(subs);
return -ENOMEM;
}
-   if (!is_playback && !(*purb)->transfer_buffer) {
+
+   if (!is_playback) {
/* allocate a capture buffer per urb */
-   (*purb)->transfer_buffer =
-   kmalloc_array(subs->maxpacksize,
- nr_of_packs(), GFP_KERNEL);
-   if (NULL == (*purb)->transfer_buffer) {
+   buf = kmalloc_array(subs->maxpacksize, nr_of_packs(),
+   GFP_KERNEL);
+   if (!buf) {
usX2Y_urbs_release(subs);
return -ENOMEM;
}
+   len = subs->maxpacksize * nr_of_packs();
}
-   (*purb)->dev = dev;
-   (*purb)->pipe = pipe;
+   usb_fill_int_urb(*purb, dev, pipe, buf, len,
+i_usX2Y_subs_startup, subs, 1);
(*purb)->number_of_packets = nr_of_packs();
-   (*purb)->context = subs;
-   (*purb)->interval = 1;
-   (*purb)->complete = i_usX2Y_subs_startup;
}
return 0;
 }
@@ -485,12 +486,10 @@ static int usX2Y_urbs_start(struct snd_usX2Y_substream 
*subs)
unsigned long pack;
if (0 == i)
atomic_set(&subs->state, state_STARTING3);
-   urb->dev = usX2Y->dev;
for (pack = 0; pack < nr_of_packs(); pack++) {
urb->iso_frame_desc[pack].offset = 
subs->maxpacksize * pack;
urb->iso_frame_desc[pack].length = 
subs->maxpacksize;
}
-   urb->transfer_buffer_length = subs->maxpacksize * 
nr_of_packs(); 
if ((err = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
snd_printk (KERN_ERR "cannot submit datapipe 
for urb %d, err = %d\n", i, err);
err = -EPIPE;
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()

2018-06-20 Thread Sebastian Andrzej Siewior 
On 2018-06-20 16:52:45 [+0200], Takashi Iwai wrote:
> > but then we end up with two multiplications.
> 
> Yes, but it's no hot path, and the code won't bigger.
> And I bet you won't notice any performance lost by that :)
> 
> > What about pulling the
> > overflow-mul macro out of malloc_array() and doing this instead:
> 
> Well, it's neither smaller nor more readable...

okay, as you wish:

> thanks,
> 
> Takashi

--- >8

Subject: [PATCH 8/9 v3] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()

Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.

The "&& !(*purb)->transfer_buffer" check has been removed because the
URB has been freshly allocated a few lines above so ->transfer_buffer
has to be NULL here.
The `dev' and `transfer_size' assignments have been moved from
usX2Y_urbs_start() to usX2Y_urbs_allocate() because they don't change
overtime.
The multiplication via check_mul_overflow() has been extracted from
kmalloc_array() to avoid two multiplication (one with overflow check and
one without for the length argument). This requires to change the type
`maxpacksize' to int so there is only one type involved in the
multiplication.

Cc: Jaroslav Kysela 
Cc: Takashi Iwai 
Signed-off-by: Sebastian Andrzej Siewior 
---
 sound/usb/usx2y/usbusx2yaudio.c | 23 +++
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
index 2b833054e3b0..ee8d21a784fd 100644
--- a/sound/usb/usx2y/usbusx2yaudio.c
+++ b/sound/usb/usx2y/usbusx2yaudio.c
@@ -425,6 +425,9 @@ static int usX2Y_urbs_allocate(struct snd_usX2Y_substream 
*subs)
/* allocate and initialize data urbs */
for (i = 0; i < NRURBS; i++) {
struct urb **purb = subs->urb + i;
+   void *buf = NULL;
+   int len = 0;
+
if (*purb) {
usb_kill_urb(*purb);
continue;
@@ -434,22 +437,20 @@ static int usX2Y_urbs_allocate(struct snd_usX2Y_substream 
*subs)
usX2Y_urbs_release(subs);
return -ENOMEM;
}
-   if (!is_playback && !(*purb)->transfer_buffer) {
+
+   if (!is_playback) {
/* allocate a capture buffer per urb */
-   (*purb)->transfer_buffer =
-   kmalloc_array(subs->maxpacksize,
- nr_of_packs(), GFP_KERNEL);
-   if (NULL == (*purb)->transfer_buffer) {
+   buf = kmalloc_array(subs->maxpacksize, nr_of_packs,
+   GFP_KERNEL);
+   if (!buf) {
usX2Y_urbs_release(subs);
return -ENOMEM;
}
+   len = subs->maxpacksize * nr_of_packs;
}
-   (*purb)->dev = dev;
-   (*purb)->pipe = pipe;
+   usb_fill_int_urb(*purb, dev, pipe, buf, len,
+i_usX2Y_subs_startup, subs, 1);
(*purb)->number_of_packets = nr_of_packs();
-   (*purb)->context = subs;
-   (*purb)->interval = 1;
-   (*purb)->complete = i_usX2Y_subs_startup;
}
return 0;
 }
@@ -485,12 +486,10 @@ static int usX2Y_urbs_start(struct snd_usX2Y_substream 
*subs)
unsigned long pack;
if (0 == i)
atomic_set(&subs->state, state_STARTING3);
-   urb->dev = usX2Y->dev;
for (pack = 0; pack < nr_of_packs(); pack++) {
urb->iso_frame_desc[pack].offset = 
subs->maxpacksize * pack;
urb->iso_frame_desc[pack].length = 
subs->maxpacksize;
}
-   urb->transfer_buffer_length = subs->maxpacksize * 
nr_of_packs(); 
if ((err = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
snd_printk (KERN_ERR "cannot submit datapipe 
for urb %d, err = %d\n", i, err);
err = -EPIPE;
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()

2018-06-20 Thread Sebastian Andrzej Siewior 
On 2018-06-20 14:51:12 [+0200], Takashi Iwai wrote:
> On Tue, 19 Jun 2018 23:55:20 +0200,
> Sebastian Andrzej Siewior wrote:
> > -   (*purb)->transfer_buffer =
> > -   kmalloc_array(subs->maxpacksize,
> > - nr_of_packs(), GFP_KERNEL);
> > -   if (NULL == (*purb)->transfer_buffer) {
> > +   len = subs->maxpacksize * nr_of_packs();
> > +   buf = kmalloc(len, GFP_KERNEL);
> 
> I'd keep kmalloc_array() as is, and just put subs->maxpacksize *
> nr_of_packs() in usb_fill_int_urb().  Otherwise it's a step backward.

but then we end up with two multiplications. What about pulling the
overflow-mul macro out of malloc_array() and doing this instead:

--- >8

Subject: [PATCH 8/9 v2] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()

Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.

The "&& !(*purb)->transfer_buffer" check has been removed because the
URB has been freshly allocated a few lines above so ->transfer_buffer
has to be NULL here.
The `dev' and `transfer_size' assignments have been moved from
usX2Y_urbs_start() to usX2Y_urbs_allocate() because they don't change
overtime.
The multiplication via check_mul_overflow() has been extracted from
kmalloc_array() to avoid two multiplication (one with overflow check and
one without for the length argument). This requires to change the type
`maxpacksize' to int so there is only one type involved in the
multiplication.

Cc: Jaroslav Kysela 
Cc: Takashi Iwai 
Signed-off-by: Sebastian Andrzej Siewior 
---
 sound/usb/usx2y/usbusx2y.h  |  2 +-
 sound/usb/usx2y/usbusx2yaudio.c | 38 -
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/sound/usb/usx2y/usbusx2y.h b/sound/usb/usx2y/usbusx2y.h
index e0f77172ce8f..2bec412589b4 100644
--- a/sound/usb/usx2y/usbusx2y.h
+++ b/sound/usb/usx2y/usbusx2y.h
@@ -56,7 +56,7 @@ struct snd_usX2Y_substream {
struct snd_pcm_substream *pcm_substream;
 
int endpoint;   
-   unsigned intmaxpacksize;/* max packet size in 
bytes */
+   int maxpacksize;/* max packet size in 
bytes */
 
atomic_tstate;
 #define state_STOPPED  0
diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
index 2b833054e3b0..e5580cb59858 100644
--- a/sound/usb/usx2y/usbusx2yaudio.c
+++ b/sound/usb/usx2y/usbusx2yaudio.c
@@ -425,33 +425,35 @@ static int usX2Y_urbs_allocate(struct snd_usX2Y_substream 
*subs)
/* allocate and initialize data urbs */
for (i = 0; i < NRURBS; i++) {
struct urb **purb = subs->urb + i;
+   void *buf = NULL;
+   int len = 0;
+
if (*purb) {
usb_kill_urb(*purb);
continue;
}
*purb = usb_alloc_urb(nr_of_packs(), GFP_KERNEL);
-   if (NULL == *purb) {
-   usX2Y_urbs_release(subs);
-   return -ENOMEM;
-   }
-   if (!is_playback && !(*purb)->transfer_buffer) {
+   if (NULL == *purb)
+   goto err_free;
+
+   if (!is_playback) {
/* allocate a capture buffer per urb */
-   (*purb)->transfer_buffer =
-   kmalloc_array(subs->maxpacksize,
- nr_of_packs(), GFP_KERNEL);
-   if (NULL == (*purb)->transfer_buffer) {
-   usX2Y_urbs_release(subs);
-   return -ENOMEM;
-   }
+   if (check_mul_overflow(subs->maxpacksize,
+  nr_of_packs(),
+  &len))
+   goto err_free;
+   buf = kmalloc(len, GFP_KERNEL);
+   if (!buf)
+   goto err_free;
}
-   (*purb)->dev = dev;
-   (*purb)->pipe = pipe;
+   usb_fill_int_urb(*purb, dev, pipe, buf, len,
+i_usX2Y_subs_startup, subs, 1);
(*purb)->number_of_packets = nr_of_packs();
-   (*purb)->context = subs;
-   (*purb)->interval = 1;
-   (*purb)->complete = i_usX2Y_subs_startup;
}
return 0;
+err_free:
+   usX2Y_urbs_release(subs);
+   return -ENOMEM;
 }
 
 static void usX

Re: [PATCH 4/9 v2] ALSA: usb-audio: use usb_fill_int_urb()

2018-06-20 Thread Sebastian Andrzej Siewior 
On 2018-06-20 15:21:49 [+0200], Takashi Iwai wrote:
> > usb_fill_int_urb() ensures that syncinterval is within the allowed range
> > on HS/SS. The interval value seems to come from
> > snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage
> > 1 … 4. So in order to keep the magic working I pass datainterval + 1.
> 
> This needs more explanation.  By this conversion, the interval
> computation becomes really tricky.
> 
> There are two interval calculations.  The first one is
> fp->datainterval and it's from snd_usb_parse_datainterval() as you
> mentioned.  And a tricky part is that fp->datainterval is 0 on
> FULL_SPEED.  Casually, fp->datainterval+1 will be translated to the
> correct value since (0 + 1) == (1 << 0).
> 
> OTOH, for the sync EP, it's taken from ep->syncinterval, which is set
> in snd_usb_add_endpoint().  Luckily, again, ep->syncinterval + 1 works
> for FULL_SPEED as well, as (1 + 1) == (1 << 1).

Do you want me to add your additional explanation to the description?
 
> thanks,
> 
> Takashi

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/9 v2] ALSA: usb-audio: use usb_fill_int_urb()

2018-06-20 Thread Sebastian Andrzej Siewior 
Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.

data_ep_set_params() additionally sets urb->transfer_buffer_length which
was not the case earlier.
usb_fill_int_urb() ensures that syncinterval is within the allowed range
on HS/SS. The interval value seems to come from
snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage
1 … 4. So in order to keep the magic working I pass datainterval + 1.

Cc: Jaroslav Kysela 
Cc: Takashi Iwai 
Signed-off-by: Sebastian Andrzej Siewior 
---
v1…v2: description changes (spelling + usb_fill_int_urb() ensures
   "syncinterval" not data_ep_set_params())

 sound/usb/endpoint.c | 24 ++--
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index c90607ebe155..bbc02db5b417 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -772,6 +772,8 @@ static int data_ep_set_params(struct snd_usb_endpoint *ep,
/* allocate and initialize data urbs */
for (i = 0; i < ep->nurbs; i++) {
struct snd_urb_ctx *u = &ep->urb[i];
+   void *buf;
+
u->index = i;
u->ep = ep;
u->packets = urb_packs;
@@ -783,16 +785,13 @@ static int data_ep_set_params(struct snd_usb_endpoint *ep,
if (!u->urb)
goto out_of_memory;
 
-   u->urb->transfer_buffer =
-   usb_alloc_coherent(ep->chip->dev, u->buffer_size,
-  GFP_KERNEL, &u->urb->transfer_dma);
-   if (!u->urb->transfer_buffer)
+   buf = usb_alloc_coherent(ep->chip->dev, u->buffer_size,
+GFP_KERNEL, &u->urb->transfer_dma);
+   if (!buf)
goto out_of_memory;
-   u->urb->pipe = ep->pipe;
+   usb_fill_int_urb(u->urb, NULL, ep->pipe, buf, u->buffer_size,
+snd_complete_urb, u, ep->datainterval + 1);
u->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
-   u->urb->interval = 1 << ep->datainterval;
-   u->urb->context = u;
-   u->urb->complete = snd_complete_urb;
INIT_LIST_HEAD(&u->ready_list);
}
 
@@ -823,15 +822,12 @@ static int sync_ep_set_params(struct snd_usb_endpoint *ep)
u->urb = usb_alloc_urb(1, GFP_KERNEL);
if (!u->urb)
goto out_of_memory;
-   u->urb->transfer_buffer = ep->syncbuf + i * 4;
+   usb_fill_int_urb(u->urb, NULL, ep->pipe, ep->syncbuf + i * 4, 4,
+snd_complete_urb, u, ep->syncinterval + 1);
+
u->urb->transfer_dma = ep->sync_dma + i * 4;
-   u->urb->transfer_buffer_length = 4;
-   u->urb->pipe = ep->pipe;
u->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
u->urb->number_of_packets = 1;
-   u->urb->interval = 1 << ep->syncinterval;
-   u->urb->context = u;
-   u->urb->complete = snd_complete_urb;
}
 
ep->nurbs = SYNC_URBS;
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] HID: usbhid: use irqsave() in USB's complete callback

2018-06-19 Thread Sebastian Andrzej Siewior 
The USB completion callback does not disable interrupts while acquiring
the ->lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Jiri Kosina 
Cc: Benjamin Tissoires 
Cc: linux-in...@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/hid/usbhid/hid-core.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index af0e0d061b15..11103efebbaa 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -480,6 +480,7 @@ static void hid_ctrl(struct urb *urb)
 {
struct hid_device *hid = urb->context;
struct usbhid_device *usbhid = hid->driver_data;
+   unsigned long flags;
int unplug = 0, status = urb->status;
 
switch (status) {
@@ -501,7 +502,7 @@ static void hid_ctrl(struct urb *urb)
hid_warn(urb->dev, "ctrl urb status %d received\n", status);
}
 
-   spin_lock(&usbhid->lock);
+   spin_lock_irqsave(&usbhid->lock, flags);
 
if (unplug) {
usbhid->ctrltail = usbhid->ctrlhead;
@@ -511,13 +512,13 @@ static void hid_ctrl(struct urb *urb)
if (usbhid->ctrlhead != usbhid->ctrltail &&
hid_submit_ctrl(hid) == 0) {
/* Successfully submitted next urb in queue */
-   spin_unlock(&usbhid->lock);
+   spin_unlock_irqrestore(&usbhid->lock, flags);
return;
}
}
 
clear_bit(HID_CTRL_RUNNING, &usbhid->iofl);
-   spin_unlock(&usbhid->lock);
+   spin_unlock_irqrestore(&usbhid->lock, flags);
usb_autopm_put_interface_async(usbhid->intf);
wake_up(&usbhid->wait);
 }
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Bluetooth: btusb: use irqsave() in URB's complete callback

2018-06-19 Thread Sebastian Andrzej Siewior 
The USB completion callback does not disable interrupts while acquiring
the ->lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave variant of the locking primitives.

Cc: Marcel Holtmann 
Cc: Johan Hedberg 
Cc: linux-blueto...@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/bluetooth/btusb.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index f73a27ea28cc..f262163fecd5 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -509,9 +509,10 @@ static inline void btusb_free_frags(struct btusb_data 
*data)
 static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
 {
struct sk_buff *skb;
+   unsigned long flags;
int err = 0;
 
-   spin_lock(&data->rxlock);
+   spin_lock_irqsave(&data->rxlock, flags);
skb = data->evt_skb;
 
while (count) {
@@ -556,7 +557,7 @@ static int btusb_recv_intr(struct btusb_data *data, void 
*buffer, int count)
}
 
data->evt_skb = skb;
-   spin_unlock(&data->rxlock);
+   spin_unlock_irqrestore(&data->rxlock, flags);
 
return err;
 }
@@ -564,9 +565,10 @@ static int btusb_recv_intr(struct btusb_data *data, void 
*buffer, int count)
 static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
 {
struct sk_buff *skb;
+   unsigned long flags;
int err = 0;
 
-   spin_lock(&data->rxlock);
+   spin_lock_irqsave(&data->rxlock, flags);
skb = data->acl_skb;
 
while (count) {
@@ -613,7 +615,7 @@ static int btusb_recv_bulk(struct btusb_data *data, void 
*buffer, int count)
}
 
data->acl_skb = skb;
-   spin_unlock(&data->rxlock);
+   spin_unlock_irqrestore(&data->rxlock, flags);
 
return err;
 }
@@ -621,9 +623,10 @@ static int btusb_recv_bulk(struct btusb_data *data, void 
*buffer, int count)
 static int btusb_recv_isoc(struct btusb_data *data, void *buffer, int count)
 {
struct sk_buff *skb;
+   unsigned long flags;
int err = 0;
 
-   spin_lock(&data->rxlock);
+   spin_lock_irqsave(&data->rxlock, flags);
skb = data->sco_skb;
 
while (count) {
@@ -668,7 +671,7 @@ static int btusb_recv_isoc(struct btusb_data *data, void 
*buffer, int count)
}
 
data->sco_skb = skb;
-   spin_unlock(&data->rxlock);
+   spin_unlock_irqrestore(&data->rxlock, flags);
 
return err;
 }
@@ -1066,6 +1069,7 @@ static void btusb_tx_complete(struct urb *urb)
struct sk_buff *skb = urb->context;
struct hci_dev *hdev = (struct hci_dev *)skb->dev;
struct btusb_data *data = hci_get_drvdata(hdev);
+   unsigned long flags;
 
BT_DBG("%s urb %p status %d count %d", hdev->name, urb, urb->status,
   urb->actual_length);
@@ -1079,9 +1083,9 @@ static void btusb_tx_complete(struct urb *urb)
hdev->stat.err_tx++;
 
 done:
-   spin_lock(&data->txlock);
+   spin_lock_irqsave(&data->txlock, flags);
data->tx_in_flight--;
-   spin_unlock(&data->txlock);
+   spin_unlock_irqrestore(&data->txlock, flags);
 
kfree(urb->setup_packet);
 
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/9] ALSA: usb-audio: use usb_fill_int_urb()

2018-06-19 Thread Sebastian Andrzej Siewior 
Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.

data_ep_set_params() additionally sets urb->transfer_buffer_length which
was not the case earlier.
data_ep_set_params() and data_ep_set_params() ensure that syncinterval is
within the allowed range on HS/SS. The interval value seems to come from
snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage 1 … 4.
So in order to keep the magic wokring I pass datainterval + 1.

Cc: Jaroslav Kysela 
Cc: Takashi Iwai 
Signed-off-by: Sebastian Andrzej Siewior 
---
 sound/usb/endpoint.c | 24 ++--
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index c90607ebe155..bbc02db5b417 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -772,6 +772,8 @@ static int data_ep_set_params(struct snd_usb_endpoint *ep,
/* allocate and initialize data urbs */
for (i = 0; i < ep->nurbs; i++) {
struct snd_urb_ctx *u = &ep->urb[i];
+   void *buf;
+
u->index = i;
u->ep = ep;
u->packets = urb_packs;
@@ -783,16 +785,13 @@ static int data_ep_set_params(struct snd_usb_endpoint *ep,
if (!u->urb)
goto out_of_memory;
 
-   u->urb->transfer_buffer =
-   usb_alloc_coherent(ep->chip->dev, u->buffer_size,
-  GFP_KERNEL, &u->urb->transfer_dma);
-   if (!u->urb->transfer_buffer)
+   buf = usb_alloc_coherent(ep->chip->dev, u->buffer_size,
+GFP_KERNEL, &u->urb->transfer_dma);
+   if (!buf)
goto out_of_memory;
-   u->urb->pipe = ep->pipe;
+   usb_fill_int_urb(u->urb, NULL, ep->pipe, buf, u->buffer_size,
+snd_complete_urb, u, ep->datainterval + 1);
u->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
-   u->urb->interval = 1 << ep->datainterval;
-   u->urb->context = u;
-   u->urb->complete = snd_complete_urb;
INIT_LIST_HEAD(&u->ready_list);
}
 
@@ -823,15 +822,12 @@ static int sync_ep_set_params(struct snd_usb_endpoint *ep)
u->urb = usb_alloc_urb(1, GFP_KERNEL);
if (!u->urb)
goto out_of_memory;
-   u->urb->transfer_buffer = ep->syncbuf + i * 4;
+   usb_fill_int_urb(u->urb, NULL, ep->pipe, ep->syncbuf + i * 4, 4,
+snd_complete_urb, u, ep->syncinterval + 1);
+
u->urb->transfer_dma = ep->sync_dma + i * 4;
-   u->urb->transfer_buffer_length = 4;
-   u->urb->pipe = ep->pipe;
u->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
u->urb->number_of_packets = 1;
-   u->urb->interval = 1 << ep->syncinterval;
-   u->urb->context = u;
-   u->urb->complete = snd_complete_urb;
}
 
ep->nurbs = SYNC_URBS;
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 9/9] ALSA: usx2y: usx2yhwdeppcm: use usb_fill_int_urb()

2018-06-19 Thread Sebastian Andrzej Siewior 
Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.
I'm keeping the transfer-length initialisation in
usX2Y_usbpcm_urbs_start() because I am not certain if this does not
change over time.

Cc: Jaroslav Kysela 
Cc: Takashi Iwai 
Signed-off-by: Sebastian Andrzej Siewior 
---
 sound/usb/usx2y/usx2yhwdeppcm.c | 23 +--
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/sound/usb/usx2y/usx2yhwdeppcm.c b/sound/usb/usx2y/usx2yhwdeppcm.c
index 4fd9276b8e50..0a14612f2178 100644
--- a/sound/usb/usx2y/usx2yhwdeppcm.c
+++ b/sound/usb/usx2y/usx2yhwdeppcm.c
@@ -325,6 +325,8 @@ static int usX2Y_usbpcm_urbs_allocate(struct 
snd_usX2Y_substream *subs)
/* allocate and initialize data urbs */
for (i = 0; i < NRURBS; i++) {
struct urb **purb = subs->urb + i;
+   void *buf;
+
if (*purb) {
usb_kill_urb(*purb);
continue;
@@ -334,18 +336,19 @@ static int usX2Y_usbpcm_urbs_allocate(struct 
snd_usX2Y_substream *subs)
usX2Y_usbpcm_urbs_release(subs);
return -ENOMEM;
}
-   (*purb)->transfer_buffer = is_playback ?
-   subs->usX2Y->hwdep_pcm_shm->playback : (
-   subs->endpoint == 0x8 ?
-   subs->usX2Y->hwdep_pcm_shm->capture0x8 :
-   subs->usX2Y->hwdep_pcm_shm->capture0xA);
+   if (is_playback) {
+   buf = subs->usX2Y->hwdep_pcm_shm->playback;
+   } else {
+   if (subs->endpoint == 0x8)
+   buf = subs->usX2Y->hwdep_pcm_shm->capture0x8;
+   else
+   buf = subs->usX2Y->hwdep_pcm_shm->capture0xA;
+   }
+   usb_fill_int_urb(*purb, dev, pipe, buf,
+subs->maxpacksize * nr_of_packs(),
+i_usX2Y_usbpcm_subs_startup, subs, 1);
 
-   (*purb)->dev = dev;
-   (*purb)->pipe = pipe;
(*purb)->number_of_packets = nr_of_packs();
-   (*purb)->context = subs;
-   (*purb)->interval = 1;
-   (*purb)->complete = i_usX2Y_usbpcm_subs_startup;
}
return 0;
 }
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 7/9] ALSA: usb-midi: use irqsave() in USB's complete callback

2018-06-19 Thread Sebastian Andrzej Siewior 
From: John Ogness 

The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Clemens Ladisch 
Cc: Jaroslav Kysela 
Cc: Takashi Iwai 
Signed-off-by: John Ogness 
Signed-off-by: Sebastian Andrzej Siewior 
---
 sound/usb/midi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/usb/midi.c b/sound/usb/midi.c
index 2c1aaa3292bf..dcfc546d81b9 100644
--- a/sound/usb/midi.c
+++ b/sound/usb/midi.c
@@ -281,15 +281,16 @@ static void snd_usbmidi_out_urb_complete(struct urb *urb)
struct out_urb_context *context = urb->context;
struct snd_usb_midi_out_endpoint *ep = context->ep;
unsigned int urb_index;
+   unsigned long flags;
 
-   spin_lock(&ep->buffer_lock);
+   spin_lock_irqsave(&ep->buffer_lock, flags);
urb_index = context - ep->urbs;
ep->active_urbs &= ~(1 << urb_index);
if (unlikely(ep->drain_urbs)) {
ep->drain_urbs &= ~(1 << urb_index);
wake_up(&ep->drain_wait);
}
-   spin_unlock(&ep->buffer_lock);
+   spin_unlock_irqrestore(&ep->buffer_lock, flags);
if (urb->status < 0) {
int err = snd_usbmidi_urb_error(urb);
if (err < 0) {
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/9] ALSA: usb: caiaq: use usb_fill_int_urb()

2018-06-19 Thread Sebastian Andrzej Siewior 
Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.

Cc: Daniel Mack 
Cc: Jaroslav Kysela 
Cc: Takashi Iwai 
Signed-off-by: Sebastian Andrzej Siewior 
---
 sound/usb/caiaq/audio.c | 23 +++
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
index 15344d39a6cd..e10d5790099f 100644
--- a/sound/usb/caiaq/audio.c
+++ b/sound/usb/caiaq/audio.c
@@ -736,16 +736,17 @@ static struct urb **alloc_urbs(struct snd_usb_caiaqdev 
*cdev, int dir, int *ret)
}
 
for (i = 0; i < N_URBS; i++) {
+   void *buf;
+
urbs[i] = usb_alloc_urb(FRAMES_PER_URB, GFP_KERNEL);
if (!urbs[i]) {
*ret = -ENOMEM;
return urbs;
}
 
-   urbs[i]->transfer_buffer =
-   kmalloc_array(BYTES_PER_FRAME, FRAMES_PER_URB,
- GFP_KERNEL);
-   if (!urbs[i]->transfer_buffer) {
+   buf = kmalloc_array(BYTES_PER_FRAME, FRAMES_PER_URB,
+   GFP_KERNEL);
+   if (!buf) {
*ret = -ENOMEM;
return urbs;
}
@@ -758,15 +759,13 @@ static struct urb **alloc_urbs(struct snd_usb_caiaqdev 
*cdev, int dir, int *ret)
iso->length = BYTES_PER_FRAME;
}
 
-   urbs[i]->dev = usb_dev;
-   urbs[i]->pipe = pipe;
-   urbs[i]->transfer_buffer_length = FRAMES_PER_URB
-   * BYTES_PER_FRAME;
-   urbs[i]->context = &cdev->data_cb_info[i];
-   urbs[i]->interval = 1;
+   usb_fill_int_urb(urbs[i], usb_dev, pipe, buf,
+FRAMES_PER_URB * BYTES_PER_FRAME,
+(dir == SNDRV_PCM_STREAM_CAPTURE) ?
+read_completed : write_completed,
+&cdev->data_cb_info[i], 1);
+
urbs[i]->number_of_packets = FRAMES_PER_URB;
-   urbs[i]->complete = (dir == SNDRV_PCM_STREAM_CAPTURE) ?
-   read_completed : write_completed;
}
 
*ret = 0;
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

ALSA: use irqsave() in URB completion + usb_fill_int_urb

2018-06-19 Thread Sebastian Andrzej Siewior 
This series is mostly about using _irqsave() primitives in the
completion callback in order to get rid of local_irq_save() in
__usb_hcd_giveback_urb(). While at it, I also tried to move drivers to
use usb_fill_int_urb() otherwise it is hard find users of a certain API.

Sebastian
 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/9] ALSA: ua101: use usb_fill_int_urb()

2018-06-19 Thread Sebastian Andrzej Siewior 
Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.

Cc: Jaroslav Kysela 
Cc: Takashi Iwai 
Cc: Clemens Ladisch 
Signed-off-by: Sebastian Andrzej Siewior 
---
 sound/usb/misc/ua101.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/sound/usb/misc/ua101.c b/sound/usb/misc/ua101.c
index 386fbfd5c617..7002fb4c1bce 100644
--- a/sound/usb/misc/ua101.c
+++ b/sound/usb/misc/ua101.c
@@ -1118,16 +1118,12 @@ static int alloc_stream_urbs(struct ua101 *ua, struct 
ua101_stream *stream,
if (!urb)
return -ENOMEM;
usb_init_urb(&urb->urb);
-   urb->urb.dev = ua->dev;
-   urb->urb.pipe = stream->usb_pipe;
+   usb_fill_int_urb(&urb->urb, ua->dev, stream->usb_pipe,
+addr, max_packet_size, urb_complete,
+ua, 1);
urb->urb.transfer_flags = URB_NO_TRANSFER_DMA_MAP;
-   urb->urb.transfer_buffer = addr;
urb->urb.transfer_dma = dma;
-   urb->urb.transfer_buffer_length = max_packet_size;
urb->urb.number_of_packets = 1;
-   urb->urb.interval = 1;
-   urb->urb.context = ua;
-   urb->urb.complete = urb_complete;
urb->urb.iso_frame_desc[0].offset = 0;
urb->urb.iso_frame_desc[0].length = max_packet_size;
stream->urbs[u++] = urb;
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()

2018-06-19 Thread Sebastian Andrzej Siewior 
Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.

The "&& !(*purb)->transfer_buffer" check has been removed because the
URB has been freshly allocated a few lines above so ->transfer_buffer
has to be NULL here.
The `dev' and `transfer_size' assignments have been moved from
usX2Y_urbs_start() to usX2Y_urbs_allocate() because they don't change
overtime.

Cc: Jaroslav Kysela 
Cc: Takashi Iwai 
Signed-off-by: Sebastian Andrzej Siewior 
---
 sound/usb/usx2y/usbusx2yaudio.c | 21 +
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
index 2b833054e3b0..9a49bdb07508 100644
--- a/sound/usb/usx2y/usbusx2yaudio.c
+++ b/sound/usb/usx2y/usbusx2yaudio.c
@@ -425,6 +425,9 @@ static int usX2Y_urbs_allocate(struct snd_usX2Y_substream 
*subs)
/* allocate and initialize data urbs */
for (i = 0; i < NRURBS; i++) {
struct urb **purb = subs->urb + i;
+   void *buf = NULL;
+   unsigned int len = 0;
+
if (*purb) {
usb_kill_urb(*purb);
continue;
@@ -434,22 +437,18 @@ static int usX2Y_urbs_allocate(struct snd_usX2Y_substream 
*subs)
usX2Y_urbs_release(subs);
return -ENOMEM;
}
-   if (!is_playback && !(*purb)->transfer_buffer) {
+   if (!is_playback) {
/* allocate a capture buffer per urb */
-   (*purb)->transfer_buffer =
-   kmalloc_array(subs->maxpacksize,
- nr_of_packs(), GFP_KERNEL);
-   if (NULL == (*purb)->transfer_buffer) {
+   len = subs->maxpacksize * nr_of_packs();
+   buf = kmalloc(len, GFP_KERNEL);
+   if (!buf) {
usX2Y_urbs_release(subs);
return -ENOMEM;
}
}
-   (*purb)->dev = dev;
-   (*purb)->pipe = pipe;
+   usb_fill_int_urb(*purb, dev, pipe, buf, len,
+i_usX2Y_subs_startup, subs, 1);
(*purb)->number_of_packets = nr_of_packs();
-   (*purb)->context = subs;
-   (*purb)->interval = 1;
-   (*purb)->complete = i_usX2Y_subs_startup;
}
return 0;
 }
@@ -485,12 +484,10 @@ static int usX2Y_urbs_start(struct snd_usX2Y_substream 
*subs)
unsigned long pack;
if (0 == i)
atomic_set(&subs->state, state_STARTING3);
-   urb->dev = usX2Y->dev;
for (pack = 0; pack < nr_of_packs(); pack++) {
urb->iso_frame_desc[pack].offset = 
subs->maxpacksize * pack;
urb->iso_frame_desc[pack].length = 
subs->maxpacksize;
}
-   urb->transfer_buffer_length = subs->maxpacksize * 
nr_of_packs(); 
if ((err = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
snd_printk (KERN_ERR "cannot submit datapipe 
for urb %d, err = %d\n", i, err);
err = -EPIPE;
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/9] ALSA: line6: use usb_fill_int_urb()

2018-06-19 Thread Sebastian Andrzej Siewior 
Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.
Data pointer & size is filled out later so they are not considered.

Cc: Jaroslav Kysela 
Cc: Takashi Iwai 
Signed-off-by: Sebastian Andrzej Siewior 
---
 sound/usb/line6/capture.c  | 16 ++--
 sound/usb/line6/playback.c | 16 ++--
 2 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/sound/usb/line6/capture.c b/sound/usb/line6/capture.c
index d8a14d769f48..4ee54ba436df 100644
--- a/sound/usb/line6/capture.c
+++ b/sound/usb/line6/capture.c
@@ -52,7 +52,6 @@ static int submit_audio_in_urb(struct snd_line6_pcm *line6pcm)
line6pcm->in.buffer +
index * LINE6_ISO_PACKETS * line6pcm->max_packet_size_in;
urb_in->transfer_buffer_length = urb_size;
-   urb_in->context = line6pcm;
 
ret = usb_submit_urb(urb_in, GFP_ATOMIC);
 
@@ -280,17 +279,14 @@ int line6_create_audio_in_urbs(struct snd_line6_pcm 
*line6pcm)
if (urb == NULL)
return -ENOMEM;
 
-   urb->dev = line6->usbdev;
-   urb->pipe =
-   usb_rcvisocpipe(line6->usbdev,
-   line6->properties->ep_audio_r &
-   USB_ENDPOINT_NUMBER_MASK);
+   usb_fill_int_urb(urb, line6->usbdev,
+usb_rcvisocpipe(line6->usbdev,
+line6->properties->ep_audio_r &
+USB_ENDPOINT_NUMBER_MASK),
+NULL, 0, audio_in_callback, line6pcm,
+LINE6_ISO_INTERVAL);
urb->transfer_flags = URB_ISO_ASAP;
-   urb->start_frame = -1;
urb->number_of_packets = LINE6_ISO_PACKETS;
-   urb->interval = LINE6_ISO_INTERVAL;
-   urb->error_count = 0;
-   urb->complete = audio_in_callback;
}
 
return 0;
diff --git a/sound/usb/line6/playback.c b/sound/usb/line6/playback.c
index dec89d2beb57..af1ca09c260e 100644
--- a/sound/usb/line6/playback.c
+++ b/sound/usb/line6/playback.c
@@ -202,7 +202,6 @@ static int submit_audio_out_urb(struct snd_line6_pcm 
*line6pcm)
line6pcm->out.buffer +
index * LINE6_ISO_PACKETS * line6pcm->max_packet_size_out;
urb_out->transfer_buffer_length = urb_size;
-   urb_out->context = line6pcm;
 
if (test_bit(LINE6_STREAM_PCM, &line6pcm->out.running) &&
!test_bit(LINE6_FLAG_PAUSE_PLAYBACK, &line6pcm->flags)) {
@@ -425,17 +424,14 @@ int line6_create_audio_out_urbs(struct snd_line6_pcm 
*line6pcm)
if (urb == NULL)
return -ENOMEM;
 
-   urb->dev = line6->usbdev;
-   urb->pipe =
-   usb_sndisocpipe(line6->usbdev,
-   line6->properties->ep_audio_w &
-   USB_ENDPOINT_NUMBER_MASK);
+   usb_fill_int_urb(urb, line6->usbdev,
+usb_sndisocpipe(line6->usbdev,
+line6->properties->ep_audio_w &
+USB_ENDPOINT_NUMBER_MASK),
+NULL, 0, audio_out_callback, line6pcm,
+LINE6_ISO_INTERVAL);
urb->transfer_flags = URB_ISO_ASAP;
-   urb->start_frame = -1;
urb->number_of_packets = LINE6_ISO_PACKETS;
-   urb->interval = LINE6_ISO_INTERVAL;
-   urb->error_count = 0;
-   urb->complete = audio_out_callback;
}
 
return 0;
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/9] ALSA: usb: caiaq: audio: use irqsave() in USB's complete callback

2018-06-19 Thread Sebastian Andrzej Siewior 
From: John Ogness 

The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Daniel Mack 
Cc: Jaroslav Kysela 
Cc: Takashi Iwai 
Signed-off-by: John Ogness 
Signed-off-by: Sebastian Andrzej Siewior 
---
 sound/usb/caiaq/audio.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
index f35d29f49ffe..15344d39a6cd 100644
--- a/sound/usb/caiaq/audio.c
+++ b/sound/usb/caiaq/audio.c
@@ -636,6 +636,7 @@ static void read_completed(struct urb *urb)
struct device *dev;
struct urb *out = NULL;
int i, frame, len, send_it = 0, outframe = 0;
+   unsigned long flags;
size_t offset = 0;
 
if (urb->status || !info)
@@ -672,10 +673,10 @@ static void read_completed(struct urb *urb)
offset += len;
 
if (len > 0) {
-   spin_lock(&cdev->spinlock);
+   spin_lock_irqsave(&cdev->spinlock, flags);
fill_out_urb(cdev, out, &out->iso_frame_desc[outframe]);
read_in_urb(cdev, urb, &urb->iso_frame_desc[frame]);
-   spin_unlock(&cdev->spinlock);
+   spin_unlock_irqrestore(&cdev->spinlock, flags);
check_for_elapsed_periods(cdev, cdev->sub_playback);
check_for_elapsed_periods(cdev, cdev->sub_capture);
send_it = 1;
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/9] ALSA: 6fire: use usb_fill_int_urb()

2018-06-19 Thread Sebastian Andrzej Siewior 
Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.
usb6fire_comm_init_urb() passes no transfer length.

Cc: Jaroslav Kysela 
Cc: Takashi Iwai 
Signed-off-by: Sebastian Andrzej Siewior 
---
 sound/usb/6fire/comm.c | 20 +++-
 sound/usb/6fire/pcm.c  | 25 +++--
 2 files changed, 18 insertions(+), 27 deletions(-)

diff --git a/sound/usb/6fire/comm.c b/sound/usb/6fire/comm.c
index 161215d78d95..6adc09a0c6fa 100644
--- a/sound/usb/6fire/comm.c
+++ b/sound/usb/6fire/comm.c
@@ -26,12 +26,9 @@ static void usb6fire_comm_init_urb(struct comm_runtime *rt, 
struct urb *urb,
u8 *buffer, void *context, void(*handler)(struct urb *urb))
 {
usb_init_urb(urb);
-   urb->transfer_buffer = buffer;
-   urb->pipe = usb_sndintpipe(rt->chip->dev, COMM_EP);
-   urb->complete = handler;
-   urb->context = context;
-   urb->interval = 1;
-   urb->dev = rt->chip->dev;
+   usb_fill_int_urb(urb, rt->chip->dev,
+usb_sndintpipe(rt->chip->dev, COMM_EP),
+buffer, 0, handler, context, 1);
 }
 
 static void usb6fire_comm_receiver_handler(struct urb *urb)
@@ -168,13 +165,10 @@ int usb6fire_comm_init(struct sfire_chip *chip)
rt->write16 = usb6fire_comm_write16;
 
/* submit an urb that receives communication data from device */
-   urb->transfer_buffer = rt->receiver_buffer;
-   urb->transfer_buffer_length = COMM_RECEIVER_BUFSIZE;
-   urb->pipe = usb_rcvintpipe(chip->dev, COMM_EP);
-   urb->dev = chip->dev;
-   urb->complete = usb6fire_comm_receiver_handler;
-   urb->context = rt;
-   urb->interval = 1;
+   usb_fill_int_urb(urb, chip->dev, usb_rcvintpipe(chip->dev, COMM_EP),
+rt->receiver_buffer, COMM_RECEIVER_BUFSIZE,
+usb6fire_comm_receiver_handler, rt, 1);
+
ret = usb_submit_urb(urb, GFP_KERNEL);
if (ret < 0) {
kfree(rt->receiver_buffer);
diff --git a/sound/usb/6fire/pcm.c b/sound/usb/6fire/pcm.c
index 2dd2518a71d3..e32cca0f3c2a 100644
--- a/sound/usb/6fire/pcm.c
+++ b/sound/usb/6fire/pcm.c
@@ -569,20 +569,14 @@ static const struct snd_pcm_ops pcm_ops = {
 };
 
 static void usb6fire_pcm_init_urb(struct pcm_urb *urb,
- struct sfire_chip *chip, bool in, int ep,
+ struct sfire_chip *chip, unsigned int pipe,
  void (*handler)(struct urb *))
 {
urb->chip = chip;
usb_init_urb(&urb->instance);
-   urb->instance.transfer_buffer = urb->buffer;
-   urb->instance.transfer_buffer_length =
-   PCM_N_PACKETS_PER_URB * PCM_MAX_PACKET_SIZE;
-   urb->instance.dev = chip->dev;
-   urb->instance.pipe = in ? usb_rcvisocpipe(chip->dev, ep)
-   : usb_sndisocpipe(chip->dev, ep);
-   urb->instance.interval = 1;
-   urb->instance.complete = handler;
-   urb->instance.context = urb;
+   usb_fill_int_urb(&urb->instance, chip->dev, pipe, urb->buffer,
+PCM_N_PACKETS_PER_URB * PCM_MAX_PACKET_SIZE,
+handler, urb, 1);
urb->instance.number_of_packets = PCM_N_PACKETS_PER_URB;
 }
 
@@ -643,10 +637,13 @@ int usb6fire_pcm_init(struct sfire_chip *chip)
spin_lock_init(&rt->capture.lock);
 
for (i = 0; i < PCM_N_URBS; i++) {
-   usb6fire_pcm_init_urb(&rt->in_urbs[i], chip, true, IN_EP,
-   usb6fire_pcm_in_urb_handler);
-   usb6fire_pcm_init_urb(&rt->out_urbs[i], chip, false, OUT_EP,
-   usb6fire_pcm_out_urb_handler);
+   usb6fire_pcm_init_urb(&rt->in_urbs[i], chip,
+ usb_rcvisocpipe(chip->dev, IN_EP),
+ usb6fire_pcm_in_urb_handler);
+
+   usb6fire_pcm_init_urb(&rt->out_urbs[i], chip,
+ usb_sndisocpipe(chip->dev, OUT_EP),
+ usb6fire_pcm_out_urb_handler);
 
rt->in_urbs[i].peer = &rt->out_urbs[i];
rt->out_urbs[i].peer = &rt->in_urbs[i];
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4] USB: cdc-wdm: don't enable interrupts in USB-giveback

2018-06-14 Thread Sebastian Andrzej Siewior 
In the code path
  __usb_hcd_giveback_urb()
  -> wdm_in_callback()
   -> service_outstanding_interrupt()

The function service_outstanding_interrupt() will unconditionally enable
interrupts during unlock and invoke usb_submit_urb() with GFP_KERNEL.
If the HCD completes in BH (like ehci does) then the context remains
atomic due local_bh_disable() and enabling interrupts does not change
this.

Defer the error case handling to a workqueue as suggested by Oliver
Neukum. In case of an error the worker performs the read out and wakes
the user.

Fixes: c1da59dad0eb ("cdc-wdm: Clear read pipeline in case of error")
Cc: Robert Foss 
Cc: Oliver Neukum 
Signed-off-by: Sebastian Andrzej Siewior 
---
v3…v4: also cancel ->service_outs_intr during suspend.
v2…v3: set WDM_READ once error recovery is done so the user does not see
   WDM_READ before the _last_ submitted.

v1…v2: invoke service_outstanding_interrupt() from a worker

 drivers/usb/class/cdc-wdm.c | 31 ---
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index a0d284ef3f40..203bbd378858 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -96,6 +96,7 @@ struct wdm_device {
struct mutexrlock;
wait_queue_head_t   wait;
struct work_struct  rxwork;
+   struct work_struct  service_outs_intr;
int werr;
int rerr;
int resp_count;
@@ -151,9 +152,6 @@ static void wdm_out_callback(struct urb *urb)
wake_up(&desc->wait);
 }
 
-/* forward declaration */
-static int service_outstanding_interrupt(struct wdm_device *desc);
-
 static void wdm_in_callback(struct urb *urb)
 {
struct wdm_device *desc = urb->context;
@@ -209,8 +207,6 @@ static void wdm_in_callback(struct urb *urb)
}
}
 skip_error:
-   set_bit(WDM_READ, &desc->flags);
-   wake_up(&desc->wait);
 
if (desc->rerr) {
/*
@@ -219,9 +215,11 @@ static void wdm_in_callback(struct urb *urb)
 * We should respond to further attempts from the device to send
 * data, so that we can get unstuck.
 */
-   service_outstanding_interrupt(desc);
+   schedule_work(&desc->service_outs_intr);
+   } else {
+   set_bit(WDM_READ, &desc->flags);
+   wake_up(&desc->wait);
}
-
spin_unlock(&desc->iuspin);
 }
 
@@ -758,6 +756,21 @@ static void wdm_rxwork(struct work_struct *work)
}
 }
 
+static void service_interrupt_work(struct work_struct *work)
+{
+   struct wdm_device *desc;
+
+   desc = container_of(work, struct wdm_device, service_outs_intr);
+
+   spin_lock_irq(&desc->iuspin);
+   service_outstanding_interrupt(desc);
+   if (!desc->resp_count) {
+   set_bit(WDM_READ, &desc->flags);
+   wake_up(&desc->wait);
+   }
+   spin_unlock_irq(&desc->iuspin);
+}
+
 /* --- hotplug --- */
 
 static int wdm_create(struct usb_interface *intf, struct 
usb_endpoint_descriptor *ep,
@@ -779,6 +792,7 @@ static int wdm_create(struct usb_interface *intf, struct 
usb_endpoint_descriptor
desc->inum = 
cpu_to_le16((u16)intf->cur_altsetting->desc.bInterfaceNumber);
desc->intf = intf;
INIT_WORK(&desc->rxwork, wdm_rxwork);
+   INIT_WORK(&desc->service_outs_intr, service_interrupt_work);
 
rv = -EINVAL;
if (!usb_endpoint_is_int_in(ep))
@@ -964,6 +978,7 @@ static void wdm_disconnect(struct usb_interface *intf)
mutex_lock(&desc->wlock);
kill_urbs(desc);
cancel_work_sync(&desc->rxwork);
+   cancel_work_sync(&desc->service_outs_intr);
mutex_unlock(&desc->wlock);
mutex_unlock(&desc->rlock);
 
@@ -1006,6 +1021,7 @@ static int wdm_suspend(struct usb_interface *intf, 
pm_message_t message)
/* callback submits work - order is essential */
kill_urbs(desc);
cancel_work_sync(&desc->rxwork);
+   cancel_work_sync(&desc->service_outs_intr);
}
if (!PMSG_IS_AUTO(message)) {
mutex_unlock(&desc->wlock);
@@ -1065,6 +1081,7 @@ static int wdm_pre_reset(struct usb_interface *intf)
mutex_lock(&desc->wlock);
kill_urbs(desc);
cancel_work_sync(&desc->rxwork);
+   cancel_work_sync(&desc->service_outs_intr);
return 0;
 }
 
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] USB: cdc-wdm: don't enable interrupts in USB-giveback

2018-06-14 Thread Sebastian Andrzej Siewior 
On 2018-06-14 10:44:20 [+0200], Oliver Neukum wrote:
> On Mi, 2018-06-13 at 22:28 +0200, Sebastian Andrzej Siewior wrote:
> > On 2018-06-13 19:43:55 [+0200], Oliver Neukum wrote:
> Hi Sebastian,
Hi Oliver,

> > > I am just looking at your patch and I am wondering why
> > > wdm_in_callback() won't just call service_outstanding_interrupt()
> > > again and again? OK, maybe I am dense and it may well be present now,
> > > but it just looks to me that way.
> > 
> > But this part didn't change, did it?
> 
> Right, it didn't change, but that does not make it correct.

Yes, I just wanted make sure that the breakage is not part of the patch.

> > The user blocks in wdmw_read()
> 
> We can only hope that he does. The wait is interruptible.
> If a signal comes at the wrong time, nobody will be waiting.
> 
> > Maybe we should delay the WDM_READ flag in the error case until the
> > worker is done (before the wakeup).
> 
> I don't think that will help. 

Why not? If it won't see WDM_READ flag set then it will return with
-EBUSY.

> It seems like we need to make sure
> that error recovery is a one shot activity.

I think it will be if we delay the WDM_READ because the user will not be
aware of the situation until the error recovery is done and not after
the URB has been submitted:

--->8

Subject: [PATCH v3] USB: cdc-wdm: don't enable interrupts in USB-giveback

In the code path
  __usb_hcd_giveback_urb()
  -> wdm_in_callback()
   -> service_outstanding_interrupt()

The function service_outstanding_interrupt() will unconditionally enable
interrupts during unlock and invoke usb_submit_urb() with GFP_KERNEL.
If the HCD completes in BH (like ehci does) then the context remains
atomic due local_bh_disable() and enabling interrupts does not change
this.

Defer the error case handling to a workqueue as suggested by Oliver
Neukum. In case of an error the worker performs the read out and wakes
the user.

Fixes: c1da59dad0eb ("cdc-wdm: Clear read pipeline in case of error")
Cc: Robert Foss 
Cc: Oliver Neukum 
Signed-off-by: Sebastian Andrzej Siewior 
---
v2…v3: set WDM_READ once error recovery is done so the user does not see
   WDM_READ before the _last_ submitted.

v1…v2: invoke service_outstanding_interrupt() from a worker

 drivers/usb/class/cdc-wdm.c | 25 +++--
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index a0d284ef3f40..ddf55f93d4ca 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -96,6 +96,7 @@ struct wdm_device {
struct mutexrlock;
wait_queue_head_t   wait;
struct work_struct  rxwork;
+   struct work_struct  service_outs_intr;
int werr;
int rerr;
int resp_count;
@@ -151,9 +152,6 @@ static void wdm_out_callback(struct urb *urb)
wake_up(&desc->wait);
 }
 
-/* forward declaration */
-static int service_outstanding_interrupt(struct wdm_device *desc);
-
 static void wdm_in_callback(struct urb *urb)
 {
struct wdm_device *desc = urb->context;
@@ -210,7 +208,6 @@ static void wdm_in_callback(struct urb *urb)
}
 skip_error:
set_bit(WDM_READ, &desc->flags);
-   wake_up(&desc->wait);
 
if (desc->rerr) {
/*
@@ -219,9 +216,10 @@ static void wdm_in_callback(struct urb *urb)
 * We should respond to further attempts from the device to send
 * data, so that we can get unstuck.
 */
-   service_outstanding_interrupt(desc);
+   schedule_work(&desc->service_outs_intr);
+   } else {
+   wake_up(&desc->wait);
}
-
spin_unlock(&desc->iuspin);
 }
 
@@ -758,6 +756,18 @@ static void wdm_rxwork(struct work_struct *work)
}
 }
 
+static void service_interrupt_work(struct work_struct *work)
+{
+   struct wdm_device *desc;
+
+   desc = container_of(work, struct wdm_device, service_outs_intr);
+
+   spin_lock_irq(&desc->iuspin);
+   service_outstanding_interrupt(desc);
+   wake_up(&desc->wait);
+   spin_unlock_irq(&desc->iuspin);
+}
+
 /* --- hotplug --- */
 
 static int wdm_create(struct usb_interface *intf, struct 
usb_endpoint_descriptor *ep,
@@ -779,6 +789,7 @@ static int wdm_create(struct usb_interface *intf, struct 
usb_endpoint_descriptor
desc->inum = 
cpu_to_le16((u16)intf->cur_altsetting->desc.bInterfaceNumber);
desc->intf = intf;
INIT_WORK(&desc->rxwork, wdm_rxwork);
+   INIT_WORK(&desc->service_outs_intr, service_interrupt_work);
 
rv = -EINVAL;
if (!usb_endpoint_is_int_in(ep))
@@ -964,

Re: [PATCH v2] USB: cdc-wdm: don't enable interrupts in USB-giveback

2018-06-13 Thread Sebastian Andrzej Siewior 
On 2018-06-13 19:43:55 [+0200], Oliver Neukum wrote:
> On Mi, 2018-06-13 at 18:28 +0200, Sebastian Andrzej Siewior wrote:
> > The function service_outstanding_interrupt() will unconditionally enable
> > interrupts during unlock and invoke usb_submit_urb() with GFP_KERNEL.
> > If the HCD completes in BH (like ehci does) then the context remains
> > atomic due local_bh_disable() and enabling interrupts does not change
> > this.
> 
> Hi,
Hi Oliver,

> I am just looking at your patch and I am wondering why
> wdm_in_callback() won't just call service_outstanding_interrupt()
> again and again? OK, maybe I am dense and it may well be present now,
> but it just looks to me that way.

But this part didn't change, did it? The user blocks in wdmw_read()
waiting for the WDM_READ to be set and a wakeup. After rhe wakeup it
clears the ->rerr.

| static ssize_t wdm_read
| (struct file *file, char __user *buffer, size_t count, loff_t *ppos)
| {
|…
|rv = wait_event_interruptible(desc->wait,
| test_bit(WDM_READ, &desc->flags));
|… 
|
|if (desc->rerr) { /* read completed, error happened */
| rv = usb_translate_errors(desc->rerr);
| desc->rerr = 0;
| spin_unlock_irq(&desc->iuspin);
| goto err;
| }

Maybe we should delay the WDM_READ flag in the error case until the
worker is done (before the wakeup).

>   Regards
>   Oliver

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] USB: cdc-wdm: don't enable interrupts in USB-giveback

2018-06-13 Thread Sebastian Andrzej Siewior 
In the code path
  __usb_hcd_giveback_urb()
  -> wdm_in_callback()
   -> service_outstanding_interrupt()

The function service_outstanding_interrupt() will unconditionally enable
interrupts during unlock and invoke usb_submit_urb() with GFP_KERNEL.
If the HCD completes in BH (like ehci does) then the context remains
atomic due local_bh_disable() and enabling interrupts does not change
this.

Defer the error case handling to a workqueue as suggested by Oliver
Neukum. In case of an error the worker performs the read out and wakes
the user.

Fixes: c1da59dad0eb ("cdc-wdm: Clear read pipeline in case of error")
Cc: Robert Foss 
Cc: Oliver Neukum 
Signed-off-by: Sebastian Andrzej Siewior 
---
On 2018-06-13 10:30:18 [+0200], Oliver Neukum wrote:
> It seems to me that the core of the problem is handling an error
> in irq context potentially. How about shifting it to a work queue?

Something like this then?

 drivers/usb/class/cdc-wdm.c | 25 +++--
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index a0d284ef3f40..ddf55f93d4ca 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -96,6 +96,7 @@ struct wdm_device {
struct mutexrlock;
wait_queue_head_t   wait;
struct work_struct  rxwork;
+   struct work_struct  service_outs_intr;
int werr;
int rerr;
int resp_count;
@@ -151,9 +152,6 @@ static void wdm_out_callback(struct urb *urb)
wake_up(&desc->wait);
 }
 
-/* forward declaration */
-static int service_outstanding_interrupt(struct wdm_device *desc);
-
 static void wdm_in_callback(struct urb *urb)
 {
struct wdm_device *desc = urb->context;
@@ -210,7 +208,6 @@ static void wdm_in_callback(struct urb *urb)
}
 skip_error:
set_bit(WDM_READ, &desc->flags);
-   wake_up(&desc->wait);
 
if (desc->rerr) {
/*
@@ -219,9 +216,10 @@ static void wdm_in_callback(struct urb *urb)
 * We should respond to further attempts from the device to send
 * data, so that we can get unstuck.
 */
-   service_outstanding_interrupt(desc);
+   schedule_work(&desc->service_outs_intr);
+   } else {
+   wake_up(&desc->wait);
}
-
spin_unlock(&desc->iuspin);
 }
 
@@ -758,6 +756,18 @@ static void wdm_rxwork(struct work_struct *work)
}
 }
 
+static void service_interrupt_work(struct work_struct *work)
+{
+   struct wdm_device *desc;
+
+   desc = container_of(work, struct wdm_device, service_outs_intr);
+
+   spin_lock_irq(&desc->iuspin);
+   service_outstanding_interrupt(desc);
+   wake_up(&desc->wait);
+   spin_unlock_irq(&desc->iuspin);
+}
+
 /* --- hotplug --- */
 
 static int wdm_create(struct usb_interface *intf, struct 
usb_endpoint_descriptor *ep,
@@ -779,6 +789,7 @@ static int wdm_create(struct usb_interface *intf, struct 
usb_endpoint_descriptor
desc->inum = 
cpu_to_le16((u16)intf->cur_altsetting->desc.bInterfaceNumber);
desc->intf = intf;
INIT_WORK(&desc->rxwork, wdm_rxwork);
+   INIT_WORK(&desc->service_outs_intr, service_interrupt_work);
 
rv = -EINVAL;
if (!usb_endpoint_is_int_in(ep))
@@ -964,6 +975,7 @@ static void wdm_disconnect(struct usb_interface *intf)
mutex_lock(&desc->wlock);
kill_urbs(desc);
cancel_work_sync(&desc->rxwork);
+   cancel_work_sync(&desc->service_outs_intr);
mutex_unlock(&desc->wlock);
mutex_unlock(&desc->rlock);
 
@@ -1065,6 +1077,7 @@ static int wdm_pre_reset(struct usb_interface *intf)
mutex_lock(&desc->wlock);
kill_urbs(desc);
cancel_work_sync(&desc->rxwork);
+   cancel_work_sync(&desc->service_outs_intr);
return 0;
 }
 
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: cdc-wdm: don't enable interrupts in USB-giveback

2018-06-12 Thread Sebastian Andrzej Siewior 
On 2018-06-12 20:28:27 [+0200], Bjørn Mork wrote:
> Yes, the atomic case should be rare.  It will only happen on errors, and
> IIUC that's only to work around issues caused by reporting errors back
> to userspace without actually wanting to err out anyway.

Yup. The missing part is if this was done to workaround a specific
userland application or most/all of them.

> I believe it would be better to decide one an error policy and stick to
> that.  Then you could just simplify away that whole mess, by either
> ignoring the error and continue or bailing out and die.

"Bailing out and die" would be a revert of commit c1da59dad0eb
("cdc-wdm: Clear read pipeline in case of error")?
And ignoring the error would be "not updating rerr" in
wdm_in_callback().
I don't care either way. I can do whatever works for you/users best.

> Bjørn

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: cdc-wdm: don't enable interrupts in USB-giveback

2018-06-12 Thread Sebastian Andrzej Siewior 
On 2018-06-12 12:43:01 [-0400], Alan Stern wrote:
> On Tue, 12 Jun 2018, Sebastian Andrzej Siewior wrote:
> 
> > In the code path
> >   __usb_hcd_giveback_urb()
> >   -> wdm_in_callback()
> >-> service_outstanding_interrupt()
> > 
> > The function service_outstanding_interrupt() will unconditionally enable
> > interrupts during unlock and invoke usb_submit_urb() with GFP_KERNEL.
> > If the HCD completes in BH (like ehci does) then the context remains
> > atomic due local_bh_disable() and enabling interrupts does not change
> > this.
> > 
> > Add an argument to service_outstanding_interrupt() which decides
> > whether or not it is save to enable interrupts during unlocking and use
> > GFP_KERNEL or not.
> 
> Wouldn't it be easier just to use spin_lock_irqsave() and GFP_ATOMIC
> all the time?  That's what people normally do with code that can be 
> called in both process and interrupt contexts.

service_outstanding_interrupt() does unlock + lock instead lock +
unlock. If you want to have this "always" working (without the
argument), we could do the false case:
+   spin_unlock(&desc->iuspin);
+   rv = usb_submit_urb(desc->response, GFP_ATOMIC);
+   spin_lock(&desc->iuspin);

all the time. I wanted to preserve the GFP_KERNEL part which is probably
used more often but fell that this is not needed I could drop that part.

> Alan Stern
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] USB: cdc-wdm: don't enable interrupts in USB-giveback

2018-06-12 Thread Sebastian Andrzej Siewior 
In the code path
  __usb_hcd_giveback_urb()
  -> wdm_in_callback()
   -> service_outstanding_interrupt()

The function service_outstanding_interrupt() will unconditionally enable
interrupts during unlock and invoke usb_submit_urb() with GFP_KERNEL.
If the HCD completes in BH (like ehci does) then the context remains
atomic due local_bh_disable() and enabling interrupts does not change
this.

Add an argument to service_outstanding_interrupt() which decides
whether or not it is save to enable interrupts during unlocking and use
GFP_KERNEL or not.

Fixes: c1da59dad0eb ("cdc-wdm: Clear read pipeline in case of error")
Cc: Robert Foss 
Cc: Oliver Neukum 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/class/cdc-wdm.c | 22 ++
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index a0d284ef3f40..45076b9c7c5e 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -152,7 +152,7 @@ static void wdm_out_callback(struct urb *urb)
 }
 
 /* forward declaration */
-static int service_outstanding_interrupt(struct wdm_device *desc);
+static int service_outstanding_interrupt(struct wdm_device *desc, bool en_irq);
 
 static void wdm_in_callback(struct urb *urb)
 {
@@ -219,7 +219,7 @@ static void wdm_in_callback(struct urb *urb)
 * We should respond to further attempts from the device to send
 * data, so that we can get unstuck.
 */
-   service_outstanding_interrupt(desc);
+   service_outstanding_interrupt(desc, false);
}
 
spin_unlock(&desc->iuspin);
@@ -448,7 +448,7 @@ static ssize_t wdm_write
  *
  * Called with desc->iuspin locked
  */
-static int service_outstanding_interrupt(struct wdm_device *desc)
+static int service_outstanding_interrupt(struct wdm_device *desc, bool en_irq)
 {
int rv = 0;
 
@@ -457,9 +457,15 @@ static int service_outstanding_interrupt(struct wdm_device 
*desc)
goto out;
 
set_bit(WDM_RESPONDING, &desc->flags);
-   spin_unlock_irq(&desc->iuspin);
-   rv = usb_submit_urb(desc->response, GFP_KERNEL);
-   spin_lock_irq(&desc->iuspin);
+   if (en_irq) {
+   spin_unlock_irq(&desc->iuspin);
+   rv = usb_submit_urb(desc->response, GFP_KERNEL);
+   spin_lock_irq(&desc->iuspin);
+   } else {
+   spin_unlock(&desc->iuspin);
+   rv = usb_submit_urb(desc->response, GFP_ATOMIC);
+   spin_lock(&desc->iuspin);
+   }
if (rv) {
dev_err(&desc->intf->dev,
"usb_submit_urb failed with result %d\n", rv);
@@ -544,7 +550,7 @@ static ssize_t wdm_read
if (!desc->reslength) { /* zero length read */
dev_dbg(&desc->intf->dev, "zero length - clearing 
WDM_READ\n");
clear_bit(WDM_READ, &desc->flags);
-   rv = service_outstanding_interrupt(desc);
+   rv = service_outstanding_interrupt(desc, true);
spin_unlock_irq(&desc->iuspin);
if (rv < 0)
goto err;
@@ -571,7 +577,7 @@ static ssize_t wdm_read
/* in case we had outstanding data */
if (!desc->length) {
clear_bit(WDM_READ, &desc->flags);
-   service_outstanding_interrupt(desc);
+   service_outstanding_interrupt(desc, true);
}
spin_unlock_irq(&desc->iuspin);
rv = cntr;
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Threaded interrupts for USB HCD instead of tasklets

2018-06-01 Thread Sebastian Andrzej Siewior 
On 2018-05-22 15:14:17 [-0400], Alan Stern wrote:
> Sorry, I don't understand that sentence at all.  And I don't see how it 
> could be relevant to the point I was trying to make.
> 
> Consider, for example, drivers/hid/usbhid/hid-core.c.  In that file,
> hid_io_error() is called by hid_irq_in(), which is an URB completion
> handler.  hid_io_error() acquires usbhid_lock.  Therefore it would be
> necessary to audit the usbhid driver to see whether interrupts are
> enabled or disabled any place where usbhid_lock is acquired.  And in
> fact, hid_ctrl() (another completion handler) calls
> spin_lock(&usbhid->lock) -- this could cause problems for you.  And
> usbhid->lock is acquired in other places, ones that are not inside
> completion handlers.

To pick up this example. hid_io_error() is called from process context
(usb_hid_open()) or the completion handler and disables interrupts while
acquiring the lock. hid_ctrl() acquires the lock without disabling the
interrupts. This is not a problem because hid_ctrl() can not be
preempted while it is holding the  lock by anything that also wants the
lock. So hid-core could stay as-is and we would be fine. However
lockdep would complain if hid-core would be used on ehci
(softirq/tasklet completion) and ohci (IRQ-handler completion) because
then lockdep would think that hid_ctrl() invoked by ehci could be
preempted by the ohci driver.

> None of this has anything to do with IRQ usage or hrtimers.
hid-core sets up a timer_list timer hid_retry_timeout(). This timer runs
in softirq context which means it can't preempt the completion handler
(hid_ctrlhid_ctrl()) which does just spin_lock() (both run in BH).
However, if hid_retry_timeout() were a hrtimer timer then it would be
invoked in IRQ-context. In that case it could preempt hid_ctrlhid_ctrl()
and _irqsave() would be required in hid_ctrlhid_ctrl().

My counting reveals ~250 USB drivers. I think it is best to audit them
first and make sure that completion handler like hid_ctrl() do
_irqsave(). Once that is done, the local_irq_save() in
__usb_hcd_giveback_urb() could go.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Threaded interrupts for USB HCD instead of tasklets

2018-05-23 Thread Sebastian Andrzej Siewior 
On 2018-05-22 15:14:17 [-0400], Alan Stern wrote:
> On Tue, 22 May 2018, Sebastian Andrzej Siewior wrote:
> 
> > On 2018-05-07 11:37:29 [-0400], Alan Stern wrote:
> > > > As far as I understand it there should be no deadlock. Without the
> > > > local_irq_save() things should not deadlock because the HCD invokes USB
> > > > driver's (usb-storage for instance) ->complete callback always in the
> > > > same way. If you mix the usb-driver with two different HCDs (say ehci
> > > > and xhci) then lockdep would complain. However, the locks which were
> > > > allocated by usb-storage for the ehci driver would never be used in the
> > > > context of the xhci driver. So lockdep would report a deacklock but
> > > > there wouldn't be one (again, if I got the piece right).
> > > 
> > > That argument would be correct if the completion routines were the only
> > > places where the higher-level drivers (such as usb-storage or usbhid)  
> > > acquired their locks.  But we can't depend on this being true; you
> > > would have to audit each USB driver to make sure.
> > 
> > an entry point for IRQ usage outside of the driver would be the usage of
> > hrtimer.
> 
> Sorry, I don't understand that sentence at all.  And I don't see how it 
> could be relevant to the point I was trying to make.
> 
> Consider, for example, drivers/hid/usbhid/hid-core.c.  In that file,
> hid_io_error() is called by hid_irq_in(), which is an URB completion
> handler.  hid_io_error() acquires usbhid_lock.  Therefore it would be
> necessary to audit the usbhid driver to see whether interrupts are
> enabled or disabled any place where usbhid_lock is acquired.  And in
> fact, hid_ctrl() (another completion handler) calls
> spin_lock(&usbhid->lock) -- this could cause problems for you.  And
> usbhid->lock is acquired in other places, ones that are not inside
> completion handlers.
> 
> None of this has anything to do with IRQ usage or hrtimers.

Yeah, I get what you mean.

> > You mean the "Reserved Bandwidth Transfers:" paragraph?
> 
> Paragraphs (plural).  Three paragraphs, to be precise.
> 
> > > It's possible to rewrite the HCDs not to rely on this (I did exactly
> > > that for ehci-hcd), but it is a nontrivial job.
> > 
> > are you referring to commit 9118f9eb4f1e ("USB: EHCI: improve interrupt
> > qh unlink")?
> 
> That one, plus:
> 
>   46c73d1d3ebc ("USB: EHCI: handle isochronous underruns with tasklets")
>   e4e18cbd52c8 ("USB: EHCI: code rearrangement in iso_stream_schedule()")
>   24f531371de1 ("USB: EHCI: accept very late isochronous URBs")
>   35371e4fbc3e ("USB: EHCI: improve ehci_endpoint_disable")
> 
> Not all parts of all those commits were relevant, but as far as I
> recall, they each contributed something.  And I may have omitted
> one or two commits by mistake.

Thank you, let me look at those so I can see what is needed…

> Alan Stern

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Threaded interrupts for USB HCD instead of tasklets

2018-05-22 Thread Sebastian Andrzej Siewior 
On 2018-05-07 11:37:29 [-0400], Alan Stern wrote:
> > As far as I understand it there should be no deadlock. Without the
> > local_irq_save() things should not deadlock because the HCD invokes USB
> > driver's (usb-storage for instance) ->complete callback always in the
> > same way. If you mix the usb-driver with two different HCDs (say ehci
> > and xhci) then lockdep would complain. However, the locks which were
> > allocated by usb-storage for the ehci driver would never be used in the
> > context of the xhci driver. So lockdep would report a deacklock but
> > there wouldn't be one (again, if I got the piece right).
> 
> That argument would be correct if the completion routines were the only
> places where the higher-level drivers (such as usb-storage or usbhid)  
> acquired their locks.  But we can't depend on this being true; you
> would have to audit each USB driver to make sure.

an entry point for IRQ usage outside of the driver would be the usage of
hrtimer. We have a flag to let the hrtimer run in softirq but yes, we
need to audit them.

> > And I was thinking about converting all drivers to one model and then we
> > could get rid of the block I quoted above.
> > 
> > If nobody rejects the approach as such I would go and start hacking…
> > 
> > > And even for those two, the conversion will not be easy.  Simply
> > > changing the giveback routines would violate the documented guarantees
> > > for isochronous transfers.
> > 
> > The requirement was that the ISO urb is completed before the BULK urb,
> > right?
> 
> No, that's not what I meant.  For one thing, isochronous URBs don't
> need to complete before bulk URBs in general (although they do have
> higher priority).
> 
> However, I was really referring to the kerneldoc for usb_submit_urb().  
> The part that talks about scheduling of isochronous and interrupt URBs
> lists a bunch of requirements.  In order to meet these requirements
> some of the host controller drivers may rely on the fact that when they
> give back an URB, the URB's completion routine will return before the
> giveback call finishes.

You mean the "Reserved Bandwidth Transfers:" paragraph?

> It's possible to rewrite the HCDs not to rely on this (I did exactly
> that for ehci-hcd), but it is a nontrivial job.

are you referring to commit 9118f9eb4f1e ("USB: EHCI: improve interrupt
qh unlink")?

> Alan Stern

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Threaded interrupts for USB HCD instead of tasklets

2018-05-07 Thread Sebastian Andrzej Siewior 
On 2018-05-04 16:07:22 [-0400], Alan Stern wrote:
> On Fri, 4 May 2018, Sebastian Andrzej Siewior wrote:
> 
> > Hi Alan,
> > 
> > I posted a RFC [0] to let the HCD complete the URB in threaded-IRQ
> > context to avoid the softirq/tasklet. Mauro reported that this does not
> > help him and never came back after that.
> > You did not oppose the approach as long as there is no performance issue
> > which I did not see. Would you mind if I revisit the approach and
> > convert all HCDs to threaded IRQs while at it?
> 
> I don't mind revisiting the approach, although it certainly would be 
> good to find out why it doesn't help Mauro's video problem.
> 
> However, converting _all_ the HCDs to threaded IRQs is a different
> story.  It should be okay to convert the ones that currently use
> tasklets, but I can't approve changes to all the others.  Only the
> drivers that I maintain (ohci-hcd and uhci-hcd).

Sure. The plan was to convert all drivers to one model so we could get
rid of:

|static void __usb_hcd_giveback_urb(struct urb *urb)
|{
|…
| /* 
|  * We disable local IRQs here avoid possible deadlock because
|  * drivers may call spin_lock() to hold lock which might be
|  * acquired in one hard interrupt handler.
|  * 
|  * The local_irq_save()/local_irq_restore() around complete()
|  * will be removed if current USB drivers have been cleaned up
|  * and no one may trigger the above deadlock situation when
|  * running complete() in tasklet.
|  */
| local_irq_save(flags);
| urb->complete(urb);
| local_irq_restore(flags);
|…
|}

As far as I understand it there should be no deadlock. Without the
local_irq_save() things should not deadlock because the HCD invokes USB
driver's (usb-storage for instance) ->complete callback always in the
same way. If you mix the usb-driver with two different HCDs (say ehci
and xhci) then lockdep would complain. However, the locks which were
allocated by usb-storage for the ehci driver would never be used in the
context of the xhci driver. So lockdep would report a deacklock but
there wouldn't be one (again, if I got the piece right).

And I was thinking about converting all drivers to one model and then we
could get rid of the block I quoted above.

If nobody rejects the approach as such I would go and start hacking…

> And even for those two, the conversion will not be easy.  Simply
> changing the giveback routines would violate the documented guarantees
> for isochronous transfers.

The requirement was that the ISO urb is completed before the BULK urb,
right?
If so, the last patch would enqueue the ISO urb and wait until the BULK
urb was completed before it completed the ISO urb.

> Alan Stern

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Threaded interrupts for USB HCD instead of tasklets

2018-05-04 Thread Sebastian Andrzej Siewior 
Hi Alan,

I posted a RFC [0] to let the HCD complete the URB in threaded-IRQ
context to avoid the softirq/tasklet. Mauro reported that this does not
help him and never came back after that.
You did not oppose the approach as long as there is no performance issue
which I did not see. Would you mind if I revisit the approach and
convert all HCDs to threaded IRQs while at it?

[0] https://lkml.kernel.org/r/20180216170450.yl5owfphuvlts...@breakpoint.cc

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] usb: hcd: complete URBs in threaded-IRQ context instead of tasklet

2018-04-16 Thread Sebastian Andrzej Siewior 
On 2018-03-08 10:57:39 [+0100], To Mauro Carvalho Chehab wrote:
> On 2018-02-27 14:39:34 [-0300], Mauro Carvalho Chehab wrote:
> > Hi Sebastian,
> Hi Mauro,
> 
> > Sorry for taking some time to test it, has been busy those days...
> :)
> 
> > Anyway, I tested it today. Didn't work. It keep losing data.
> 
> Okay, this was unexpected. What I learned from the thread is that you
> use the dwc2 controller and once upgrade to a kernel which completes the
> URBs in BH context then you starting losing data from your DVB-s USB
> device. And it was assumed that this is because BH/ksoftirq is getting
> "paused" if it is running for too long. If that is the case then a
> revert of "let us complete the URB in BH context" should get it working
> again. Is that so?

ping

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] usb: hcd: complete URBs in threaded-IRQ context instead of tasklet

2018-03-08 Thread Sebastian Andrzej Siewior 
On 2018-02-27 14:39:34 [-0300], Mauro Carvalho Chehab wrote:
> Hi Sebastian,
Hi Mauro,

> Sorry for taking some time to test it, has been busy those days...
:)

> Anyway, I tested it today. Didn't work. It keep losing data.

Okay, this was unexpected. What I learned from the thread is that you
use the dwc2 controller and once upgrade to a kernel which completes the
URBs in BH context then you starting losing data from your DVB-s USB
device. And it was assumed that this is because BH/ksoftirq is getting
"paused" if it is running for too long. If that is the case then a
revert of "let us complete the URB in BH context" should get it working
again. Is that so?

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4287,7 +4287,9 @@ void dwc2_host_complete(struct dwc2_hsotg *hsotg, struct 
dwc2_qtd *qtd,
kfree(qtd->urb);
qtd->urb = NULL;
 
+   spin_unlock(&hsotg->lock);
usb_hcd_giveback_urb(dwc2_hsotg_to_hcd(hsotg), urb, status);
+   spin_lock(&hsotg->lock);
 }
 
 /*
@@ -4968,7 +4970,7 @@ static struct hc_driver dwc2_hc_driver = {
.hcd_priv_size = sizeof(struct wrapper_priv_data),
 
.irq = _dwc2_hcd_irq,
-   .flags = HCD_MEMORY | HCD_USB2 | HCD_BH,
+   .flags = HCD_MEMORY | HCD_USB2,
 
.start = _dwc2_hcd_start,
.stop = _dwc2_hcd_stop,
-- 
2.16.2


> Regards,

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] usb: hcd: complete URBs in threaded-IRQ context instead of tasklet

2018-02-22 Thread Sebastian Andrzej Siewior 
On 2018-02-16 16:46:41 [-0500], Alan Stern wrote:
> > The theaded interrupt runs SCHED_FIFO priority 50 by default. The only
> > thing that can interrupt it are interrupts, a softirq (not ksoftirqd)
> > and other tasks with a higher priority than 50.
> > There should be no downside performance wise.
> 
> Maybe.  It would be nice to see some real measurements.

I had an usb3 flash stick behind the EHCI controller which was passed
through from the host to a kvm guest. The performance numbers in the
guest were equal (some noise was there) with and without the patch.
The numbers with the patch were worse if lockdep was enabled which isn't
much of a surprise.
If you have anything specific requirements for a measurement then please
let me know and I see what I can do.

> Alan Stern

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] usb: hcd: complete URBs in threaded-IRQ context instead of tasklet

2018-02-16 Thread Sebastian Andrzej Siewior 
On 2018-02-16 13:29:01 [-0500], Alan Stern wrote:
> We originally used tasklets because we didn't want to incur the delays 
> associated with running in a process context.  It seems odd to be 
> reversing that decision now.

The theaded interrupt runs SCHED_FIFO priority 50 by default. The only
thing that can interrupt it are interrupts, a softirq (not ksoftirqd)
and other tasks with a higher priority than 50.
There should be no downside performance wise.

> > The URBs from the root-hub never create an interrupt so I currently
> > process them in a workqueue (I'm not sure if an URB-enqueue in the
> > completion handler would break something).
> 
> It worked okay before we changed over to using tasklets.

Ah okay. I've seen that HCDs were no longer dropping their internal lock
and I wasn't sure if such a change was also applied in RH-code.

> Alan Stern

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH] usb: hcd: complete URBs in threaded-IRQ context instead of tasklet

2018-02-16 Thread Sebastian Andrzej Siewior 
I've been going over Frederic's softirq patches and it seems that there
were two problems. One was network related, the other was Mauro's USB
dvb-[stc] device which was not able to stream properly over time.

Here is an attempt to let the URB complete in the threaded handler
instead of going to the tasklet. In case the system is SMP then the
patch [0] would be required in order to have the IRQ and its thread on
the same CPU.

Mauro, would you mind giving it a shot?

[0] genirq: Let irq thread follow the effective hard irq affinity
https://git.kernel.org/tip/cbf866a6e7f2f674b3a2a4cef9f666ff613e

---

The urb->complete callback was initially invoked in IRQ context after
the HCD dropped its lock because the callback could re-queue the URB
again. Later this completion was deferred to the tasklet allowing the
HCD hold the lock. Also the BH handler can be interrupted by the IRQ
handler adding more "completed" requests to its queue.
While this batching is good in general, the softirq defers its doing for
short period of time if it is running constantly without a break. This
breaks some use cases where constant USB throughput is required.
As an alternative approach to tasklet handling, I defer the URB
completion to the HCD's threaded handler. There are two lists for
"high-prio" proccessing and lower priority (to mimic current behaviour).
The URBs in the high-priority list are always preffered over the URBs
in the low-priority list.
The URBs from the root-hub never create an interrupt so I currently
process them in a workqueue (I'm not sure if an URB-enqueue in the
completion handler would break something).

Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/core/hcd.c  | 131 
 include/linux/usb/hcd.h |  14 +++---
 2 files changed, 95 insertions(+), 50 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index fc32391a34d5..8d6dd4d3cbe7 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1775,33 +1775,75 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
usb_put_urb(urb);
 }
 
-static void usb_giveback_urb_bh(unsigned long param)
+static void usb_hcd_rh_gb_urb(struct work_struct *work)
 {
-   struct giveback_urb_bh *bh = (struct giveback_urb_bh *)param;
-   struct list_head local_list;
+   struct giveback_urb *bh = container_of(work, struct giveback_urb,
+  rh_compl);
+   struct list_headurb_list;
 
spin_lock_irq(&bh->lock);
-   bh->running = true;
- restart:
-   list_replace_init(&bh->head, &local_list);
+   list_replace_init(&bh->rh_head, &urb_list);
spin_unlock_irq(&bh->lock);
 
-   while (!list_empty(&local_list)) {
+   while (!list_empty(&urb_list)) {
struct urb *urb;
 
-   urb = list_entry(local_list.next, struct urb, urb_list);
+   urb = list_first_entry(&urb_list, struct urb, urb_list);
list_del_init(&urb->urb_list);
-   bh->completing_ep = urb->ep;
__usb_hcd_giveback_urb(urb);
-   bh->completing_ep = NULL;
+   }
+}
+
+
+#define URB_PRIO_HIGH  0
+#define URB_PRIO_LOW   1
+
+static irqreturn_t usb_hcd_gb_urb(int irq, void *__hcd)
+{
+   struct usb_hcd  *hcd = __hcd;
+   struct giveback_urb *bh = &hcd->gb_urb;
+   struct list_headurb_list[2];
+   int i;
+
+   INIT_LIST_HEAD(&urb_list[URB_PRIO_HIGH]);
+   INIT_LIST_HEAD(&urb_list[URB_PRIO_LOW]);
+
+   spin_lock_irq(&bh->lock);
+ restart:
+   list_splice_tail_init(&bh->prio_hi_head, &urb_list[URB_PRIO_HIGH]);
+   list_splice_tail_init(&bh->prio_lo_head, &urb_list[URB_PRIO_LOW]);
+   spin_unlock_irq(&bh->lock);
+
+   for (i = 0; i < ARRAY_SIZE(urb_list); i++) {
+   while (!list_empty(&urb_list[i])) {
+   struct urb *urb;
+
+   urb = list_first_entry(&urb_list[i],
+  struct urb, urb_list);
+   list_del_init(&urb->urb_list);
+   if (i == URB_PRIO_HIGH)
+   bh->completing_ep = urb->ep;
+
+   __usb_hcd_giveback_urb(urb);
+
+   if (i == URB_PRIO_HIGH)
+   bh->completing_ep = NULL;
+
+   if (i == URB_PRIO_LOW &&
+   !list_empty_careful(&urb_list[URB_PRIO_HIGH])) {
+   spin_lock_irq(&bh->lock);
+   goto restart;
+   }
+   }
}
 
/* check if there are new URBs to giveback */

[PATCH 24/25 v2] net/cdc_ncm: Replace tasklet with softirq hrtimer

2017-09-05 Thread Sebastian Andrzej Siewior 
From: Thomas Gleixner 

The bh tasklet is used in invoke the hrtimer (cdc_ncm_tx_timer_cb) in
softirq context. This can be also achieved without the tasklet but with
CLOCK_MONOTONIC_SOFT as hrtimer base.

Signed-off-by: Thomas Gleixner 
Signed-off-by: Anna-Maria Gleixner 
Cc: Oliver Neukum 
Cc: Greg Kroah-Hartman 
Cc: linux-usb@vger.kernel.org
Cc: net...@vger.kernel.org
Acked-by: Greg Kroah-Hartman 
[bigeasy: using usb_get_intfdata() as suggested by Bjørn Mork]
Signed-off-by: Sebastian Andrzej Siewior 
---
On 2017-08-31 15:57:04 [+0200], Bjørn Mork wrote:
> I believe the struct usbnet pointer is redundant.  We already have lots
> of pointers back and forth here.  This should work, but is not tested:
> 
>   struct usbnet *dev = usb_get_intfdata(ctx->control):

I think so, too. Still untested as I don't have a working gadget around.

v1…v2: Updated as suggested by Bjørn and added Greg's Acked-by.

 drivers/net/usb/cdc_ncm.c   | 36 +++-
 include/linux/usb/cdc_ncm.h |  1 -
 2 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 8f572b9f3625..42f7bd90e6a4 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -61,7 +61,6 @@ static bool prefer_mbim;
 module_param(prefer_mbim, bool, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(prefer_mbim, "Prefer MBIM setting on dual NCM/MBIM 
functions");
 
-static void cdc_ncm_txpath_bh(unsigned long param);
 static void cdc_ncm_tx_timeout_start(struct cdc_ncm_ctx *ctx);
 static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *hr_timer);
 static struct usb_driver cdc_ncm_driver;
@@ -777,10 +776,8 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct 
usb_interface *intf, u8 data_
if (!ctx)
return -ENOMEM;
 
-   hrtimer_init(&ctx->tx_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+   hrtimer_init(&ctx->tx_timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL);
ctx->tx_timer.function = &cdc_ncm_tx_timer_cb;
-   ctx->bh.data = (unsigned long)dev;
-   ctx->bh.func = cdc_ncm_txpath_bh;
atomic_set(&ctx->stop, 0);
spin_lock_init(&ctx->mtx);
 
@@ -967,10 +964,7 @@ void cdc_ncm_unbind(struct usbnet *dev, struct 
usb_interface *intf)
 
atomic_set(&ctx->stop, 1);
 
-   if (hrtimer_active(&ctx->tx_timer))
-   hrtimer_cancel(&ctx->tx_timer);
-
-   tasklet_kill(&ctx->bh);
+   hrtimer_cancel(&ctx->tx_timer);
 
/* handle devices with combined control and data interface */
if (ctx->control == ctx->data)
@@ -1348,20 +1342,9 @@ static void cdc_ncm_tx_timeout_start(struct cdc_ncm_ctx 
*ctx)
HRTIMER_MODE_REL);
 }
 
-static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *timer)
+static void cdc_ncm_txpath_bh(struct cdc_ncm_ctx *ctx)
 {
-   struct cdc_ncm_ctx *ctx =
-   container_of(timer, struct cdc_ncm_ctx, tx_timer);
-
-   if (!atomic_read(&ctx->stop))
-   tasklet_schedule(&ctx->bh);
-   return HRTIMER_NORESTART;
-}
-
-static void cdc_ncm_txpath_bh(unsigned long param)
-{
-   struct usbnet *dev = (struct usbnet *)param;
-   struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
+   struct usbnet *dev = usb_get_intfdata(ctx->control);
 
spin_lock_bh(&ctx->mtx);
if (ctx->tx_timer_pending != 0) {
@@ -1379,6 +1362,17 @@ static void cdc_ncm_txpath_bh(unsigned long param)
}
 }
 
+static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *timer)
+{
+   struct cdc_ncm_ctx *ctx =
+   container_of(timer, struct cdc_ncm_ctx, tx_timer);
+
+   if (!atomic_read(&ctx->stop))
+   cdc_ncm_txpath_bh(ctx);
+
+   return HRTIMER_NORESTART;
+}
+
 struct sk_buff *
 cdc_ncm_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
 {
diff --git a/include/linux/usb/cdc_ncm.h b/include/linux/usb/cdc_ncm.h
index 1a59699cf82a..62b506fddf8d 100644
--- a/include/linux/usb/cdc_ncm.h
+++ b/include/linux/usb/cdc_ncm.h
@@ -92,7 +92,6 @@
 struct cdc_ncm_ctx {
struct usb_cdc_ncm_ntb_parameters ncm_parm;
struct hrtimer tx_timer;
-   struct tasklet_struct bh;
 
const struct usb_cdc_ncm_desc *func_desc;
const struct usb_cdc_mbim_desc *mbim_desc;
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: musb: cppi41: don't check early-TX-interrupt for Isoch transfer

2017-03-02 Thread Sebastian Andrzej Siewior 
On 2017-03-01 20:40:04 [-0600], Bin Liu wrote:
> The CPPI41 driver polls register to workaround the pre-mature TX
> interrupt issue, but it causes audio playback underrun when triggered in
> Isoch transfers.
> 
> Isoch doesn't do back-to-back transfers, the TX should be done by the
> time the next transfer is scheduled. So skip this polling workaround for
> Isoch transfer.
> 
> Fixes: a655f481d83d6 ("usb: musb: musb_cppi41: handle pre-mature TX complete 
> interrupt")
> Cc:  #4.1+
> Reported-by: Alexandre Bailon 
> Signed-off-by: Bin Liu 

Looks reasonable.

Acked-by: Sebastian Andrzej Siewior 

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] usb: xhci-mem: use passed in GFP flags instead of GFP_KERNEL

2016-11-11 Thread Sebastian Andrzej Siewior 
On 2016-11-10 22:33:17 [+0300], Dan Carpenter wrote:
> We normally use the passed in gfp flags for allocations, it's just these
> two which were missed.

You seem to be right. xhci_mem_init() has only one caller with
GFP_KERNEL as argument. You could unwind it.

Acked-by: Sebastian Andrzej Siewior 

> Fixes: 22d45f01a836 ("usb/xhci: replace pci_*_consistent() with 
> dma_*_coherent()")
> Signed-off-by: Dan Carpenter 

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 00/14] Equivalent of tcm_usb_gadget with configfs

2015-11-30 Thread Sebastian Andrzej Siewior 
On 11/29/2015 07:16 AM, Nicholas A. Bellinger wrote:
> Ping (2) on this series to Joel + Sebastian.  ;)
> 
> If there aren't any immediate objections on the fs/configfs/ side from
> JLBEC, I'll assume this series can be added as target-pending/for-next
> WIP code this week, yes..?

Yes, please. If there is anything then it will be fixed up later…

> 
> Thank you,
> 
> --nab
> 
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/15] Equivalent of tcm_usb_gadget with configfs

2015-10-20 Thread Sebastian Andrzej Siewior 
On 10/20/2015 02:32 PM, Andrzej Pietrasiewicz wrote:
> Dear All,
> 
> This series adds support to tcm usb gadget for composing it with configfs.

oh great. With this you could have two tcm functions right? (that was
one of the limitations of the legacy gadget due to the API we had).

I will try to take a look on this sometime next week…

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: commit ef11982dd7a657512c362242508bb4021e0d67b6 breaks musb

2015-03-11 Thread Sebastian Andrzej Siewior 
On 03/10/2015 10:22 PM, Felipe Balbi wrote:
> On Thu, Mar 05, 2015 at 12:25:48PM +0530, Amit Virdi wrote:
>> +cc Sebastian, Alan Stern
>>
>> Hello Felipe,
>>
>> On 2/28/2015 3:18 AM, Felipe Balbi wrote:
>>> Hi Amit,
>>>
>>> commit ef11982dd7a657512c362242508bb4021e0d67b6 (Add support for
>>> interrupt EP) actually broke testusb for MUSB when MUSB is the gadget.
>>>
>>> The reason is that we're requesting an endpoint with a 64-byte FIFO, but
>>> later deciding to use the same endpoint with wMaxPacketSize set to 1024
>>> and MUSB errors out because the endpoint was selected for 64-byte only.
>>>
>>> This only happens when trying to set alternate setting 2 and it's pretty
>>> easy to trigger.
>>>
>>
>> This issue was brought up earlier by Sebastian. He even submitted a patch
>> for the same and there were discussions over it.
>> http://www.spinics.net/lists/linux-usb/msg117962.html
>>
>> However, things were never concluded. Using module parameters should get it
>> working but that isn't a clean solution.
>>
>> What do you suggest?
> 
> allocate endpoints based on largest wMaxPacketSize ? If we have more
> FIFO space than needed, that's not a problem. The problem is when we
> allocate a FIFO which is not large enough.

Please either revert the patch that broke things or apply mine until
the whole endpoint allocations is reworked so this is not an issue
anymore.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fixing style warnings.

2015-03-03 Thread Sebastian Andrzej Siewior 
On 03/03/2015 06:19 AM, cfredric wrote:
> Signed-off-by: cfredric 

You could use your full name here.

> ---
>  drivers/usb/core/buffer.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
> index 506b969..89f2e77 100644
> --- a/drivers/usb/core/buffer.c
> +++ b/drivers/usb/core/buffer.c
> @@ -70,7 +70,7 @@ int hcd_buffer_create(struct usb_hcd *hcd)
>   size = pool_max[i];
>   if (!size)
>   continue;
> - snprintf(name, sizeof name, "buffer-%d", size);
> + snprintf(name, sizeof(name), "buffer-%d", size);

This looks like checkpactch warning you fixed. You could add something
to the patch description that says so.

>   hcd->pool[i] = dma_pool_create(name, hcd->self.controller,
>   size, size, 0);
>   if (!hcd->pool[i]) {
> @@ -95,6 +95,7 @@ void hcd_buffer_destroy(struct usb_hcd *hcd)
>  
>   for (i = 0; i < HCD_BUFFER_POOLS; i++) {
>   struct dma_pool *pool = hcd->pool[i];
> +

This looks unrelated.

>   if (pool) {
>   dma_pool_destroy(pool);
>   hcd->pool[i] = NULL;
> 

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: phy: am335x-control: check return value of bus_find_device

2015-02-08 Thread Sebastian Andrzej Siewior 
On 02/08/2015 04:29 PM, David Dueck wrote:
> This fixes a potential null pointer dereference.
> 
> Signed-off-by: David Dueck 

Acked-by: Sebastian Andrzej Siewior 
Fixes: d4332013919a ("driver core: dev_get_drvdata: Don't check for NULL
dev")

Greg, this is a regression since d43320139 ("driver core:
dev_get_drvdata: Don't check for NULL dev"). I didn't check for NULL
after bus_find_device() because I knew that dev_get_drvdata() will do
it.

> ---
>  drivers/usb/phy/phy-am335x-control.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/phy/phy-am335x-control.c 
> b/drivers/usb/phy/phy-am335x-control.c
> index 403fab7..7b3035f 100644
> --- a/drivers/usb/phy/phy-am335x-control.c
> +++ b/drivers/usb/phy/phy-am335x-control.c
> @@ -126,6 +126,9 @@ struct phy_control *am335x_get_phy_control(struct device 
> *dev)
>   return NULL;
>  
>   dev = bus_find_device(&platform_bus_type, NULL, node, match);
> + if (!dev)
> + return NULL;
> +
>   ctrl_usb = dev_get_drvdata(dev)
>   if (!ctrl_usb)
>   return NULL;
> 

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] usb: musb: try a race-free wakeup

2015-02-03 Thread Sebastian Andrzej Siewior 
On 02/02/2015 11:44 PM, Bin Liu wrote:
> Sebastian,

Hi Bin,

> I think I found the cause. Plugging a device behind a hub, this is
> related to runtime PM, so the finish_resume_work() should also be
> added in musb_runtime_resume(), also musb_host_resume_root_hub()
> should not be moved as did in your patch, this call is used to trigger
> the bus resume.
> 
> A patch will be coming soon - tomorrow.

Thank you.

> 
> Regards,
> -Bin.
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: gadget: composite: Provide list of registered functions

2015-01-28 Thread Sebastian Andrzej Siewior 
* Krzysztof Opasiak | 2015-01-28 13:38:33 [+0100]:

>> the first thing a user of usb-gadget has to do, is create a
>> directory, not a file.
>
>This:
>
>$ modprobe libcomposite
>$ mount none -t configfs /sys/kernel/config/usb-gadget
don't you mount it at config and cd to usb-gadget then?

>$ mkdir available_functions
>$ cd available_functions
>$ mkdir functions/ecm.eth0
>$ mkdir configs/c.1
>$ ln -s functions/ecm.eth0 configs/c.1
>$ echo `ls /sys/class/udc | tail -n 1` > UDC
>
>works fine with current kernel and  will *fail* after adding
>available_functions file due to name already taken even
>immediately after system boot.

Seriously? This will break?

>> > Moreover some broken userspace programs may relay on fact that
>> > usb-gadget directory contains only gadget dirs. And some other
>> things
>> > there is a lot of possible breaks in userspace.
>> 
>> if there are any users who rely on the fact that *only* directories
>> exist under usb-gadget, that's wrong.
>
>I know that it's wrong but that's what Documentation/ABI says
>about usb-gadget directory.

You started with an incomplete interface which lacks something as you
describe it essential and you documented it ABI. Now you try to
duct-tape it somewhere so you don't break the ABI you created?
So you have two ways to get out of it:
- ignore this special case ABI breakage you came up with
- drop "usb-gadget" (or deprecate it) and use usb-gadget2 instead with
  available_functions

This reached kindergaden level by now.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] usb: musb: try a race-free wakeup

2015-01-26 Thread Sebastian Andrzej Siewior 
On 01/21/2015 06:06 PM, Bin Liu wrote:
> Hi Sebastian,

Hi Bin,

> With this patch, hubs stop working on TI AM335x EVMs when autosuspend
> is enabled.
> 
> I booted the board, connected a hub to the USB1 host port, it got
> enumerated properly, then connected a thumb drive to the hub, but the
> drive was not enumerated, no any log from kernel. Removing the drive,
> no any kernel message either. Finally removed the hub, no disconnect
> for the hub. Now check MUSB1 DEVCTL register, it value is 0x5D.
> 
> Reverted this patch, the issue disappeared. Or disable usbcore
> autosuspend, the issue did not happen.
> 
> I tested 7+ hubs, all have the same issue.
> 
> I tested on git://git.ti.com/ti-linux-kernel/ti-linux-kernel.git, tag
> ti2014.10.01. have not tested with mainline kernel yet.
> 
> Have you validated this test case?

No, I had no USB-hub attached. It would be nice if you could figure out
which part is responsible for the missing detection because otherwise
suspend/resume is broken.

> Thanks,
> -Bin.

Sebastian

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: musb: stuff leak of struct usb_hcd

2014-12-11 Thread Sebastian Andrzej Siewior 
since the split of host+gadget mode in commit 74c2e9360058 ("usb: musb:
factor out hcd initalization") we leak the usb_hcd struct. We call now
musb_host_cleanup() which does basically usb_remove_hcd() and also sets
the hcd variable to NULL. Doing so makes the finall call to
musb_host_free() basically a nop and the usb_hcd remains around for ever
without anowner.
This patch drops that NULL assignment for that reason.

Fixes: 74c2e9360058 ("usb: musb: factor out hcd initalization")
Cc: sta...@vger.kernel.org
Cc: Daniel Mack 
Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/musb/musb_host.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 855793d701bb..4500610356f2 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -2663,7 +2663,6 @@ void musb_host_cleanup(struct musb *musb)
if (musb->port_mode == MUSB_PORT_MODE_GADGET)
return;
usb_remove_hcd(musb->hcd);
-   musb->hcd = NULL;
 }
 
 void musb_host_free(struct musb *musb)
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: hcd: get/put device and hcd for hcd_buffers()

2014-12-10 Thread Sebastian Andrzej Siewior 
* 'Greg Kroah-Hartman' | 2014-12-09 11:54:15 [-0500]:

>> You can unbind the HCD driver from the PCI-device via sysfs and this is
>> not something not only a developer does. This "unbind" calls the remove
>> function of the driver and the only difference between unbind and rmmod
>> is that the module remains inserted (but this is no news for you).
>
>If unbind causes a problem, it's the same problem that could happen if
>the hardware is hot-unplugged (like on a PCI card.)  Stuff like that
>_does_ need to be fixed, and if your test shows this is a problem, I am
>all for fixing that.

so I tried this with unbind and it didn't explode as assumed. On musb,
for some reason the hcd struct never gets cleaned up. The driver free(s)
URB memory after the hcd_pools are gone but since its size is 32KiB it
uses dma_free_coherent() and this seems to work fine (sice the device is
still there).
So tried the same thing with EHCI. EHCI-hcd cleans up its HCD-struct as
expected so I would have to poke at musb and figure out why it does not
happen.
Also, there is another difference: with EHCI I see first removal of
buffers followed by removal of the pools. So it happens after disconnect
but before the HCD pools are gone. I'm not sure why this happens with
EHCI but not with MUSB. It seems that for some reason unbind triggers an
error on /dev/video0 which makes gst-launch close that file. Once all users
are gone, that hcd struct is cleaned up. Again, it works as I would
expect it with EHCI but not with MUSB. 
So maybe, once I learned how MUSB dafeated the userspace notification
about a gone device I might gain the same behavior that EHCI has. Also
not freed hcd struct of musb looks also important :)

>thanks,
>
>greg k-h

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: hcd: get/put device and hcd for hcd_buffers()

2014-12-09 Thread Sebastian Andrzej Siewior 
On 12/09/2014 04:24 PM, 'Greg Kroah-Hartman' wrote:
> On Mon, Dec 08, 2014 at 09:44:05AM +, David Laight wrote:
>> From: Greg Kroah-Hartman
>>> On Fri, Dec 05, 2014 at 09:03:57PM +0100, Sebastian Andrzej Siewior wrote:
>>>> Consider the following scenario:
>>>> - plugin a webcam
>>>> - play the stream via gst-launch-0.10 v4l2src device=/dev/video0
>>>> - remove the USB-HCD during playback via "rmmod $HCD"
>>>>
>>>> and now wait for the crash
>>>
>>> Which you deserve, why did you ever remove a kernel module?  That's racy
>>> and _never_ recommended, which is why it never happens automatically and
>>> only root can do it.
>>
>> Really drivers and subsystems should have the required locking (etc) to
>> ensure that kernel modules can either be unloaded, or that the unload
>> request itself fails if the device is busy.
>>
>> It shouldn't be considered a 'shoot self in foot' operation.
>> OTOH there are likely to be bugs.
> 
> This is not always the case, sorry, removing a kernel module is a known
> racy condition, and sometimes adding all of the locking required to try
> to make it "safe" just isn't worth it overall, as this is something that
> _only_ a developer does.

I wasn't are of that. rmmod does not mention this. Kconfig does not
mention this and suggest y as default (for MODULE_UNLOAD) . rmmod -f
likely causes problems but this is not the case here. If you want to
avoid rmmod why not mark a driver that it is not safe to remove it? And
why not make it work?

You can unbind the HCD driver from the PCI-device via sysfs and this is
not something not only a developer does. This "unbind" calls the remove
function of the driver and the only difference between unbind and rmmod
is that the module remains inserted (but this is no news for you).

Now, this unbind happens if you choose to pass a PCI-device to a qemu
guest. This is a fairly common use-case for a non-developer since it is
quite easy to setup in virt-manager for instance. All you need is a
hardware with IOMMU support. I used this to get the usb.org testsuite
running in my Windows guest which needs access to EHCI registers). I
could also mention hacking on XHCI and not crashing the physical
machine if something goes south but then I would rise the developer
card again.

I even rmmod & modprobe my mmc controller on my notebook because for
some reason it does not work otherwise after a suspend + resume cycle
(and my motivation to look after this is quite low since I barely use
my notebook at all).

I am really surprised that you as a core developer and maintainer of
the drivers infrastructure say that one should not remove a driver.

> greg k-h

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: hcd: get/put device and hcd for hcd_buffers()

2014-12-08 Thread Sebastian Andrzej Siewior 
* Sebastian Andrzej Siewior | 2014-12-06 00:23:27 [+0100]:

>I had one patch doing that. Let me grab it out on Monday.

okay, this is it. Laurent, any idea why this could not fly? I haven't
seen anything odd so far.

diff --git a/drivers/media/usb/uvc/uvc_driver.c 
b/drivers/media/usb/uvc/uvc_driver.c
index 7c8322d4fc63..d656c7de25ef 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1703,6 +1703,7 @@ static void uvc_unregister_video(struct uvc_device *dev)
stream->vdev = NULL;
 
uvc_debugfs_cleanup_stream(stream);
+   uvc_video_enable(stream, 0);
}
 
/* Decrement the stream count and call uvc_delete explicitly if there
@@ -1950,10 +1951,6 @@ static void uvc_disconnect(struct usb_interface *intf)
 */
usb_set_intfdata(intf, NULL);
 
-   if (intf->cur_altsetting->desc.bInterfaceSubClass ==
-   UVC_SC_VIDEOSTREAMING)
-   return;
-
dev->state |= UVC_DEV_DISCONNECTED;
 
uvc_unregister_video(dev);
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: hcd: get/put device and hcd for hcd_buffers()

2014-12-05 Thread Sebastian Andrzej Siewior 
* Alan Stern | 2014-12-05 16:21:02 [-0500]:

>On Fri, 5 Dec 2014, Sebastian Andrzej Siewior wrote:
>> So instead, I hold the device struct in the HCD and the HCD struct on
>> every USB-buf-alloc. That means after a disconnect we still have a
>> refcount on usb_hcd and device and it will be cleaned "later" once the
>> last USB-buffer is released.
>
>This is not a valid solution.  Notice that your _hcd_buffer_free still 
>dereferences hcd->driver; that will not point to anything useful if you 
>rmmod the HCD.
Hmm. You're right, that one is gone.

>Also, you neglected to move the calls to hcd_buffer_destroy from 
>usb_remove_hcd to hcd_release.
I add them, I didn't move them.

>On the whole, it would be easier if the UVC driver could release its 
>coherent DMA buffers during the disconnect callback.  If that's not 
>feasible we'll have to find some other solution.

I had one patch doing that. Let me grab it out on Monday.

>Alan Stern
>
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: hcd: get/put device and hcd for hcd_buffers()

2014-12-05 Thread Sebastian Andrzej Siewior 
* Greg Kroah-Hartman | 2014-12-05 13:19:32 [-0800]:

>On Fri, Dec 05, 2014 at 09:03:57PM +0100, Sebastian Andrzej Siewior wrote:
>> Consider the following scenario:
>> - plugin a webcam
>> - play the stream via gst-launch-0.10 v4l2src device=/dev/video0…
>> - remove the USB-HCD during playback via "rmmod $HCD"
>> 
>> and now wait for the crash
>
>Which you deserve, why did you ever remove a kernel module?  That's racy
its been found by the testing team and looks legitimate.

>and _never_ recommended, which is why it never happens automatically and
>only root can do it.
I beg your pardon. So it is okay to remove the UVC-driver / plug the
cable and expect that things continue to work but removing the HCD is a
no no? I always assumed that kernel should BUG() no matter what the user
does unless he really begs for it. If there is a race then it is a bug
that deserves to be fixed, right?

>> diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
>> index 506b969ea7fd..01e080a61519 100644
>> --- a/drivers/usb/core/buffer.c
>> +++ b/drivers/usb/core/buffer.c
>> @@ -107,7 +107,7 @@ void hcd_buffer_destroy(struct usb_hcd *hcd)
>>   * better sharing and to leverage mm/slab.c intelligence.
>>   */
>>  
>> -void *hcd_buffer_alloc(
>> +static void *_hcd_buffer_alloc(
>
>Looks like this isn't really needed here, right?

either this or I would have the tree callers if the allocation succeded
or not in order not to take a reference if the allocation failed.

>>  struct usb_bus  *bus,
>>  size_t  size,
>>  gfp_t   mem_flags,
>> @@ -131,7 +131,19 @@ void *hcd_buffer_alloc(
>>  return dma_alloc_coherent(hcd->self.controller, size, dma, mem_flags);
>>  }
>>  
>> -void hcd_buffer_free(
>> +void *hcd_buffer_alloc(struct usb_bus *bus, size_t size, gfp_t mem_flags,
>> +   dma_addr_t *dma)
>> +{
>> +struct usb_hcd *hcd = bus_to_hcd(bus);
>> +void *ret;
>> +
>> +ret = _hcd_buffer_alloc(bus, size, mem_flags, dma);
>> +if (ret)
>> +usb_get_hcd(hcd);
>
>I'm all for some good reference counting, but this is going to cause a
>_lot_ of churn on this reference count, what is the performance issue
>with doing this for every buffer?
The UVC allocates the buffers once and reuses them. If a driver does
any kind of high-performance transfers and allocates new buffers on each
transfer then I would expect this kref_get() is in the noise area. But
if you want real numbers I would have to go ahead and test it.
A single get() on first allocation and its counter part on cleanup would
be enough if you are too concerned about it on every allocation (it
would be transparent to the user).

>thanks,
>
>greg k-h

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: hcd: get/put device and hcd for hcd_buffers()

2014-12-05 Thread Sebastian Andrzej Siewior 
Consider the following scenario:
- plugin a webcam
- play the stream via gst-launch-0.10 v4l2src device=/dev/video0…
- remove the USB-HCD during playback via "rmmod $HCD"

and now wait for the crash

|musb-hdrc musb-hdrc.2.auto: remove, state 1
|usb usb2: USB disconnect, device number 1
|usb 2-1: USB disconnect, device number 3
|uvcvideo: Failed to resubmit video URB (-19).
|musb-hdrc musb-hdrc.2.auto: USB bus 2 deregistered
|musb-hdrc musb-hdrc.1.auto: remove, state 4
|usb usb1: USB disconnect, device number 1
|musb-hdrc musb-hdrc.1.auto: USB bus 1 deregistered
|Unable to handle kernel paging request at virtual address 6b6b6b6f
|pgd = c0004000
|[6b6b6b6f] *pgd=
|Internal error: Oops: 5 [#1] ARM
|Modules linked in: uvcvideo]
|CPU: 0 PID: 2613 Comm: gst-launch-0.10 Tainted: GW3.14.25+ #40
|task: ec2b8100 ti: ec38e000 task.ti: ec38e000
|PC is at hcd_buffer_free+0x64/0xc0
|LR is at uvc_free_urb_buffers+0x34/0x50 [uvcvideo]
|Process gst-launch-0.10 (pid: 2613, stack limit = 0xec38e240)
|[] (hcd_buffer_free)
|[] (uvc_free_urb_buffers [uvcvideo])
|[] (uvc_video_enable [uvcvideo])
|[] (uvc_v4l2_release [uvcvideo])
|[] (v4l2_release [videodev])
|[] (__fput)
|[] (task_work_run)
|[] (do_exit)
|[] (do_group_exit)

as part of the device-removal the HCD removes its dma-buffers, the HCD
structure itself and even the struct device is gone. That means if UVC
removes its URBs after its last user (/dev/videoX) is gone and not from
the ->disconnect() callback then it is too late because the HCD might
gone.

First, I fixed by in the UVC driver by ignoring the UVC_SC_VIDEOSTREAMING
in its ->disconnect callback and calling uvc_video_enable(, 0) in
uvc_unregister_video(). But then I though that it might not be clever to
release that memory if there is userspace using it.

So instead, I hold the device struct in the HCD and the HCD struct on
every USB-buf-alloc. That means after a disconnect we still have a
refcount on usb_hcd and device and it will be cleaned "later" once the
last USB-buffer is released.

Signed-off-by: Sebastian Andrzej Siewior 
---
With this applied, I only see this three times (which is not new)

| [ cut here ]
| WARNING: CPU: 0 PID: 1755 at fs/sysfs/group.c:219 
sysfs_remove_group+0x88/0x98()
| sysfs group c08a70d4 not found for kobject 'event4'
| Modules linked in: uvcvideo videobuf2_vmalloc videobuf2_memops videobuf2_core 
v4l2_common videodev ipv6 musb_hdrc udc_core us]
| CPU: 0 PID: 1755 Comm: gst-launch-0.10 Not tainted 
3.18.0-rc7-00065-gabcefb342fbf-dirty #1813
| [] (unwind_backtrace) from [] (show_stack+0x10/0x14)
| [] (show_stack) from [] (dump_stack+0x80/0x9c)
| [] (dump_stack) from [] (warn_slowpath_common+0x68/0x8c)
| [] (warn_slowpath_common) from [] 
(warn_slowpath_fmt+0x30/0x40)
| [] (warn_slowpath_fmt) from [] 
(sysfs_remove_group+0x88/0x98)
| [] (sysfs_remove_group) from [] (device_del+0x34/0x198)
| [] (device_del) from [] (evdev_disconnect+0x18/0x44)
| [] (evdev_disconnect) from [] 
(__input_unregister_device+0xa4/0x148)
| [] (__input_unregister_device) from [] 
(input_unregister_device+0x40/0x74)
| [] (input_unregister_device) from [] 
(uvc_delete+0x20/0x10c [uvcvideo])
| [] (uvc_delete [uvcvideo]) from [] 
(v4l2_device_release+0x9c/0xc4 [videodev])
| [] (v4l2_device_release [videodev]) from [] 
(device_release+0x2c/0x90)
| [] (device_release) from [] (kobject_release+0x48/0x7c)
| [] (kobject_release) from [] (v4l2_release+0x50/0x78 
[videodev])
| [] (v4l2_release [videodev]) from [] (__fput+0x80/0x1c4)
| [] (__fput) from [] (task_work_run+0xb4/0xe4)
| [] (task_work_run) from [] (do_exit+0x2dc/0x92c)
| [] (do_exit) from [] (do_group_exit+0x3c/0xb0)
| [] (do_group_exit) from [] (__wake_up_parent+0x0/0x18)
| ---[ end trace b54a8f3c8129180e ]---
anyone an idea?

 drivers/usb/core/buffer.c | 30 +-
 drivers/usb/core/hcd.c|  2 ++
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
index 506b969ea7fd..01e080a61519 100644
--- a/drivers/usb/core/buffer.c
+++ b/drivers/usb/core/buffer.c
@@ -107,7 +107,7 @@ void hcd_buffer_destroy(struct usb_hcd *hcd)
  * better sharing and to leverage mm/slab.c intelligence.
  */
 
-void *hcd_buffer_alloc(
+static void *_hcd_buffer_alloc(
struct usb_bus  *bus,
size_t  size,
gfp_t   mem_flags,
@@ -131,7 +131,19 @@ void *hcd_buffer_alloc(
return dma_alloc_coherent(hcd->self.controller, size, dma, mem_flags);
 }
 
-void hcd_buffer_free(
+void *hcd_buffer_alloc(struct usb_bus *bus, size_t size, gfp_t mem_flags,
+  dma_addr_t *dma)
+{
+   struct usb_hcd *hcd = bus_to_hcd(bus);
+   void *ret;
+
+   ret = _hcd_buffer_alloc(bus, size, mem_flags, dma);
+   if (ret)
+   usb_get_hcd(hcd);
+   return ret;
+}
+
+static void _hcd_buffer_free(
struct usb_bus  *bu

[PATCH v4] usb: core: buffer: smallest buffer should start at ARCH_DMA_MINALIGN

2014-12-05 Thread Sebastian Andrzej Siewior 
the following error pops up during "testusb -a -t 10"
| musb-hdrc musb-hdrc.1.auto: dma_pool_free buffer-128, f134e000/be842000 (bad 
dma)
hcd_buffer_create() creates a few buffers, the smallest has 32 bytes of
size. ARCH_KMALLOC_MINALIGN is set to 64 bytes. This combo results in
hcd_buffer_alloc() returning memory which is 32 bytes aligned and it
might by identified by buffer_offset() as another buffer. This means the
buffer which is on a 32 byte boundary will not get freed, instead it
tries to free another buffer with the error message.

This patch fixes the issue by creating the smallest DMA buffer with the
size of ARCH_KMALLOC_MINALIGN (or 32 in case ARCH_KMALLOC_MINALIGN is
smaller). This might be 32, 64 or even 128 bytes. The next three pools
will have the size 128, 512 and 2048.
In case the smallest pool is 128 bytes then we have only three pools
instead of four (and zero the first entry in the array).
The last pool size is always 2048 bytes which is the assumed PAGE_SIZE /
2 of 4096. I doubt it makes sense to continue using PAGE_SIZE / 2 where
we would end up with 8KiB buffer in case we have 16KiB pages.
Instead I think it makes sense to have a common size(s) and extend them
if there is need to.
There is a BUILD_BUG_ON() now in case someone has a minalign of more than
128 bytes.

Cc: sta...@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior 
---
v3…v4: rewrite as suggested per Alan Stern so it is less confusing.
v2…v3:
- rewrite and use usb_init_pool_max() instead. Albeit the value
  __alignof__(x) is known at compile time it can't be used in #if
  statement by the CPP. The max_t and if statment in this patch is
  optimized away by the compiler
- replace PAGE_SIZE / 2 by 2048.

v1…v2: rewrite pool_max so it is less confusing.

 drivers/usb/core/buffer.c | 26 +-
 drivers/usb/core/usb.c|  1 +
 include/linux/usb/hcd.h   |  1 +
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
index 684ef70dc09d..506b969ea7fd 100644
--- a/drivers/usb/core/buffer.c
+++ b/drivers/usb/core/buffer.c
@@ -22,17 +22,25 @@
  */
 
 /* FIXME tune these based on pool statistics ... */
-static const size_tpool_max[HCD_BUFFER_POOLS] = {
-   /* platforms without dma-friendly caches might need to
-* prevent cacheline sharing...
-*/
-   32,
-   128,
-   512,
-   PAGE_SIZE / 2
-   /* bigger --> allocate pages */
+static size_t pool_max[HCD_BUFFER_POOLS] = {
+   32, 128, 512, 2048,
 };
 
+void __init usb_init_pool_max(void)
+{
+   /*
+* The pool_max values must never be smaller than
+* ARCH_KMALLOC_MINALIGN.
+*/
+   if (ARCH_KMALLOC_MINALIGN <= 32)
+   ;   /* Original value is okay */
+   else if (ARCH_KMALLOC_MINALIGN <= 64)
+   pool_max[0] = 64;
+   else if (ARCH_KMALLOC_MINALIGN <= 128)
+   pool_max[0] = 0;/* Don't use this pool */
+   else
+   BUILD_BUG();/* We don't allow this */
+}
 
 /* SETUP primitives */
 
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 2dd2362198d2..29ee9363faa5 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -1051,6 +1051,7 @@ static int __init usb_init(void)
pr_info("%s: USB support disabled\n", usbcore_name);
return 0;
}
+   usb_init_pool_max();
 
retval = usb_debugfs_init();
if (retval)
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index cd96a2bc3388..9e8d161bf2db 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -450,6 +450,7 @@ extern const struct dev_pm_ops usb_hcd_pci_pm_ops;
 #endif /* CONFIG_PCI */
 
 /* pci-ish (pdev null is ok) buffer alloc/mapping support */
+void usb_init_pool_max(void);
 int hcd_buffer_create(struct usb_hcd *hcd);
 void hcd_buffer_destroy(struct usb_hcd *hcd);
 
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH] usb: musb: cppi41: replace TX-complete-timer by TX-FIFO-EMPTY interrupt

2014-12-04 Thread Sebastian Andrzej Siewior 
The TX-interrupt fires (sometimes) too early and therefore we have the
early_tx timer to check periodically if the transfer is done. Initially
I started I started with a 150us delay which seemed to work well.
This value was reduced further, because depending on the usecase a
smaller poll interval was desired.
Instead of tunning the number further I hereby try to get rid of the
timer and instead use the TX-FIFO-empty interrupt. I've been playing
with it for a while and tried various things. Here is a small summary:

- Enable TX-empty interrupt "late"
  The TX-empty interrupt would be enabled after "DMA IRQ" if the FIFO is
  still full. This seems to work in generall but after removing the
  debug code the TX-empty interrupt isn't generated.

- Use one of the two interrups
  In general, the TX-empty interrupt comes after the DMA-interrupt. But
  I've also seen it the other way around. So it not an option.

- Use both interrupts.
  This patch.

For every TX-transfer we receive two interrupts: one from the DMA side
another from USB (FIFO empty). Once we seen both interrupts we consider
the USB-transfer as completed. The "downside" is that we have two
interrupts per TX-transfer. On the other hand we don't have the timer
anymore which may lead may to multiple timer-interrupts especially on
slow USB-disk media (where the device sends plenty of NAKs).

On the testing side:
- mass storage seems to play fine. The write perfomance is unchanged.
- g_zero test 12 gives sometimes "did not went well"

Testing is very welcome.

Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/usb/musb/musb_cppi41.c | 147 +
 drivers/usb/musb/musb_dsps.c   |  90 -
 drivers/usb/musb/musb_dsps.h   |  83 +++
 3 files changed, 158 insertions(+), 162 deletions(-)
 create mode 100644 drivers/usb/musb/musb_dsps.h

diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
index f64fd964dc6d..08846d05e671 100644
--- a/drivers/usb/musb/musb_cppi41.c
+++ b/drivers/usb/musb/musb_cppi41.c
@@ -6,6 +6,7 @@
 #include 
 
 #include "musb_core.h"
+#include "musb_dsps.h"
 
 #define RNDIS_REG(x) (0x80 + ((x - 1) * 4))
 
@@ -32,6 +33,7 @@ struct cppi41_dma_channel {
u8 is_tx;
u8 is_allocated;
u8 usb_toggle;
+   unsigned done_irqs;
 
dma_addr_t buf_addr;
u32 total_len;
@@ -49,8 +51,6 @@ struct cppi41_dma_controller {
struct cppi41_dma_channel rx_channel[MUSB_DMA_NUM_CHANNELS];
struct cppi41_dma_channel tx_channel[MUSB_DMA_NUM_CHANNELS];
struct musb *musb;
-   struct hrtimer early_tx;
-   struct list_head early_tx_list;
u32 rx_mode;
u32 tx_mode;
u32 auto_req;
@@ -184,40 +184,40 @@ static void cppi41_trans_done(struct cppi41_dma_channel 
*cppi41_channel)
}
 }
 
-static enum hrtimer_restart cppi41_recheck_tx_req(struct hrtimer *timer)
+static void glue_tx_fifo_empty_irq(struct musb *musb, u8 epnum, bool enable)
+{
+   struct device *dev = musb->controller;
+   struct platform_device *pdev = to_platform_device(dev->parent);
+   struct dsps_glue *glue = platform_get_drvdata(pdev);
+   const struct dsps_musb_wrapper *wrp = glue->wrp;
+   u32 ep_mask;
+   u32 reg;
+
+   ep_mask = 1 << (16 + epnum);
+   if (enable)
+   reg = wrp->coreintr_set;
+   else
+   reg = wrp->coreintr_clear;
+   musb_writel(musb->ctrl_base, reg, ep_mask);
+}
+
+static void cppi41_txep_complete(struct musb *musb, u8 epnum)
 {
struct cppi41_dma_controller *controller;
-   struct cppi41_dma_channel *cppi41_channel, *n;
-   struct musb *musb;
-   unsigned long flags;
-   enum hrtimer_restart ret = HRTIMER_NORESTART;
+   struct cppi41_dma_channel *cppi41_channel;
 
-   controller = container_of(timer, struct cppi41_dma_controller,
-   early_tx);
-   musb = controller->musb;
+   controller = container_of(musb->dma_controller,
+ struct cppi41_dma_controller, controller);
+   cppi41_channel = &controller->tx_channel[epnum - 1];
 
-   spin_lock_irqsave(&musb->lock, flags);
-   list_for_each_entry_safe(cppi41_channel, n, &controller->early_tx_list,
-   tx_check) {
-   bool empty;
-   struct musb_hw_ep *hw_ep = cppi41_channel->hw_ep;
-
-   empty = musb_is_tx_fifo_empty(hw_ep);
-   if (empty) {
-   list_del_init(&cppi41_channel->tx_check);
-   cppi41_trans_done(cppi41_channel);
-   }
-   }
-
-   if (!list_empty(&controller->early_tx_list) &&
-   !hrtimer_is_queued(&controller->early_tx)) {
-   ret = HRTIMER_RESTART;
-

[PATCH v3] usb: core: buffer: smallest buffer should start at ARCH_DMA_MINALIGN

2014-12-03 Thread Sebastian Andrzej Siewior 
the following error pops up during "testusb -a -t 10"
| musb-hdrc musb-hdrc.1.auto: dma_pool_free buffer-128, f134e000/be842000 (bad 
dma)
hcd_buffer_create() creates a few buffers, the smallest has 32 bytes of
size. ARCH_KMALLOC_MINALIGN is set to 64 bytes. This combo results in
hcd_buffer_alloc() returning memory which is 32 bytes aligned and it
might by identified by buffer_offset() as another buffer. This means the
buffer which is on a 32 byte boundary will not get freed, instead it
tries to free another buffer with the error message.

This patch fixes the issue by creating the smallest DMA buffer with the
size of ARCH_KMALLOC_MINALIGN (or 32 in case ARCH_KMALLOC_MINALIGN is smaller).
This might be 32, 64 or even 128 bytes. The next three pools will have
the size 128, 512 and 2048.
In case the smallest pool is 128 bytes then we have only three pools
instead of four.
The last pool size is always 2048 bytes which is the assumed PAGE_SIZE /
2 of 4096. I doubt it makes sense to continue using PAGE_SIZE / 2 where
we would end up with 8KiB buffer in case we have 16KiB pages.
Instead I think it makes sense to have a common size(s) and extend them
if there is need to.
There is a BUILD_BUG_ON() now in case someone has a minalign of more than
128 bytes.

Cc: sta...@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior 
---
v2…v3:
- rewrite and use usb_init_pool_max() instead. Albeit the value
  __alignof__(x) is known at compile time it can't be used in #if
  statement by the CPP. The max_t and if statment in this patch is
  optimized away by the compiler
- replace PAGE_SIZE / 2 by 2048.

v1…v2: rewrite pool_max so it is less confusing.

 drivers/usb/core/buffer.c | 31 ++-
 drivers/usb/core/usb.c|  1 +
 include/linux/usb/hcd.h   |  1 +
 3 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
index 684ef70dc09d..8f58b88e9c6b 100644
--- a/drivers/usb/core/buffer.c
+++ b/drivers/usb/core/buffer.c
@@ -22,17 +22,30 @@
  */
 
 /* FIXME tune these based on pool statistics ... */
-static const size_tpool_max[HCD_BUFFER_POOLS] = {
-   /* platforms without dma-friendly caches might need to
+static size_t pool_max[HCD_BUFFER_POOLS];
+
+void __init usb_init_pool_max(void)
+{
+   unsigned pool_size = max_t(unsigned, ARCH_KMALLOC_MINALIGN, 32);
+   int i = 0;
+
+   /*
+* platforms without dma-friendly caches might need to
 * prevent cacheline sharing...
+* MIN  [1] [2] [3]
+* 32   128 512 2048
+* 64   128 512 2048
+* 128  512 2048-
 */
-   32,
-   128,
-   512,
-   PAGE_SIZE / 2
-   /* bigger --> allocate pages */
-};
-
+   pool_max[i++] = pool_size;
+   if (pool_size < 128)
+   pool_max[i++] = 128;
+   pool_max[i++] = 512;
+   pool_max[i++] = 2048;
+
+   /* MINALIGN > 128 hasn't been considered yet */
+   BUILD_BUG_ON(ARCH_KMALLOC_MINALIGN > 128);
+}
 
 /* SETUP primitives */
 
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 2dd2362198d2..29ee9363faa5 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -1051,6 +1051,7 @@ static int __init usb_init(void)
pr_info("%s: USB support disabled\n", usbcore_name);
return 0;
}
+   usb_init_pool_max();
 
retval = usb_debugfs_init();
if (retval)
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index cd96a2bc3388..9e8d161bf2db 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -450,6 +450,7 @@ extern const struct dev_pm_ops usb_hcd_pci_pm_ops;
 #endif /* CONFIG_PCI */
 
 /* pci-ish (pdev null is ok) buffer alloc/mapping support */
+void usb_init_pool_max(void);
 int hcd_buffer_create(struct usb_hcd *hcd);
 void hcd_buffer_destroy(struct usb_hcd *hcd);
 
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] dma: cppi41: add a delay while setting the TD bit

2014-12-03 Thread Sebastian Andrzej Siewior 
The manual says that we need to (repeatedly) set the TearDown-bit for
the endpoint in order to get the active transfer descriptor released.
Doing this "real" quick over and over again seems to work but it also
seems that the hardware might not have enough time to breathe. So I
though, hey lets add a udelay() between between the individual sets
of the bit.
This change with the g_zero testcase resulted in a warning about missing
transfer descriptor (we got the tear-down one). It seems that if the
hardware has some time it manages to release the transfer-descriptor on
the completion queue after the teaddown descriptor.
With this change, I observe that the transfer descriptor is released
after 20-30 retry loops.

Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/dma/cppi41.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
index cb97b73482da..54e9db1cd833 100644
--- a/drivers/dma/cppi41.c
+++ b/drivers/dma/cppi41.c
@@ -1,3 +1,4 @@
+#include 
 #include 
 #include 
 #include 
@@ -603,12 +604,16 @@ static int cppi41_tear_down_chan(struct cppi41_channel *c)
 * descriptor before the TD we fetch it from enqueue, it has to be
 * there waiting for us.
 */
-   if (!c->td_seen && c->td_retry)
+   if (!c->td_seen && c->td_retry) {
+   udelay(1);
return -EAGAIN;
-
+   }
WARN_ON(!c->td_retry);
+
if (!c->td_desc_seen) {
desc_phys = cppi41_pop_desc(cdd, c->q_num);
+   if (!desc_phys)
+   desc_phys = cppi41_pop_desc(cdd, c->q_comp_num);
WARN_ON(!desc_phys);
}
 
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] dma: cppi41: wait longer for the HW to return the descriptor

2014-12-03 Thread Sebastian Andrzej Siewior 
For a "complete" teardown we have to wait until the teardown descriptor
is returned by the hardware. The g_zero testcase "testusb -a -t 9" triggers
the following warning quite reliable:

|[ cut here ]
|WARNING: CPU: 0 PID: 0 at drivers/dma/cppi41.c:609 
cppi41_dma_control+0x198/0x304()
|[] (warn_slowpath_null) from []
|[] (cppi41_dma_control) from []
|[] (cppi41_dma_channel_abort [musb_hdrc])
|[] (nuke.constprop.10 [musb_hdrc])
|[] (musb_gadget_disable [musb_hdrc])
|[] (disable_endpoints [usb_f_ss_lb])
|[] (disable_source_sink [usb_f_ss_lb])
|[] (sourcesink_set_alt [usb_f_ss_lb])
|[] (composite_setup [libcomposite])
|[] (musb_g_ep0_irq [musb_hdrc])
|[] (musb_interrupt [musb_hdrc])
|[] (dsps_interrupt [musb_dsps])
|[] (handle_irq_event_percpu)
|[] (handle_irq_event)
|[] (handle_level_irq)
|[] (generic_handle_irq)
|[] (handle_IRQ)
|[] (omap3_intc_handle_irq)

and complains about a TD descriptor which is not returned. I've been
looking at several things and haven't noticed anything unusual that
might lead to this.
The manual says "to try again" until the descriptor comes out. I limited
the amount of retries to 100 retries in order to avoid an infinite number
of retries and so a busy-loop. Back then testing revealed that the
number of retries were around 20-30 so 100 seemed a good upper limit.
This g_zero test reaches without a problem 98 retries and it jumps
sometimes to 101 on am335x-evm and so the WARN_ON() triggers. Same test
run on beaglebone black and the retries start at 122 and my max value so
far was at 128.
So lets rise the limit to 500.

Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/dma/cppi41.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
index a58eec3b2cad..cb97b73482da 100644
--- a/drivers/dma/cppi41.c
+++ b/drivers/dma/cppi41.c
@@ -567,7 +567,7 @@ static int cppi41_tear_down_chan(struct cppi41_channel *c)
reg |= GCR_TEARDOWN;
cppi_writel(reg, c->gcr_reg);
c->td_queued = 1;
-   c->td_retry = 100;
+   c->td_retry = 500;
}
 
if (!c->td_seen || !c->td_desc_seen) {
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: gadget: zero: fix INT endpoint assignment

2014-12-02 Thread Sebastian Andrzej Siewior 
On 11/26/2014 10:40 PM, Alan Stern wrote:
> On Wed, 26 Nov 2014, Sebastian Andrzej Siewior wrote:
> 
>> On 11/26/2014 04:24 PM, Alan Stern wrote:
>>>> On 11/25/2014 10:52 PM, Sebastian Andrzej Siewior wrote:
>>>>> The max packet size within the FS descriptor has to be highest possible
>>>>> value and _not_ the one that is (or will be) used on FS.
>>>>
>>>> The current code sets wMaxPacketSize of FS interrupt endpoint descriptor 
>>>> as 64, which is in accordance with the usb 2.0 specification, section 5.7.3
>>>>
>>>>The maximum allowable interrupt data payload size is 64 bytes
>>>>or less for full-speed. High-speed endpoints are allowed
>>>>maximum data payload sizes up to 1024 bytes.
>>>
>>> The real problem is that we are assuming endpoints can be allocated in
>>> a single way that will work correctly for all possible connection
>>> speeds.  (I suspect it was done this way out of laziness and a desire
>>> to minimize the code size.)  This assumption is obviously incorrect
>>> when the hardware has an interrupt endpoint that supports packet sizes
>>> of 64 but no larger.
>>
>> The code will check properly if you pass 1024 and check the size
>> accordingly. You have to "downsize" your FS descriptor later. This
>> function will only fail to do this if your gadget isn't dual speed. In
>> that case it expects 64 as the upper limit for INT (if I recall
>> correctly).
> 
> It will also fail in situations where you use up a lot of endpoints.  
> For example, let's say the UDC only supports 4 endpoints, one of which
> must have a maxpacket value <= 64.  If you want to have four interrupt
> endpoints, you can't run at high speed -- but you can run at full
> speed.  However, the approach you outlined above won't allow it.

fair enough. So we could try to look for one with 1024 and it fails we
could re-try with 512 and then 64. If all three fail we would continue
without INT support.

>> Ah. And endpoints are never returned to the allocator. Some gadgets set
>> ->private to NULL, other just ignore it and let the core do it. So
>> re-doing the endpoint allocator is probably the right thing to do. And
> 
> The allocator doesn't need to be changed.  We already have a 
> usb_ep_autoconfig_reset() function.

yes, that one that frees them all.

>> then force every gadget to allocate an endpoint for FS/HS/SS and give
>> it back _and_ please edit the copy of the descriptor and not the global
>> "original".
> 
> Yes.
> 
>> But the work will not be done before we have the next release is out
>> and as of now it breaks g_zero on musb, net2280 and maybe others.
> 
> Felipe will have to decide how to handle this.
> 
> Alan Stern
> 
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: gadget: zero: fix INT endpoint assignment

2014-12-02 Thread Sebastian Andrzej Siewior 
On 11/25/2014 08:39 PM, Paul Zimmerman wrote:
>> diff --git a/drivers/usb/gadget/function/f_sourcesink.c 
>> b/drivers/usb/gadget/function/f_sourcesink.c
>> index 80be25b32cd7..7d8f0216e1a6 100644
>> --- a/drivers/usb/gadget/function/f_sourcesink.c
>> +++ b/drivers/usb/gadget/function/f_sourcesink.c
>> @@ -161,7 +161,7 @@ static struct usb_endpoint_descriptor fs_int_source_desc 
>> = {
>>
>>  .bEndpointAddress = USB_DIR_IN,
>>  .bmAttributes = USB_ENDPOINT_XFER_INT,
>> -.wMaxPacketSize =   cpu_to_le16(64),
>> +.wMaxPacketSize =   cpu_to_le16(1024),
> 
> This seems strange. You are setting the max packet size in the FS Intr
> endpoint descriptor to a value that is illegal for FS. Won't that cause
> usb_ep_autoconfig() to fail if the UDC only has a FS Intr endpoint?

Funny at it is, tests have shown to work as expected. Only if UDC is FS
only then it won't pass. This could be fixed…

> Maybe you should set wMaxPacketSize to 0 instead? The ep_matches()
> function in epautoconf.c has this code:
>   /*
>* If the protocol driver hasn't yet decided on wMaxPacketSize
>* and wants to know the maximum possible, provide the info.
>*/
>   if (desc->wMaxPacketSize == 0)
>   desc->wMaxPacketSize = cpu_to_le16(ep->maxpacket_limit);
> 
This means we get most likely the smallest possible endpoint and won't
be able to perform transfers @1024 even if we would have such an
endpoint.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

  1   2   3   4   5   6   7   8   9   10   >