[1/2] regulator: qcom-labibb: avoid unbalanced IRQ enable

Commit Message

Vaittinen, Matti Feb. 2, 2021, 7:36 a.m. UTC

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.

Fixes: 390af53e04114 ("regulator: qcom-labibb: Implement short-circuit and over-current IRQs")

Signed-off-by: Matti Vaittinen <[e-mail]>
---
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(-)


base-commit: 4288b4ccda966c2a49ec7c67100208378bdb34d2

Comments

AngeloGioacchino Del Regno Feb. 2, 2021, 2:42 p.m. UTC | #1

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 <[e-mail]>
> ---
> 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
>

Mark Brown Feb. 2, 2021, 4:32 p.m. UTC | #2

On Tue, 2 Feb 2021 09:36:34 +0200, Matti Vaittinen wrote:
> 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.
> 
> Fixes: 390af53e04114 ("regulator: qcom-labibb: Implement short-circuit and over-current IRQs")

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[2/2] regulator: qcom-labibb: Use disable_irq_nosync from isr
      commit: 337710b3121a4f4183c38ff056f6f9ef516cc34f

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Vaittinen, Matti Feb. 3, 2021, 6:15 a.m. UTC | #3

Hello Angelo,

On Tue, 2021-02-02 at 15:42 +0100, AngeloGioacchino Del Regno wrote:
> 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.

Then the comment just above this check should be adjusted. It states:
/*
 * If we (unlikely) can't read this register, to prevent hardware
 * damage at all costs, we assume that the overcurrent event was
 * real; Moreover, if the status register is not signaling OCP,
 * it was a spurious event, so it's all ok.
 */

The " if the status register is not signaling OCP, it was a spurious
event, so it's all ok." is incredibly misleading. That comment combined
with comment above qcom_labibb_check_ocp_status()

 * This function checks the STATUS1 register for the VREG_OK bit: if it
is
 * set, then there is no Over-Current event.
 *
 * Returns: Zero if there is no over-current, 1 if in over-current or
 *          negative number for error

made me to _assume_ that when qcom_labibb_check_ocp_status returns zero
we have spurious event - for which just returning the IRQ_NONE should
be perfectly sane thing to do.

> 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?

As I said, I don't have the HW or specifications or expertise with this
IC. If this was not spurious event then I don't know what is the right
thing to do. I am just shooting this blindly. Feel free to take over
this fix and also adjust the comments so that they match the HW
behaviour :)

Best Regards
	--Matti

Patch

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++;