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: <aFE4j86NaZcMvxBF@shikoro>
Date: Tue, 17 Jun 2025 11:42:39 +0200
From: Wolfram Sang <wsa+renesas@...g-engineering.com>
To: Frank Li <Frank.li@....com>
Cc: 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

Hi,

some status updates...

> It is similar ADI version. Can you simple descript hardware behavor to show
> why need RENESAS_I3C_MAX_DEVS,

I hope the documentation links I sent were helpful.

> Add common header in drivers/i3c/master/core.h implement inline helper
> function

I decided to handle this incrementally. I need to work on the real show
stoppers first, I'd say.

> #define PRTS                 0x00
> #define  PRTS_PRTMD          BIT(0)
> 
> Add extra space help distinguish register and field define.

Done.

> > +#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
> 
> check other define

Done. Also with GETMASK

> > +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?

Done.

> > +out:
> > +	kfree(xfer);
> 
> you can __free(kfree) xfer = NULL at declear to avoid this goto branch.

Do you insist on this? It makes the driver less consistent because there
are other 'out'-branches with kfree() in them which we need to keep
anyhow, because there are more instructions needed. I can do the change
if you want but I personally would prefer to leave the code as is.

> > +		if (!i3c_xfers[i].rnw && i3c_xfers[i].len > 4) {
> 
> Only pre fill fifo when len >4? what happen if only write 1 byte?

That was a really good catch, thank you! I think the code is completely
redundant because we fill the FIFO again at a later time. Then, also
handling the case of < 4 bytes correctly. Sadly, I can't test this with
my current setup right now. I hope to have tested this by next week.

> > +		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.

As said, I will try to discuss this at the I3C plugfest.

> > +	i3c->addrs[pos] = dev->info.dyn_addr ? : dev->info.static_addr;
> 
> Are there extension of "?" operator in C ? I remember

As agreed, I will keep this.

> > +		ret = devm_request_irq(&pdev->dev, ret, renesas_i3c_irqs[i].isr,
> > +				       0, renesas_i3c_irqs[i].desc, i3c);
> > +		if (ret) {
> > +			dev_err(&pdev->dev, "failed to request irq %s\n",
> > +				renesas_i3c_irqs[i].desc);
> > +			return ret;

I removed the printout comepletely. No need to do it for irqs.

I plan to resubmit the non-RFC version next week after the plugfest.
Fingers crossed.

Thanks and all the best,

   Wolfram


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)