linux-kernel - Re: [PATCH 1/2] regulator: qcom-labibb: avoid unbalanced IRQ enable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 2 Feb 2021 15:42:20 +0100
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...ainline.org>
To:     Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
        mazziesaccount@...il.com
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] regulator: qcom-labibb: avoid unbalanced IRQ enable

Il 02/02/21 08:36, Matti Vaittinen ha scritto:
> If a spurious OCP IRQ occurs the isr schedules delayed work
> but does not disable the IRQ. The delayed work assumes IRQ was
> disabled in handler and attempts enabling it again causing
> unbalanced enable.
> 

You break the logic like this. Though, I also see the problem.
It is critical for the recovery worker to be executed whenever we enter
the OCP interrupt routine, as we get in there only something wrong
happened.

Please fix this patch.
P.S.: You can't disable irq before qcom_labibb_check_ocp_status;
       perhaps just after it, or in the if branch before goto?

Thank you!
-- Angelo

> Fixes: 390af53e04114 ("regulator: qcom-labibb: Implement short-circuit and over-current IRQs")
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
> ---
> This fix is done purely based on code reading. No testing is done.
> 
> I don't have the HW (and even if I did I might have hard time producing
> these errors) I have not tested this and I am unsure if my code-reading
> is correct => I would _really_ appreciate second opinion and/or testing
> 
>   drivers/regulator/qcom-labibb-regulator.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
> index dbb4511c3c6d..5ac4566f9b7f 100644
> --- a/drivers/regulator/qcom-labibb-regulator.c
> +++ b/drivers/regulator/qcom-labibb-regulator.c
> @@ -275,7 +275,7 @@ static irqreturn_t qcom_labibb_ocp_isr(int irq, void *chip)
>   	ret = qcom_labibb_check_ocp_status(vreg);
>   	if (ret == 0) {
>   		vreg->ocp_irq_count = 0;
> -		goto end;
> +		return IRQ_NONE;
>   	}
>   	vreg->ocp_irq_count++;
>   
> 
> base-commit: 4288b4ccda966c2a49ec7c67100208378bdb34d2
>