linux-kernel - Re: [PATCH 2/4] ARM: mstar: Add machine for MStar infinity family SoCs

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]

Message-ID: <20191016203219.GA5191@shiro>
Date:   Thu, 17 Oct 2019 05:32:20 +0900
From:   Daniel Palmer <daniel@...f.com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/4] ARM: mstar: Add machine for MStar infinity family
 SoCs

> > +
> > +static void __init infinity_map_io(void)
> > +{
> > +       iotable_init(infinity_io_desc, ARRAY_SIZE(infinity_io_desc));
> > +       miu_flush = (void __iomem *)(infinity_io_desc[0].virtual
> > +                       + INFINITY_L3BRIDGE_FLUSH);
> > +       miu_status = (void __iomem *)(infinity_io_desc[0].virtual
> > +                       + INFINITY_L3BRIDGE_STATUS);
> > +}
> 
> If you do this a little later in .init_machine, you can use a simple ioremap()
> rather than picking a hardcoded physical address. It looks like nothing
> uses the mapping before you set soc_mb anyway.

I've moved this into infinity_barriers_init() using ioremap() as suggested.
I'd like to keep the fixed remap address for now as there are some
drivers in the vendor code that might be useful until rewrites are done but 
are littered with hard coded addresses.

> > +static DEFINE_SPINLOCK(infinity_mb_lock);
> > +
> > +static void infinity_mb(void)
> > +{
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&infinity_mb_lock, flags);
> > +       /* toggle the flush miu pipe fire bit */
> > +       writel_relaxed(0, miu_flush);
> > +       writel_relaxed(INFINITY_L3BRIDGE_FLUSH_TRIGGER, miu_flush);
> > +       while (!(readl_relaxed(miu_status) & INFINITY_L3BRIDGE_STATUS_DONE)) {
> > +               /* wait for flush to complete */
> > +       }
> > +       spin_unlock_irqrestore(&infinity_mb_lock, flags);
> > +}
> 
> Wow, this is a heavy barrier. From your description it doesn't sound like
> there is anything to be done about it unfortunately.

It's possible there is a better way once I can find out what the L3 bridge
actually is. There is a small amount of documentation for the miu (DDR
controller) that says it has an 8 or 4 operation configurable pipeline but
this flushing bit is in a totally different area that's only documented
by the comment about it in u-boot.

> Two possible issues I see here:
> 
> * It looks like it relies on CONFIG_ARM_HEAVY_BARRIER, but your Kconfig
>   entry does not select that. In many configurations, CACHE_L2X0 would
>   be set, but there is no need for yours on the Cortex-A7.

Fixed.
 
>    Not sure if it matters in practice, as almost nothing uses fiq any more.
>    OTOH, maybe the lock is not needed at all? AFAICT if the sequence
>    gets interrupted by a handler that also calls mb(), you would still
>    continue in the original thread while observing a full l3 barrier. ;-)

I've taken the lock out and tested that the ethernet isn't sending garbage
and everything looks good.

I'm still hoping for some feedback on the other parts of the series.
I'll post a v2 series in a few days.

Thanks!

Daniel