All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] regulator: qcom-labibb: avoid unbalanced IRQ enable
@ 2021-02-02  7:36 Matti Vaittinen
  2021-02-02  7:37 ` [PATCH 2/2] regulator: qcom-labibb: Use disable_irq_nosync from isr Matti Vaittinen
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Matti Vaittinen @ 2021-02-02  7:36 UTC (permalink / raw)
  To: matti.vaittinen, mazziesaccount
  Cc: Andy Gross, Bjorn Andersson, Liam Girdwood, Mark Brown,
	AngeloGioacchino Del Regno, linux-arm-msm, linux-kernel

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 =] 

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] regulator: qcom-labibb: Use disable_irq_nosync from isr
  2021-02-02  7:36 [PATCH 1/2] regulator: qcom-labibb: avoid unbalanced IRQ enable Matti Vaittinen
@ 2021-02-02  7:37 ` Matti Vaittinen
  2021-02-02 14:36   ` AngeloGioacchino Del Regno
  2021-02-02 14:42 ` [PATCH 1/2] regulator: qcom-labibb: avoid unbalanced IRQ enable AngeloGioacchino Del Regno
  2021-02-02 16:32 ` (subset) " Mark Brown
  2 siblings, 1 reply; 6+ messages in thread
From: Matti Vaittinen @ 2021-02-02  7:37 UTC (permalink / raw)
  To: matti.vaittinen, mazziesaccount
  Cc: Andy Gross, Bjorn Andersson, Liam Girdwood, Mark Brown,
	AngeloGioacchino Del Regno, linux-arm-msm, linux-kernel

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 =] 

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] regulator: qcom-labibb: Use disable_irq_nosync from isr
  2021-02-02  7:37 ` [PATCH 2/2] regulator: qcom-labibb: Use disable_irq_nosync from isr Matti Vaittinen
@ 2021-02-02 14:36   ` AngeloGioacchino Del Regno
  0 siblings, 0 replies; 6+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-02-02 14:36 UTC (permalink / raw)
  To: Matti Vaittinen, mazziesaccount
  Cc: Andy Gross, Bjorn Andersson, Liam Girdwood, Mark Brown,
	linux-arm-msm, linux-kernel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] regulator: qcom-labibb: avoid unbalanced IRQ enable
  2021-02-02  7:36 [PATCH 1/2] regulator: qcom-labibb: avoid unbalanced IRQ enable Matti Vaittinen
  2021-02-02  7:37 ` [PATCH 2/2] regulator: qcom-labibb: Use disable_irq_nosync from isr Matti Vaittinen
@ 2021-02-02 14:42 ` AngeloGioacchino Del Regno
  2021-02-03  6:15   ` Matti Vaittinen
  2021-02-02 16:32 ` (subset) " Mark Brown
  2 siblings, 1 reply; 6+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-02-02 14:42 UTC (permalink / raw)
  To: Matti Vaittinen, mazziesaccount
  Cc: Andy Gross, Bjorn Andersson, Liam Girdwood, Mark Brown,
	linux-arm-msm, linux-kernel

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
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: (subset) [PATCH 1/2] regulator: qcom-labibb: avoid unbalanced IRQ enable
  2021-02-02  7:36 [PATCH 1/2] regulator: qcom-labibb: avoid unbalanced IRQ enable Matti Vaittinen
  2021-02-02  7:37 ` [PATCH 2/2] regulator: qcom-labibb: Use disable_irq_nosync from isr Matti Vaittinen
  2021-02-02 14:42 ` [PATCH 1/2] regulator: qcom-labibb: avoid unbalanced IRQ enable AngeloGioacchino Del Regno
@ 2021-02-02 16:32 ` Mark Brown
  2 siblings, 0 replies; 6+ messages in thread
From: Mark Brown @ 2021-02-02 16:32 UTC (permalink / raw)
  To: mazziesaccount, Matti Vaittinen
  Cc: AngeloGioacchino Del Regno, Andy Gross, linux-kernel,
	Liam Girdwood, Bjorn Andersson, linux-arm-msm

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] regulator: qcom-labibb: avoid unbalanced IRQ enable
  2021-02-02 14:42 ` [PATCH 1/2] regulator: qcom-labibb: avoid unbalanced IRQ enable AngeloGioacchino Del Regno
@ 2021-02-03  6:15   ` Matti Vaittinen
  0 siblings, 0 replies; 6+ messages in thread
From: Matti Vaittinen @ 2021-02-03  6:15 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno
  Cc: Andy Gross, Bjorn Andersson, Liam Girdwood, Mark Brown,
	linux-arm-msm, linux-kernel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-02-03  6:16 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-02  7:36 [PATCH 1/2] regulator: qcom-labibb: avoid unbalanced IRQ enable Matti Vaittinen
2021-02-02  7:37 ` [PATCH 2/2] regulator: qcom-labibb: Use disable_irq_nosync from isr Matti Vaittinen
2021-02-02 14:36   ` AngeloGioacchino Del Regno
2021-02-02 14:42 ` [PATCH 1/2] regulator: qcom-labibb: avoid unbalanced IRQ enable AngeloGioacchino Del Regno
2021-02-03  6:15   ` Matti Vaittinen
2021-02-02 16:32 ` (subset) " Mark Brown

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.