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

[Changhuang Liang]

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