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

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

[Matti Vaittinen]

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 <matti.vaittinen@fi.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
-- 
2.25.4


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[PATCH 2/2] regulator: qcom-labibb: Use disable_irq_nosync from isr

[Matti Vaittinen]

Calling the disable_irq() from irq handler might be a bad idea as
disable_irq() should wait for handlers to run. I don't see why
this wouldn't deadlock in wait_event waiting for the threaded
handler to complete.

Use disable_irq_nosync() instead.

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

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
index 5ac4566f9b7f..40e92670e307 100644
--- a/drivers/regulator/qcom-labibb-regulator.c
+++ b/drivers/regulator/qcom-labibb-regulator.c
@@ -283,7 +283,7 @@ static irqreturn_t qcom_labibb_ocp_isr(int irq, void *chip)
 	 * Disable the interrupt temporarily, or it will fire continuously;
 	 * we will re-enable it in the recovery worker function.
 	 */
-	disable_irq(irq);
+	disable_irq_nosync(irq);
 
 	/* Warn the user for overcurrent */
 	dev_warn(vreg->dev, "Over-Current interrupt fired!\n");
@@ -536,7 +536,7 @@ static irqreturn_t qcom_labibb_sc_isr(int irq, void *chip)
 	 * Disable the interrupt temporarily, or it will fire continuously;
 	 * we will re-enable it in the recovery worker function.
 	 */
-	disable_irq(irq);
+	disable_irq_nosync(irq);
 
 	/* Signal out of regulation event to drivers */
 	regulator_notifier_call_chain(vreg->rdev,
-- 
2.25.4


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

Re: [PATCH 2/2] regulator: qcom-labibb: Use disable_irq_nosync from isr

[AngeloGioacchino Del Regno]

Il 02/02/21 08:37, Matti Vaittinen ha scritto:
> Calling the disable_irq() from irq handler might be a bad idea as
> disable_irq() should wait for handlers to run. I don't see why
> this wouldn't deadlock in wait_event waiting for the threaded
> handler to complete.
> 
> Use disable_irq_nosync() instead.
> 

It didn't deadlock, but looking at it again -- oh my, I agree with you.

Reviewed-by: AngeloGioacchino Del Regno 
<angelogioacchino.delregno@somainline.org>

> Fixes: 390af53e04114 ("regulator: qcom-labibb: Implement short-circuit and over-current IRQs")
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.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 | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
> index 5ac4566f9b7f..40e92670e307 100644
> --- a/drivers/regulator/qcom-labibb-regulator.c
> +++ b/drivers/regulator/qcom-labibb-regulator.c
> @@ -283,7 +283,7 @@ static irqreturn_t qcom_labibb_ocp_isr(int irq, void *chip)
>   	 * Disable the interrupt temporarily, or it will fire continuously;
>   	 * we will re-enable it in the recovery worker function.
>   	 */
> -	disable_irq(irq);
> +	disable_irq_nosync(irq);
>   
>   	/* Warn the user for overcurrent */
>   	dev_warn(vreg->dev, "Over-Current interrupt fired!\n");
> @@ -536,7 +536,7 @@ static irqreturn_t qcom_labibb_sc_isr(int irq, void *chip)
>   	 * Disable the interrupt temporarily, or it will fire continuously;
>   	 * we will re-enable it in the recovery worker function.
>   	 */
> -	disable_irq(irq);
> +	disable_irq_nosync(irq);
>   
>   	/* Signal out of regulation event to drivers */
>   	regulator_notifier_call_chain(vreg->rdev,
> 

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

[AngeloGioacchino Del Regno]

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@fi.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
> 

Re: (subset) [PATCH 1/2] regulator: qcom-labibb: avoid unbalanced IRQ enable

[Mark Brown]

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

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

[Matti Vaittinen]

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