linux-kernel - Re: [PATCH 11/21] irqchip: Add irqchip for ADI ADSP-SC5xx platform

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: <87ed4yyezx.ffs@tglx>
Date: Wed, 02 Oct 2024 12:29:38 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Arturs Artamonovs via B4 Relay
 <devnull+arturs.artamonovs.analog.com@...nel.org>, Catalin Marinas
 <catalin.marinas@....com>, Will Deacon <will@...nel.org>, Greg Malysa
 <greg.malysa@...esys.com>, Philipp Zabel <p.zabel@...gutronix.de>, Rob
 Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor
 Dooley <conor+dt@...nel.org>, Utsav Agarwal <Utsav.Agarwal@...log.com>,
 Michael Turquette <mturquette@...libre.com>, Stephen Boyd
 <sboyd@...nel.org>, Linus Walleij <linus.walleij@...aro.org>, Bartosz
 Golaszewski <brgl@...ev.pl>, Andi Shyti <andi.shyti@...nel.org>, Greg
 Kroah-Hartman <gregkh@...uxfoundation.org>, Jiri Slaby
 <jirislaby@...nel.org>, Arnd Bergmann <arnd@...db.de>, Olof Johansson
 <olof@...om.net>, soc@...nel.org
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
 linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org,
 linux-serial@...r.kernel.org, Arturs Artamonovs
 <arturs.artamonovs@...log.com>, adsp-linux@...log.com, Arturs Artamonovs
 <Arturs.Artamonovs@...log.com>, Nathan Barrett-Morrison
 <nathan.morrison@...esys.com>
Subject: Re: [PATCH 11/21] irqchip: Add irqchip for ADI ADSP-SC5xx platform

On Thu, Sep 12 2024 at 19:24, Arturs Artamonovs via wrote:
> From: Arturs Artamonovs <arturs.artamonovs@...log.com>
>
> Support seting extra indepdendent interrupt on pin activity.

So the subject says it adds a new interrupt chip. Now the changelog
mumbles about support of something extra.

Please describe your changes properly and explain what this is
about. Also spell check your change log.

> +struct adsp_pint {
> +	struct irq_chip chip;
> +	void __iomem *regs;
> +	struct irq_domain *domain;
> +	unsigned int irq;

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

And please read and follow the rest of that document too.

> + * This relies on the default configuration of the hardware, which we do not
> + * expose an interface to change.
> + */
> +int adsp_attach_pint_to_gpio(struct adsp_gpio_port *port)

Where is this function declared and where is it used?

> +static void adsp_pint_dispatch_irq(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct adsp_pint *pint = to_adsp_pint(chip);
> +	unsigned int type = irqd_get_trigger_type(&desc->irq_data);
> +	u32 pos = BIT(desc->irq_data.hwirq);
> +
> +	/* for both edge interrupt, toggle invert bit to catch next edge */
> +	if (type == IRQ_TYPE_EDGE_BOTH) {
> +		u32 invert = readl(pint->regs + ADSP_PINT_REG_INVERT_SET) & pos;
> +
> +		if (invert)
> +			writel(pos, pint->regs + ADSP_PINT_REG_INVERT_CLEAR);
> +		else
> +			writel(pos, pint->regs + ADSP_PINT_REG_INVERT_SET);

What protects pint->regs against concurrent modifications?

> +static void adsp_pint_irq_mask(struct irq_data *d)
> +{
> +	struct adsp_pint *pint = irq_data_get_irq_chip_data(d);
> +
> +	writel(BIT(d->hwirq), pint->regs + ADSP_PINT_REG_MASK_CLEAR);

Same question.

> +static int adsp_pint_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct adsp_pint *pint;
> +
> +	pint = devm_kzalloc(dev, sizeof(*pint), GFP_KERNEL);
> +	if (!pint)
> +		return -ENOMEM;
> +
> +	pint->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(pint->regs))
> +		return PTR_ERR(pint->regs);
> +
> +	pint->chip.name = "adsp-pint";
> +	pint->chip.irq_ack = adsp_pint_irq_ack;
> +	pint->chip.irq_mask = adsp_pint_irq_mask;
> +	pint->chip.irq_unmask = adsp_pint_irq_unmask;
> +	pint->chip.irq_set_type = adsp_pint_irq_set_type;
> +	// @todo potentially only SEC supports wake options, not gic
> +
> +	// @todo determine if we actually need a raw spinlock

This should have been determined before posting, no?

> +	pint->domain = irq_domain_add_linear(np, ADSP_PINT_IRQS,
> +		&adsp_irq_domain_ops, pint);

devm_irq_domain_instantiate()

> +	if (!pint->domain) {
> +		dev_err(dev, "Could not create irq domain\n");
> +		return -EINVAL;
> +	}
> +
> +	pint->irq = platform_get_irq(pdev, 0);
> +	if (!pint->irq) {
> +		dev_err(dev, "Could not find parent interrupt for port\n");
> +		return -EINVAL;

Then this would not leak the interrupt domain. Also why is this not
checked _before_ instantiating the domain?

> +static int __init adsp_pint_init(void)
> +{
> +	return platform_driver_register(&adsp_pint_driver);
> +}
> +

Pointless new line

> +arch_initcall(adsp_pint_init);
> +
> +MODULE_DESCRIPTION("Analog Devices IRQChip driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Greg Malysa <greg.malysa@...esys.com>");
> \ No newline at end of file

This message has a meaning, no?

Thanks,

        tglx