Message-ID: <d9556a14e8d64f83b89a2d1d231003f4@realtek.com>
Date: Tue, 19 Dec 2023 03:15:52 +0000
From: James Tai [戴志峰] <james.tai@...ltek.com>
To: Thomas Gleixner <tglx@...utronix.de>, Marc Zyngier <maz@...nel.org>,
        "Rob
 Herring" <robh+dt@...nel.org>,
        Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        kernel test robot
	<lkp@...el.com>, Dan Carpenter <error27@...il.com>
Subject: RE: [PATCH v3 2/6] irqchip: Add interrupt controller support for Realtek DHC SoCs

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@...el.com>
>> Reported-by: Dan Carpenter <error27@...il.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

Powered by blists - more mailing lists