| 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: <Z79HCj+wxYDN6W0e@lizhi-Precision-Tower-5810>
Date: Wed, 26 Feb 2025 11:53:30 -0500
From: Frank Li <Frank.li@....com>
To: Stanley Chu <stanley.chuys@...il.com>
Cc: miquel.raynal@...tlin.com, alexandre.belloni@...tlin.com,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
linux-i3c@...ts.infradead.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, tomer.maimon@...oton.com,
kwliu@...oton.com, yschu@...oton.com
Subject: Re: [PATCH v4 3/5] i3c: master: svc: Fix npcm845 FIFO empty issue
On Wed, Feb 26, 2025 at 11:26:24AM +0800, Stanley Chu wrote:
> On Wed, Feb 26, 2025 at 12:32 AM Frank Li <Frank.li@....com> wrote:
> >
> > On Tue, Feb 25, 2025 at 05:28:48PM +0800, Stanley Chu wrote:
> > > On Tue, Feb 25, 2025 at 12:32 AM Frank Li <Frank.li@....com> wrote:
> > > >
> > > > On Mon, Feb 24, 2025 at 04:39:06PM +0800, Stanley Chu wrote:
> > > > > From: Stanley Chu <yschu@...oton.com>
> > > > >
> > > > > I3C HW stalls the write transfer if the transmit FIFO becomes empty,
> > > > > when new data is written to FIFO, I3C HW resumes the transfer but the
> > > > > first transmitted data bit may have the wrong value.
> > > > > Fill the FIFO in advance to prevent FIFO from becoming empty.
> > > > >
> > > > > Signed-off-by: Stanley Chu <yschu@...oton.com>
> > > > > ---
> > > > > drivers/i3c/master/svc-i3c-master.c | 44 ++++++++++++++++++++---------
> > > > > 1 file changed, 31 insertions(+), 13 deletions(-)
> > > > >
> > > > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> > > > > index 8834f87a4767..07506ae0f914 100644
> > > > > --- a/drivers/i3c/master/svc-i3c-master.c
> > > > > +++ b/drivers/i3c/master/svc-i3c-master.c
> > > > > @@ -942,7 +942,7 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
> > > > > u8 *addrs, unsigned int *count)
> > > > > {
> > > > > u64 prov_id[SVC_I3C_MAX_DEVS] = {}, nacking_prov_id = 0;
> > > > > - unsigned int dev_nb = 0, last_addr = 0;
> > > > > + unsigned int dev_nb = 0, last_addr = 0, dyn_addr;
> > > > > u32 reg;
> > > > > int ret, i;
> > > > >
> > > > > @@ -985,6 +985,17 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
> > > > > if (SVC_I3C_MSTATUS_RXPEND(reg)) {
> > > > > u8 data[6];
> > > > >
> > > > > + /*
> > > > > + * One slave sends its ID to request for address assignment,
> > > > > + * pre-filling the dynamic address can reduce SCL clock stalls
> > > > > + * and also fix the SVC_I3C_QUIRK_FIFO_EMPTY quirk.
> > > > > + */
> > > > > + dyn_addr = i3c_master_get_free_addr(&master->base, last_addr + 1);
> > > > > + if (dyn_addr < 0)
> > > > > + return -ENOSPC;
> > > > > +
> > > > > + writel(dyn_addr, master->regs + SVC_I3C_MWDATAB);
> > > > > +
> > > >
> > > > Although there is 64 clock time after issue do_daa, it is still better if
> > > > prefill dyn_addr before sent do daa command?
> > > >
> > > > If add a debug message before i3c_master_get_free_addr(), does it trigger
> > > > hardware issue?
> > > >
> > > > Frank
> > >
> > > Ideally, prefilling before the processDAA command is better. However,
> > > it requires an additional check to write the dyn_addr at the right time
> > > because the driver needs to write the processDAA command twice for one
> > > assignment
> > >
> > > Prefilling here is safe and efficient because the FIFO starts filling
> > > within a few hundred nanoseconds on the npcm845, which is significantly
> > > faster compared to the 64 SCL clock cycles.
> >
> > Okay, please this to comments.
> >
> > >
> > >
> > > >
> > > > > /*
> > > > > * We only care about the 48-bit provisioned ID yet to
> > > > > * be sure a device does not nack an address twice.
> > > > > @@ -1063,21 +1074,16 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
> > > > > if (ret)
> > > > > break;
> > > > >
> > > > > - /* Give the slave device a suitable dynamic address */
> > > > > - ret = i3c_master_get_free_addr(&master->base, last_addr + 1);
> > > > > - if (ret < 0)
> > > > > - break;
> > > > > -
> > > > > - addrs[dev_nb] = ret;
> > > > > + addrs[dev_nb] = dyn_addr;
> > > > > dev_dbg(master->dev, "DAA: device %d assigned to 0x%02x\n",
> > > > > dev_nb, addrs[dev_nb]);
> > > > > -
> > > > > - writel(addrs[dev_nb], master->regs + SVC_I3C_MWDATAB);
> > > > > last_addr = addrs[dev_nb++];
> > > > > }
> > > > >
> > > > > /* Need manual issue STOP except for Complete condition */
> > > > > svc_i3c_master_emit_stop(master);
> > > > > + svc_i3c_master_flush_fifo(master);
> > > > > +
> > > > > return ret;
> > > > > }
> > > > >
> > > > > @@ -1225,8 +1231,8 @@ static int svc_i3c_master_read(struct svc_i3c_master *master,
> > > > > return offset;
> > > > > }
> > > > >
> > > > > -static int svc_i3c_master_write(struct svc_i3c_master *master,
> > > > > - const u8 *out, unsigned int len)
> > > > > +static int svc_i3c_master_write(struct svc_i3c_master *master, const u8 *out,
> > > > > + unsigned int len, bool last)
> > > > > {
> > > > > int offset = 0, ret;
> > > > > u32 mdctrl;
> > > > > @@ -1243,7 +1249,7 @@ static int svc_i3c_master_write(struct svc_i3c_master *master,
> > > > > * The last byte to be sent over the bus must either have the
> > > > > * "end" bit set or be written in MWDATABE.
> > > > > */
> > > > > - if (likely(offset < (len - 1)))
> > > > > + if (likely(offset < (len - 1)) || !last)
> > > > > writel(out[offset++], master->regs + SVC_I3C_MWDATAB);
> > > > > else
> > > > > writel(out[offset++], master->regs + SVC_I3C_MWDATABE);
> > > > > @@ -1274,6 +1280,17 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> > > > > SVC_I3C_MCTRL_RDTERM(*actual_len),
> > > > > master->regs + SVC_I3C_MCTRL);
> > > > >
> > > > > + if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len) {
> > > > > + u32 len = min_t(u32, xfer_len, SVC_I3C_FIFO_SIZE);
> > > > > +
> > > > > + ret = svc_i3c_master_write(master, out, len,
> > > > > + xfer_len <= SVC_I3C_FIFO_SIZE);
> > > > > + if (ret < 0)
> > > > > + goto emit_stop;
> > > > > + xfer_len -= len;
> > > > > + out += len;
> > > > > + }
> > > > > +
> > > >
> > > > The same here, you prefill data after issue sent out address, timing still
> > > > tight, only 9 SCL clock time. should it prefill before issue address?
> > > >
> > > > Frank
> > >
> > > The entire transaction can consist of multiple read and write
> > > transfers. In the case
> > > of S+7E/W+Sr+dyn_addr/W+data+P, If the data is prefilled before Sr, it
> > > will be emitted
> >
> > I think S+7E/W should be xfer[0]
> > Sr+dyn_addr/W + data + P should be xfer[1]
> >
> > this function only handle one xfer each call. xfer[0]'s size is 0, no
> > pre fill data.
> >
> > Only have prefill data at xfer[1].
> >
>
> Hi Frank,
>
> Please check the example, a transaction contains 2 transfers.
> xfer[0].addr=7E/W, no data
> xfer[1].addr=dyn_addr/W, with data
>
> send xfer[0]:
> 1. Emit Start+7E/W
> 2. Wait for MCTRLDONE
> send xfer[1]:
> 3. Prefill xfer[1].data // data is emitted immediately
Yes, you are right.
> 4. Emit RepeatedStart+dyn_addr/W
> 5. Wait for MCTRLDONE
> 6. Write remaining data to FIFO if needed
> 7. Emit STOP
>
> Part of xfer[1].data is emitted in xfer[0], which is the problem of prefill
> before EmitStartAddr. This is the reason I moved step 3 after step 4.
Need comments to show the reason.
>
>
> > > immediately and become part of the previous transfer.
> > >
> > > It is not a problem to fill FIFO here, the reason is the same as above.
> > > I will also modify the code as below to make it efficient and keep
> > > svc_i3c_master_write unchanged.
> >
> > no issue to modify svc_i3c_master_write(). I consider prefill data before
> > actually.
> >
> > This hardware is not prefect. Although it aleady in spin lock, it may run
> > some secuity firmware in secuity domain. There are 100us timeout. If a
> > hypervisor manage firmware interrupt transfer, one timeout may happen.
> >
> > If prefill data before send address, it was safe at least for lenght less
> > than FIFO case.
> >
> > Frank
> >
>
> Since prefill before EmitStartAddr has the problem I mentioned above, the FIFO
> should start filling as soon as possible to work around the hardware issue.
> This is why I prefer writing MWDATAB here instead of using svc_i3c_master_write,
> as it can save some polling time.
svc_i3c_master_xfer() is too long and not easy to read. You can create
new helper function/macro for it if you like.
>
> The hardware-specific quirk is added to avoid affecting other platforms that
> do not have this issue.
>
> Stanley
>
>
> > >
> > > if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) &&
> > > !rnw && xfer_len) {
> > > u32 len = min_t(u32, xfer_len, SVC_I3C_FIFO_SIZE);
> > >
> > > while (len--) {
> > > if (xfer_len == 1)
> > > writel(out[0], master->regs +
> > > SVC_I3C_MWDATABE);
> > > else
> > > writel(out[0], master->regs +
> > > SVC_I3C_MWDATAB);
Or
#define SVC_I3C_MWD(last) ((last) ? SVC_I3C_MWDATABE : SVC_I3C_MWDATAB)
writel(out[0], master->regs + SVC_I3C_MWD(xfer_len == 1))
Frank
> > > xfer_len--;
> > > out++;
> > > }
> > > }
> > >
> > >
> > > >
> > > > > ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> > > > > SVC_I3C_MSTATUS_MCTRLDONE(reg), 0, 1000);
> > > > > if (ret)
> > > > > @@ -1335,7 +1352,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> > > > > if (rnw)
> > > > > ret = svc_i3c_master_read(master, in, xfer_len);
> > > > > else
> > > > > - ret = svc_i3c_master_write(master, out, xfer_len);
> > > > > + ret = svc_i3c_master_write(master, out, xfer_len, true);
> > > > > if (ret < 0)
> > > > > goto emit_stop;
> > > > >
> > > > > @@ -1362,6 +1379,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> > > > > emit_stop:
> > > > > svc_i3c_master_emit_stop(master);
> > > > > svc_i3c_master_clear_merrwarn(master);
> > > > > + svc_i3c_master_flush_fifo(master);
> > > > >
> > > > > return ret;
> > > > > }
> > > > > --
> > > > > 2.34.1
> > > > >
> > >
> > > --
> > > linux-i3c mailing list
> > > linux-i3c@...ts.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-i3c
Powered by blists - more mailing lists