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:	Thu, 7 Feb 2013 15:31:16 +0100
From:	Michal Simek <monstr@...str.eu>
To:	Alexey Brodkin <Alexey.Brodkin@...opsys.com>
Cc:	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	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, gsmecher@...eespeedlogic.com
Subject: Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16
 with generic iowrite(read)8/16(be)

2013/2/7 Alexey Brodkin <Alexey.Brodkin@...opsys.com>:
> 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.

The origin patch (after some microblaze ioread/iowrite fixes) works ok,
These additional changes is breaking sysace on microblaze big endian
ml505 16bit mode.
8bit mode just works

Also I have looked at our tree and I see that the mainline kernel misses
one patch which was sent by Graeme Smecher and it was also Acked-by Grant.
It is called "Fix device name assignment for SystemACE (from "xs`" to "xsa")."
Let me resend it too.

Alexey: Can you please confirm that on your arc platform you see broken CF
partitions name?

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
--
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/