linux-hardening - Re: [PATCH v2 2/2] i3c: master: Add basic driver for the Renesas I3C controller

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: <aHomfzV_dJMwFyCN@shikoro>
Date: Fri, 18 Jul 2025 12:48:31 +0200
From: Wolfram Sang <wsa+renesas@...g-engineering.com>
To: Frank Li <Frank.li@....com>
Cc: linux-renesas-soc@...r.kernel.org,
	Alexandre Belloni <alexandre.belloni@...tlin.com>,
	Tommaso Merciai <tommaso.merciai.xr@...renesas.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 v2 2/2] i3c: master: Add basic driver for the Renesas I3C
 controller

Hi Frank,

> > +#define NDBSTLV0		0x398
> > +#define  NDBSTLV0_RDBLV(x)	(((x) >> 8) & 0xff)
> 
> Can you use FILE_GET()?

You mean FIELD_GET? Probably yes.

> > +#define RENESAS_I3C_MAX_DEVS	8
> > +#define I2C_INIT_MSG		-1
> > +
> > +/* Bus condition timing */
> > +#define I3C_BUS_THIGH_MIXED_NS	40		/* 40ns */
> > +#define I3C_BUS_FREE_TIME_NS	1300		/* 1.3us for Mixed Bus with I2C FM Device*/
> > +#define I3C_BUS_AVAL_TIME_NS	1000		/* 1us */
> > +#define I3C_BUS_IDLE_TIME_NS	200000		/* 200us */
> 
> Do you have document reference to such timeout value?  If it is spec defined
> timeout, please move to master.h and add ref to spec sections number.

They are all in the specs. Will move them.

> > +#define XFER_TIMEOUT		(msecs_to_jiffies(1000))
> 
> Is it engineer choosen timeout or spec defined? add comments to show why
> choose this timeout value.

Consistency. All current I3C controller drivers use this value. If we
want to improve it, we should do it in a seperate series for all drivers
IMO.

> > +	/* Wait for reset completion  */
> > +	return readl_relaxed_poll_timeout(i3c->regs + RSTCTL, val,
> > +					  !(val & RSTCTL_RI3CRST), 0, 1000);
> 
> All you use customer's readl at other place. here, you should use
> read_poll_timeout(renesas_i3c_reg_read, ...) to keep consistent. check other
> place.

I will use regmap_read_poll_timeout().

> > +			pp_high_ticks = DIV_ROUND_UP(I3C_BUS_THIGH_MIXED_NS,
> > +						     1000000000 / rate);
> 
> 100000000 use NSEC_PER_SEC, check other place.

Ack.

> > +	/* Extended Bit Rate setting */
> > +	renesas_i3c_reg_write(i3c->regs, EXTBR, EXTBR_EBRLO(od_low_ticks) |
> > +					   EXTBR_EBRHO(od_high_ticks) |
> > +					   EXTBR_EBRLP(pp_low_ticks) |
> > +					   EXTBR_EBRHP(pp_high_ticks));
> 
> I feel renesas_i3c_reg_write is too long, renesas_write() should be enough.

It is long, but precise. renesas_write() could mean anything. It would
also be confusing if some functions start with renesas_* only and some
with renesas_i3c_*. But this is already too much bike-shedding for my
taste. I will do the extra work and switch to regmap and hope that the
overhead is not noticable. I hope I can squeeze this in today.

> > +static struct renesas_i3c_irq_desc renesas_i3c_irqs[] = {
> 
> const?

Yes.

Kind regards,

   Wolfram