| 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
| ||
|
Message-ID: <aHeStuQ4ymIlyNE4@shikoro>
Date: Wed, 16 Jul 2025 13:53:26 +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 2/2] i3c: add driver for Renesas I3C controller
Hi Frank,
thank you for the review!
> Suggest commit message:
>
> i3c: master: Add basic driver for Renesas RZ/G3S and G3E SoCs
>
> Add a basic I3C master driver for the I3C controller found in Renesas
> RZ/G3S and G3E SoCs. Support I3C pure busses (tested with two targets) and
> mixed busses (two I3C devices plus various I2C targets). DAA and
> communication with temperature sensors worked reliably at various speeds
>
> Missing features such as IBI, HotJoin, and target mode will be added
> incrementally.
Ok, I wil use this message. Thanks for providing it.
> > +F: drivers/i3c/master/renesas-i3c.c
>
> Add i3c mail list?
It is inherited from the I3C subsystem entry.
> > +#define STDBR 0x74
> > +#define STDBR_SBRLO(cond, x) FIELD_PREP(GENMASK(7, 0), (x) >> (cond))
> > +#define STDBR_SBRHO(cond, x) FIELD_PREP(GENMASK(15, 8), (x) >> (cond))
>
> I think pass down od_low_ticks instead of cond will be easy to understand.
>
> STDBR_SBRLO(l, x) FIELD_PREP(GENMASK(7, 0), (x) >> ((l) > 0xFF > 1: 0)
Well, I think the fact that you got it wrong is indicating that it is
not so easy to understand :) It should be:
STDBR_SBRLO(l, x) FIELD_PREP(GENMASK(7, 0), (x) >> ((l) > 0xFF ? 1: 0)
I also think it won't gain us much. We still need the 'double_SBR'
variable to set a specific bit at the same time we use the macro.
Unless you want a dedicated macro for STDBR_DSBRPO as well.
> But still strange, l > 0xFF, you just shift right just 1 bits.
Yes.
> what's happen if l is 0x3ff.
The driver bails out:
+ if ((od_low_ticks / 2) > 0xFF || pp_low_ticks > 0x3F) {
+ dev_err(&m->dev, "invalid speed (i2c-scl = %lu Hz, i3c-scl = %lu Hz). Too slow.\n",
+ (unsigned long)bus->scl_rate.i2c, (unsigned long)bus->scl_rate.i3c);
+ ret = -EINVAL;
+ return ret;
+ }
Oh, the last two lines can be merged into one...
>
> > +#define STDBR_SBRLP(x) FIELD_PREP(GENMASK(21, 16), x)
> > +#define STDBR_SBRHP(x) FIELD_PREP(GENMASK(29, 24), x)
> > +#define STDBR_DSBRPO BIT(31)
> > +
> > +#define EXTBR 0x78
> > +#define EXTBR_EBRLO(x) FIELD_PREP(GENMASK(7, 0), x)
> > +#define EXTBR_EBRHO(x) FIELD_PREP(GENMASK(15, 8), x)
> > +#define EXTBR_EBRLP(x) FIELD_PREP(GENMASK(21, 16), x)
> > +#define EXTBR_EBRHP(x) FIELD_PREP(GENMASK(29, 24), x)
>
> did this pass check_patch.sh? I remember need (x)
Yes, it passed.
> maybe run check_patch.sh --strict
I used --strict. Does it complain on your side?
> > +static void i3c_reg_update_bit(void __iomem *base, u32 reg,
> > + u32 mask, u32 val)
> > +{
> > + i3c_reg_update(base + reg, mask, val);
> > +}
>
> It is similor to regmap. If you use mmios' regmap, needn't above help
> functions.
Is this a suggestion or a requirement? I'd like to keep it this way.
...
> > +static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
> > + struct i2c_msg *i2c_xfers,
> > + int i2c_nxfers)
> > +{
> > + struct i3c_master_controller *m = i2c_dev_get_master(dev);
> > + struct renesas_i3c *i3c = to_renesas_i3c(m);
> > + struct renesas_i3c_xfer *xfer;
> > + struct renesas_i3c_cmd *cmd;
> > + u8 start_bit = CNDCTL_STCND;
> > + int ret, i;
> > +
> > + if (!i2c_nxfers)
> > + return 0;
> > +
> > + renesas_i3c_bus_enable(m, false);
> > +
> > + xfer = renesas_i3c_alloc_xfer(i3c, 1);
> > + if (!xfer)
> > + return -ENOMEM;
> > +
> > + init_completion(&xfer->comp);
> > + xfer->is_i2c_xfer = true;
> > + cmd = xfer->cmds;
> > +
> > + if (!(i3c_reg_read(i3c->regs, BCST) & BCST_BFREF)) {
> > + cmd->err = -EBUSY;
> > + goto out;
> > + }
> > +
> > + i3c_reg_write(i3c->regs, BST, 0);
> > +
> > + renesas_i3c_enqueue_xfer(i3c, xfer);
>
> You can use refer mutex GUARD define to pair renesas_i3c_enqueue_xfer()
> and renesas_i3c_dequeue_xfer().
Well, looking again, I won't need this. There is no 'goto out' after
enqueueing. So, the label is wrongly placed. Might be an argument to
remove it...
>
> > +
> > + for (i = 0; i < i2c_nxfers; i++) {
> > + cmd->i2c_bytes_left = I2C_INIT_MSG;
> > + cmd->i2c_buf = i2c_xfers[i].buf;
> > + cmd->msg = &i2c_xfers[i];
> > + cmd->i2c_is_last = (i == i2c_nxfers - 1);
> > +
> > + i3c_reg_set_bit(i3c->regs, BIE, BIE_NACKDIE);
> > + i3c_reg_set_bit(i3c->regs, NTIE, NTIE_TDBEIE0);
> > + i3c_reg_set_bit(i3c->regs, BIE, BIE_STCNDDIE);
> > +
> > + /* Issue Start condition */
> > + i3c_reg_set_bit(i3c->regs, CNDCTL, start_bit);
> > +
> > + i3c_reg_set_bit(i3c->regs, NTSTE, NTSTE_TDBEE0);
> > +
> > + wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
> > +
> > + if (cmd->err)
> > + break;
> > +
> > + start_bit = CNDCTL_SRCND;
> > + }
> > +out:
> > + renesas_i3c_dequeue_xfer(i3c, xfer);
> > + ret = cmd->err;
> > + kfree(xfer);
>
> struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
... by doing this.
> > + /* Clear the Transmit Buffer Empty status flag. */
> > + i3c_reg_clear_bit(i3c->regs, BST, BST_TENDF);
>
> Are you sure you can clear FIFO empty status? Generally it is read only.
Yes. The datasheet says so. If you want to double check, page 1715 of
the document which link I provided last time.
Thanks for the input, I will work on this now...
Wolfram
Powered by blists - more mailing lists