linux-kernel - Re: [PATCH 2/3] irqchip: mstar: msc313-intc interrupt controller driver

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:   Thu, 6 Aug 2020 19:03:10 +0900
From:   Daniel Palmer <daniel@...f.com>
To:     Marc Zyngier <maz@...nel.org>
Cc:     linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        linux-kernel@...r.kernel.org, DTML <devicetree@...r.kernel.org>,
        tglx@...utronix.de, jason@...edaemon.net,
        Rob Herring <robh+dt@...nel.org>,
        Arnd Bergmann <arnd@...db.de>, Willy Tarreau <w@....eu>,
        mark-pk.tsai@...iatek.com
Subject: Re: [PATCH 2/3] irqchip: mstar: msc313-intc interrupt controller driver

Hi Marc,

On Thu, 6 Aug 2020 at 01:26, Marc Zyngier <maz@...nel.org> wrote:
> > +struct msc313_intc {
> > +     struct irq_domain *domain;
> > +     void __iomem *base;
> > +     struct irq_chip irqchip;
>
> Why do you need to embed the irq_chip on a per-controller basis?

Current chips have 1 instance of each type of controller but some of the
newer ones seem to have an extra copy of the non-FIQ version with different
offset to the GIC.

> > +};
> > +
> > +static void msc313_intc_maskunmask(struct msc313_intc *intc, int
> > hwirq, bool mask)
> > +{
> > +     int regoff = REGOFF(hwirq);
> > +     void __iomem *addr = intc->base + REGOFF_MASK + regoff;
> > +     u16 bit = IRQBIT(hwirq);
> > +     u16 reg = readw_relaxed(addr);
> > +
> > +     if (mask)
> > +             reg |= bit;
> > +     else
> > +             reg &= ~bit;
> > +
> > +     writew_relaxed(reg, addr);
>
> RMW on a shared MMIO register. Not going to end well. This is valid
> for all the callbacks, I believe.

Do you have any suggestions on how to resolve that? It seems usually
an interrupt controller has set and clear registers to get around this.
Would defining a spinlock at the top of the driver and using that around
the read and modify sequences be good enough?

> > +
> > +     if (flow_type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_LEVEL_HIGH))
> > +             reg &= ~bit;
> > +     else
> > +             reg |= bit;
>
> I don't follow grasp the logic here. What happens on EDGE_BOTH, for
> example?

To be honest I don't quite remember. I'll check and rewrite this.

> This driver has a massive feeling of déja-vu. It is almost
> a copy of the one posted at [1], which I reviewed early
> this week. The issues are the exact same, and I'm 98%
> sure this is the same IP block used by two SoC vendors.

This would make a lot of sense considering MediaTek bought MStar
for their TV SoCs. The weirdness with only using 16 bits in a register
suggests they've inherited the shared ARM/8051 bus that the MStar
chips have. Thanks for the tip off.

Cheers,

Daniel