Re: [PATCH v6 4/4] iio: adc: qcom-pm8xxx-xoadc: add support for reading channel labels

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The new `.read_label` callback and pre-existing `.read_raw` fail to uniquely identify channels, causing incorrect labels and voltage readings.
- [High] Race condition in probe exposes uncalibrated device, risking divide-by-zero kernel panic.
--

commit 50e7569708562dacb984c644e8571f2c7c6624cb
Author: Antony Kurniawan Soemardi <linux@smankusors.com>

iio: adc: qcom-pm8xxx-xoadc: add support for reading channel labels

This commit implements the .read_label callback to allow userspace to
identify ADC channels via the device tree label property. The name field
in pm8xxx_chan_info is renamed to label, defaulting to the datasheet name.

> diff --git a/drivers/iio/adc/qcom-pm8xxx-xoadc.c b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
> index 282a67b46a5eb..4a1a0cfb4699d 100644
> --- a/drivers/iio/adc/qcom-pm8xxx-xoadc.c
> +++ b/drivers/iio/adc/qcom-pm8xxx-xoadc.c

[ ... ]

> @@ -716,8 +716,21 @@ static int pm8xxx_fwnode_xlate(struct iio_dev *indio_dev,
>  	return -EINVAL;
>  }
>  
> +static int pm8xxx_read_label(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, char *label)
> +{
> +	struct pm8xxx_xoadc *adc = iio_priv(indio_dev);
> +	const struct pm8xxx_chan_info *ch;
> +
> +	ch = pm8xxx_get_channel(adc, chan->address);

Is amux_channel guaranteed to be a unique identifier?

Looking at the hardware definition array, multiple channels (like vcoin and
atest_8) use amux_channel = 0x00 and differ only in their pre_scale_mux
values.

Since pm8xxx_get_channel() only checks chan->address against
ch->hwchan->amux_channel:

pm8xxx_get_channel() {
    ...
    for (i = 0; i < adc->nchans; i++) {
        struct pm8xxx_chan_info *ch = &adc->chans[i];
        if (ch->hwchan->amux_channel == chan)
            return ch;
    }
    ...
}

Could this return the first matching channel instead of the correct one if
multiple overlapping channels are configured, causing this new callback to
return incorrect labels?

This also appears to be a pre-existing issue in pm8xxx_read_raw() which
might configure the multiplexer incorrectly and read the wrong hardware
voltages.

Since adc->chans and adc->iio_chans are allocated together, could the
correct channel be resolved using pointer arithmetic, such as
&adc->chans[chan - adc->iio_chans]?

> +	if (!ch)
> +		return -EINVAL;
> +	return sysfs_emit(label, "%s\n", ch->label);
> +}
> +
>  static const struct iio_info pm8xxx_xoadc_info = {

While reviewing this driver, I also noticed a pre-existing race condition in
pm8xxx_xoadc_probe().

Does calling iio_device_register() before pm8xxx_calibrate_device() expose
the IIO sysfs attributes to userspace too early?

If userspace reads a processed channel value concurrently, pm8xxx_read_raw()
will invoke qcom_vadc_scale().

Since adc->graph is allocated via devm_iio_device_alloc() and hasn't been
populated by the calibration routine yet, calib_graph->dy will be 0.

Could this result in a divide-by-zero panic in qcom_vadc_scale_calib()
when computing div64_s64(*scale_voltage, calib_graph->dy)?

Should iio_device_register() be moved to the end of the probe function,
after all hardware initialization and calibration are complete?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510-pm8xxx-xoadc-label-v6-0-49700fd03005@smankusors.com?part=4