linux-kernel - Re: [PATCH v2] mfd: axp20x: Generalise handling without interrupt

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <22003475.EfDdHjke4D@archlinux>
Date:   Tue, 05 Sep 2023 22:28:10 +0200
From:   Jernej Škrabec <jernej.skrabec@...il.com>
To:     Lee Jones <lee@...nel.org>, Chen-Yu Tsai <wens@...e.org>,
        Andre Przywara <andre.przywara@....com>
Cc:     linux-sunxi@...ts.linux.dev, linux-kernel@...r.kernel.org,
        Shengyu Qu <wiagn233@...look.com>,
        Martin Botka <martin.botka1@...il.com>,
        Matthew Croughan <matthew.croughan@....how>
Subject: Re: [PATCH v2] mfd: axp20x: Generalise handling without interrupt

On Monday, August 28, 2023 11:32:29 PM CEST 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.
> 
> Generalise the handling of AXP MFD devices when the platform tables
> describe no interrupt, by allowing each device to specify an alternative
> MFD list for this case. If no specific alternative is specified, we go
> with the safe default of "just the regulators", which matches the current
> situation.
> 
> This enables new devices using the AXP313a PMIC, but not connecting the
> IRQ pin.
> 
> Signed-off-by: Andre Przywara <andre.przywara@....com>
> ---
> Changelog v2 .. v1:
> - drop reordering approach, use separate cell lists
> 
>  drivers/mfd/axp20x.c | 44 ++++++++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index c03bc5cda080a..239e7f18956ae 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -1133,6 +1133,8 @@ 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;
> +	const struct mfd_cell *cells_no_irq = NULL;
> +	int nr_cells_no_irq = 0;
> 
>  	if (dev->of_node) {
>  		of_id = of_match_device(dev->driver->of_match_table, 
dev);
> @@ -1207,14 +1209,15 @@ 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 = ARRAY_SIZE(axp806_cells);
> +		cells_no_irq = axp806_cells;
>  		axp20x->regmap_cfg = &axp806_regmap_config;
>  		axp20x->regmap_irq_chip = &axp806_regmap_irq_chip;
>  		break;
> @@ -1238,24 +1241,8 @@ 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);
> +		axp20x->cells = axp15060_cells;
>  		axp20x->regmap_cfg = &axp15060_regmap_config;
>  		axp20x->regmap_irq_chip = &axp15060_regmap_irq_chip;
>  		break;
> @@ -1263,6 +1250,23 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
>  		dev_err(dev, "unsupported AXP20X ID %lu\n", axp20x-
>variant);
>  		return -EINVAL;
>  	}
> +
> +	/*
> +	 * Use an alternative cell array when no interrupt line is 
connected,
> +	 * since IRQs are required by some drivers.
> +	 * The default is the safe "regulator-only", as this works fine 
without
> +	 * an interrupt specified.
> +	 */
> +	if (axp20x->irq <= 0) {
> +		if (cells_no_irq) {
> +			axp20x->nr_cells = nr_cells_no_irq;
> +			axp20x->cells = cells_no_irq;
> +		} else {
> +			axp20x->nr_cells = 
ARRAY_SIZE(axp_regulator_only_cells);
> +			axp20x->cells = axp_regulator_only_cells;

axp806_cells (old value for AXP806_ID without irq) and 
axp_regulator_only_cells differs in id field. Is that an issue?

Best regards,
Jernej

> +		}
> +	}
> +
>  	dev_info(dev, "AXP20x variant %s found\n",
>  		 axp20x_model_names[axp20x->variant]);