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