@@ -309,6 +309,10 @@ static int axp20x_pek_probe(struct platform_device *pdev)axp20x_pek->axp20x=dev_get_drvdata(pdev->dev.parent);+/* In case there is no interrupt line from the AXP towards the CPU. */+if(!axp20x_pek->axp20x->regmap_irqc)+return-ENODEV;+axp20x_pek->irq_dbr=platform_get_irq_byname(pdev,"PEK_DBR");if(axp20x_pek->irq_dbr<0)returnaxp20x_pek->irq_dbr;
Hi Andre,
On Wed, Jan 27, 2021 at 05:24:45PM +0000, Andre Przywara wrote:
On at least one board (Orangepi Zero2) the AXP305 PMIC does not have its
interrupt line connected to the CPU (mostly because the H616 SoC does
not feature an NMI pin anymore).
After allowing the AXP driver to proceed without an "interrupts"
property [1], the axp20x-pek driver crashes with a NULL pointer
dereference (see below).
Check for the regmap_irqc member to be not NULL before proceeding with
probe. This gets normally filled by the call to regmap_add_irq_chip(),
which we allow to skip now, when the DT node lacks an interrupt
property.
No, the driver is not the right place to patch this; regmap should be
fixed so it does not crash instead.
Thanks.
--
Dmitry
From: Andre Przywara <andre.przywara@arm.com> Date: 2021-01-28 11:13:21
On Wed, 27 Jan 2021 11:42:15 -0800
Dmitry Torokhov [off-list ref] wrote:
Hi Dmitry,
thanks for your feedback!
On Wed, Jan 27, 2021 at 05:24:45PM +0000, Andre Przywara wrote:
quoted
On at least one board (Orangepi Zero2) the AXP305 PMIC does not have its
interrupt line connected to the CPU (mostly because the H616 SoC does
not feature an NMI pin anymore).
After allowing the AXP driver to proceed without an "interrupts"
property [1], the axp20x-pek driver crashes with a NULL pointer
dereference (see below).
Check for the regmap_irqc member to be not NULL before proceeding with
probe. This gets normally filled by the call to regmap_add_irq_chip(),
which we allow to skip now, when the DT node lacks an interrupt
property.
No, the driver is not the right place to patch this; regmap should be
fixed so it does not crash instead.
I am not sure this is the right approach, those regmap functions look
more like an internal interface to me, with lots of wrapper functions
happily dereferencing pointers and reaching into structs. Moving
NULL checks into those does not sound like the right thing. CC:ing Mark
for more opinions on this.
A more general solution would be to not instantiate this driver here
at all, when we don't have an interrupt line.
However at the moment the AXP MFD driver uses a const struct to hold
all MFD cells, so there is no easy way of omitting the power key
device dynamically. And even then it would hard code the requirement
for an interrupt into the MFD driver, when this could be considered an
implementation detail of the axp20x-pek driver.
That's why I came up with this patch here, which was the easiest and
cleanest: This driver *requires* a valid regmap_irqc, so it should
verify this at probe time, kind of like a normal driver would bail out
if no IRQ line could be reserved.
Let me know what you think!
Cheers,
Andre
From: Mark Brown <broonie@kernel.org> Date: 2021-01-28 11:37:33
On Thu, Jan 28, 2021 at 11:11:28AM +0000, Andre Przywara wrote:
Dmitry Torokhov [off-list ref] wrote:
quoted
On Wed, Jan 27, 2021 at 05:24:45PM +0000, Andre Przywara wrote:
quoted
quoted
Check for the regmap_irqc member to be not NULL before proceeding with
probe. This gets normally filled by the call to regmap_add_irq_chip(),
which we allow to skip now, when the DT node lacks an interrupt
property.
It sounds like you're trying to register an IRQ chip with a somehow
bogus configuration?
quoted
No, the driver is not the right place to patch this; regmap should be
fixed so it does not crash instead.
I am not sure this is the right approach, those regmap functions look
more like an internal interface to me, with lots of wrapper functions
happily dereferencing pointers and reaching into structs. Moving
NULL checks into those does not sound like the right thing. CC:ing Mark
for more opinions on this.
Without having seen the actual issue if you're trying to register an
interrupt controller with a known broken hardware configuration that
does seem like something the caller just shouldn't be doing, it's not
something that's going to transiently happen at runtime and we're very
much trusting that the caller got things right.
A more general solution would be to not instantiate this driver here
at all, when we don't have an interrupt line.
However at the moment the AXP MFD driver uses a const struct to hold
all MFD cells, so there is no easy way of omitting the power key
device dynamically. And even then it would hard code the requirement
for an interrupt into the MFD driver, when this could be considered an
implementation detail of the axp20x-pek driver.
Another approach is to just register the optional device separately.
From: Andre Przywara <andre.przywara@arm.com> Date: 2021-01-28 12:33:25
On Thu, 28 Jan 2021 11:36:01 +0000
Mark Brown [off-list ref] wrote:
On Thu, Jan 28, 2021 at 11:11:28AM +0000, Andre Przywara wrote:
quoted
Dmitry Torokhov [off-list ref] wrote:
quoted
On Wed, Jan 27, 2021 at 05:24:45PM +0000, Andre Przywara wrote:
quoted
quoted
quoted
Check for the regmap_irqc member to be not NULL before proceeding with
probe. This gets normally filled by the call to regmap_add_irq_chip(),
which we allow to skip now, when the DT node lacks an interrupt
property.
It sounds like you're trying to register an IRQ chip with a somehow
bogus configuration?
Quick background: Those AXP PMICs have an IRQ pin, that was always
connected to the NMI pin on Allwinner SoCs. This was used for the power
button GPIO interrupt. Now the H616 does not have this pin anymore, and
the board does not use a GPIO either.
I patched the AXP MFD driver [1] to skip the regmap-irq creation when no
interrupts DT property was found, but this NULL pointer now
understandably confuses the -pek driver, and leads to this crash:
http://lists.infradead.org/pipermail/linux-arm-kernel/2021-January/634969.html
Hence I wanted to plug this hole, which seems useful regardless of this
particular issue.
[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2021-January/634971.html
quoted
quoted
No, the driver is not the right place to patch this; regmap should be
fixed so it does not crash instead.
quoted
I am not sure this is the right approach, those regmap functions look
more like an internal interface to me, with lots of wrapper functions
happily dereferencing pointers and reaching into structs. Moving
NULL checks into those does not sound like the right thing. CC:ing Mark
for more opinions on this.
Without having seen the actual issue if you're trying to register an
interrupt controller with a known broken hardware configuration that
does seem like something the caller just shouldn't be doing, it's not
something that's going to transiently happen at runtime and we're very
much trusting that the caller got things right.
quoted
A more general solution would be to not instantiate this driver here
at all, when we don't have an interrupt line.
However at the moment the AXP MFD driver uses a const struct to hold
all MFD cells, so there is no easy way of omitting the power key
device dynamically. And even then it would hard code the requirement
for an interrupt into the MFD driver, when this could be considered an
implementation detail of the axp20x-pek driver.
Another approach is to just register the optional device separately.
I will have a look at how much this takes.
Thanks,
Andre
From: Mark Brown <broonie@kernel.org> Date: 2021-01-28 15:08:22
On Thu, Jan 28, 2021 at 12:31:36PM +0000, Andre Przywara wrote:
Mark Brown [off-list ref] wrote:
quoted
It sounds like you're trying to register an IRQ chip with a somehow
bogus configuration?
I patched the AXP MFD driver [1] to skip the regmap-irq creation when no
interrupts DT property was found, but this NULL pointer now
understandably confuses the -pek driver, and leads to this crash:
Hence I wanted to plug this hole, which seems useful regardless of this
particular issue.
The driver code here looks pretty confused. It appears to be looking up
the interrupt to use from a resource (which is what I'd expect for a MFD
child) then for reasons I can't fathom trying to pass that resource into
regmap_irq_get_virq() which is at best going to just return the value
that was passed in but may potentially end up just returning a random
interrupt other than the one that was asked for since we're passing in a
global interrupt number rather than a controller relative one. I really
can't tell what's supposed to be going on there. A driver should either
use resources or it should use regmap_irq_get_virq(), using both is a
bug.
The MFD for this device is also just plain buggy in that it is providing
IRQ resources to the children when there is in fact no support for the
interrupts on the device in the system. This means that the MFD core
sees that it has no interrupt domain, assumes that those interrupt
resources are in fact absolute interrupt numbers and passes them
straight through to the children. This means that the children will
just be requesting random interrupts in the system which may actually
exist and be requestable which probably isn't going to end well. When
there is no interrupt controller the parent should not be trying to
supply interrupt resources to the children at all.