| 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
| ||
|
Message-ID: <20220511081810.bwxjkhfuzfvov4iw@h-e2.ddg>
Date: Wed, 11 May 2022 11:18:10 +0300
From: Ivan Bornyakov <i.bornyakov@...rotek.ru>
To: Conor Dooley <mail@...chuod.ie>
Cc: Conor.Dooley@...rochip.com, 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 Wed, May 11, 2022 at 11:15:32AM +0300, Ivan Bornyakov wrote:
> On Tue, May 10, 2022 at 12:29:54PM +0100, Conor Dooley wrote:
> > On 09/05/2022 19:56, Conor Dooley wrote:
> > > 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.
> >
> > I put a printk("status %x, ret %d", status, ret); into the failure
> > path of mpf_read_status() & it looks like a status 0xA is being
> > returned - error & ready? That seems like a very odd combo to be
> > getting back out of it. It shouldn't be dodgy driver/connection
> > either, b/c that's what I see if I connect my protocol analyser:
> > https://i.imgur.com/VbjgfCk.png
> >
> > That's mosi (hex), ss, sclk, mosi, miso (hex), miso in descending
> > order.
> >
> > I think what was happening was with the snippet you returned one
> > of the following: -EBUSY, ret (aka -errno) or status. Since status
> > is positive, the checks in mpf_spi_write.*() saw nothing wrong at
> > all and programming continued despite there being a problem.
> >
> > The new version fixes this by returning -EIO rather than status from
> > poll_status_not_busy().
> >
> > I wish I had a socketable PolarFire so I could investigate further,
> > but this looks like it might a be hardware issue somewhere on my
> > end?
> >
> > So ye, sorry for the noise and carry on! I'll try tofind what is to
> > blame for it.
> >
> > Thanks,
> > Conor.
> >
>
> Hi, Conor.
>
> I've just noticed in SPI-DirectC User Guide [1] ch. 9 SmartFusion2 and
> IGLOO2 SPI-Slave Programming Waveform Analysis, that hw status checked
> two times every time. Does MPF family also need double check hw status?
> Does adding second mpf_read_status() to poll_status_not_busy() routine
> help with your issue?
Sorry, forgot to insert link to SPI-DirectC User Guide. Here it is:
[1] https://coredocs.s3.amazonaws.com/DirectC/2021_2/spi_directc.pdf
Powered by blists - more mailing lists