diff mbox

[PATCHv6,2/5] USB: gadget: atmel_usba_udc: Request an auto disabled Vbus signal IRQ instead of an auto enabled IRQ request followed by IRQ disable

Message ID 1421945805-31129-3-git-send-email-sylvain.rochet@finsecur.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sylvain Rochet Jan. 22, 2015, 4:56 p.m. UTC 
Vbus IRQ handler needs a started UDC driver to work because it uses
udc->driver, which is set by the UDC start handler. The previous way
chosen was to return from interrupt if udc->driver is NULL using a
spinlock around the check.

We now request an auto disabled (IRQ_NOAUTOEN) Vbus signal IRQ instead
of an auto enabled IRQ followed by disable_irq(). This way we remove the
very small timeslot of enabled IRQ which existed previously between
request() and disable(). We don't need anymore to check if udc->driver
is NULL in IRQ handler.

Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/usb/gadget/udc/atmel_usba_udc.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

Comments

Jean-Christophe PLAGNIOL-VILLARD Jan. 23, 2015, 7:43 a.m. UTC | #1 
> On Jan 23, 2015, at 12:56 AM, Sylvain Rochet <sylvain.rochet@finsecur.com> wrote:
> 
> Vbus IRQ handler needs a started UDC driver to work because it uses
> udc->driver, which is set by the UDC start handler. The previous way
> chosen was to return from interrupt if udc->driver is NULL using a
> spinlock around the check.
> 
> We now request an auto disabled (IRQ_NOAUTOEN) Vbus signal IRQ instead
> of an auto enabled IRQ followed by disable_irq(). This way we remove the
> very small timeslot of enabled IRQ which existed previously between
> request() and disable(). We don't need anymore to check if udc->driver
> is NULL in IRQ handler.
> 
> Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
> Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> drivers/usb/gadget/udc/atmel_usba_udc.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index 40e5fc7..f9f305f 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -1745,10 +1745,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid)
> 
> 	spin_lock(&udc->lock);
> 
> -	/* May happen if Vbus pin toggles during probe() */
> -	if (!udc->driver)
> -		goto out;
> -
> 	vbus = vbus_is_present(udc);
> 	if (vbus != udc->vbus_prev) {
> 		if (vbus) {
> @@ -1769,7 +1765,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid)
> 		udc->vbus_prev = vbus;
> 	}
> 
> -out:
> 	spin_unlock(&udc->lock);
> 
> 	return IRQ_HANDLED;
> @@ -2109,6 +2104,8 @@ static int usba_udc_probe(struct platform_device *pdev)
> 
> 	if (gpio_is_valid(udc->vbus_pin)) {
> 		if (!devm_gpio_request(&pdev->dev, udc->vbus_pin, "atmel_usba_udc")) {
> +			irq_set_status_flags(gpio_to_irq(udc->vbus_pin),
> +					IRQ_NOAUTOEN);

not happy about still using the broken gpio_to_irq API

Linus can pass the IRQ via board file?

and via DT we can use IRQ directly

Best Regards,
J.
> 			ret = devm_request_irq(&pdev->dev,
> 					gpio_to_irq(udc->vbus_pin),
> 					usba_vbus_irq, 0,
> @@ -2118,8 +2115,6 @@ static int usba_udc_probe(struct platform_device *pdev)
> 				dev_warn(&udc->pdev->dev,
> 					 "failed to request vbus irq; "
> 					 "assuming always on\n");
> -			} else {
> -				disable_irq(gpio_to_irq(udc->vbus_pin));
> 			}
> 		} else {
> 			/* gpio_request fail so use -EINVAL for gpio_is_valid */
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Nicolas Ferre Jan. 23, 2015, 8:59 a.m. UTC | #2 
Le 23/01/2015 08:43, Jean-Christophe PLAGNIOL-VILLARD a écrit :
> 
>> On Jan 23, 2015, at 12:56 AM, Sylvain Rochet <sylvain.rochet@finsecur.com> wrote:
>>
>> Vbus IRQ handler needs a started UDC driver to work because it uses
>> udc->driver, which is set by the UDC start handler. The previous way
>> chosen was to return from interrupt if udc->driver is NULL using a
>> spinlock around the check.
>>
>> We now request an auto disabled (IRQ_NOAUTOEN) Vbus signal IRQ instead
>> of an auto enabled IRQ followed by disable_irq(). This way we remove the
>> very small timeslot of enabled IRQ which existed previously between
>> request() and disable(). We don't need anymore to check if udc->driver
>> is NULL in IRQ handler.
>>
>> Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
>> Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
>> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
>> ---
>> drivers/usb/gadget/udc/atmel_usba_udc.c | 9 ++-------
>> 1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> index 40e5fc7..f9f305f 100644
>> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
>> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> @@ -1745,10 +1745,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid)
>>
>> 	spin_lock(&udc->lock);
>>
>> -	/* May happen if Vbus pin toggles during probe() */
>> -	if (!udc->driver)
>> -		goto out;
>> -
>> 	vbus = vbus_is_present(udc);
>> 	if (vbus != udc->vbus_prev) {
>> 		if (vbus) {
>> @@ -1769,7 +1765,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid)
>> 		udc->vbus_prev = vbus;
>> 	}
>>
>> -out:
>> 	spin_unlock(&udc->lock);
>>
>> 	return IRQ_HANDLED;
>> @@ -2109,6 +2104,8 @@ static int usba_udc_probe(struct platform_device *pdev)
>>
>> 	if (gpio_is_valid(udc->vbus_pin)) {
>> 		if (!devm_gpio_request(&pdev->dev, udc->vbus_pin, "atmel_usba_udc")) {
>> +			irq_set_status_flags(gpio_to_irq(udc->vbus_pin),
>> +					IRQ_NOAUTOEN);
> 
> not happy about still using the broken gpio_to_irq API
> 
> Linus can pass the IRQ via board file?
> 
> and via DT we can use IRQ directly

Absolutely not the topic of this patch series.
Maybe the subject of another patch later?

Best regards,


>> 			ret = devm_request_irq(&pdev->dev,
>> 					gpio_to_irq(udc->vbus_pin),
>> 					usba_vbus_irq, 0,
>> @@ -2118,8 +2115,6 @@ static int usba_udc_probe(struct platform_device *pdev)
>> 				dev_warn(&udc->pdev->dev,
>> 					 "failed to request vbus irq; "
>> 					 "assuming always on\n");
>> -			} else {
>> -				disable_irq(gpio_to_irq(udc->vbus_pin));
>> 			}
>> 		} else {
>> 			/* gpio_request fail so use -EINVAL for gpio_is_valid */
>> -- 
>> 2.1.4
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
>
Sylvain Rochet Jan. 23, 2015, 9:39 a.m. UTC | #3 
Hello,

On Fri, Jan 23, 2015 at 09:59:58AM +0100, Nicolas Ferre wrote:
> Le 23/01/2015 08:43, Jean-Christophe PLAGNIOL-VILLARD a écrit :
> > > On Jan 23, 2015, at 12:56 AM, Sylvain Rochet <sylvain.rochet@finsecur.com> wrote:
> > > +			irq_set_status_flags(gpio_to_irq(udc->vbus_pin),
> > > +					IRQ_NOAUTOEN);
> > 
> > not happy about still using the broken gpio_to_irq API
> > 
> > Linus can pass the IRQ via board file?
> > 
> > and via DT we can use IRQ directly

Although I agree on the general principle, this driver is also used by 
non-DT boards.

I have concerns at least for at32ap700x-based boards which use this 
driver and are non-DT. The suggested change requires changing platform 
data of at32ap700x in such a way we may break it in my opinion.


> Absolutely not the topic of this patch series.
> Maybe the subject of another patch later?

Indeed. This series is only about PM support, harmless rework, or 
necessary rework for PM. Changing the way we acquire the IRQ is not 
harmless, this should be checked carefully for DT and particularly 
non-DT boards.


Sylvain
Nicolas Ferre Feb. 5, 2015, 11:28 a.m. UTC | #4 
Le 22/01/2015 17:56, Sylvain Rochet a écrit :
> Vbus IRQ handler needs a started UDC driver to work because it uses
> udc->driver, which is set by the UDC start handler. The previous way
> chosen was to return from interrupt if udc->driver is NULL using a
> spinlock around the check.
> 
> We now request an auto disabled (IRQ_NOAUTOEN) Vbus signal IRQ instead
> of an auto enabled IRQ followed by disable_irq(). This way we remove the
> very small timeslot of enabled IRQ which existed previously between
> request() and disable(). We don't need anymore to check if udc->driver
> is NULL in IRQ handler.
> 
> Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
> Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

> ---
>  drivers/usb/gadget/udc/atmel_usba_udc.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index 40e5fc7..f9f305f 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -1745,10 +1745,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid)
>  
>  	spin_lock(&udc->lock);
>  
> -	/* May happen if Vbus pin toggles during probe() */
> -	if (!udc->driver)
> -		goto out;
> -
>  	vbus = vbus_is_present(udc);
>  	if (vbus != udc->vbus_prev) {
>  		if (vbus) {
> @@ -1769,7 +1765,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid)
>  		udc->vbus_prev = vbus;
>  	}
>  
> -out:
>  	spin_unlock(&udc->lock);
>  
>  	return IRQ_HANDLED;
> @@ -2109,6 +2104,8 @@ static int usba_udc_probe(struct platform_device *pdev)
>  
>  	if (gpio_is_valid(udc->vbus_pin)) {
>  		if (!devm_gpio_request(&pdev->dev, udc->vbus_pin, "atmel_usba_udc")) {
> +			irq_set_status_flags(gpio_to_irq(udc->vbus_pin),
> +					IRQ_NOAUTOEN);
>  			ret = devm_request_irq(&pdev->dev,
>  					gpio_to_irq(udc->vbus_pin),
>  					usba_vbus_irq, 0,
> @@ -2118,8 +2115,6 @@ static int usba_udc_probe(struct platform_device *pdev)
>  				dev_warn(&udc->pdev->dev,
>  					 "failed to request vbus irq; "
>  					 "assuming always on\n");
> -			} else {
> -				disable_irq(gpio_to_irq(udc->vbus_pin));
>  			}
>  		} else {
>  			/* gpio_request fail so use -EINVAL for gpio_is_valid */
>
diff mbox

Patch

diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
index 40e5fc7..f9f305f 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -1745,10 +1745,6 @@  static irqreturn_t usba_vbus_irq(int irq, void *devid)
 
 	spin_lock(&udc->lock);
 
-	/* May happen if Vbus pin toggles during probe() */
-	if (!udc->driver)
-		goto out;
-
 	vbus = vbus_is_present(udc);
 	if (vbus != udc->vbus_prev) {
 		if (vbus) {
@@ -1769,7 +1765,6 @@  static irqreturn_t usba_vbus_irq(int irq, void *devid)
 		udc->vbus_prev = vbus;
 	}
 
-out:
 	spin_unlock(&udc->lock);
 
 	return IRQ_HANDLED;
@@ -2109,6 +2104,8 @@  static int usba_udc_probe(struct platform_device *pdev)
 
 	if (gpio_is_valid(udc->vbus_pin)) {
 		if (!devm_gpio_request(&pdev->dev, udc->vbus_pin, "atmel_usba_udc")) {
+			irq_set_status_flags(gpio_to_irq(udc->vbus_pin),
+					IRQ_NOAUTOEN);
 			ret = devm_request_irq(&pdev->dev,
 					gpio_to_irq(udc->vbus_pin),
 					usba_vbus_irq, 0,
@@ -2118,8 +2115,6 @@  static int usba_udc_probe(struct platform_device *pdev)
 				dev_warn(&udc->pdev->dev,
 					 "failed to request vbus irq; "
 					 "assuming always on\n");
-			} else {
-				disable_irq(gpio_to_irq(udc->vbus_pin));
 			}
 		} else {
 			/* gpio_request fail so use -EINVAL for gpio_is_valid */