linux-kernel - Re: [PATCH v13 2/3] fpga: microchip-spi: add Microchip MPF FPGA manager

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: <20220529185158.gjoxrb65gpnh4g3k@x260>
Date:   Sun, 29 May 2022 21:51:58 +0300
From:   Ivan Bornyakov <i.bornyakov@...rotek.ru>
To:     Conor.Dooley@...rochip.com
Cc:     yilun.xu@...el.com, mdf@...nel.org, hao.wu@...el.com,
        trix@...hat.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, linux-fpga@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        system@...rotek.ru
Subject: Re: [PATCH v13 2/3] fpga: microchip-spi: add Microchip MPF FPGA
 manager

On Sun, May 29, 2022 at 01:03:10PM +0000, Conor.Dooley@...rochip.com wrote:
> On 29/05/2022 13:39, Xu Yilun wrote:
> > On Thu, May 26, 2022 at 09:13:43PM +0300, Ivan Bornyakov wrote:
> >> Add support to the FPGA manager for programming Microchip Polarfire
> >> FPGAs over slave SPI interface with .dat formatted bitsream image.
> > 
> > From previous mail thread, there are still some hardware operations yet
> > to be clarified, so I may need a Reviewed-by from Conor.Dooley@...rochip.com.
> 
> Yeah, was really busy last week. Planning on giving this version a go
> tomorrow. I *think* I explained the reason the status check needed to be a
> sync_transfer() but it hasn't been reflected in a comment yet.
> 
> I didn't know the answers to the two other questions & passed them on to the
> designers of the hardware blocks - but both are traveling so not got a
> response yet. There's one bit of clarification I'd like from your:
> 
> >>> +static int mpf_ops_write(struct fpga_manager *mgr, const char *buf, size_t count)
> >>> +{
> >>> +	u8 tmp_buf[MPF_SPI_FRAME_SIZE + 1] = { MPF_SPI_FRAME, };
> >>> +	struct mpf_priv *priv = mgr->priv;
> >>> +	struct device *dev = &mgr->dev;
> >>> +	struct spi_device *spi;
> >>> +	int ret, i;
> >>> +
> >>> +	if (count % MPF_SPI_FRAME_SIZE) {
> >>> +		dev_err(dev, "Bitstream size is not a multiple of %d\n",
> >>> +			MPF_SPI_FRAME_SIZE);
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	spi = priv->spi;
> >>> +
> >>> +	for (i = 0; i < count / MPF_SPI_FRAME_SIZE; i++) {
> >>> +		memcpy(tmp_buf + 1, buf + i * MPF_SPI_FRAME_SIZE,
> >>> +		       MPF_SPI_FRAME_SIZE);
> >>> +
> >>> +		ret = mpf_spi_write(spi, tmp_buf, sizeof(tmp_buf));
> >>
> >> As I mentioned before, is it possible we use spi_sync_transfer to avoid
> >> memcpy the whole bitstream?
> >
> > Unfortunately, I didn't succeed with spi_sunc_transfer here. May be
> > Conor or other folks with more insight on Microchip's HW would be able
> > to eliminate this memcpy...
> 
> I understood this as being a question about why it was required to check
> the status of the hardware between frames of the bitstream rather than
> using spi_sync_transfer() to copy each frame back to back.
> 
> Is that correct?

No.
The issue here is memcpy() a bitstream data frame to temporary buffer
before sending it to the device.
The reason for memcpy() is that we need to send to the device 17 bytes:
1st byte 0xEE and next 16 bytes - bitstream data.
Possible solution to eliminate memcpy() is to use spi_sync_transfer()
instead of spi_write() for sending bitstream frames, like so:

diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
index 7579b0de119f..bf62ee7fd630 100644
--- a/drivers/fpga/microchip-spi.c
+++ b/drivers/fpga/microchip-spi.c
@@ -270,7 +270,8 @@ static int mpf_ops_write_init(struct fpga_manager *mgr,

 static int mpf_ops_write(struct fpga_manager *mgr, const char *buf, size_t count)
 {
-       u8 tmp_buf[MPF_SPI_FRAME_SIZE + 1] = { MPF_SPI_FRAME, };
+       u8 spi_frame_command = MPF_SPI_FRAME;
+       struct spi_transfer xfers[2] = { 0 };
        struct mpf_priv *priv = mgr->priv;
        struct device *dev = &mgr->dev;
        struct spi_device *spi;
@@ -285,10 +286,15 @@ static int mpf_ops_write(struct fpga_manager *mgr, const char *buf, size_t count
        spi = priv->spi;

        for (i = 0; i < count / MPF_SPI_FRAME_SIZE; i++) {
-               memcpy(tmp_buf + 1, buf + i * MPF_SPI_FRAME_SIZE,
-                      MPF_SPI_FRAME_SIZE);
+               xfers[0].tx_buf = &spi_frame_command;
+               xfers[0].len = 1;
+               xfers[1].tx_buf = buf + i * MPF_SPI_FRAME_SIZE;
+               xfers[1].len = MPF_SPI_FRAME_SIZE;
+
+               ret = mpf_poll_status(spi, 0);
+               if (ret >= 0)
+                       ret = spi_sync_transfer(spi, xfers, 2);

-               ret = mpf_spi_write(spi, tmp_buf, sizeof(tmp_buf));
                if (ret) {
                        dev_err(dev, "Failed to write bitstream frame %d/%zu\n",
                                i, count / MPF_SPI_FRAME_SIZE);

Note that this is not a working solution.