linux-hardening - Re: [PATCH RFC 4/7] i3c: add driver for Renesas I3C IP

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: <aEsDj5Vcb4zFJFlo@lizhi-Precision-Tower-5810>
Date: Thu, 12 Jun 2025 12:42:56 -0400
From: Frank Li <Frank.li@....com>
To: Wolfram Sang <wsa+renesas@...g-engineering.com>,
	linux-renesas-soc@...r.kernel.org,
	Tommaso Merciai <tommaso.merciai.xr@...renesas.com>,
	Alexandre Belloni <alexandre.belloni@...tlin.com>,
	Kees Cook <kees@...nel.org>,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	Magnus Damm <magnus.damm@...il.com>, linux-i3c@...ts.infradead.org,
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH RFC 4/7] i3c: add driver for Renesas I3C IP

On Thu, Jun 12, 2025 at 04:55:31PM +0200, Wolfram Sang wrote:
> Hi Frank,
>
> thanks again for the super-fast review.
>
> > > - RENESAS_I3C_MAX_DEVS is a constant currently. So, we could get rid of
> > >   the 'maxdevs' variable. I tend to keep it this way in case future
> > >   controllers may use a different value, then we can change it easily.
> >
> > It is similar ADI version. Can you simple descript hardware behavor to show
> > why need RENESAS_I3C_MAX_DEVS,
>
> Yes, for example this register has basic status info per target:
>
> > > +#define DATBAS(x)		(0x224 + 0x8 * (x))
>
> And there are only 8 of these registers. So, there is a maximum of 8 for
> this controller. We could hardcode 8. But we could leave the handling as
> is, just in case a future controller has more or less of these
> registers.

Okay, can you point me spec link.

>
> > > - For accessing the FIFOs, this driver uses the same code as existing
> > >   upstream drivers or the recenlty submitted "ADI" driver. There, the
> > >   question came up, if this could be a helper function?
> >
> > Add common header in drivers/i3c/master/core.h implement inline helper
> > function
>
> Sure thing. I think I didn't get feedback on my original suggestion so
> far. If I now know you are positive about it, I will give it a try.

Sorry, linux-i3c mail list always delay your post, did you register linux-i3c
mail list.

>
> > #define PRTS                 0x00
> > #define  PRTS_PRTMD          BIT(0)
> >
> > Add extra space help distinguish register and field define.
>
> Okay.
>
> >
> > > +
> > > +#define BCTL			0x14
> > > +#define BCTL_HJACKCTL		BIT(8)
> > > +#define BCTL_ABT		BIT(29)
> > > +#define BCTL_BUSE		BIT(31)
> > > +
> > > +#define MSDVAD			0x18
> > > +#define MSDVAD_MDYAD(x)		(((x) & 0x3f) << 16)
> >
> > Use GEN_MASK
>
> Sigh, if you think this is more readable, then OK.
>
> > > +#define STDBR			0x74
> > > +#define STDBR_SBRLO(cond, x)	((((x) >> (cond)) & 0xff) << 0)
> > > +#define STDBR_SBRHO(cond, x)	((((x) >> (cond)) & 0xff) << 8)
> >
> > FIELD_GET FIELD_PREP
>
> Ditto.
>
> > > +static inline void i3c_reg_update(u32 mask, u32 val, void __iomem *reg)
> > > +{
> > > +	u32 data = readl(reg);
> > > +
> > > +	data &= ~mask;
> > > +	data |= (val & mask);
> > > +	writel(data, reg);
> > > +}
> >
> > can you move reg to first argument to align below help function?
>
> Yup, coccinelle should make that easy.
>
> > > +out:
> > > +	kfree(xfer);
> >
> > you can __free(kfree) xfer = NULL at declear to avoid this goto branch.
>
> I'll give it a try...
>
> > > +		if (!i3c_xfers[i].rnw && i3c_xfers[i].len > 4) {
> >
> > Only pre fill fifo when len >4? what happen if only write 1 byte?
>
> Ehrm, good catch. I will check this in more detail.
>
> > > +			renesas_i3c_write_to_tx_fifo(i3c, cmd->tx_buf, cmd->len);
> > > +			if (cmd->len > NTDTBP0_DEPTH * sizeof(u32))
> > > +				i3c_reg_set_bit(i3c->regs, NTIE, NTIE_TDBEIE0);
> > > +		}
> > > +
> > > +		renesas_i3c_enqueue_xfer(i3c, xfer);
> > > +		if (!wait_for_completion_timeout(&xfer->comp, XFER_TIMEOUT))
> > > +			renesas_i3c_dequeue_xfer(i3c, xfer);
> >
> > This may common problem, I3C spec have 100us timeout, target side may
> > timeout when driver wait for irq. So target side treat "repeat start" as
> > "start" and issue address arbitration.
>
> There is a specified timeout? I couldn't find one in the specs, can you
> kindly point me to it? So, the solution is to use 100us as timeout?

See: 5.1.2.5 Controller Clock Stalling

The spec have not defined what exactly happen if stall clock more than
100us.

I think it is hard to resolve this problem if hardware have not DMA queue.

Frank

>
> > > +	i3c->addrs[pos] = dev->info.dyn_addr ? : dev->info.static_addr;
> >
> > Are there extension of "?" operator in C ? I remember
> >
> > dev->info.dyn_addr ? dev->info.dyn_addr : dev->info.static_addr;
>
> Dunno if it made it into the standard these days, but as a GCC extension
> it is used in the kernel for ages. I encourage its use in I2C, other
> maintainers don't like it much. Mileages vary.
>
> > > +		if (ret) {
> > > +			dev_err(&pdev->dev, "failed to request irq %s\n",
> > > +				renesas_i3c_irqs[i].desc);
> > > +			return ret;
> >
> > return dev_err_probe()
>
> OK.
>
> Thanks and happy hacking,
>
>    Wolfram
>