linux-kernel - Re: [PATCH v1] i3c: master: svc: Fix npcm845 FIFO

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: <CAPwEoQNfdit9fOiUY-WzQvS377nEqBu=s8S1zqS7tdSZfQeCHQ@mail.gmail.com>
Date: Fri, 25 Jul 2025 22:50:34 +0800
From: Stanley Chu <stanley.chuys@...il.com>
To: Frank Li <Frank.li@....com>
Cc: miquel.raynal@...tlin.com, alexandre.belloni@...tlin.com, 
	linux-i3c@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	tomer.maimon@...oton.com, kwliu@...oton.com, yschu@...oton.com
Subject: Re: [PATCH v1] i3c: master: svc: Fix npcm845 FIFO_EMPTY quirk

Hi Frank,

On Fri, Jul 25, 2025 at 9:45 PM Frank Li <Frank.li@....com> wrote:
>
> On Fri, Jul 25, 2025 at 03:03:10PM +0800, Stanley Chu wrote:
> > From: Stanley Chu <yschu@...oton.com>
> >
> > Prefilling in private write transfers is only necessary when the FIFO
> > is empty. Otherwise, if the transfer is NACKed due to IBIWON and retries
> > continue, data may be prefilled again but could be lost because the FIFO
> > is not empty.
>
> why "maybe prefilled", please use certain words.
The original code didn't consider the retry case, data lost happen in
the retry case only when total write bytes is larger than FIFO size,
not always happen.
In the example of writing 17 bytes (FIFO size is 16), the last byte is lost.
1. Emit S+Addr/W
2. Write 16 bytes to FIFO (FIFO is full)
3. NACKed due to IBIWON
4. (retry) Emit Sr+Addr/W
5. Write last byte to FIFO (data lost)

I just need to prefill FIFO in the beginning to make sure FIFO is not
empty when HW starts transmitting.
So, this patch adds a condition that prefill FIFO only when FIFO is empty.

>
> >
> > Fixes: 4008a74e0f9b ("i3c: master: svc: Fix npcm845 FIFO empty issue")
> > Signed-off-by: Stanley Chu <yschu@...oton.com>
> > ---
> >  drivers/i3c/master/svc-i3c-master.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> > index 7e1a7cb94b43..34b6e125b18a 100644
> > --- a/drivers/i3c/master/svc-i3c-master.c
> > +++ b/drivers/i3c/master/svc-i3c-master.c
> > @@ -104,6 +104,7 @@
> >  #define   SVC_I3C_MDATACTRL_TXTRIG_FIFO_NOT_FULL GENMASK(5, 4)
> >  #define   SVC_I3C_MDATACTRL_RXTRIG_FIFO_NOT_EMPTY 0
> >  #define   SVC_I3C_MDATACTRL_RXCOUNT(x) FIELD_GET(GENMASK(28, 24), (x))
> > +#define   SVC_I3C_MDATACTRL_TXCOUNT(x) FIELD_GET(GENMASK(20, 16), (x))
> >  #define   SVC_I3C_MDATACTRL_TXFULL BIT(30)
> >  #define   SVC_I3C_MDATACTRL_RXEMPTY BIT(31)
> >
> > @@ -280,6 +281,13 @@ static inline bool is_events_enabled(struct svc_i3c_master *master, u32 mask)
> >       return !!(master->enabled_events & mask);
> >  }
> >
> > +static inline bool svc_i3c_master_tx_empty(struct svc_i3c_master *master)
> > +{
> > +     u32 reg = readl(master->regs + SVC_I3C_MDATACTRL);
> > +
> > +     return (SVC_I3C_MDATACTRL_TXCOUNT(reg) == 0);
> > +}
> > +
> >  static bool svc_i3c_master_error(struct svc_i3c_master *master)
> >  {
> >       u32 mstatus, merrwarn;
> > @@ -1303,7 +1311,8 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> >                * The only way to work around this hardware issue is to let the
> >                * FIFO start filling as soon as possible after EmitStartAddr.
> >                */
> > -             if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len) {
> > +             if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len &&
> > +                 svc_i3c_master_tx_empty(master)) {
> >                       u32 end = xfer_len > SVC_I3C_FIFO_SIZE ? 0 : SVC_I3C_MWDATAB_END;
> >                       u32 len = min_t(u32, xfer_len, SVC_I3C_FIFO_SIZE);
>
> if prefill to FIFO SIZE each time, such as
>
> replace SVC_I3C_FIFO_SIZE with
>
> SVC_I3C_FIFO_SIZE - SVC_I3C_MDATACTRL_TXCOUNT(readl(master->regs + SVC_I3C_MDATACTRL)).
>
The free space is SVC_I3C_FIFO_SIZE when FIFO is empty, no need to
check TX count again.

--
Stanley

> Frank
> >
> > --
> > 2.34.1
> >