linux-kernel - Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sat, 29 Jan 2022 20:50:51 -0600
From:   Samuel Holland <samuel@...lland.org>
To:     Guo Ren <guoren@...nel.org>
Cc:     Marc Zyngier <maz@...nel.org>, Anup Patel <anup@...infault.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Palmer Dabbelt <palmer@...belt.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        devicetree <devicetree@...r.kernel.org>,
        Guo Ren <guoren@...ux.alibaba.com>
Subject: Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt
 parse in opensbi

On 1/29/22 8:08 PM, Guo Ren wrote:
> On Sun, Jan 30, 2022 at 2:32 AM Marc Zyngier <maz@...nel.org> wrote:
>>
>> On Sat, 29 Jan 2022 16:27:26 +0000,
>> guoren@...nel.org wrote:
>>>
>>> From: Guo Ren <guoren@...ux.alibaba.com>
>>>
>>> The thead,c900-plic has been used in opensbi to distinguish
>>> PLIC [1]. Although PLICs have the same behaviors in Linux,
>>> they are different hardware with some custom initializing in
>>> firmware(opensbi).
>>>
>>> [1]: https://github.com/riscv-software-src/opensbi/commit/78c2b19218bd62653b9fb31623a42ced45f38ea6
>>>
>>> Signed-off-by: Guo Ren <guoren@...ux.alibaba.com>
>>> Cc: Anup Patel <anup@...infault.org>
>>> Cc: Marc Zyngier <maz@...nel.org>
>>> Cc: Palmer Dabbelt <palmer@...belt.com>
>>> Cc: Samuel Holland <samuel@...lland.org>
>>> Cc: Thomas Gleixner <tglx@...utronix.de>
>>> ---
>>>  drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
>>>  1 file changed, 23 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
>>> index 259065d271ef..245655928076 100644
>>> --- a/drivers/irqchip/irq-sifive-plic.c
>>> +++ b/drivers/irqchip/irq-sifive-plic.c
>>> @@ -172,7 +172,7 @@ static void plic_irq_eoi(struct irq_data *d)
>>>       }
>>>  }
>>>
>>> -static struct irq_chip plic_chip = {
>>> +static struct irq_chip sifive_plic_chip = {
>>>       .name           = "SiFive PLIC",
>>>       .irq_mask       = plic_irq_mask,
>>>       .irq_unmask     = plic_irq_unmask,
>>> @@ -182,12 +182,24 @@ static struct irq_chip plic_chip = {
>>>  #endif
>>>  };
>>>
>>> +static struct irq_chip thead_plic_chip = {
>>> +     .name           = "T-Head PLIC",
>>> +     .irq_mask       = plic_irq_mask,
>>> +     .irq_unmask     = plic_irq_unmask,
>>> +     .irq_eoi        = plic_irq_eoi,
>>> +#ifdef CONFIG_SMP
>>> +     .irq_set_affinity = plic_set_affinity,
>>> +#endif
>>> +};
>>
>> For pure entertainment, let's compare the two structures:
>>
>> static struct irq_chip plic_chip = {
>>         .name           = "SiFive PLIC",
>>         .irq_mask       = plic_irq_mask,
>>         .irq_unmask     = plic_irq_unmask,
>>         .irq_eoi        = plic_irq_eoi,
>> #ifdef CONFIG_SMP
>>         .irq_set_affinity = plic_set_affinity,
>> #endif
>> };
>>
>> Oh wait: a string. Must be really important. Not.
> No, pls see below comment.
> 
>>
>>> +
>>> +static struct irq_chip *def_plic_chip = &sifive_plic_chip;
>>> +
>>>  static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
>>>                             irq_hw_number_t hwirq)
>>>  {
>>>       struct plic_priv *priv = d->host_data;
>>>
>>> -     irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
>>> +     irq_domain_set_info(d, irq, hwirq, def_plic_chip, d->host_data,
>>>                           handle_fasteoi_irq, NULL, NULL);
>>>       irq_set_noprobe(irq);
>>>       irq_set_affinity(irq, &priv->lmask);
>>> @@ -396,5 +408,14 @@ static int __init plic_init(struct device_node *node,
>>>       return error;
>>>  }
>>>
>>> +static int __init thead_c900_plic_init(struct device_node *node,
>>> +             struct device_node *parent)
>>> +{
>>> +     def_plic_chip = &thead_plic_chip;
>>> +
>>> +     return plic_init(node, parent);
>>> +}
>>> +
>>>  IRQCHIP_DECLARE(sifive_plic, "sifive,plic-1.0.0", plic_init);
>>>  IRQCHIP_DECLARE(riscv_plic0, "riscv,plic0", plic_init); /* for legacy systems */
>>> +IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", thead_c900_plic_init);
>>
>> Sorry, but I can't see any point to this patch.
> You didn't see the link I've put in the patch. In that opensbi patch:
> 
>                 intc: interrupt-controller@...00000 {
>                         #interrupt-cells = <1>;
> -                       compatible = "riscv,plic0";
> +                       compatible = "allwinner,sun20i-d1-plic",
> +                                    "thead,c900-plic";
> 
> +#define THEAD_PLIC_CTRL_REG 0x1ffffc
> +
> +static void thead_plic_plat_init(struct plic_data *pd)
> +{
> +       writel_relaxed(BIT(0), (void *)pd->addr + THEAD_PLIC_CTRL_REG);
> +}
> +
>  static const struct fdt_match irqchip_plic_match[] = {
>         { .compatible = "riscv,plic0" },
>         { .compatible = "sifive,plic-1.0.0" },
> +       { .compatible = "thead,c900-plic",
> +         .data = thead_plic_plat_init },
>         { },
>  };
> 
> We've changed the compatible name for thead,c900-plic, and there is no
> riscv,plic0 / sifive,plic-1.0.0 in dts. Without the patch, the newest
> opensbi + newest Linux would be broken in the Allwinner D1 dev board.

Yes, some patch is still necessary, because the hardware is indeed incompatible
with riscv,plic0. However, this driver does not care about the difference. So
all you need to do is hook up the existing code to the new compatible:

+IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", plic_init);

Regards,
Samuel