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@xxxxxxxxxx>
> Reviewed-by: Vadim Pasternak <vadimp@xxxxxxxxxx>
> ---
> 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;
>
