[PATCH platform] platform/mellanox: mlxreg-hotplug: remove IRQF_NO_AUTOEN flag from request_irq

[PATCH platform] platform/mellanox: mlxreg-hotplug: remove IRQF_NO_AUTOEN flag from request_irq

[Mykola Kostenok]

This flag causes mlxreg-hotplug probing failure after flag "IRQF_NO_AUTOEN"
has been added to:
	err = devm_request_irq(&pdev->dev, priv->irq,
			       mlxreg_hotplug_irq_handler, IRQF_TRIGGER_FALLING
			       | IRQF_SHARED | IRQF_NO_AUTOEN,
			       "mlxreg-hotplug", priv);

This is because request_threaded_irq() returns EINVAL due to true value of
condition:
((irqflags & IRQF_SHARED) && (irqflags & IRQF_NO_AUTOEN))

Fixes: bee3ecfed0fc9 ("platform/mellanox: mlxreg-hotplug: move to use request_irq by IRQF_NO_AUTOEN flag")
Signed-off-by: Mykola Kostenok <c_mykolak@nvidia.com>
Reviewed-by: Vadim Pasternak <vadimp@nvidia.com>
---
 drivers/platform/mellanox/mlxreg-hotplug.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/platform/mellanox/mlxreg-hotplug.c b/drivers/platform/mellanox/mlxreg-hotplug.c
index a9db2f32658f..07706f0a6d77 100644
--- a/drivers/platform/mellanox/mlxreg-hotplug.c
+++ b/drivers/platform/mellanox/mlxreg-hotplug.c
@@ -683,8 +683,7 @@ static int mlxreg_hotplug_probe(struct platform_device *pdev)
 
 	err = devm_request_irq(&pdev->dev, priv->irq,
 			       mlxreg_hotplug_irq_handler, IRQF_TRIGGER_FALLING
-			       | IRQF_SHARED | IRQF_NO_AUTOEN,
-			       "mlxreg-hotplug", priv);
+			       | IRQF_SHARED, "mlxreg-hotplug", priv);
 	if (err) {
 		dev_err(&pdev->dev, "Failed to request irq: %d\n", err);
 		return err;
-- 
2.20.1

Re: [PATCH platform] platform/mellanox: mlxreg-hotplug: remove IRQF_NO_AUTOEN flag from request_irq

[Hans de Goede]

Hi,

On 6/1/21 1:17 PM, Mykola Kostenok wrote:
> This flag causes mlxreg-hotplug probing failure after flag "IRQF_NO_AUTOEN"
> has been added to:
> 	err = devm_request_irq(&pdev->dev, priv->irq,
> 			       mlxreg_hotplug_irq_handler, IRQF_TRIGGER_FALLING
> 			       | IRQF_SHARED | IRQF_NO_AUTOEN,
> 			       "mlxreg-hotplug", priv);


Right, but if you look at commit bee3ecfed0fc9 ("platform/mellanox: mlxreg-hotplug:
move to use request_irq by IRQF_NO_AUTOEN flag") then that also removes a

disable_irq(priv->irq);

Immediately after this call, if the IRQF_NO_AUTOEN flag is going to be dropped
then that call should be re-added. In cases like this it is usually better to
just do a git revert of the offending patch, that would have also re-added
the disable_irq() call. Also see below.

> This is because request_threaded_irq() returns EINVAL due to true value of
> condition:
> ((irqflags & IRQF_SHARED) && (irqflags & IRQF_NO_AUTOEN))

Is the IRQF_SHARED flag really necessary though ? IOW is the IRQ actually
shared?  If it is really shared then the disable_irq() call will also block
the irq for other users of the irq. Drivers which are properly coded to share
interrupts should thus avoid using disable_irq().  But often the IRQF_SHARED
flag has just been copied from other code without the IRQ actually being
shared.

Please check if the IRQF_SHARED flag is really necessary and if it is not
necessary, please drop that instead.

Regards,

Hans




> Fixes: bee3ecfed0fc9 ("platform/mellanox: mlxreg-hotplug: move to use request_irq by IRQF_NO_AUTOEN flag")
> Signed-off-by: Mykola Kostenok <c_mykolak@nvidia.com>
> Reviewed-by: Vadim Pasternak <vadimp@nvidia.com>
> ---
>  drivers/platform/mellanox/mlxreg-hotplug.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/mellanox/mlxreg-hotplug.c b/drivers/platform/mellanox/mlxreg-hotplug.c
> index a9db2f32658f..07706f0a6d77 100644
> --- a/drivers/platform/mellanox/mlxreg-hotplug.c
> +++ b/drivers/platform/mellanox/mlxreg-hotplug.c
> @@ -683,8 +683,7 @@ static int mlxreg_hotplug_probe(struct platform_device *pdev)
>  
>  	err = devm_request_irq(&pdev->dev, priv->irq,
>  			       mlxreg_hotplug_irq_handler, IRQF_TRIGGER_FALLING
> -			       | IRQF_SHARED | IRQF_NO_AUTOEN,
> -			       "mlxreg-hotplug", priv);
> +			       | IRQF_SHARED, "mlxreg-hotplug", priv);
>  	if (err) {
>  		dev_err(&pdev->dev, "Failed to request irq: %d\n", err);
>  		return err;
> 

RE: [PATCH platform] platform/mellanox: mlxreg-hotplug: remove IRQF_NO_AUTOEN flag from request_irq

[Mykola Kostenok]

Hi,
We must have IRQF_SHARED flag. This is for system interrupts (FAN, PS, cable power, ASIC health) and now it's used for new line card driver, which should be released soon (this is for new modular system).
Actually "mlxreg-hotplug" driver is responsible to instantiate any other driver, which Will share this IRQ line.
So, at the moment "mlxreg-hotplug" is probed, no other users shared this line are exist.

It means that " disable_irq(priv->irq);" in this driver will not impact anyone who is supposed to use this IRQ line. 

It was initial intension to revert commit, added IRQF_NO_AUTOEN flag.
However, it seems safe remove "disable_irq(priv->irq);" line.

But maybe it more logical to just move disable_irq() to mlxreg_hotplug_set_irq().
Until mlxreg_hotplug_set_irq() doesn't open top aggregation mask, interrupts can not get to CPU.
So, maybe I modify patch in this way.

Best regards, Mykola Kostenok


> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: Tuesday, June 1, 2021 3:10 PM
> To: Mykola Kostenok <c_mykolak@nvidia.com>
> Cc: platform-driver-x86@vger.kernel.org
> Subject: Re: [PATCH platform] platform/mellanox: mlxreg-hotplug: remove
> IRQF_NO_AUTOEN flag from request_irq
> 
> Hi,
> 
> On 6/1/21 1:17 PM, Mykola Kostenok wrote:
> > This flag causes mlxreg-hotplug probing failure after flag
> "IRQF_NO_AUTOEN"
> > has been added to:
> > 	err = devm_request_irq(&pdev->dev, priv->irq,
> > 			       mlxreg_hotplug_irq_handler,
> IRQF_TRIGGER_FALLING
> > 			       | IRQF_SHARED | IRQF_NO_AUTOEN,
> > 			       "mlxreg-hotplug", priv);
> 
> 
> Right, but if you look at commit bee3ecfed0fc9 ("platform/mellanox: mlxreg-
> hotplug:
> move to use request_irq by IRQF_NO_AUTOEN flag") then that also removes
> a
> 
> disable_irq(priv->irq);
> 
> Immediately after this call, if the IRQF_NO_AUTOEN flag is going to be
> dropped then that call should be re-added. In cases like this it is usually
> better to just do a git revert of the offending patch, that would have also re-
> added the disable_irq() call. Also see below.
> 
> > This is because request_threaded_irq() returns EINVAL due to true
> > value of
> > condition:
> > ((irqflags & IRQF_SHARED) && (irqflags & IRQF_NO_AUTOEN))
> 
> Is the IRQF_SHARED flag really necessary though ? IOW is the IRQ actually
> shared?  If it is really shared then the disable_irq() call will also block the irq
> for other users of the irq. Drivers which are properly coded to share
> interrupts should thus avoid using disable_irq().  But often the IRQF_SHARED
> flag has just been copied from other code without the IRQ actually being
> shared.
> 
> Please check if the IRQF_SHARED flag is really necessary and if it is not
> necessary, please drop that instead.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> > Fixes: bee3ecfed0fc9 ("platform/mellanox: mlxreg-hotplug: move to use
> > request_irq by IRQF_NO_AUTOEN flag")
> > Signed-off-by: Mykola Kostenok <c_mykolak@nvidia.com>
> > Reviewed-by: Vadim Pasternak <vadimp@nvidia.com>
> > ---
> >  drivers/platform/mellanox/mlxreg-hotplug.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/platform/mellanox/mlxreg-hotplug.c
> > b/drivers/platform/mellanox/mlxreg-hotplug.c
> > index a9db2f32658f..07706f0a6d77 100644
> > --- a/drivers/platform/mellanox/mlxreg-hotplug.c
> > +++ b/drivers/platform/mellanox/mlxreg-hotplug.c
> > @@ -683,8 +683,7 @@ static int mlxreg_hotplug_probe(struct
> > platform_device *pdev)
> >
> >  	err = devm_request_irq(&pdev->dev, priv->irq,
> >  			       mlxreg_hotplug_irq_handler,
> IRQF_TRIGGER_FALLING
> > -			       | IRQF_SHARED | IRQF_NO_AUTOEN,
> > -			       "mlxreg-hotplug", priv);
> > +			       | IRQF_SHARED, "mlxreg-hotplug", priv);
> >  	if (err) {
> >  		dev_err(&pdev->dev, "Failed to request irq: %d\n", err);
> >  		return err;
> >

Re: [PATCH platform] platform/mellanox: mlxreg-hotplug: remove IRQF_NO_AUTOEN flag from request_irq

[Hans de Goede]

Hi,

On 6/1/21 4:47 PM, Mykola Kostenok wrote:
> Hi,
> We must have IRQF_SHARED flag. This is for system interrupts (FAN, PS, cable power, ASIC health) and now it's used for new line card driver, which should be released soon (this is for new modular system).
> Actually "mlxreg-hotplug" driver is responsible to instantiate any other driver, which Will share this IRQ line.
> So, at the moment "mlxreg-hotplug" is probed, no other users shared this line are exist.
> 
> It means that " disable_irq(priv->irq);" in this driver will not impact anyone who is supposed to use this IRQ line. 
> 
> It was initial intension to revert commit, added IRQF_NO_AUTOEN flag.
> However, it seems safe remove "disable_irq(priv->irq);" line.
> 
> But maybe it more logical to just move disable_irq() to mlxreg_hotplug_set_irq().
> Until mlxreg_hotplug_set_irq() doesn't open top aggregation mask, interrupts can not get to CPU.
> So, maybe I modify patch in this way.

This talk about a top aggregation mask and about mlxreg-hotplug instantiating
other devices sounds like this should use MFD (which I think you already do?)
in combination with an irqchip at the MFD driver level which de-multiplexes
the interrupt in per instantiated device interrupts and the per-instantiated-device
drivers then use the child interrupts of this irqchip.  This is how this is normally
done.

Also if you clear the top aggregation mask *before* requesting the IRQ then the
(temporarily) disabling of the IRQ should not be necessary.

For now though, lets just go with a simple revert, including restoring the
disable_irq() call, so that I can queue up this revert as a fix for 5.13.

Regards,

Hans



> 
> Best regards, Mykola Kostenok
> 
> 
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@redhat.com>
>> Sent: Tuesday, June 1, 2021 3:10 PM
>> To: Mykola Kostenok <c_mykolak@nvidia.com>
>> Cc: platform-driver-x86@vger.kernel.org
>> Subject: Re: [PATCH platform] platform/mellanox: mlxreg-hotplug: remove
>> IRQF_NO_AUTOEN flag from request_irq
>>
>> Hi,
>>
>> On 6/1/21 1:17 PM, Mykola Kostenok wrote:
>>> This flag causes mlxreg-hotplug probing failure after flag
>> "IRQF_NO_AUTOEN"
>>> has been added to:
>>> 	err = devm_request_irq(&pdev->dev, priv->irq,
>>> 			       mlxreg_hotplug_irq_handler,
>> IRQF_TRIGGER_FALLING
>>> 			       | IRQF_SHARED | IRQF_NO_AUTOEN,
>>> 			       "mlxreg-hotplug", priv);
>>
>>
>> Right, but if you look at commit bee3ecfed0fc9 ("platform/mellanox: mlxreg-
>> hotplug:
>> move to use request_irq by IRQF_NO_AUTOEN flag") then that also removes
>> a
>>
>> disable_irq(priv->irq);
>>
>> Immediately after this call, if the IRQF_NO_AUTOEN flag is going to be
>> dropped then that call should be re-added. In cases like this it is usually
>> better to just do a git revert of the offending patch, that would have also re-
>> added the disable_irq() call. Also see below.
>>
>>> This is because request_threaded_irq() returns EINVAL due to true
>>> value of
>>> condition:
>>> ((irqflags & IRQF_SHARED) && (irqflags & IRQF_NO_AUTOEN))
>>
>> Is the IRQF_SHARED flag really necessary though ? IOW is the IRQ actually
>> shared?  If it is really shared then the disable_irq() call will also block the irq
>> for other users of the irq. Drivers which are properly coded to share
>> interrupts should thus avoid using disable_irq().  But often the IRQF_SHARED
>> flag has just been copied from other code without the IRQ actually being
>> shared.
>>
>> Please check if the IRQF_SHARED flag is really necessary and if it is not
>> necessary, please drop that instead.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>> Fixes: bee3ecfed0fc9 ("platform/mellanox: mlxreg-hotplug: move to use
>>> request_irq by IRQF_NO_AUTOEN flag")
>>> Signed-off-by: Mykola Kostenok <c_mykolak@nvidia.com>
>>> Reviewed-by: Vadim Pasternak <vadimp@nvidia.com>
>>> ---
>>>  drivers/platform/mellanox/mlxreg-hotplug.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/platform/mellanox/mlxreg-hotplug.c
>>> b/drivers/platform/mellanox/mlxreg-hotplug.c
>>> index a9db2f32658f..07706f0a6d77 100644
>>> --- a/drivers/platform/mellanox/mlxreg-hotplug.c
>>> +++ b/drivers/platform/mellanox/mlxreg-hotplug.c
>>> @@ -683,8 +683,7 @@ static int mlxreg_hotplug_probe(struct
>>> platform_device *pdev)
>>>
>>>  	err = devm_request_irq(&pdev->dev, priv->irq,
>>>  			       mlxreg_hotplug_irq_handler,
>> IRQF_TRIGGER_FALLING
>>> -			       | IRQF_SHARED | IRQF_NO_AUTOEN,
>>> -			       "mlxreg-hotplug", priv);
>>> +			       | IRQF_SHARED, "mlxreg-hotplug", priv);
>>>  	if (err) {
>>>  		dev_err(&pdev->dev, "Failed to request irq: %d\n", err);
>>>  		return err;
>>>
>