Re: [PATCH v5 2/2] iio: adc: adding support for PAC193x

From: Conor Dooley <conor.dooley@microchip.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: <marius.cristea@microchip.com>, <lars@metafoo.de>,
	<robh+dt@kernel.org>, <jdelvare@suse.com>, <linux@roeck-us.net>,
	<linux-hwmon@vger.kernel.org>,
	<krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
	<linux-iio@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 2/2] iio: adc: adding support for PAC193x
Date: Fri, 5 Apr 2024 13:41:44 +0100	[thread overview]
Message-ID: <20240405-embellish-bonnet-ab5f10560d93@wendy> (raw)
In-Reply-To: <20240224191559.40d233db@jic23-huawei>

[-- Attachment #1: Type: text/plain, Size: 2742 bytes --]

On Sat, Feb 24, 2024 at 07:15:59PM +0000, Jonathan Cameron wrote:
> On Thu, 22 Feb 2024 18:42:06 +0200
> <marius.cristea@microchip.com> wrote:
> 
> > From: Marius Cristea <marius.cristea@microchip.com>
> > 
> > This is the iio driver for Microchip
> > PAC193X series of Power Monitor with Accumulator chip family.
> > 
> > Signed-off-by: Marius Cristea <marius.cristea@microchip.com>
> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]