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

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

[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

[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 <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 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

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

[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 <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 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++) {

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

[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 <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 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

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

[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 <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 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++) {

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

[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 <stern@rowland.harvard.edu>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 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

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

[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 <stern@rowland.harvard.edu>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 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);

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

[Mauro Carvalho Chehab]

Em Mon, 10 Sep 2018 11:19:57 +0200
Sebastian Andrzej Siewior <bigeasy@linutronix.de> 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.

So, for patches 1 and 2:

Acked-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

Thanks,
Mauro

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

[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 <bigeasy@linutronix.de> 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 <mchehab+samsung@kernel.org>

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

> Thanks,
> Mauro

Sebastian

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

[Greg Kroah-Hartman]

On Mon, Sep 10, 2018 at 11:30:30AM +0200, Sebastian Andrzej Siewior wrote:
> On 2018-09-10 06:25:57 [-0300], Mauro Carvalho Chehab wrote:
> > Em Mon, 10 Sep 2018 11:19:57 +0200
> > Sebastian Andrzej Siewior <bigeasy@linutronix.de> 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 <mchehab+samsung@kernel.org>
> 
> Thank you. It would be nice if this would hit Linus in this cycle :)

I can take these all through my tree, but no, it's too late for 4.19 for
the last patch. Sorry, this is a 4.20 change as it touches the core USB code.

thanks,

greg k-h

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

[Alan Stern]

On Mon, 10 Sep 2018, Sebastian Andrzej Siewior wrote:

> 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 <stern@rowland.harvard.edu>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  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);

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

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

[Alan Stern]

On Mon, 10 Sep 2018, Sebastian Andrzej Siewior wrote:

> 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 <stern@rowland.harvard.edu>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  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);

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

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

[Mauro Carvalho Chehab]

Em Mon, 10 Sep 2018 15:37:58 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> escreveu:

> On Mon, Sep 10, 2018 at 11:30:30AM +0200, Sebastian Andrzej Siewior wrote:
> > On 2018-09-10 06:25:57 [-0300], Mauro Carvalho Chehab wrote:  
> > > Em Mon, 10 Sep 2018 11:19:57 +0200
> > > Sebastian Andrzej Siewior <bigeasy@linutronix.de> 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?

Yeah, it makes more sense to merge at via USB tree. Feel free to add it there.

Regards,
Mauro