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]

Message-ID: <da1e5125-de6b-11a8-a52d-7e6e5f45ab70@conchuod.ie>
Date:   Mon, 9 May 2022 19:56:15 +0100
From:   Conor Dooley <mail@...chuod.ie>
To:     Ivan Bornyakov <i.bornyakov@...rotek.ru>,
        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 09/05/2022 18:16, Ivan Bornyakov wrote:
> 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?
> 

I was in a bit of a rush today & didn't have time to do proper
debugging, I'll put some debug code in tomorrow and try to find
exactly what is different between the two.

Off the top of my head, since I don't have a board on me to test,
the only difference I can see is that with the snippet you only
checked if spi_sync_transfer was negative whereas now you check
if it has a value at all w/ that ternary operator.

But even that seems like it *shouldn't* be the problem, since ret
should contain -errno or zero, right?
Either way, I will do some digging tomorrow.

Thanks,
Conor.