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] [day] [month] [year] [list]

Message-ID: <aHpb6rSpxOv6oV14@lizhi-Precision-Tower-5810>
Date: Fri, 18 Jul 2025 10:36:26 -0400
From: Frank Li <Frank.li@....com>
To: Wolfram Sang <wsa+renesas@...g-engineering.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

On Fri, Jul 18, 2025 at 12:48:31PM +0200, Wolfram Sang wrote:
> 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.

You can put it at your first patch. Then your driver is first user.

we can create new clean up patch for other drivers after your patch merged.

patch1: Add standard TIMEMOUT value in master.h
patch2: binding doc
Patch3: drivers

new series:
cleanup other drivers.

Frank
>
> > > +	/* 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
>