Re: [PATCH v3] clk: rs9: Fix I2C accessors

Alexander Stein
2022-09-29T07:17:26Z

Hi Marek,

Am Mittwoch, 28. September 2022, 14:59:07 CEST schrieb Marek Vasut:
> On 9/28/22 09:35, Alexander Stein wrote:
> > Hi Marek,
>
> Hi,
>
> > thanks for the update.
>
> Thanks for the reviews.
>
> > Am Dienstag, 27. September 2022, 23:44:14 CEST schrieb Marek Vasut:
> >> Add custom I2C accessors to this driver, since the regular I2C regmap
> >> ones
> >> do not generate the exact I2C transfers required by the chip. On I2C
> >> write,
> >> it is mandatory to send transfer length first, on read the chip returns
> >> the
> >> transfer length in first byte. Instead of always reading back 8 bytes,
> >> which is the default and also the size of the entire register file, set
> >> BCP register to 1 to read out 1 byte which is less wasteful.
> >>
> >> Fixes: 892e0ddea1aa6 ("clk: rs9: Add Renesas 9-series PCIe clock
> >> generator
> >> driver") Reported-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> ---
> >> V2: Fix endianness handling in rs9_regmap_i2c_read() i2c_transfer
> >> V3: - Disable regcache, the driver does a couple of I2C writes on boot
> >>
> >> and that is all, so it only adds complexity
> >>
> >> - Set regmap max_register to RS9_REG_BCP which is the correct one
> >>
> >> ---
> >> Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
> >> Cc: Michael Turquette <mturquette@baylibre.com>
> >> Cc: Stephen Boyd <sboyd@kernel.org>
> >> ---
> >>
> >> drivers/clk/clk-renesas-pcie.c | 60 ++++++++++++++++++++++++++++++++--
> >> 1 file changed, 57 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/clk/clk-renesas-pcie.c
> >> b/drivers/clk/clk-renesas-pcie.c index 4f5df1fc74b46..138aaab05fc8a
> >> 100644
> >> --- a/drivers/clk/clk-renesas-pcie.c
> >> +++ b/drivers/clk/clk-renesas-pcie.c
> >> @@ -90,13 +90,61 @@ static const struct regmap_access_table
> >> rs9_writeable_table = { .n_yes_ranges = ARRAY_SIZE(rs9_writeable_ranges),
> >>
> >> };
> >>
> >> +static int rs9_regmap_i2c_write(void *context,
> >> + unsigned int reg, unsigned int
> >
> > val)
> >
> >> +{
> >> + struct i2c_client *i2c = context;
> >> + const u8 data[3] = { reg, 1, val };
> >> + const int count = ARRAY_SIZE(data);
> >> + int ret;
> >> +
> >> + ret = i2c_master_send(i2c, data, count);
> >> + if (ret == count)
> >> + return 0;
> >> + else if (ret < 0)
> >> + return ret;
> >> + else
> >> + return -EIO;
> >> +}
> >> +
> >> +static int rs9_regmap_i2c_read(void *context,
> >> + unsigned int reg, unsigned int *val)
> >> +{
> >> + struct i2c_client *i2c = context;
> >> + struct i2c_msg xfer[2];
> >> + u8 txdata = reg;
> >> + u8 rxdata[2];
> >> + int ret;
> >> +
> >> + xfer[0].addr = i2c->addr;
> >> + xfer[0].flags = 0;
> >> + xfer[0].len = 1;
> >> + xfer[0].buf = (void *)&txdata;
> >> +
> >> + xfer[1].addr = i2c->addr;
> >> + xfer[1].flags = I2C_M_RD | I2C_M_RECV_LEN;
> >> + xfer[1].len = 1;
> >
> > I'm still in favor of removing I2C_M_RECV_LEN and setting len=2. This
> > currently works only, because there is no read access between
> > devm_regmap_init() call and regmap_write() to RS9_REG_BCP.
> > Enabling cache later on again this will corrupt the stack.
>
> The device does send you the amount of data in first byte of I2C READ,
> so I think I2C_M_RECV_LEN is the right flag here.

But you'll need to take I2C_M_RECV_LEN into account, and store at least
I2C_SMBUS_BLOCK_MAX + length byte in the reception buffer, see [1].
Otherwise bad things can happen, see below.

> I had a look into the above topic too before sending V3, and I think you
> can use regcache_cache_bypass(..., true) right after devm_regmap_init()
> and then again regcache_cache_bypass(..., false) close to the end of
> probe() callback to deal with the problem you're describing.
>
> Would that work ?

Unfortunately no, if cache is enabled and cache size is set as well, you'll
read from hardware from within devm_regmap_init call, no way to enable cache
bypassing (yet).

--8<--
diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
index 4e627855583f..99e5fd867efc 100644
--- a/drivers/clk/clk-renesas-pcie.c
+++ b/drivers/clk/clk-renesas-pcie.c
@@ -139,8 +139,9 @@ static int rs9_regmap_i2c_read(void *context,
static const struct regmap_config rs9_regmap_config = {
.reg_bits = 8,
.val_bits = 8,
- .cache_type = REGCACHE_NONE,
+ .cache_type = REGCACHE_FLAT,
.max_register = RS9_REG_BCP,
+ .num_reg_defaults_raw = 0x8,
.rd_table = &rs9_readable_table,
.wr_table = &rs9_writeable_table,
.reg_write = rs9_regmap_i2c_write,
--8<--

This results in the following kernel panic:

clk-renesas-pcie-9series 1-0068: No cache defaults, reading back from HW
Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in:
rs9_regmap_i2c_read+0xb4/0xb4 [clk_renesas_pcie]
CPU: 1 PID: 278 Comm: systemd-udevd Not tainted 6.0.0-rc7-next-20220927+ #816
5af64c0444274ab7984baf7c2d7f82d1e1bca080
Hardware name: TQ-Systems GmbH i.MX8MQ TQMa8MQ on MBa8Mx (DT)
Call trace:
dump_backtrace+0xd4/0x130
show_stack+0x14/0x40
dump_stack_lvl+0x64/0x7c
dump_stack+0x14/0x2c
panic+0x19c/0x364
__stack_chk_fail+0x24/0x30
rs9_get_common_config+0x0/0x19c [clk_renesas_pcie
0f42365202f6346c1b8a6cb8867a1ff0d4899aee]
_regmap_read+0x74/0x160
regmap_read+0x48/0x70
regcache_hw_init+0x184/0x2d0
regcache_init+0x1d4/0x2c0
__regmap_init+0x7dc/0xf30
__devm_regmap_init+0x74/0xc0
rs9_probe+0x110/0x298 [clk_renesas_pcie
0f42365202f6346c1b8a6cb8867a1ff0d4899aee]
i2c_device_probe+0x100/0x340
call_driver_probe+0x28/0x140
really_probe+0xc0/0x334
__driver_probe_device+0x84/0x144
driver_probe_device+0x38/0x150
__driver_attach+0xac/0x244
bus_for_each_dev+0x6c/0xc0
driver_attach+0x20/0x30
bus_add_driver+0x174/0x244
driver_register+0x74/0x120
i2c_register_driver+0x50/0xf0
rs9_driver_init+0x20/0x1000 [clk_renesas_pcie
0f42365202f6346c1b8a6cb8867a1ff0d4899aee]
do_one_initcall+0x58/0x200
do_init_module+0x40/0x1d4
load_module+0x634/0x6e4
__do_sys_finit_module+0xc0/0x140
__arm64_sys_finit_module+0x1c/0x24
invoke_syscall+0x6c/0xf0
el0_svc_common.constprop.0+0xc0/0xe0
do_el0_svc+0x24/0x30
el0_svc+0x1c/0x50
el0t_64_sync_handler+0xb0/0xb4
el0t_64_sync+0x148/0x14c
SMP: stopping secondary CPUs
Kernel Offset: disabled
CPU features: 0x00000,00800084,0000420b
Memory Limit: none
---[ end Kernel panic - not syncing: stack-protector: Kernel stack is
corrupted in: rs9_regmap_i2c_read+0xb4/0xb4 [clk_renesas_pcie] ]---

IMHO, I2C_M_RECV_LEN makes only sense for regmap's (raw) 'read' callback which
supports reading multiple registers.
But for a single register read, there is no need to take length into account,
just read 2 bytes (length + data).

Best regards,
Alexander

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/
include/uapi/linux/i2c.h#n45