[PATCH] platform/surface: aggregator: move to use request_irq by IRQF_NO_AUTOEN flag

[PATCH] platform/surface: aggregator: move to use request_irq by IRQF_NO_AUTOEN flag

[Tian Tao]

disable_irq() after request_irq() still has a time gap in which
interrupts can come. request_irq() with IRQF_NO_AUTOEN flag will
disable IRQ auto-enable because of requesting.

this patch is made base on "add IRQF_NO_AUTOEN for request_irq" which
is being merged: https://lore.kernel.org/patchwork/patch/1388765/

Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
---
 drivers/platform/surface/aggregator/controller.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c
index aa6f37b..00e3828 100644
--- a/drivers/platform/surface/aggregator/controller.c
+++ b/drivers/platform/surface/aggregator/controller.c
@@ -2483,7 +2483,8 @@ int ssam_irq_setup(struct ssam_controller *ctrl)
 	 * interrupt, and let the SAM resume callback during the controller
 	 * resume process clear it.
 	 */
-	const int irqf = IRQF_SHARED | IRQF_ONESHOT | IRQF_TRIGGER_RISING;
+	const int irqf = IRQF_SHARED | IRQF_ONESHOT |
+			 IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN;
 
 	gpiod = gpiod_get(dev, "ssam_wakeup-int", GPIOD_ASIS);
 	if (IS_ERR(gpiod))
@@ -2501,7 +2502,6 @@ int ssam_irq_setup(struct ssam_controller *ctrl)
 		return status;
 
 	ctrl->irq.num = irq;
-	disable_irq(ctrl->irq.num);
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH] platform/surface: aggregator: move to use request_irq by IRQF_NO_AUTOEN flag

[Maximilian Luz]

On 4/7/21 9:00 AM, Tian Tao wrote:
> disable_irq() after request_irq() still has a time gap in which
> interrupts can come. request_irq() with IRQF_NO_AUTOEN flag will
> disable IRQ auto-enable because of requesting.
> 
> this patch is made base on "add IRQF_NO_AUTOEN for request_irq" which
> is being merged: https://lore.kernel.org/patchwork/patch/1388765/
> 
> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>

Looks good to me.

Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>

Thanks,
Max

> ---
>   drivers/platform/surface/aggregator/controller.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c
> index aa6f37b..00e3828 100644
> --- a/drivers/platform/surface/aggregator/controller.c
> +++ b/drivers/platform/surface/aggregator/controller.c
> @@ -2483,7 +2483,8 @@ int ssam_irq_setup(struct ssam_controller *ctrl)
>   	 * interrupt, and let the SAM resume callback during the controller
>   	 * resume process clear it.
>   	 */
> -	const int irqf = IRQF_SHARED | IRQF_ONESHOT | IRQF_TRIGGER_RISING;
> +	const int irqf = IRQF_SHARED | IRQF_ONESHOT |
> +			 IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN;
>   
>   	gpiod = gpiod_get(dev, "ssam_wakeup-int", GPIOD_ASIS);
>   	if (IS_ERR(gpiod))
> @@ -2501,7 +2502,6 @@ int ssam_irq_setup(struct ssam_controller *ctrl)
>   		return status;
>   
>   	ctrl->irq.num = irq;
> -	disable_irq(ctrl->irq.num);
>   	return 0;
>   }
>   
> 

Re: [PATCH] platform/surface: aggregator: move to use request_irq by IRQF_NO_AUTOEN flag

[Hans de Goede]

Hi,

On 4/7/21 9:00 AM, Tian Tao wrote:
> disable_irq() after request_irq() still has a time gap in which
> interrupts can come. request_irq() with IRQF_NO_AUTOEN flag will
> disable IRQ auto-enable because of requesting.

Thank you for your patch, it is good to see the issue of there not
being a simply way to request IRQs in a way where they are initially
disabled being solved.

> this patch is made base on "add IRQF_NO_AUTOEN for request_irq" which
> is being merged: https://lore.kernel.org/patchwork/patch/1388765/

So this is not yet in Linus' tree. When you write "which is being merged",
I assume that means that this is going upstream in the 5.13 window.

So what is the plan to merge follow-up patches using the new API which
the "add IRQF_NO_AUTOEN for request_irq" patch introduces?

Will there be an immutable branch provided with that patch which other
subsystem maintainers can merge ?

Regards,

Hans




> 
> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> ---
>  drivers/platform/surface/aggregator/controller.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c
> index aa6f37b..00e3828 100644
> --- a/drivers/platform/surface/aggregator/controller.c
> +++ b/drivers/platform/surface/aggregator/controller.c
> @@ -2483,7 +2483,8 @@ int ssam_irq_setup(struct ssam_controller *ctrl)
>  	 * interrupt, and let the SAM resume callback during the controller
>  	 * resume process clear it.
>  	 */
> -	const int irqf = IRQF_SHARED | IRQF_ONESHOT | IRQF_TRIGGER_RISING;
> +	const int irqf = IRQF_SHARED | IRQF_ONESHOT |
> +			 IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN;
>  
>  	gpiod = gpiod_get(dev, "ssam_wakeup-int", GPIOD_ASIS);
>  	if (IS_ERR(gpiod))
> @@ -2501,7 +2502,6 @@ int ssam_irq_setup(struct ssam_controller *ctrl)
>  		return status;
>  
>  	ctrl->irq.num = irq;
> -	disable_irq(ctrl->irq.num);
>  	return 0;
>  }
>  
> 

Re: [PATCH] platform/surface: aggregator: move to use request_irq by IRQF_NO_AUTOEN flag

[tiantao (H)]

在 2021/4/7 17:51, Hans de Goede 写道:
> Hi,
>
> On 4/7/21 9:00 AM, Tian Tao wrote:
>> disable_irq() after request_irq() still has a time gap in which
>> interrupts can come. request_irq() with IRQF_NO_AUTOEN flag will
>> disable IRQ auto-enable because of requesting.
> Thank you for your patch, it is good to see the issue of there not
> being a simply way to request IRQs in a way where they are initially
> disabled being solved.
>
>> this patch is made base on "add IRQF_NO_AUTOEN for request_irq" which
>> is being merged: https://lore.kernel.org/patchwork/patch/1388765/
> So this is not yet in Linus' tree. When you write "which is being merged",
> I assume that means that this is going upstream in the 5.13 window.
this is already in linux-next.git.

cbe16f3 genirq: Add IRQF_NO_AUTOEN for request_irq/nmi()
>
> So what is the plan to merge follow-up patches using the new API which
> the "add IRQF_NO_AUTOEN for request_irq" patch introduces?
>
> Will there be an immutable branch provided with that patch which other
> subsystem maintainers can merge ?
>
> Regards,
>
> Hans
>
>
>
>
>> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
>> ---
>>   drivers/platform/surface/aggregator/controller.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c
>> index aa6f37b..00e3828 100644
>> --- a/drivers/platform/surface/aggregator/controller.c
>> +++ b/drivers/platform/surface/aggregator/controller.c
>> @@ -2483,7 +2483,8 @@ int ssam_irq_setup(struct ssam_controller *ctrl)
>>   	 * interrupt, and let the SAM resume callback during the controller
>>   	 * resume process clear it.
>>   	 */
>> -	const int irqf = IRQF_SHARED | IRQF_ONESHOT | IRQF_TRIGGER_RISING;
>> +	const int irqf = IRQF_SHARED | IRQF_ONESHOT |
>> +			 IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN;
>>   
>>   	gpiod = gpiod_get(dev, "ssam_wakeup-int", GPIOD_ASIS);
>>   	if (IS_ERR(gpiod))
>> @@ -2501,7 +2502,6 @@ int ssam_irq_setup(struct ssam_controller *ctrl)
>>   		return status;
>>   
>>   	ctrl->irq.num = irq;
>> -	disable_irq(ctrl->irq.num);
>>   	return 0;
>>   }
>>   
>>
> .
>

Re: [PATCH] platform/surface: aggregator: move to use request_irq by IRQF_NO_AUTOEN flag

[Hans de Goede]

Hi,

On 4/7/21 2:30 PM, tiantao (H) wrote:
> 
> 在 2021/4/7 17:51, Hans de Goede 写道:
>> Hi,
>>
>> On 4/7/21 9:00 AM, Tian Tao wrote:
>>> disable_irq() after request_irq() still has a time gap in which
>>> interrupts can come. request_irq() with IRQF_NO_AUTOEN flag will
>>> disable IRQ auto-enable because of requesting.
>> Thank you for your patch, it is good to see the issue of there not
>> being a simply way to request IRQs in a way where they are initially
>> disabled being solved.
>>
>>> this patch is made base on "add IRQF_NO_AUTOEN for request_irq" which
>>> is being merged: https://lore.kernel.org/patchwork/patch/1388765/
>> So this is not yet in Linus' tree. When you write "which is being merged",
>> I assume that means that this is going upstream in the 5.13 window.
> this is already in linux-next.git.
> 
> cbe16f3 genirq: Add IRQF_NO_AUTOEN for request_irq/nmi()

That does not help though, since all subsystem branches/trees, including
mine are based on 5.12-rc2, which does not contain the commit.

But I see that tglx has already created a signed-tag for the input-subsys
maintainer to solve the same issue, so I can just re-use that:

https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/tag/?h=irq-no-autoen-2021-03-25

Regards,

Hans



>>> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
>>> ---
>>>   drivers/platform/surface/aggregator/controller.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c
>>> index aa6f37b..00e3828 100644
>>> --- a/drivers/platform/surface/aggregator/controller.c
>>> +++ b/drivers/platform/surface/aggregator/controller.c
>>> @@ -2483,7 +2483,8 @@ int ssam_irq_setup(struct ssam_controller *ctrl)
>>>        * interrupt, and let the SAM resume callback during the controller
>>>        * resume process clear it.
>>>        */
>>> -    const int irqf = IRQF_SHARED | IRQF_ONESHOT | IRQF_TRIGGER_RISING;
>>> +    const int irqf = IRQF_SHARED | IRQF_ONESHOT |
>>> +             IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN;
>>>         gpiod = gpiod_get(dev, "ssam_wakeup-int", GPIOD_ASIS);
>>>       if (IS_ERR(gpiod))
>>> @@ -2501,7 +2502,6 @@ int ssam_irq_setup(struct ssam_controller *ctrl)
>>>           return status;
>>>         ctrl->irq.num = irq;
>>> -    disable_irq(ctrl->irq.num);
>>>       return 0;
>>>   }
>>>  
>> .
>>
> 

Re: [PATCH] platform/surface: aggregator: move to use request_irq by IRQF_NO_AUTOEN flag

[Hans de Goede]

Hi,

On 4/7/21 9:00 AM, Tian Tao wrote:
> disable_irq() after request_irq() still has a time gap in which
> interrupts can come. request_irq() with IRQF_NO_AUTOEN flag will
> disable IRQ auto-enable because of requesting.
> 
> this patch is made base on "add IRQF_NO_AUTOEN for request_irq" which
> is being merged: https://lore.kernel.org/patchwork/patch/1388765/
> 
> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans


> ---
>  drivers/platform/surface/aggregator/controller.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c
> index aa6f37b..00e3828 100644
> --- a/drivers/platform/surface/aggregator/controller.c
> +++ b/drivers/platform/surface/aggregator/controller.c
> @@ -2483,7 +2483,8 @@ int ssam_irq_setup(struct ssam_controller *ctrl)
>  	 * interrupt, and let the SAM resume callback during the controller
>  	 * resume process clear it.
>  	 */
> -	const int irqf = IRQF_SHARED | IRQF_ONESHOT | IRQF_TRIGGER_RISING;
> +	const int irqf = IRQF_SHARED | IRQF_ONESHOT |
> +			 IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN;
>  
>  	gpiod = gpiod_get(dev, "ssam_wakeup-int", GPIOD_ASIS);
>  	if (IS_ERR(gpiod))
> @@ -2501,7 +2502,6 @@ int ssam_irq_setup(struct ssam_controller *ctrl)
>  		return status;
>  
>  	ctrl->irq.num = irq;
> -	disable_irq(ctrl->irq.num);
>  	return 0;
>  }
>  
>