linux-kernel - Re: [PATCH v11 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]

Date:   Mon, 9 May 2022 20:16:21 +0300
From:   Ivan Bornyakov <i.bornyakov@...rotek.ru>
To:     Conor.Dooley@...rochip.com
Cc:     mdf@...nel.org, hao.wu@...el.com, yilun.xu@...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 v11 2/3] fpga: microchip-spi: add Microchip MPF FPGA
 manager

On Mon, May 09, 2022 at 11:41:18AM +0000, Conor.Dooley@...rochip.com wrote:
> Hey Ivan, one comment below.
> Thanks,
> Conor.
> 
> On 07/05/2022 08:43, Ivan Bornyakov wrote:
> > ... snip ...
> > +static int mpf_read_status(struct spi_device *spi)
> > +{
> > +       u8 status, status_command = MPF_SPI_READ_STATUS;
> > +       struct spi_transfer xfer = {
> > +               .tx_buf = &status_command,
> > +               .rx_buf = &status,
> > +               .len = 1,
> > +       };
> > +       int ret = spi_sync_transfer(spi, &xfer, 1);
> > +
> > +       if ((status & MPF_STATUS_SPI_VIOLATION) ||
> > +           (status & MPF_STATUS_SPI_ERROR))
> > +               ret = -EIO;
> > +
> > +       return ret ? : status;
> > +}
> > +
> > ... snip ...
> > +
> > +static int poll_status_not_busy(struct spi_device *spi, u8 mask)
> > +{
> > +       int status, timeout = MPF_STATUS_POLL_TIMEOUT;
> > +
> > +       while (timeout--) {
> > +               status = mpf_read_status(spi);
> > +               if (status < 0 ||
> > +                   (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask))))
> > +                       return status;
> > +
> > +               usleep_range(1000, 2000);
> > +       }
> > +
> > +       return -EBUSY;
> > +}
> 
> Is there a reason you changed this from the snippet you sent me
> in the responses to version 8:
> static int poll_status_not_busy(struct spi_device *spi, u8 mask)
> {
> 	u8 status, status_command = MPF_SPI_READ_STATUS;
> 	int ret, timeout = MPF_STATUS_POLL_TIMEOUT;
> 	struct spi_transfer xfer = {
> 		.tx_buf = &status_command,
> 		.rx_buf = &status,
> 		.len = 1,
> 	};
> 
> 	while (timeout--) {
> 		ret = spi_sync_transfer(spi, &xfer, 1);
> 		if (ret < 0)
> 			return ret;
> 
> 		if (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask)))
> 			return status;
> 
> 		usleep_range(1000, 2000);
> 	}
> 
> 	return -EBUSY;
> }
> 
> With the current version, I hit the "Failed to write bitstream
> frame" check in mpf_ops_write at random points in the transfer.
> Replacing poll_status_not_busy with the above allows it to run
> to completion.

In my eyes they are equivalent, aren't they?