Re: [PATCH 2/6] irqchip: Add interrupt controller support for Realtek DHC SoCs

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, Nov 02 2023 at 22:27, James Tai wrote:
> Realtek DHC (Digital Home Center) SoCs share a common interrupt controller design.
> This universal interrupt controller driver provides support for various variants
> within the Realtek DHC SoC family.
>
> Each DHC SoC features two sets of extended interrupt controllers, each capable of
> handling up to 32 interrupts. These expansion controllers are connected to the GIC
> (Generic Interrupt Controller).
>
> Signed-off-by: James Tai <james.tai@realtek.com>
> Change-Id: I1e711c70414c97c2c8497bd4ac9e8bbd964225c3

Please remove these internal change ids. They are not useful for anyone
outside realtek.
  
> +config REALTEK_DHC_INTC
> +	tristate
> +	select IRQ_DOMAIN
> +


> +static unsigned int realtek_intc_get_ints(struct realtek_intc_data *data)

static inline perhaps?

> +{
> +	return readl(data->base + data->info->isr_offset);
> +}
> +
> +static void realtek_intc_clear_ints_bit(struct realtek_intc_data *data, int bit)
> +{
> +	writel(BIT(bit) & ~1, data->base + data->info->isr_offset);
> +}
> +
> +static unsigned int realtek_intc_get_inte(struct realtek_intc_data *data)
> +{
> +	unsigned int val;
> +	unsigned long flags;

	unsigned long flags;
	unsigned int val;

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html

Please fix the variable declarations all over the place.

> +	spin_lock_irqsave(&data->lock, flags);

This needs to be a raw spinlock.

> +	val = readl(data->base + data->info->scpu_int_en_offset);
> +	spin_unlock_irqrestore(&data->lock, flags);
> +
> +	return val;
> +}


> +static void realtek_intc_enable_irq(struct irq_data *data)
> +{
> +	struct realtek_intc_data *intc_data = irq_data_get_irq_chip_data(data);
> +	unsigned long flags;
> +	u32 scpu_int_en, mask;

See above.

> +
> +	mask = intc_data->info->isr_to_scpu_int_en_mask[data->hwirq];
> +	if (!mask)
> +		return;
> +
> +	spin_lock_irqsave(&intc_data->lock, flags);
> +
> +	scpu_int_en = readl(intc_data->base + intc_data->info->scpu_int_en_offset);
> +	scpu_int_en |= mask;
> +	writel(scpu_int_en, intc_data->base + intc_data->info->umsk_isr_offset);
> +
> +	spin_unlock_irqrestore(&intc_data->lock, flags);
> +}
> +
> +static void realtek_intc_disable_irq(struct irq_data *data)
> +{
> +	struct realtek_intc_data *intc_data = irq_data_get_irq_chip_data(data);
> +	unsigned long flags;
> +	u32 scpu_int_en, mask;
> +
> +	mask = intc_data->info->isr_to_scpu_int_en_mask[data->hwirq];
> +	if (!mask)
> +		return;
> +
> +	spin_lock_irqsave(&intc_data->lock, flags);
> +
> +	scpu_int_en = readl(intc_data->base + intc_data->info->scpu_int_en_offset);
> +	scpu_int_en &= ~mask;
> +	writel(scpu_int_en, intc_data->base + intc_data->info->umsk_isr_offset);
> +
> +	spin_unlock_irqrestore(&intc_data->lock, flags);
> +}
> +
> +static int realtek_intc_lookup_parent_irq(struct realtek_intc_data *intc_data, struct irq_data *d)
> +{
> +	unsigned int mask = BIT(d->hwirq);
> +	int i;
> +
> +	for (i = 0; i < intc_data->subset_data_num; i++)
> +		if (intc_data->subset_data[i].cfg->ints_mask & mask)
> +			return intc_data->subset_data[i].parent_irq;

Lacks brackets around the for loop. See documentation.

> +
> +	return -EINVAL;
> +}
> +
> +static int realtek_intc_set_affinity(struct irq_data *d, const struct cpumask *mask_val, bool force)
> +{
> +	struct realtek_intc_data *intc_data = irq_data_get_irq_chip_data(d);
> +	int irq;
> +	struct irq_chip *chip;
> +	struct irq_data *data;
> +
> +	irq = realtek_intc_lookup_parent_irq(intc_data, d);
> +	if (irq < 0)
> +		return irq;
> +
> +	chip = irq_get_chip(irq);
> +	data = irq_get_irq_data(irq);

So instead of two lookups you want to do:

	data = irq_get_irq_data(irq);
        if (!data)
        	return;
        chip = irq_data_get_irq_chip(data);        
...
       
> +
> +	irq_data_update_effective_affinity(d, cpu_online_mask);

So you update the effective affinity even if it cannot be set or if the
parent irq returns an error code?

Aside of that setting it to cpu_online mask is just wrong. This is _NOT_
the effective affinity because the underlying GIC selects a single
target CPU out of the caller provides cpu mask.

That said, this is also completely inconsistent vs. the other interrupts
which share that GIC interrupt instance. I.e. /proc/irq/$N/affinity and
effective_affinity become random number generators. That'll confuse
existing userspace tools.

Having an affinity setter for demultiplexes interrupts is simply wrong.

> +	if (chip && chip->irq_set_affinity)
> +		return chip->irq_set_affinity(data, mask_val, force);
> +	else
> +		return -EINVAL;
> +}
> +
> +static struct irq_chip realtek_intc_chip = {
> +	.name = "realtek-intc",
> +	.irq_mask = realtek_intc_mask_irq,
> +	.irq_unmask = realtek_intc_unmask_irq,
> +	.irq_enable = realtek_intc_enable_irq,
> +	.irq_disable = realtek_intc_disable_irq,
> +	.irq_set_affinity = realtek_intc_set_affinity,

See docs vs. formatting of struct initializers.

> +};
> +
> +
> +	data->subset_data_num = info->cfg_num;
> +	for (i = 0; i < info->cfg_num; i++) {
> +		ret = realtek_intc_subset(node, data, i);
> +		WARN(ret, "failed to init subset %d: %d", i, ret);

If this fails, then you still expose the affected interrupts as
functional?

> +/**
> + * realtek_intc_subset_cfg - subset interrupt mask
> + * @ints_mask: inetrrupt mask
> + */
> +struct realtek_intc_subset_cfg {
> +	unsigned int ints_mask;
> +};
> +
> +/**
> + * realtek_intc_info - interrupt controller data.
> + * @isr_offset: interrupt status register offset.
> + * @umsk_isr_offset: unmask interrupt status register offset.
> + * @scpu_int_en_offset: interrupt enable register offset.
> + * @cfg: cfg of the subset.
> + * @cfg_num: number of cfg.
> + */
> +struct realtek_intc_info {
> +	unsigned int isr_offset;
> +	unsigned int umsk_isr_offset;
> +	unsigned int scpu_int_en_offset;
> +	const u32 *isr_to_scpu_int_en_mask;
> +	const struct realtek_intc_subset_cfg *cfg;
> +	int cfg_num;

See formatting doc.

> +
> +#define IRQ_ALWAYS_ENABLED  (-1)

U32_MAX ?

> +#define DISABLE_INTC (0)
> +#define CLEAN_INTC_STATUS (0xfffffffe)

That's what GENMASK() is for.

Thanks,

        tglx