linux-kernel - Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)

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:	Wed, 6 Feb 2013 10:14:30 +0000
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Benjamin Herrenschmidt <benh@...nel.crashing.org>
Cc:	Alexey Brodkin <Alexey.Brodkin@...opsys.com>,
	Michal Simek <monstr@...str.eu>, Arnd Bergmann <arnd@...db.de>,
	Vineet Gupta <Vineet.Gupta1@...opsys.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	dahinds@...rs.sourceforge.net
Subject: Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16
 with generic iowrite(read)8/16(be)

On Tue, Feb 5, 2013 at 11:07 PM, Benjamin Herrenschmidt
<benh@...nel.crashing.org> wrote:
> On Wed, 2013-02-06 at 01:25 +0400, Alexey Brodkin wrote:
>> Sounds good but how should one tell which approach is correct? For
>> example here - is the one implemented by Xilinx is golden reference or
>> not?
>
> So I'm reading that PDF you pointed to. So far what I can see is:
>
>  - In 8-bit mode you only do 8-bit accesses, so endianness should be
> totally irrelevant (at least the pdf says so)
>
>  - In 16-bit mode, that's where things become interesting... the doc
> says:
>
>   PLB Data Bus          | System ACE Data Bus
>   ----------------------+--------------------
>   PLB_DBus[8 : 15]      | SysACE_MPD[15 : 8]
>   PLB_DBus[0 : 7]       | SysACE_MPD[7 : 0]
>
> Now, I'm not 100% of the bit numbering used by Xilinx here but it smells
> like PLB used ppc numbering and SystemACE use the classic numbering, in
> which case the above would mean that the MSB of the PLB is connected to
> the LSB of the SystemACE and vice-versa.
>
> If that is the case then this is the *correct* wiring and means that the
> data port (if any) doesn't need any byteswapping. It also means that the
> registers need byteswap on BE, as expected for a LE device. IE. Just
> always use ioread32 (or _rep variants for the data port if there's such
> a thing on it).
>
> So that looks good... unless I misunderstood the Xilinx spec, this looks
> like the right way to do and the only one we should support.

Huh? That makes no sense. This device out in the wild with both big
and little endian bus attachments. You can argue all day that one of
them is wrong, but it doesn't matter. It exists, is used, and must be
supported. In fact, the driver already knows about this and figures
out at runtime how the device is wired up to the bus. This is not the
problem.

BTW, that document describes only one of the systemace bus
attachments. There is a different on used on Microblaze little-endian,
and some boards have the SystemACE directly wired to the SoC external
bus (no adapter IP).

The only problem that I see is that the ARM and Microblaze
ioread16be/iowrite16be helpers are missing barriers which smells like
a bug and should be fixed.

Michal, have you tested Alexey's patch? If it works for you then I'm
comfortable with acking it. It looks correct to me.

g.

--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/