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@xxxxxxxxx> > Reported-by: Dan Carpenter <error27@xxxxxxxxx> > Closes: https://lore.kernel.org/r/202311201929.2FpvMRlg-lkp@xxxxxxxxx/ 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
