| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230825230418.47c93fa5@slackpad.lan>
Date: Fri, 25 Aug 2023 23:04:18 +0100
From: Andre Przywara <andre.przywara@....com>
To: Lee Jones <lee@...nel.org>
Cc: Chen-Yu Tsai <wens@...e.org>, linux-kernel@...r.kernel.org,
Jernej Skrabec <jernej.skrabec@...il.com>,
Shengyu Qu <wiagn233@...look.com>, martin.botka1@...il.com,
Matthew Croughan <matthew.croughan@....how>
Subject: Re: [PATCH] mfd: axp20x: Generalise handling without interrupts
On Thu, 17 Aug 2023 12:55:29 +0100
Lee Jones <lee@...nel.org> wrote:
Hi Lee,
> On Mon, 07 Aug 2023, Andre Przywara wrote:
>
> > At the moment we allow the AXP15060 and the AXP806 PMICs to omit the
> > interrupt line to the SoC, and we skip registering the PEK (power key)
> > driver in this case, since that crashes when no IRQ is described in the
> > DT node.
> > The IRQ pin potentially not being connected to anything does affect more
> > PMICs, though, and the PEK driver is not the only one requiring an
> > interrupt: at least the AC power supply driver crashes in a similar
> > fashion, for instance.
> >
> > Generalise the handling of AXP MFD devices when the platform tables
> > describe no interrupt, by putting devices requiring an IRQ *last* in
> > the MFD cell array. We then can easily cut short the number of devices
> > to be registered in this case.
> >
> > This patch just enables that for three PMIC models for now: the two
> > already handled, plus the AXP313a, for which we now have mulitple examples
> > of boards without the IRQ pin connected.
> > To stay consistent with the current behaviour, we still (try to) register
> > all devices for the other PMICs, even though this will probably crash
> > without an interrupt specified, if those problematic drivers are loaded.
> > But this new approach can now easily be extended to other PMICs with more
> > devices, should the need arise: currently all in-tree users are fine.
> >
> > This fixes operation on the first boards using the AXP313a, which do not
> > bother to connect the PMIC's IRQ pin.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@....com>
> > Reported-by: Shengyu Qu <wiagn233@...look.com>
> > ---
> > drivers/mfd/axp20x.c | 48 +++++++++++++++++---------------------------
> > 1 file changed, 18 insertions(+), 30 deletions(-)
>
> Relying on the ordering of static struct elements is fragile.
>
> Please separate into new structs.
Well, I was hoping to avoid this churn, since the IRQ-less lists are
purely a subset of the original structs. At first I thought about some
filtering (marking IRQ-required cells somehow), but looking at the
current scope (just needed for three simple PMICs at the moment), I
found an easier solution, which is both simple (avoids specifying both
lists for *all* PMICs), but also flexible.
Will tests this and post ASAP.
Cheers,
Andre
>
> > diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> > index c03bc5cda080a..c9623604bf6c1 100644
> > --- a/drivers/mfd/axp20x.c
> > +++ b/drivers/mfd/axp20x.c
> > @@ -1031,12 +1031,12 @@ static const struct mfd_cell axp803_cells[] = {
> > };
> >
> > static const struct mfd_cell axp806_self_working_cells[] = {
> > + { .name = "axp20x-regulator" },
> > {
> > .name = "axp221-pek",
> > .num_resources = ARRAY_SIZE(axp806_pek_resources),
> > .resources = axp806_pek_resources,
> > },
> > - { .name = "axp20x-regulator" },
> > };
> >
> > static const struct mfd_cell axp806_cells[] = {
> > @@ -1090,19 +1090,11 @@ static const struct mfd_cell axp813_cells[] = {
> > };
> >
> > static const struct mfd_cell axp15060_cells[] = {
> > + { .name = "axp20x-regulator", },
> > {
> > .name = "axp221-pek",
> > .num_resources = ARRAY_SIZE(axp15060_pek_resources),
> > .resources = axp15060_pek_resources,
> > - }, {
> > - .name = "axp20x-regulator",
> > - },
> > -};
> > -
> > -/* For boards that don't have IRQ line connected to SOC. */
> > -static const struct mfd_cell axp_regulator_only_cells[] = {
> > - {
> > - .name = "axp20x-regulator",
> > },
> > };
> >
> > @@ -1133,6 +1125,7 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
> > struct device *dev = axp20x->dev;
> > const struct acpi_device_id *acpi_id;
> > const struct of_device_id *of_id;
> > + int nr_cells_no_irq = 0;
> >
> > if (dev->of_node) {
> > of_id = of_match_device(dev->driver->of_match_table, dev);
> > @@ -1191,6 +1184,7 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
> > break;
> > case AXP313A_ID:
> > axp20x->nr_cells = ARRAY_SIZE(axp313a_cells);
> > + nr_cells_no_irq = 1;
> > axp20x->cells = axp313a_cells;
> > axp20x->regmap_cfg = &axp313a_regmap_config;
> > axp20x->regmap_irq_chip = &axp313a_regmap_irq_chip;
> > @@ -1207,14 +1201,14 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
> > * if there is no interrupt line.
> > */
> > if (of_property_read_bool(axp20x->dev->of_node,
> > - "x-powers,self-working-mode") &&
> > - axp20x->irq > 0) {
> > + "x-powers,self-working-mode")) {
> > axp20x->nr_cells = ARRAY_SIZE(axp806_self_working_cells);
> > axp20x->cells = axp806_self_working_cells;
> > } else {
> > axp20x->nr_cells = ARRAY_SIZE(axp806_cells);
> > axp20x->cells = axp806_cells;
> > }
> > + nr_cells_no_irq = 1;
> > axp20x->regmap_cfg = &axp806_regmap_config;
> > axp20x->regmap_irq_chip = &axp806_regmap_irq_chip;
> > break;
> > @@ -1238,24 +1232,9 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
> > axp20x->regmap_irq_chip = &axp803_regmap_irq_chip;
> > break;
> > case AXP15060_ID:
> > - /*
> > - * Don't register the power key part if there is no interrupt
> > - * line.
> > - *
> > - * Since most use cases of AXP PMICs are Allwinner SOCs, board
> > - * designers follow Allwinner's reference design and connects
> > - * IRQ line to SOC, there's no need for those variants to deal
> > - * with cases that IRQ isn't connected. However, AXP15660 is
> > - * used by some other vendors' SOCs that didn't connect IRQ
> > - * line, we need to deal with this case.
> > - */
> > - if (axp20x->irq > 0) {
> > - axp20x->nr_cells = ARRAY_SIZE(axp15060_cells);
> > - axp20x->cells = axp15060_cells;
> > - } else {
> > - axp20x->nr_cells = ARRAY_SIZE(axp_regulator_only_cells);
> > - axp20x->cells = axp_regulator_only_cells;
> > - }
> > + axp20x->nr_cells = ARRAY_SIZE(axp15060_cells);
> > + nr_cells_no_irq = 1;
> > + axp20x->cells = axp15060_cells;
> > axp20x->regmap_cfg = &axp15060_regmap_config;
> > axp20x->regmap_irq_chip = &axp15060_regmap_irq_chip;
> > break;
> > @@ -1263,6 +1242,15 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
> > dev_err(dev, "unsupported AXP20X ID %lu\n", axp20x->variant);
> > return -EINVAL;
> > }
> > +
> > + /*
> > + * Skip registering some MFD cells if there is no interrupt
> > + * line, as IRQs might be required by some drivers.
> > + * Those components must be the last in the cell array.
> > + */
> > + if (axp20x->irq <= 0)
> > + axp20x->nr_cells -= nr_cells_no_irq;
> > +
> > dev_info(dev, "AXP20x variant %s found\n",
> > axp20x_model_names[axp20x->variant]);
> >
> > --
> > 2.25.1
> >
>
Powered by blists - more mailing lists