On Sat, Feb 24, 2024 at 07:15:59PM +0000, Jonathan Cameron wrote: > On Thu, 22 Feb 2024 18:42:06 +0200 > <marius.cristea@xxxxxxxxxxxxx> wrote: > > > From: Marius Cristea <marius.cristea@xxxxxxxxxxxxx> > > > > This is the iio driver for Microchip > > PAC193X series of Power Monitor with Accumulator chip family. > > > > Signed-off-by: Marius Cristea <marius.cristea@xxxxxxxxxxxxx> > So I had a few comments on this, but nothing that can't be cleaned up later. > + I'll fix the thing the bots didn't like on the bindings. > > Series applied to the togreg branch of iio.git and pushed out > as testing for 0-day to take a look at it. I tested this out on v6.9-rc2 and prompted a backtrace when collectd started running: ------------[ cut here ]------------ UBSAN: array-index-out-of-bounds in /home/conor/stuff/linux/drivers/iio/adc/pac1934.c:857:25 index 7 is out of range for type 'u32 [4]' CPU: 1 PID: 179 Comm: iiod Not tainted 6.9.0-rc2-dirty #1 Hardware name: Microchip PolarFire-SoC Icicle Kit (DT) Call Trace: [<ffffffff80006bba>] dump_backtrace+0x28/0x30 [<ffffffff80bd67d8>] show_stack+0x38/0x44 [<ffffffff80be7820>] dump_stack_lvl+0x6e/0x9a [<ffffffff80be7864>] dump_stack+0x18/0x20 [<ffffffff80be1452>] ubsan_epilogue+0x10/0x46 [<ffffffff80615358>] __ubsan_handle_out_of_bounds+0x6a/0x78 [<ffffffff80981f3a>] pac1934_read_raw+0x20c/0x34c [<ffffffff80977c4c>] iio_read_channel_info+0x5c/0xbe [<ffffffff8073516e>] dev_attr_show+0x1c/0x4a [<ffffffff80387292>] sysfs_kf_seq_show+0x80/0xcc [<ffffffff80385b12>] kernfs_seq_show+0x3c/0x4a [<ffffffff8031e3d8>] seq_read_iter+0x136/0x2e4 [<ffffffff80385cde>] kernfs_fop_read_iter+0x38/0x16a [<ffffffff802e904a>] vfs_read+0x1be/0x2ba [<ffffffff802e9c48>] ksys_read+0x64/0xd2 [<ffffffff802e9cd6>] __riscv_sys_read+0x20/0x28 [<ffffffff80be838a>] do_trap_ecall_u+0xee/0x204 [<ffffffff80bf57d0>] ret_from_exception+0x0/0x64 ---[ end trace ]--- The device itself only has 4 channels, but in sysfs there are "fake" channels for the average voltages and currents too. UBSAN points at: case PAC1934_VSENSE_AVG_4_ADDR: *val = PAC1934_MAX_VSENSE_RSHIFTED_BY_16B; if (chan->scan_type.sign == 'u') *val2 = info->shunts[channel]; else *val2 = info->shunts[channel] >> 1; return IIO_VAL_FRACTIONAL; And info->shunts is only valid for the 4 real channels, so I guess the averaged channels probably need to do a [channel - 4] or similar. I dunno if that relation between averaged channels and their "real" counterparts is fixed or if different pac devices need different values, but on my system here that would work. I do quite like that UBSAN points out the line in question :) Cheers, Conor.
Attachment:
signature.asc
Description: PGP signature
