linux-kernel - 回复: [PATCH v2 2/2] irqchip: Add StarFive external interrupt controller

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:
 <SHXPR01MB0671BD046F6E3F3A215414A6F252A@SHXPR01MB0671.CHNPR01.prod.partner.outlook.cn>
Date: Sun, 18 Feb 2024 02:36:05 +0000
From: Changhuang Liang <changhuang.liang@...rfivetech.com>
To: Thomas Gleixner <tglx@...utronix.de>, Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley
	<conor+dt@...nel.org>, Philipp Zabel <p.zabel@...gutronix.de>
CC: Leyfoon Tan <leyfoon.tan@...rfivetech.com>, Jack Zhu
	<jack.zhu@...rfivetech.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>
Subject:
 回复: [PATCH v2 2/2] irqchip: Add StarFive external interrupt controller

Hi, Thomas

Thanks for your comment.

> On Mon, Jan 29 2024 at 21:58, Changhuang Liang wrote:
[...]
> > +static void starfive_intc_mod(struct starfive_irq_chip *irqc, u32
> > +reg, u32 mask, u32 data) {
> > +	u32 value;
> > +
> > +	value = ioread32(irqc->base + reg) & ~mask;
> > +	data &= mask;
> 
> Why?
> 

If I want to update the reg  GENMASK(7, 4)  to value 5, the data I will pass in 5 << 4

> > +	data |= value;
> > +	iowrite32(data, irqc->base + reg);
> 
> How is this serialized against concurrent invocations of this code on different
> CPUs for different interrupts?
> 
> It's not and this requires a raw_spinlock for protection.
> 

Will use raw_spinlock.

> > +}
> > +
> > +static void starfive_intc_unmask(struct irq_data *d) {
> > +	struct starfive_irq_chip *irqc = irq_data_get_irq_chip_data(d);
> > +
> > +	starfive_intc_mod(irqc, STARFIVE_INTC_SRC0_MASK, BIT(d->hwirq), 0);
> > +}
> > +
> > +static void starfive_intc_mask(struct irq_data *d) {
> > +	struct starfive_irq_chip *irqc = irq_data_get_irq_chip_data(d);
> > +
> > +	starfive_intc_mod(irqc, STARFIVE_INTC_SRC0_MASK, BIT(d->hwirq),
> > +BIT(d->hwirq)); }
> > +
[...]
> > +	irqc->root_domain = irq_domain_add_linear(intc,
> STARFIVE_INTC_SRC_IRQ_NUM,
> > +						  &starfive_intc_domain_ops, irqc);
> > +	if (!irqc->root_domain) {
> > +		pr_err("Unable to create IRQ domain\n");
> > +		ret = -EINVAL;
> > +		goto err_clk;
> > +	}
> > +
> > +	parent_irq = of_irq_get(intc, 0);
> > +	if (parent_irq < 0) {
> > +		pr_err("Failed to get main IRQ: %d\n", parent_irq);
> > +		ret = parent_irq;
> > +		goto err_clk;
> 
> Leaks the interrupt domain, no?
> 
> Thanks,
> 

Will use irq_domain_remove() free domain.

regards
Changhuang