[PATCH v1 0/1] regulator: da9211: Fix crash when irqs are pre-enabled

[PATCH v1 0/1] regulator: da9211: Fix crash when irqs are pre-enabled

[Ricardo Ribalda]

If the system does not come from reset (like when it is kexec()), the
regulator might have an IRQ waiting for us.

If we enable the IRQ handler before its structures are ready, we crash.

To: Support Opensource <support.opensource@diasemi.com>
To: Liam Girdwood <lgirdwood@gmail.com>
To: Mark Brown <broonie@kernel.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

---
Ricardo Ribalda (1):
      regulator: da9211: Use irq handler when ready

 drivers/regulator/da9211-regulator.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
---
base-commit: 4312098baf37ee17a8350725e6e0d0e8590252d4
change-id: 20221124-da9211-4b8904b4feb5

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH v1 1/1] regulator: da9211: Use irq handler when ready

[Ricardo Ribalda]

If the system does not come from reset (like when it is kexec()), the
regulator might have an IRQ waiting for us.

If we enable the IRQ handler before its structures are ready, we crash.

This patch fixes:

[    1.141839] Unable to handle kernel read from unreadable memory at virtual address 0000000000000078
[    1.144475] tpm_i2c_infineon 3-0020: 1.2 TPM (device-id 0x1A)
[    1.150883] Mem abort info:
[    1.150884]   ESR = 0x96000005
[    1.150887]   EC = 0x25: DABT (current EL), IL = 32 bits
[    1.167749]   SET = 0, FnV = 0
[    1.170794]   EA = 0, S1PTW = 0
[    1.173926] Data abort info:
[    1.176798]   ISV = 0, ISS = 0x00000005
[    1.180626]   CM = 0, WnR = 0
[    1.183584] [0000000000000078] user address but active_mm is swapper
[    1.189929] Internal error: Oops: 96000005 [#1] PREEMPT SMP
[    1.195490] Modules linked in:
[    1.198537] CPU: 2 PID: 145 Comm: irq/77-da9211 Not tainted 5.10.153-20404-gdd5053d763d6
[    1.210431] Hardware name: Google Hana (DT)
[    1.214603] pstate: 60400005 (nZCv daif +PAN -UAO -TCO BTYPE=--)
[    1.220601] pc : blocking_notifier_call_chain+0x20/0xa8
[    1.225818] lr : regulator_notifier_call_chain+0x1c/0x2c
[    1.231116] sp : ffffffc010463d40
[    1.234419] x29: ffffffc010463d40 x28: ffffff9140ad2ca4
[    1.239720] x27: ffffff914205efc0 x26: ffffff9140ad2cd8
[    1.245021] x25: 0000000000000001 x24: ffffffe9bd532b28
[    1.250321] x23: ffffff9140ad2ce0 x22: ffffff9140ad2cac
[    1.255622] x21: ffffff9140cc5a00 x20: ffffff9140ad2c00
[    1.255625] x19: ffffff9142059580 x18: fffffffffffffff5
[    1.267353] x17: 0000000000000304 x16: 00000000000003b6
[    1.267356] x15: 0000000000000002
[    1.279078] x14: 000000000018bae8
[    1.279080] x13: 0000000000100000 x12: 0000000000000239
[    1.287768] x11: 0000000000000000 x10: 0000000000000001
[    1.294202] mtk-thermal 1100b000.thermal: can't find thermal sensor 5
[    1.297582] x9 : ffffffe9bc344050 x8 : 0000000000000013
[    1.297586] x7 : 0000000000000000
[    1.309308] x6 : 0000000000000013
[    1.309310] x5 : ffffff914205ec02 x4 : 0000000000000001
[    1.309313] x3 : ffffffc010463c58 x2 : 0000000000000000
[    1.316093] x1 : 0000000000000002 x0 : 0000000000000050
[    1.316096] Call trace:
[    1.316101]  blocking_notifier_call_chain+0x20/0xa8
[    1.322757] cpu cpu0: dummy supplies not allowed for exclusive requests
[    1.327823]  regulator_notifier_call_chain+0x1c/0x2c
[    1.327825]  da9211_irq_handler+0x68/0xf8
[    1.327829]  irq_thread+0x11c/0x234
[    1.327833]  kthread+0x13c/0x154
[    1.332067] cpu cpu0: EM: created perf domain
[    1.339641]  ret_from_fork+0x10/0x30
[    1.339647] Code: a9015ff8 a90257f6 a9034ff4 910003fd (f9401408)
[    1.343215] cpu cpu2: EM: created perf domain
[    1.348335] ---[ end trace 46760cc6c7f556d8 ]---
[    1.420026] Kernel panic - not syncing: Oops: Fatal exception
[    1.425761] SMP: stopping secondary CPUs
[    1.429678] Kernel Offset: 0x29ac000000 from 0xffffffc010000000
[    1.435583] PHYS_OFFSET: 0xffffffefc0000000
[    1.439754] CPU features: 0x48240022,65806005
[    1.444099] Memory Limit: none

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

diff --git a/drivers/regulator/da9211-regulator.c b/drivers/regulator/da9211-regulator.c
index e01b32d1fa17..36a0009e23de 100644
--- a/drivers/regulator/da9211-regulator.c
+++ b/drivers/regulator/da9211-regulator.c
@@ -496,6 +496,12 @@ static int da9211_i2c_probe(struct i2c_client *i2c)
 		return PTR_ERR(chip->pdata);
 	}
 
+	ret = da9211_regulator_init(chip);
+	if (ret < 0) {
+		dev_err(chip->dev, "Failed to initialize regulator: %d\n", ret);
+		return ret;
+	}
+
 	chip->chip_irq = i2c->irq;
 
 	if (chip->chip_irq != 0) {
@@ -512,11 +518,6 @@ static int da9211_i2c_probe(struct i2c_client *i2c)
 		dev_warn(chip->dev, "No IRQ configured\n");
 	}
 
-	ret = da9211_regulator_init(chip);
-
-	if (ret < 0)
-		dev_err(chip->dev, "Failed to initialize regulator: %d\n", ret);
-
 	return ret;
 }
 

-- 
b4 0.11.0-dev-d93f8

Re: [PATCH v1 0/1] regulator: da9211: Fix crash when irqs are pre-enabled

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 492 bytes --]

On Thu, Nov 24, 2022 at 05:45:30PM +0100, Ricardo Ribalda wrote:
> If the system does not come from reset (like when it is kexec()), the
> regulator might have an IRQ waiting for us.

Please don't send cover letters for single patches, if there is anything
that needs saying put it in the changelog of the patch or after the ---
if it's administrative stuff.  This reduces mail volume and ensures that 
any important information is recorded in the changelog rather than being
lost. 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/1] regulator: da9211: Use irq handler when ready

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 817 bytes --]

On Thu, Nov 24, 2022 at 05:45:31PM +0100, Ricardo Ribalda wrote:

> [    1.141839] Unable to handle kernel read from unreadable memory at virtual address 0000000000000078
> [    1.144475] tpm_i2c_infineon 3-0020: 1.2 TPM (device-id 0x1A)
> [    1.150883] Mem abort info:
> [    1.150884]   ESR = 0x96000005
> [    1.150887]   EC = 0x25: DABT (current EL), IL = 32 bits
> [    1.167749]   SET = 0, FnV = 0
> [    1.170794]   EA = 0, S1PTW = 0

Please think hard before including complete backtraces in upstream
reports, they are very large and contain almost no useful information
relative to their size so often obscure the relevant content in your
message. If part of the backtrace is usefully illustrative (it often is
for search engines if nothing else) then it's usually better to pull out
the relevant sections.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 0/1] regulator: da9211: Fix crash when irqs are pre-enabled

[Ricardo Ribalda]

Hi Mark


On Thu, 24 Nov 2022 at 17:52, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Nov 24, 2022 at 05:45:30PM +0100, Ricardo Ribalda wrote:
> > If the system does not come from reset (like when it is kexec()), the
> > regulator might have an IRQ waiting for us.
>
> Please don't send cover letters for single patches, if there is anything
> that needs saying put it in the changelog of the patch or after the ---
> if it's administrative stuff.  This reduces mail volume and ensures that
> any important information is recorded in the changelog rather than being
> lost.

I am using the "b4 prep" workflow and I think there is no way to
disable the cover-letter.

Let me ask at tools@linux.kernel.org if they plan to support "old lore
links after the ---" for single patch

Thanks!


-- 
Ricardo Ribalda

Re: [PATCH v1 1/1] regulator: da9211: Use irq handler when ready

[Ricardo Ribalda]

Hi Mark

Thanks for the review!

On Thu, 24 Nov 2022 at 17:54, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Nov 24, 2022 at 05:45:31PM +0100, Ricardo Ribalda wrote:
>
> > [    1.141839] Unable to handle kernel read from unreadable memory at virtual address 0000000000000078
> > [    1.144475] tpm_i2c_infineon 3-0020: 1.2 TPM (device-id 0x1A)
> > [    1.150883] Mem abort info:
> > [    1.150884]   ESR = 0x96000005
> > [    1.150887]   EC = 0x25: DABT (current EL), IL = 32 bits
> > [    1.167749]   SET = 0, FnV = 0
> > [    1.170794]   EA = 0, S1PTW = 0
>
> Please think hard before including complete backtraces in upstream
> reports, they are very large and contain almost no useful information
> relative to their size so often obscure the relevant content in your
> message. If part of the backtrace is usefully illustrative (it often is
> for search engines if nothing else) then it's usually better to pull out
> the relevant sections.

Do you want that I post a v2 of the patch?

Regards



-- 
Ricardo Ribalda

Re: [PATCH v1 1/1] regulator: da9211: Use irq handler when ready

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 716 bytes --]

On Thu, Nov 24, 2022 at 06:35:42PM +0100, Ricardo Ribalda wrote:

> On Thu, 24 Nov 2022 at 17:54, Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Nov 24, 2022 at 05:45:31PM +0100, Ricardo Ribalda wrote:

> > Please think hard before including complete backtraces in upstream
> > reports, they are very large and contain almost no useful information
> > relative to their size so often obscure the relevant content in your
> > message. If part of the backtrace is usefully illustrative (it often is
> > for search engines if nothing else) then it's usually better to pull out
> > the relevant sections.

> Do you want that I post a v2 of the patch?

It's fine, just please think about this for future submissions.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH v1 1/1] regulator: da9211: Use irq handler when ready

[DLG Adam Ward]

On 24 November 2022 16:46, Ricardo Ribalda wrote:
>If the system does not come from reset (like when it is kexec()), the regulator might have an IRQ waiting for us.
>
>If we enable the IRQ handler before its structures are ready, we crash.

A good catch, thank you :-)

...
>+	ret = da9211_regulator_init(chip);
...
> 	chip->chip_irq = i2c->irq;

On the other hand... with the init call made before chip_irq is assigned, the IRQs won't be unmasked:
	https://elixir.bootlin.com/linux/latest/source/drivers/regulator/da9211-regulator.c#L430

Nacked-by: Adam Ward <DLG-Adam.Ward.opensource@dm.renesas.com>

Regards,
Adam

Re: [PATCH v1 0/1] regulator: da9211: Fix crash when irqs are pre-enabled

[Mark Brown]

On Thu, 24 Nov 2022 17:45:30 +0100, Ricardo Ribalda wrote:
> If the system does not come from reset (like when it is kexec()), the
> regulator might have an IRQ waiting for us.
> 
> If we enable the IRQ handler before its structures are ready, we crash.
> 
> 

Applied to

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

Thanks!

[1/1] regulator: da9211: Use irq handler when ready
      commit: 02228f6aa6a64d588bc31e3267d05ff184d772eb

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