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:   Mon, 7 Nov 2016 12:02:01 -0700
From:   Joel Holdsworth <joel@...webreathe.org.uk>
To:     Moritz Fischer <moritz.fischer@...us.com>
Cc:     Alan Tull <atull@...nsource.altera.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Rob Herring <robh@...nel.org>,
        Devicetree List <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-spi@...r.kernel.org,
        Marek Vašut <marex@...x.de>,
        clifford@...fford.at
Subject: Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

Hi Moritz - thanks for your comments.


>> An example of such a device is the icoBoard; a RaspberryPI HAT which
>> features an iCE40HX8K with a 1 or 8 MBit SRAM and ports for
>> Digilent-compatible PMOD modules. A PMOD module may contain a device
>> with which the kernel communicates, via the FPGA.
>
> Personally I find this a bit verbose, but that's just me.

I could cut it down a bit if you want. I was just trying to make the 
case for why this *has* to be in the kernel rather than just being done 
from user-space.


>> +       struct spi_transfer assert_cs_then_reset_delay = {.cs_change = 1,
>> +               .delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY};
>
> Formatting looks odd, can you move the .cs_change to the next line?

Marek made the same comment. I'll make the change.


>> +static int ice40_fpga_ops_write_complete(struct fpga_manager *mgr, u32 flags)
>> +{
>> +       struct ice40_fpga_priv *priv = mgr->priv;
>> +       struct spi_device *dev = priv->dev;
>> +       const u8 padding[ICE40_SPI_FPGAMGR_NUM_ACTIVATION_BYTES] = {0,};
>> +
>> +       /* Check CDONE is asserted */
>> +       if (!gpiod_get_value(priv->cdone)) {
>> +               dev_err(&dev->dev,
>> +                       "CDONE was not asserted after firmware transfer\n");
>> +               return -EIO;
>> +       }
>> +
>> +       /* Send of zero-padding to activate the firmware */
>> +       return spi_write(dev, padding, sizeof(padding));
>
> I'd move all of these into the write() callback.

Is that correct? I was under the impression that write might be called 
multiple times to load firmware in multiple chunks.



>> +       /* Enter reset */
>> +       gpiod_set_value(priv->reset, 1);
>
> I know Marek had suggested this, none of the other drivers behave like that.
> I'm not sure this is expected behavior for most people.

For me it's not a big deal either way, but I think it's quite good for 
the driver to reset the device.

When you remove most other drivers, you expect the driver to shut the 
device down, so isn't this just the same?

...but then the FPGA manager is a unique best with its own conventions, 
so perhaps it should behave differently.

How can we get a consensus about this?


Joel