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]

Message-ID: <51139974.2040601@synopsys.com>
Date:	Thu, 7 Feb 2013 16:09:24 +0400
From:	Alexey Brodkin <Alexey.Brodkin@...opsys.com>
To:	Benjamin Herrenschmidt <benh@...nel.crashing.org>
CC:	Grant Likely <grant.likely@...retlab.ca>,
	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 02/07/2013 01:35 AM, Benjamin Herrenschmidt wrote:
> On Wed, 2013-02-06 at 10:14 +0000, Grant Likely wrote:
>>
>> 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.
>
> No. That's where you are VERY wrong. We don't have to support crap and
> arguably shouldn't if that can give an incentive to vendors to fix their
> stuff. If you don't believe me, ask Linus :-)
>
>>   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.
>
> Except that this is very gross, especially when you observe that in the
> busted "big endian" case, it has to byteswap the bloody data port.
>
> So you end up having to do that gross hack with separate accessors for
> registers vs. data and not able to use the _rep variants, which also
> means that on platforms like ppc, you end up with a memory barrier on
> every access (or more), which is going to slow things down enormously.

BTW I've just realized that in case if there's no bridge between CPU and 
CF-controller or if this bridge is "transparent" (does no swapping
neither bytes nor bits) our data accessors here should be changed.

Isn't it strange in "ace_datain_le16" use "ioread16be" or the one it was 
here initially "in_be16"?
With BE ones I'd say similar changes should be done.

So finally I see them implemented this way:
===============
/* BE part */
static void ace_datain_be16(struct ace_device *ace)
{
	int i = ACE_FIFO_SIZE / 2;
	u16 *dst = ace->data_ptr;
	while (i--)
		*dst++ = ioread16be(ace->baseaddr + 0x40);
	ace->data_ptr = dst;
}

static void ace_dataout_be16(struct ace_device *ace)
{
	int i = ACE_FIFO_SIZE / 2;
	u16 *src = ace->data_ptr;
	while (i--)
		iowrite16be(*src++, ace->baseaddr + 0x40);
	ace->data_ptr = src;
}

/* LE part*/
static void ace_datain_le16(struct ace_device *ace)
{
	int i = ACE_FIFO_SIZE / 2;
	u16 *dst = ace->data_ptr;
	while (i--)
		*dst++ = ioread16(ace->baseaddr + 0x40);
	ace->data_ptr = dst;
}

static void ace_dataout_le16(struct ace_device *ace)
{
	int i = ACE_FIFO_SIZE / 2;
	u16 *src = ace->data_ptr;
	while (i--)
		iowrite16(*src++, ace->baseaddr + 0x40);
	ace->data_ptr = src;
}
===============

Correct me if I'm wrong here.

And at least these accessors for LE got xsysace perfectly working on our 
FPGA platform (little-endian ARC700 on Xilinx ml-509 with our own 
BVCI-to-MPU bridge that does no swapping).

I have to confess that I didn't properly tested initial patch on real HW 
- it was only sort of cosmetic clean-up.

-Alexey

>> 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.
>
> No, the real problem is that the "big endian" wiring is totally busted
> and the HW guys who came with it must be taught a lesson. Not supporting
> that crap might be one way to do it.
>
> Ben.
>
>
--
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/