linux-kernel - Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

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:   Tue, 8 Nov 2016 10:13:58 -0700
From:   Joel Holdsworth <joel@...webreathe.org.uk>
To:     Marek Vasut <marex@...x.de>, atull@...nsource.altera.com,
        moritz.fischer@...us.com, geert@...ux-m68k.org, robh@...nel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-spi@...r.kernel.org, clifford@...fford.at
Subject: Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

Hi Marek,

>>> Should be this way for the sake of readability, fix globally:
>>>
>>>     struct spi_transfer assert_cs_then_reset_delay = {
>>>         .cs_change    = 1,
>>>         .delay_usecs    = ICE40_SPI_FPGAMGR_RESET_DELAY
>>>     };
>>
>> Sure ok. Personally, I prefer it to be concise, but I'm happy to accept
>> the norms.
>
> I prefer it to be readable :)
>

I'll conform to what you're saying.

But I just want to point out that this...


spi_message_add_tail(&(struct spi_transfer){.cs_change = 1,
	.delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY}, &message);


...is clearly more readable than this...


struct spi_transfer assert_cs_then_reset_delay = {
     .cs_change    = 1,
     .delay_usecs  = ICE40_SPI_FPGAMGR_RESET_DELAY
};

....
<30 lines of unrelated code>
....

spi_message_add_tail(&assert_cs_then_reset_delay, &message);



...in my opinion anyway ;)



>> Previously I had a copy of spi_set_cs copy-pasted into my driver, but in
>> the end I decided to replace that with the zero-length transfers because
>> there's a danger that if the original spi_set_cs() gets rewritten some
>> time, my copy-paste code would leave around some nasty legacy.
>>
>> On the whole, I don't think the zero-length transfers are too
>> egregiously bad, and all the alternatives seem worse to me.
>
> So why not turn the CS line into GPIO and just toggle the GPIO?
>

Two reasons.

1. On some devices the CS line is built into the SPI master, rather than 
being a normal GPIO.

2. The SPI driver stack addresses SPI devices in terms of which CS line 
they are attached to. You can't have an spi_device in the kernel where 
the SPI driver machinery doesn't have a CS line to control. Moreover it 
needs to be possible for another SPI device to interrupt a running 
transfer to the FPGA. Supporting this involves the SPI framework 
managing the CS line.



Thanks
Joel