[PATCH] usb/isp1760: set IRQ flags properly

[PATCH] usb/isp1760: set IRQ flags properly

[Valentin Rothberg]

The IRQF_DISABLED is a NOOP and scheduled to be removed.  According to
commit e58aa3d2d0cc (genirq: Run irq handlers with interrupts disabled)
running IRQ handlers with interrupts enabled can cause stack overflows
when the interrupt line of the issuing device is still active.

This patch removes using this deprecated flag and additionally removes
redundantly setting IRQF_SHARED.

Signed-off-by: Valentin Rothberg <Valentin.Rothberg@lip6.fr>
---
 drivers/usb/isp1760/isp1760-core.c | 5 ++---
 drivers/usb/isp1760/isp1760-udc.c  | 4 ++--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/isp1760/isp1760-core.c b/drivers/usb/isp1760/isp1760-core.c
index b982755..9f4a0de 100644
--- a/drivers/usb/isp1760/isp1760-core.c
+++ b/drivers/usb/isp1760/isp1760-core.c
@@ -145,14 +145,13 @@ int isp1760_register(struct resource *mem, int irq, unsigned long irqflags,
 
 	if (IS_ENABLED(CONFIG_USB_ISP1760_HCD) && !usb_disabled()) {
 		ret = isp1760_hcd_register(&isp->hcd, isp->regs, mem, irq,
-					   irqflags | IRQF_SHARED, dev);
+					   irqflags, dev);
 		if (ret < 0)
 			return ret;
 	}
 
 	if (IS_ENABLED(CONFIG_USB_ISP1761_UDC) && !udc_disabled) {
-		ret = isp1760_udc_register(isp, irq, irqflags | IRQF_SHARED |
-					   IRQF_DISABLED);
+		ret = isp1760_udc_register(isp, irq, irqflags);
 		if (ret < 0) {
 			isp1760_hcd_unregister(&isp->hcd);
 			return ret;
diff --git a/drivers/usb/isp1760/isp1760-udc.c b/drivers/usb/isp1760/isp1760-udc.c
index 9612d79..0b46ff0 100644
--- a/drivers/usb/isp1760/isp1760-udc.c
+++ b/drivers/usb/isp1760/isp1760-udc.c
@@ -1451,8 +1451,8 @@ int isp1760_udc_register(struct isp1760_device *isp, int irq,
 
 	sprintf(udc->irqname, "%s (udc)", devname);
 
-	ret = request_irq(irq, isp1760_udc_irq, IRQF_SHARED | IRQF_DISABLED |
-			  irqflags, udc->irqname, udc);
+	ret = request_irq(irq, isp1760_udc_irq, IRQF_SHARED | irqflags,
+			  udc->irqname, udc);
 	if (ret < 0)
 		goto error;
 
-- 
1.9.1

Re: [PATCH] usb/isp1760: set IRQ flags properly

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 510 bytes --]

On Fri, Feb 27, 2015 at 02:29:40PM +0100, Valentin Rothberg wrote:
> The IRQF_DISABLED is a NOOP and scheduled to be removed.  According to
> commit e58aa3d2d0cc (genirq: Run irq handlers with interrupts disabled)
> running IRQ handlers with interrupts enabled can cause stack overflows
> when the interrupt line of the issuing device is still active.
> 
> This patch removes using this deprecated flag and additionally removes
> redundantly setting IRQF_SHARED.

why is it redundant ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb/isp1760: set IRQ flags properly

[Valentin Rothberg]

On Fri, Feb 27, 2015 at 4:24 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Fri, Feb 27, 2015 at 02:29:40PM +0100, Valentin Rothberg wrote:
>> The IRQF_DISABLED is a NOOP and scheduled to be removed.  According to
>> commit e58aa3d2d0cc (genirq: Run irq handlers with interrupts disabled)
>> running IRQ handlers with interrupts enabled can cause stack overflows
>> when the interrupt line of the issuing device is still active.
>>
>> This patch removes using this deprecated flag and additionally removes
>> redundantly setting IRQF_SHARED.
>
> why is it redundant ?

It's redundant in the call of isp1760_udc_register() as this function
sets the flag by requesting the IRQ.  I mistakenly removed it also in
the call of isp1760_hcd_register() which does not alter the passed
irqflags.  I will fix this in a second version of this patch.  I am
sorry for this mistake.

 Valentin

> --
> balbi

Re: [PATCH] usb/isp1760: set IRQ flags properly

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1029 bytes --]

On Fri, Feb 27, 2015 at 04:48:39PM +0100, Valentin Rothberg wrote:
> On Fri, Feb 27, 2015 at 4:24 PM, Felipe Balbi <balbi@ti.com> wrote:
> > On Fri, Feb 27, 2015 at 02:29:40PM +0100, Valentin Rothberg wrote:
> >> The IRQF_DISABLED is a NOOP and scheduled to be removed.  According to
> >> commit e58aa3d2d0cc (genirq: Run irq handlers with interrupts disabled)
> >> running IRQ handlers with interrupts enabled can cause stack overflows
> >> when the interrupt line of the issuing device is still active.
> >>
> >> This patch removes using this deprecated flag and additionally removes
> >> redundantly setting IRQF_SHARED.
> >
> > why is it redundant ?
> 
> It's redundant in the call of isp1760_udc_register() as this function
> sets the flag by requesting the IRQ.  I mistakenly removed it also in
> the call of isp1760_hcd_register() which does not alter the passed
> irqflags.  I will fix this in a second version of this patch.  I am
> sorry for this mistake.

no problem, s**t happens ;)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v2] usb/isp1760: set IRQ flags properly

[Valentin Rothberg]

The IRQF_DISABLED is a NOOP and scheduled to be removed.  According to
commit e58aa3d2d0cc ("genirq: Run irq handlers with interrupts
disabled") running IRQ handlers with interrupts enabled can cause stack
overflows when the interrupt line of the issuing device is still active.

This patch removes using this deprecated flag and additionally removes
redundantly setting IRQF_SHARED for isp1760_udc_register().

Signed-off-by: Valentin Rothberg <Valentin.Rothberg@lip6.fr>
---
v2: Reverted change that removed IRQF_SHARED in the call of
    isp1760_hcd_register().
---
 drivers/usb/isp1760/isp1760-core.c | 3 +--
 drivers/usb/isp1760/isp1760-udc.c  | 4 ++--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/isp1760/isp1760-core.c b/drivers/usb/isp1760/isp1760-core.c
index b982755..bfa402c 100644
--- a/drivers/usb/isp1760/isp1760-core.c
+++ b/drivers/usb/isp1760/isp1760-core.c
@@ -151,8 +151,7 @@ int isp1760_register(struct resource *mem, int irq, unsigned long irqflags,
 	}
 
 	if (IS_ENABLED(CONFIG_USB_ISP1761_UDC) && !udc_disabled) {
-		ret = isp1760_udc_register(isp, irq, irqflags | IRQF_SHARED |
-					   IRQF_DISABLED);
+		ret = isp1760_udc_register(isp, irq, irqflags);
 		if (ret < 0) {
 			isp1760_hcd_unregister(&isp->hcd);
 			return ret;
diff --git a/drivers/usb/isp1760/isp1760-udc.c b/drivers/usb/isp1760/isp1760-udc.c
index 9612d79..0b46ff0 100644
--- a/drivers/usb/isp1760/isp1760-udc.c
+++ b/drivers/usb/isp1760/isp1760-udc.c
@@ -1451,8 +1451,8 @@ int isp1760_udc_register(struct isp1760_device *isp, int irq,
 
 	sprintf(udc->irqname, "%s (udc)", devname);
 
-	ret = request_irq(irq, isp1760_udc_irq, IRQF_SHARED | IRQF_DISABLED |
-			  irqflags, udc->irqname, udc);
+	ret = request_irq(irq, isp1760_udc_irq, IRQF_SHARED | irqflags,
+			  udc->irqname, udc);
 	if (ret < 0)
 		goto error;
 
-- 
1.9.1

Re: [PATCH v2] usb/isp1760: set IRQ flags properly

[laurent.pinchart]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 2287 bytes --]

Hi Valentin,

Thank  you for the patch.

On Sun Mar 01 2015 17:54:32 GMT+0200 (EET), Valentin Rothberg wrote:
> The IRQF_DISABLED is a NOOP and scheduled to be removed.  According to
> commit e58aa3d2d0cc ("genirq: Run irq handlers with interrupts
> disabled") running IRQ handlers with interrupts enabled can cause stack
> overflows when the interrupt line of the issuing device is still active.
> 
> This patch removes using this deprecated flag and additionally removes
> redundantly setting IRQF_SHARED for isp1760_udc_register().
> 
> Signed-off-by: Valentin Rothberg <Valentin.Rothberg@lip6.fr>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> v2: Reverted change that removed IRQF_SHARED in the call of
>     isp1760_hcd_register().
> ---
>  drivers/usb/isp1760/isp1760-core.c | 3 +--
>  drivers/usb/isp1760/isp1760-udc.c  | 4 ++--
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/isp1760/isp1760-core.c b/drivers/usb/isp1760/isp1760-core.c
> index b982755..bfa402c 100644
> --- a/drivers/usb/isp1760/isp1760-core.c
> +++ b/drivers/usb/isp1760/isp1760-core.c
> @@ -151,8 +151,7 @@ int isp1760_register(struct resource *mem, int irq, unsigned long irqflags,
>  	}
>  
>  	if (IS_ENABLED(CONFIG_USB_ISP1761_UDC) && !udc_disabled) {
> -		ret = isp1760_udc_register(isp, irq, irqflags | IRQF_SHARED |
> -					   IRQF_DISABLED);
> +		ret = isp1760_udc_register(isp, irq, irqflags);
>  		if (ret < 0) {
>  			isp1760_hcd_unregister(&isp->hcd);
>  			return ret;
> diff --git a/drivers/usb/isp1760/isp1760-udc.c b/drivers/usb/isp1760/isp1760-udc.c
> index 9612d79..0b46ff0 100644
> --- a/drivers/usb/isp1760/isp1760-udc.c
> +++ b/drivers/usb/isp1760/isp1760-udc.c
> @@ -1451,8 +1451,8 @@ int isp1760_udc_register(struct isp1760_device *isp, int irq,
>  
>  	sprintf(udc->irqname, "%s (udc)", devname);
>  
> -	ret = request_irq(irq, isp1760_udc_irq, IRQF_SHARED | IRQF_DISABLED |
> -			  irqflags, udc->irqname, udc);
> +	ret = request_irq(irq, isp1760_udc_irq, IRQF_SHARED | irqflags,
> +			  udc->irqname, udc);
>  	if (ret < 0)
>  		goto error;
>  
> -- 
> 1.9.1
> 
>ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2] usb/isp1760: set IRQ flags properly

[Valentin Rothberg]

Andrew Morton asked me to prepare a big patch that removes
IRQF_DISABLED entirely (see https://lkml.org/lkml/2015/3/2/819).  It
seems a good way to get rid of the few references and the definition
at once.

So please don't apply this patch.  After, I can prepare another patch
later that takes care of the redundant setting of IRQF_SHARED for
isp1760_udc_register().

Kind regards,
 Valentin

On Mon, Mar 2, 2015 at 4:30 PM,  <laurent.pinchart@ideasonboard.com> wrote:
> Hi Valentin,
>
> Thank  you for the patch.
>
> On Sun Mar 01 2015 17:54:32 GMT+0200 (EET), Valentin Rothberg wrote:
>> The IRQF_DISABLED is a NOOP and scheduled to be removed.  According to
>> commit e58aa3d2d0cc ("genirq: Run irq handlers with interrupts
>> disabled") running IRQ handlers with interrupts enabled can cause stack
>> overflows when the interrupt line of the issuing device is still active.
>>
>> This patch removes using this deprecated flag and additionally removes
>> redundantly setting IRQF_SHARED for isp1760_udc_register().
>>
>> Signed-off-by: Valentin Rothberg <Valentin.Rothberg@lip6.fr>
>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
>> ---
>> v2: Reverted change that removed IRQF_SHARED in the call of
>>     isp1760_hcd_register().
>> ---
>>  drivers/usb/isp1760/isp1760-core.c | 3 +--
>>  drivers/usb/isp1760/isp1760-udc.c  | 4 ++--
>>  2 files changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/isp1760/isp1760-core.c b/drivers/usb/isp1760/isp1760-core.c
>> index b982755..bfa402c 100644
>> --- a/drivers/usb/isp1760/isp1760-core.c
>> +++ b/drivers/usb/isp1760/isp1760-core.c
>> @@ -151,8 +151,7 @@ int isp1760_register(struct resource *mem, int irq, unsigned long irqflags,
>>       }
>>
>>       if (IS_ENABLED(CONFIG_USB_ISP1761_UDC) && !udc_disabled) {
>> -             ret = isp1760_udc_register(isp, irq, irqflags | IRQF_SHARED |
>> -                                        IRQF_DISABLED);
>> +             ret = isp1760_udc_register(isp, irq, irqflags);
>>               if (ret < 0) {
>>                       isp1760_hcd_unregister(&isp->hcd);
>>                       return ret;
>> diff --git a/drivers/usb/isp1760/isp1760-udc.c b/drivers/usb/isp1760/isp1760-udc.c
>> index 9612d79..0b46ff0 100644
>> --- a/drivers/usb/isp1760/isp1760-udc.c
>> +++ b/drivers/usb/isp1760/isp1760-udc.c
>> @@ -1451,8 +1451,8 @@ int isp1760_udc_register(struct isp1760_device *isp, int irq,
>>
>>       sprintf(udc->irqname, "%s (udc)", devname);
>>
>> -     ret = request_irq(irq, isp1760_udc_irq, IRQF_SHARED | IRQF_DISABLED |
>> -                       irqflags, udc->irqname, udc);
>> +     ret = request_irq(irq, isp1760_udc_irq, IRQF_SHARED | irqflags,
>> +                       udc->irqname, udc);
>>       if (ret < 0)
>>               goto error;
>>
>> --
>> 1.9.1
>>
>>