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).

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <error27@gmail.com>
Closes: https://lore.kernel.org/r/202311201929.2FpvMRlg-lkp@intel.com/
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Marc Zyngier <maz@kernel.org>
CC: linux-kernel@vger.kernel.org
Signed-off-by: James Tai <james.tai@realtek.com>
---
v2 to v3 change:
- Resolved kernel test robot build warnings
  https://lore.kernel.org/r/202311201929.2FpvMRlg-lkp@intel.com/

v1 to v2 change:
- Fixed code style issues
- Removed the realtek_intc_set_affinity funcation
- Replaced spin_lock_irqsave with raw_spin_lock

 drivers/irqchip/Kconfig                   |   4 +
 drivers/irqchip/Makefile                  |   1 +
 drivers/irqchip/irq-realtek-intc-common.c | 208 ++++++++++++++++++++++
 drivers/irqchip/irq-realtek-intc-common.h |  77 ++++++++
 4 files changed, 290 insertions(+)
 create mode 100644 drivers/irqchip/irq-realtek-intc-common.c
 create mode 100644 drivers/irqchip/irq-realtek-intc-common.h

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index f7149d0f3d45..267c3429b48d 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -218,6 +218,10 @@ config RDA_INTC
 	bool
 	select IRQ_DOMAIN
 
+config REALTEK_DHC_INTC
+	tristate
+	select IRQ_DOMAIN
+
 config RENESAS_INTC_IRQPIN
 	bool "Renesas INTC External IRQ Pin Support" if COMPILE_TEST
 	select IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index ffd945fe71aa..f6774af7fde2 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_IRQ_MIPS_CPU)		+= irq-mips-cpu.o
 obj-$(CONFIG_IXP4XX_IRQ)		+= irq-ixp4xx.o
 obj-$(CONFIG_JCORE_AIC)			+= irq-jcore-aic.o
 obj-$(CONFIG_RDA_INTC)			+= irq-rda-intc.o
+obj-$(CONFIG_REALTEK_DHC_INTC)		+= irq-realtek-intc-common.o
 obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
 obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
 obj-$(CONFIG_RENESAS_RZA1_IRQC)		+= irq-renesas-rza1.o
diff --git a/drivers/irqchip/irq-realtek-intc-common.c b/drivers/irqchip/irq-realtek-intc-common.c
new file mode 100644
index 000000000000..64d656e5a015
--- /dev/null
+++ b/drivers/irqchip/irq-realtek-intc-common.c
@@ -0,0 +1,208 @@
+// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
+/*
+ * Realtek DHC SoCs interrupt controller driver
+ *
+ * Copyright (c) 2023 Realtek Semiconductor Corporation
+ */
+
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+#include "irq-realtek-intc-common.h"
+
+struct realtek_intc_data;
+
+static inline unsigned int realtek_intc_get_ints(struct realtek_intc_data *data)
+{
+	return readl(data->base + data->info->isr_offset);
+}
+
+static inline void realtek_intc_clear_ints_bit(struct realtek_intc_data *data, int bit)
+{
+	writel(BIT(bit) & ~1, data->base + data->info->isr_offset);
+}
+
+static inline unsigned int realtek_intc_get_inte(struct realtek_intc_data *data)
+{
+	unsigned int val;
+
+	raw_spin_lock(&data->lock);
+	val = readl(data->base + data->info->scpu_int_en_offset);
+	raw_spin_unlock(&data->lock);
+
+	return val;
+}
+
+static void realtek_intc_handler(struct irq_desc *desc)
+{
+	struct realtek_intc_subset_data *subset_data = irq_desc_get_handler_data(desc);
+	struct realtek_intc_data *data = subset_data->common;
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	u32 ints, inte, mask;
+	int irq;
+
+	chained_irq_enter(chip, desc);
+
+	ints = realtek_intc_get_ints(data) & subset_data->cfg->ints_mask;
+	inte = realtek_intc_get_inte(data);
+
+	while (ints) {
+		irq = __ffs(ints);
+		ints &= ~BIT(irq);
+
+		mask = data->info->isr_to_scpu_int_en_mask[irq];
+		if (mask != IRQ_ALWAYS_ENABLED && !(inte & mask))
+			continue;
+
+		generic_handle_irq(irq_find_mapping(data->domain, irq));
+		realtek_intc_clear_ints_bit(data, irq);
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static void realtek_intc_mask_irq(struct irq_data *data)
+{
+	struct realtek_intc_data *intc_data = irq_data_get_irq_chip_data(data);
+
+	writel(BIT(data->hwirq), intc_data->base + intc_data->info->isr_offset);
+}
+
+static void realtek_intc_unmask_irq(struct irq_data *data)
+{
+	struct realtek_intc_data *intc_data = irq_data_get_irq_chip_data(data);
+
+	writel(BIT(data->hwirq), intc_data->base + intc_data->info->umsk_isr_offset);
+}
+
+static void realtek_intc_enable_irq(struct irq_data *data)
+{
+	struct realtek_intc_data *intc_data = irq_data_get_irq_chip_data(data);
+	u32 scpu_int_en, mask;
+
+	mask = intc_data->info->isr_to_scpu_int_en_mask[data->hwirq];
+	if (!mask)
+		return;
+
+	raw_spin_lock(&intc_data->lock);
+	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);
+	raw_spin_unlock(&intc_data->lock);
+}
+
+static void realtek_intc_disable_irq(struct irq_data *data)
+{
+	struct realtek_intc_data *intc_data = irq_data_get_irq_chip_data(data);
+	u32 scpu_int_en, mask;
+
+	mask = intc_data->info->isr_to_scpu_int_en_mask[data->hwirq];
+	if (!mask)
+		return;
+
+	raw_spin_lock(&intc_data->lock);
+	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);
+	raw_spin_unlock(&intc_data->lock);
+}
+
+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,
+};
+
+static int realtek_intc_domain_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
+{
+	struct realtek_intc_data *data = d->host_data;
+
+	irq_set_chip_and_handler(irq, &realtek_intc_chip, handle_level_irq);
+	irq_set_chip_data(irq, data);
+	irq_set_probe(irq);
+
+	return 0;
+}
+
+static const struct irq_domain_ops realtek_intc_domain_ops = {
+	.xlate = irq_domain_xlate_onecell,
+	.map = realtek_intc_domain_map,
+};
+
+static int realtek_intc_subset(struct device_node *node, struct realtek_intc_data *data, int index)
+{
+	struct realtek_intc_subset_data *subset_data = &data->subset_data[index];
+	const struct realtek_intc_subset_cfg *cfg = &data->info->cfg[index];
+	int irq;
+
+	irq = irq_of_parse_and_map(node, index);
+	if (irq <= 0)
+		return irq;
+
+	subset_data->common = data;
+	subset_data->cfg = cfg;
+	subset_data->parent_irq = irq;
+	irq_set_chained_handler_and_data(irq, realtek_intc_handler, subset_data);
+
+	return 0;
+}
+
+int realtek_intc_probe(struct platform_device *pdev, const struct realtek_intc_info *info)
+{
+	struct realtek_intc_data *data;
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	int ret, i;
+
+	data = devm_kzalloc(dev, struct_size(data, subset_data, info->cfg_num), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->base = of_iomap(node, 0);
+	if (!data->base) {
+		ret = -ENOMEM;
+		goto out_cleanup;
+	}
+
+	data->info = info;
+
+	raw_spin_lock_init(&data->lock);
+
+	data->domain = irq_domain_add_linear(node, 32, &realtek_intc_domain_ops, data);
+	if (!data->domain) {
+		ret = -ENOMEM;
+		goto out_cleanup;
+	}
+
+	data->subset_data_num = info->cfg_num;
+	for (i = 0; i < info->cfg_num; i++) {
+		ret = realtek_intc_subset(node, data, i);
+		if (ret) {
+			WARN(ret, "failed to init subset %d: %d", i, ret);
+			ret = -ENOMEM;
+			goto out_cleanup;
+		}
+	}
+
+	platform_set_drvdata(pdev, data);
+
+	return 0;
+
+out_cleanup:
+
+	if (data->base)
+		iounmap(data->base);
+
+	devm_kfree(dev, data);
+
+	return ret;
+}
+EXPORT_SYMBOL(realtek_intc_probe);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Realtek DHC SoC Interrupt Controller Driver");
diff --git a/drivers/irqchip/irq-realtek-intc-common.h b/drivers/irqchip/irq-realtek-intc-common.h
new file mode 100644
index 000000000000..38be116dba60
--- /dev/null
+++ b/drivers/irqchip/irq-realtek-intc-common.h
@@ -0,0 +1,77 @@
+/* SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause) */
+/*
+ * Copyright (c) 2023 Realtek Semiconductor Corporation
+ */
+
+#ifndef _IRQ_REALTEK_COMMON_H
+#define _IRQ_REALTEK_COMMON_H
+
+#include <linux/bits.h>
+#include <linux/limits.h>
+#include <linux/hwspinlock.h>
+
+/**
+ * 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 {
+	const struct realtek_intc_subset_cfg *cfg;
+	unsigned int			     isr_offset;
+	unsigned int			     umsk_isr_offset;
+	unsigned int			     scpu_int_en_offset;
+	const u32			     *isr_to_scpu_int_en_mask;
+	int				     cfg_num;
+};
+
+/**
+ * realtek_intc_subset_data - handler of a interrupt source only handles ints
+ *                            bits in the mask.
+ * @cfg: cfg of the subset.
+ * @common: common data.
+ * @parent_irq: interrupt source.
+ */
+struct realtek_intc_subset_data {
+	const struct realtek_intc_subset_cfg *cfg;
+	struct realtek_intc_data	     *common;
+	int				     parent_irq;
+};
+
+/**
+ * realtek_intc_data - configuration data for realtek interrupt controller driver.
+ * @base: base of interrupt register
+ * @info: info of intc
+ * @domain: interrupt domain
+ * @lock: lock
+ * @saved_en: status of interrupt enable
+ * @subset_data_num: number of subset data
+ * @subset_data: subset data
+ */
+struct realtek_intc_data {
+	void __iomem			*base;
+	const struct realtek_intc_info	*info;
+	struct irq_domain		*domain;
+	struct raw_spinlock		lock;
+	unsigned int			saved_en;
+	int				subset_data_num;
+	struct realtek_intc_subset_data subset_data[];
+};
+
+#define IRQ_ALWAYS_ENABLED U32_MAX
+#define DISABLE_INTC (0)
+#define CLEAN_INTC_STATUS GENMASK(31, 1)
+
+int realtek_intc_probe(struct platform_device *pdev, const struct realtek_intc_info *info);
+
+#endif /* _IRQ_REALTEK_COMMON_H */
-- 
2.25.1

On Wed, Nov 29 2023 at 13:43, 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).
>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <error27@gmail.com>
> Closes: https://lore.kernel.org/r/202311201929.2FpvMRlg-lkp@intel.com/

These tags are pointless as they are not related to anything in
tree. You addressed review comments and 0-day fallout, but neither Dan
nor 0-day reported that the interrupt controller for Realtek DHC SoCs is
missing.

> +#include "irq-realtek-intc-common.h"
> +
> +struct realtek_intc_data;

struct realtek_intc_data is declared in irq-realtek-intc-common.h, so
what's the point of this forward declaration?

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

That '& ~1' solves what aside of preventing bit 0 from being written?

> +static int realtek_intc_domain_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
> +{
> +	struct realtek_intc_data *data = d->host_data;
> +
> +	irq_set_chip_and_handler(irq, &realtek_intc_chip, handle_level_irq);
> +	irq_set_chip_data(irq, data);
> +	irq_set_probe(irq);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops realtek_intc_domain_ops = {
> +	.xlate = irq_domain_xlate_onecell,
> +	.map = realtek_intc_domain_map,

	.xlate	= irq_domain_xlate_onecell,
	.map	= realtek_intc_domain_map,

Please.

> +};
> +
> +static int realtek_intc_subset(struct device_node *node, struct realtek_intc_data *data, int index)
> +{
> +	struct realtek_intc_subset_data *subset_data = &data->subset_data[index];
> +	const struct realtek_intc_subset_cfg *cfg = &data->info->cfg[index];
> +	int irq;
> +
> +	irq = irq_of_parse_and_map(node, index);

irq_of_parse_and_map() returns an 'unsigned int' where 0 is fail.

> +	if (irq <= 0)
> +		return irq;
> +
> +	subset_data->common = data;
> +	subset_data->cfg = cfg;
> +	subset_data->parent_irq = irq;
> +	irq_set_chained_handler_and_data(irq, realtek_intc_handler, subset_data);
> +
> +	return 0;
> +}
> +
> +int realtek_intc_probe(struct platform_device *pdev, const struct realtek_intc_info *info)
> +{
> +	struct realtek_intc_data *data;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	int ret, i;
> +
> +	data = devm_kzalloc(dev, struct_size(data, subset_data, info->cfg_num), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->base = of_iomap(node, 0);
> +	if (!data->base) {
> +		ret = -ENOMEM;
> +		goto out_cleanup;

devm_kzalloc() is automatically cleaned up when the probe function
fails, so 'return -ENOMEM;' is sufficient.

> +	}
> +
> +	data->info = info;
> +
> +	raw_spin_lock_init(&data->lock);
> +
> +	data->domain = irq_domain_add_linear(node, 32, &realtek_intc_domain_ops, data);
> +	if (!data->domain) {
> +		ret = -ENOMEM;

This 'ret = -ENOMEM;' is pointless as the only error code returned in this
function is -ENOMEM. So you can just return -ENOMEM in the error path, no?

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

                if (WARN(ret, "....."))
                	goto cleanup;

> +		}
> +	}
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	return 0;
> +
> +out_cleanup:
> +
> +	if (data->base)
> +		iounmap(data->base);

Leaks the irqdomain.

> +
> +	devm_kfree(dev, data);

Pointless exercise.

> +	return ret;
> +}
> +EXPORT_SYMBOL(realtek_intc_probe);

EXPORT_SYMBOL_GPL

> +/**
> + * realtek_intc_subset_cfg - subset interrupt mask
> + * @ints_mask: inetrrupt mask
> + */
> +struct realtek_intc_subset_cfg {
> +	unsigned int	ints_mask;
> +};

The value of a struct wrapping a single 'unsigned int' is? What's wrong
with using unsigned int (actually you want u32 as this represents a
hardware mask) directly? Not enough obfuscation, right?

> +/**
> + * 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.

 * @isr_offset:		interrupt status register offset
 * @umsk_isr_offset:	unmask interrupt status register offset
 * @scpu_int_en_offset:	interrupt enable register offset

Can you spot the difference?

Please fix all over the place.

> + */
> +struct realtek_intc_info {
> +	const struct realtek_intc_subset_cfg *cfg;
> +	unsigned int			     isr_offset;
> +	unsigned int			     umsk_isr_offset;
> +	unsigned int			     scpu_int_en_offset;
> +	const u32			     *isr_to_scpu_int_en_mask;
> +	int				     cfg_num;
> +};
> +
> +/**
> + * realtek_intc_subset_data - handler of a interrupt source only handles ints
> + *                            bits in the mask.
> + * @cfg: cfg of the subset.

Seriously. 'cfg of'? This is a description, so can you spell the words
out? This is really neither space constraint nor subject to Xitter
rules. Fix this all over the place please.

> + * @common: common data.
> + * @parent_irq: interrupt source.
> + */
> +struct realtek_intc_subset_data {
> +	const struct realtek_intc_subset_cfg *cfg;
> +	struct realtek_intc_data	     *common;
> +	int				     parent_irq;
> +};
> +
> +/**
> + * realtek_intc_data - configuration data for realtek interrupt controller driver.
> + * @base: base of interrupt register
> + * @info: info of intc
> + * @domain: interrupt domain
> + * @lock: lock
> + * @saved_en: status of interrupt enable
> + * @subset_data_num: number of subset data
> + * @subset_data: subset data
> + */
> +struct realtek_intc_data {
> +	void __iomem			*base;
> +	const struct realtek_intc_info	*info;
> +	struct irq_domain		*domain;
> +	struct raw_spinlock		lock;
> +	unsigned int			saved_en;
> +	int				subset_data_num;
> +	struct realtek_intc_subset_data subset_data[];
> +};
> +
> +#define IRQ_ALWAYS_ENABLED U32_MAX
> +#define DISABLE_INTC (0)
> +#define CLEAN_INTC_STATUS GENMASK(31, 1)

#define IRQ_ALWAYS_ENABLED	U32_MAX
#define DISABLE_INTC		(0)
#define CLEAN_INTC_STATUS	GENMASK(31, 1)

Please, as that makes this readable.

Thanks,

        tglx

Hi Thomas,

>On Wed, Nov 29 2023 at 13:43, 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).
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Reported-by: Dan Carpenter <error27@gmail.com>
>> Closes: https://lore.kernel.org/r/202311201929.2FpvMRlg-lkp@intel.com/
>
>These tags are pointless as they are not related to anything in tree. You
>addressed review comments and 0-day fallout, but neither Dan nor 0-day
>reported that the interrupt controller for Realtek DHC SoCs is missing.
>
I will remove it.

>> +#include "irq-realtek-intc-common.h"
>> +
>> +struct realtek_intc_data;
>
>struct realtek_intc_data is declared in irq-realtek-intc-common.h, so what's the
>point of this forward declaration?
>
This is a meaningless declaration, and I will remove it.

>> +static inline unsigned int realtek_intc_get_ints(struct
>> +realtek_intc_data *data) {
>> +     return readl(data->base + data->info->isr_offset); }
>> +
>> +static inline void realtek_intc_clear_ints_bit(struct
>> +realtek_intc_data *data, int bit) {
>> +     writel(BIT(bit) & ~1, data->base + data->info->isr_offset);
>
>That '& ~1' solves what aside of preventing bit 0 from being written?
>
The ISR register uses bit 0 to clear or set ISR status.
Write 0 to clear bits and write 1 to set bits.
Therefore, to clear the interrupt status, bit 0 should consistently be set to '0'.

>> +static int realtek_intc_domain_map(struct irq_domain *d, unsigned int
>> +irq, irq_hw_number_t hw) {
>> +     struct realtek_intc_data *data = d->host_data;
>> +
>> +     irq_set_chip_and_handler(irq, &realtek_intc_chip, handle_level_irq);
>> +     irq_set_chip_data(irq, data);
>> +     irq_set_probe(irq);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct irq_domain_ops realtek_intc_domain_ops = {
>> +     .xlate = irq_domain_xlate_onecell,
>> +     .map = realtek_intc_domain_map,
>
>        .xlate  = irq_domain_xlate_onecell,
>        .map    = realtek_intc_domain_map,
>
>Please.
>
I will fix it.

>> +};
>> +
>> +static int realtek_intc_subset(struct device_node *node, struct
>> +realtek_intc_data *data, int index) {
>> +     struct realtek_intc_subset_data *subset_data =
>&data->subset_data[index];
>> +     const struct realtek_intc_subset_cfg *cfg = &data->info->cfg[index];
>> +     int irq;
>> +
>> +     irq = irq_of_parse_and_map(node, index);
>
>irq_of_parse_and_map() returns an 'unsigned int' where 0 is fail.
>
I will use of_irq_get() instead.

>> +     if (irq <= 0)
>> +             return irq;
>> +
>> +     subset_data->common = data;
>> +     subset_data->cfg = cfg;
>> +     subset_data->parent_irq = irq;
>> +     irq_set_chained_handler_and_data(irq, realtek_intc_handler,
>> + subset_data);
>> +
>> +     return 0;
>> +}
>> +
>> +int realtek_intc_probe(struct platform_device *pdev, const struct
>> +realtek_intc_info *info) {
>> +     struct realtek_intc_data *data;
>> +     struct device *dev = &pdev->dev;
>> +     struct device_node *node = dev->of_node;
>> +     int ret, i;
>> +
>> +     data = devm_kzalloc(dev, struct_size(data, subset_data,
>info->cfg_num), GFP_KERNEL);
>> +     if (!data)
>> +             return -ENOMEM;
>> +
>> +     data->base = of_iomap(node, 0);
>> +     if (!data->base) {
>> +             ret = -ENOMEM;
>> +             goto out_cleanup;
>
>devm_kzalloc() is automatically cleaned up when the probe function fails, so
>'return -ENOMEM;' is sufficient.

I will adjust it.

>
>> +     }
>> +
>> +     data->info = info;
>> +
>> +     raw_spin_lock_init(&data->lock);
>> +
>> +     data->domain = irq_domain_add_linear(node, 32,
>&realtek_intc_domain_ops, data);
>> +     if (!data->domain) {
>> +             ret = -ENOMEM;
>
>This 'ret = -ENOMEM;' is pointless as the only error code returned in this
>function is -ENOMEM. So you can just return -ENOMEM in the error path, no?
>
Yes. it is pointless and I will adjust it.

>> +             goto out_cleanup;
>> +     }
>> +
>> +     data->subset_data_num = info->cfg_num;
>> +     for (i = 0; i < info->cfg_num; i++) {
>> +             ret = realtek_intc_subset(node, data, i);
>> +             if (ret) {
>> +                     WARN(ret, "failed to init subset %d: %d", i, ret);
>> +                     ret = -ENOMEM;
>> +                     goto out_cleanup;
>
>                if (WARN(ret, "....."))
>                        goto cleanup;
>
I will fix it.

>> +             }
>> +     }
>> +
>> +     platform_set_drvdata(pdev, data);
>> +
>> +     return 0;
>> +
>> +out_cleanup:
>> +
>> +     if (data->base)
>> +             iounmap(data->base);
>
>Leaks the irqdomain.
>
I will add error handle for irqdomain.

>> +
>> +     devm_kfree(dev, data);
>
>Pointless exercise.
>
I will remove it.

>> +     return ret;
>> +}
>> +EXPORT_SYMBOL(realtek_intc_probe);
>
>EXPORT_SYMBOL_GPL
>
I will fix it.

>> +/**
>> + * realtek_intc_subset_cfg - subset interrupt mask
>> + * @ints_mask: inetrrupt mask
>> + */
>> +struct realtek_intc_subset_cfg {
>> +     unsigned int    ints_mask;
>> +};
>
>The value of a struct wrapping a single 'unsigned int' is? What's wrong with
>using unsigned int (actually you want u32 as this represents a hardware mask)
>directly? Not enough obfuscation, right?
>
Yes, it represents a set of hardware masks, so using u32 instead of 'unsigned int' is more appropriate.
There are no issues with obfuscation.

>> +/**
>> + * 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.
>
> * @isr_offset:         interrupt status register offset
> * @umsk_isr_offset:    unmask interrupt status register offset
> * @scpu_int_en_offset: interrupt enable register offset
>
>Can you spot the difference?
>
The interrupt control registers of the DHC platform are not linearly arranged, which necessitates the use of a mechanism for efficient reading and writing of these registers.
The 'scpu_int_en' register serves the purpose of controlling whether peripheral devices' interrupts are enabled or disabled.
The 'isr' register represents the interrupt status. It can mask interrupts using the 'scpu_int_en' register. When the 'scpu_int_en' register disables interrupts, the 'isr' register will not reflect the interrupt status.
The 'umsk_isr' register also represents the interrupt status but is not influenced by any other control register to mask interrupts. In short, it reflects the original interrupt status.

>Please fix all over the place.
>
I will fix it.

>> + */
>> +struct realtek_intc_info {
>> +     const struct realtek_intc_subset_cfg *cfg;
>> +     unsigned int                         isr_offset;
>> +     unsigned int                         umsk_isr_offset;
>> +     unsigned int                         scpu_int_en_offset;
>> +     const u32
>*isr_to_scpu_int_en_mask;
>> +     int                                  cfg_num;
>> +};
>> +
>> +/**
>> + * realtek_intc_subset_data - handler of a interrupt source only handles ints
>> + *                            bits in the mask.
>> + * @cfg: cfg of the subset.
>
>Seriously. 'cfg of'? This is a description, so can you spell the words out? This is
>really neither space constraint nor subject to Xitter rules. Fix this all over the
>place please.

I will adjust the description so that it clearly describes the intended purpose.

>
>> + * @common: common data.
>> + * @parent_irq: interrupt source.
>> + */
>> +struct realtek_intc_subset_data {
>> +     const struct realtek_intc_subset_cfg *cfg;
>> +     struct realtek_intc_data             *common;
>> +     int                                  parent_irq;
>> +};
>> +
>> +/**
>> + * realtek_intc_data - configuration data for realtek interrupt controller
>driver.
>> + * @base: base of interrupt register
>> + * @info: info of intc
>> + * @domain: interrupt domain
>> + * @lock: lock
>> + * @saved_en: status of interrupt enable
>> + * @subset_data_num: number of subset data
>> + * @subset_data: subset data
>> + */
>> +struct realtek_intc_data {
>> +     void __iomem                    *base;
>> +     const struct realtek_intc_info  *info;
>> +     struct irq_domain               *domain;
>> +     struct raw_spinlock             lock;
>> +     unsigned int                    saved_en;
>> +     int                             subset_data_num;
>> +     struct realtek_intc_subset_data subset_data[]; };
>> +
>> +#define IRQ_ALWAYS_ENABLED U32_MAX
>> +#define DISABLE_INTC (0)
>> +#define CLEAN_INTC_STATUS GENMASK(31, 1)
>
>#define IRQ_ALWAYS_ENABLED      U32_MAX
>#define DISABLE_INTC            (0)
>#define CLEAN_INTC_STATUS       GENMASK(31, 1)
>
>Please, as that makes this readable.
>
I will improve it.

Thanks for your feedback. 

Regards,
James

On Wed, Nov 29, 2023 at 01:43:35PM +0800, James Tai wrote:
> +static int realtek_intc_subset(struct device_node *node, struct realtek_intc_data *data, int index)
> +{
> +	struct realtek_intc_subset_data *subset_data = &data->subset_data[index];
> +	const struct realtek_intc_subset_cfg *cfg = &data->info->cfg[index];
> +	int irq;
> +
> +	irq = irq_of_parse_and_map(node, index);
> +	if (irq <= 0)
> +		return irq;

I don't think irq_of_parse_and_map() can return negatives.  Only zero
on error.  Returning zero on error is a historical artifact with IRQ
functions and a constant source of bugs.  But here returning zero is
success.  See my blog for more details:
https://staticthinking.wordpress.com/2023/08/07/writing-a-check-for-zero-irq-error-codes/

So this should probably be:

	irq = irq_of_parse_and_map(node, index);
	if (!irq)
		return -ENODEV;

I should create a static checker warning for this...  Possibly I should
create a static checker warning any time someone does:

	if (foo <= 0)
		return foo;

> +
> +	subset_data->common = data;
> +	subset_data->cfg = cfg;
> +	subset_data->parent_irq = irq;
> +	irq_set_chained_handler_and_data(irq, realtek_intc_handler, subset_data);
> +
> +	return 0;
> +}
> +
> +int realtek_intc_probe(struct platform_device *pdev, const struct realtek_intc_info *info)
> +{
> +	struct realtek_intc_data *data;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	int ret, i;
> +
> +	data = devm_kzalloc(dev, struct_size(data, subset_data, info->cfg_num), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->base = of_iomap(node, 0);
> +	if (!data->base) {
> +		ret = -ENOMEM;
> +		goto out_cleanup;

devm_ allocations are cleaned up automatically so there is no need to
call devm_kfree() before returning.

regards,
dan carpenter

> +	}
> +
> +	data->info = info;
> +
> +	raw_spin_lock_init(&data->lock);
> +
> +	data->domain = irq_domain_add_linear(node, 32, &realtek_intc_domain_ops, data);
> +	if (!data->domain) {
> +		ret = -ENOMEM;
> +		goto out_cleanup;
> +	}
> +
> +	data->subset_data_num = info->cfg_num;
> +	for (i = 0; i < info->cfg_num; i++) {
> +		ret = realtek_intc_subset(node, data, i);
> +		if (ret) {
> +			WARN(ret, "failed to init subset %d: %d", i, ret);
> +			ret = -ENOMEM;
> +			goto out_cleanup;
> +		}
> +	}
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	return 0;
> +
> +out_cleanup:
> +
> +	if (data->base)
> +		iounmap(data->base);
> +
> +	devm_kfree(dev, data);
> +
> +	return ret;
> +}

On Wed, Nov 29, 2023 at 11:21:06AM +0300, Dan Carpenter wrote:
> On Wed, Nov 29, 2023 at 01:43:35PM +0800, James Tai wrote:
> > +static int realtek_intc_subset(struct device_node *node, struct realtek_intc_data *data, int index)
> > +{
> > +	struct realtek_intc_subset_data *subset_data = &data->subset_data[index];
> > +	const struct realtek_intc_subset_cfg *cfg = &data->info->cfg[index];
> > +	int irq;
> > +
> > +	irq = irq_of_parse_and_map(node, index);
> > +	if (irq <= 0)
> > +		return irq;
> 
> I don't think irq_of_parse_and_map() can return negatives.  Only zero
> on error.  Returning zero on error is a historical artifact with IRQ
> functions and a constant source of bugs.  But here returning zero is
> success.  See my blog for more details:
> https://staticthinking.wordpress.com/2023/08/07/writing-a-check-for-zero-irq-error-codes/

It's worse than that. The irq functions historically returned NO_IRQ on 
error, but that could be 0 or -1 depending on the arch.

Use of_irq_get() instead. It's a bit better in that it returns an error 
code for most cases. But still returns 0 on mapping failure.

Rob

Hi Rob,

>
>On Wed, Nov 29, 2023 at 11:21:06AM +0300, Dan Carpenter wrote:
>> On Wed, Nov 29, 2023 at 01:43:35PM +0800, James Tai wrote:
>> > +static int realtek_intc_subset(struct device_node *node, struct
>> > +realtek_intc_data *data, int index) {
>> > +   struct realtek_intc_subset_data *subset_data =
>&data->subset_data[index];
>> > +   const struct realtek_intc_subset_cfg *cfg = &data->info->cfg[index];
>> > +   int irq;
>> > +
>> > +   irq = irq_of_parse_and_map(node, index);
>> > +   if (irq <= 0)
>> > +           return irq;
>>
>> I don't think irq_of_parse_and_map() can return negatives.  Only zero
>> on error.  Returning zero on error is a historical artifact with IRQ
>> functions and a constant source of bugs.  But here returning zero is
>> success.  See my blog for more details:
>> https://staticthinking.wordpress.com/2023/08/07/writing-a-check-for-ze
>> ro-irq-error-codes/
>
>It's worse than that. The irq functions historically returned NO_IRQ on error, but
>that could be 0 or -1 depending on the arch.
>
>Use of_irq_get() instead. It's a bit better in that it returns an error code for most
>cases. But still returns 0 on mapping failure.
>

I will use of_irq_get() instead and adjust the return value of realtek_intc_subset() in the next patches.

Thanks for your feedback.

Regards,
James

On Wed, Nov 29, 2023 at 11:21:06AM +0300, Dan Carpenter wrote:
> > +int realtek_intc_probe(struct platform_device *pdev, const struct realtek_intc_info *info)
> > +{
> > +	struct realtek_intc_data *data;
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *node = dev->of_node;
> > +	int ret, i;
> > +
> > +	data = devm_kzalloc(dev, struct_size(data, subset_data, info->cfg_num), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	data->base = of_iomap(node, 0);
> > +	if (!data->base) {
> > +		ret = -ENOMEM;
> > +		goto out_cleanup;
> 
> devm_ allocations are cleaned up automatically so there is no need to
> call devm_kfree() before returning.
> 
> regards,
> dan carpenter
> 
> > +	}
> > +
> > +	data->info = info;
> > +
> > +	raw_spin_lock_init(&data->lock);
> > +
> > +	data->domain = irq_domain_add_linear(node, 32, &realtek_intc_domain_ops, data);

Btw, as I was testing the other static checker warning for <= 0, my
static checker really wants this irq_domain_add_linear() to be cleaned
up on the error path.

Otherwise it probably leads to a use after free because we free data
(automatically or manually) but it's still on a list somewhere.

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

This error path.

regards,
dan carpenter


> > +		}
> > +	}
> > +
> > +	platform_set_drvdata(pdev, data);
> > +
> > +	return 0;
> > +
> > +out_cleanup:
> > +
> > +	if (data->base)
> > +		iounmap(data->base);
> > +
> > +	devm_kfree(dev, data);
> > +
> > +	return ret;
> > +}

Hi Dan,

>> devm_ allocations are cleaned up automatically so there is no need to
>> call devm_kfree() before returning.
>>
>> regards,
>> dan carpenter
>
I will remove it. 

>> > +   }
>> > +
>> > +   data->info = info;
>> > +
>> > +   raw_spin_lock_init(&data->lock);
>> > +
>> > +   data->domain = irq_domain_add_linear(node, 32,
>> > + &realtek_intc_domain_ops, data);
>
>Btw, as I was testing the other static checker warning for <= 0, my static
>checker really wants this irq_domain_add_linear() to be cleaned up on the error
>path.
>
>Otherwise it probably leads to a use after free because we free data
>(automatically or manually) but it's still on a list somewhere.
>
I will add 'irq_domain_remove()' to release it. 

>> > +   if (!data->domain) {
>> > +           ret = -ENOMEM;
>> > +           goto out_cleanup;
>> > +   }
>> > +
>> > +   data->subset_data_num = info->cfg_num;
>> > +   for (i = 0; i < info->cfg_num; i++) {
>> > +           ret = realtek_intc_subset(node, data, i);
>> > +           if (ret) {
>> > +                   WARN(ret, "failed to init subset %d: %d", i, ret);
>> > +                   ret = -ENOMEM;
>> > +                   goto out_cleanup;
>
>This error path.
>
>regards,
>dan carpenter
>
I will add 'irq_domain_remove()' before goto cleanup.

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

Thank you for your feedback.

Regards,
James