2018-12-04 19:16:18

by Evan Green

Subject: [PATCH] iio: adc: qcom-spmi-adc5: Initialize prescale properly

adc5_get_dt_data uses a local, prop, feeds it to adc5_get_dt_channel_data,
and then puts the result into adc->chan_props. The problem is
adc5_get_dt_channel_data may not initialize that structure fully, so a
garbage value is used for prescale if the optional "qcom,pre-scaling" is
not defined in DT. adc5_read_raw then uses this as an array index,
generating a crash that looks like this:

[ 6.683186] Unable to handle kernel paging request at virtual address ffffff90e78c7964
Call trace:
qcom_vadc_scale_code_voltage_factor+0x74/0x104
qcom_vadc_scale_hw_calib_die_temp+0x20/0x60
qcom_adc5_hw_scale+0x78/0xa4
adc5_read_raw+0x3d0/0x65c
iio_channel_read+0x240/0x30c
iio_read_channel_processed+0x10c/0x150
qpnp_tm_get_temp+0xc0/0x40c
of_thermal_get_temp+0x7c/0x98
thermal_zone_get_temp+0xac/0xd8
thermal_zone_device_update+0xc0/0x38c
qpnp_tm_probe+0x624/0x81c
platform_drv_probe+0xe4/0x11c
really_probe+0x188/0x3fc
driver_probe_device+0xb8/0x188
__device_attach_driver+0x114/0x180
bus_for_each_drv+0xd8/0x118
__device_attach+0x180/0x27c
device_initial_probe+0x20/0x2c
bus_probe_device+0x78/0x124
deferred_probe_work_func+0xfc/0x138
process_one_work+0x3d8/0x8b0
process_scheduled_works+0x48/0x6c
worker_thread+0x488/0x7cc
kthread+0x24c/0x264
ret_from_fork+0x10/0x18

Unfortunately, when I went to add the initializer for this and tried to
boot it, my machine shut down immediately, complaining that it was
hotter than the sun. It appears that adc5_chans_pmic and adc5_chans_rev2
were initializing prescale_index as if it were directly a divisor,
rather than the index into adc5_prescale_ratios that it is.

Fix the uninitialized value, and change the static initialization to use
indices into adc5_prescale_ratios.

Signed-off-by: Evan Green <[email protected]>
---

drivers/iio/adc/qcom-spmi-adc5.c | 58 +++++++++++++++++---------------
1 file changed, 31 insertions(+), 27 deletions(-)

diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c
index f9af6b082916b..6a866cc187f7d 100644
--- a/drivers/iio/adc/qcom-spmi-adc5.c
+++ b/drivers/iio/adc/qcom-spmi-adc5.c
@@ -423,6 +423,7 @@ struct adc5_channels {
enum vadc_scale_fn_type scale_fn_type;
};

+/* In these definitions, _pre refers to an index into adc5_prescale_ratios. */
#define ADC5_CHAN(_dname, _type, _mask, _pre, _scale) \
{ \
.datasheet_name = _dname, \
@@ -443,63 +444,63 @@ struct adc5_channels {
_pre, _scale) \

static const struct adc5_channels adc5_chans_pmic[ADC5_MAX_CHANNEL] = {
- [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 1,
+ [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 0,
SCALE_HW_CALIB_DEFAULT)
- [ADC5_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 1,
+ [ADC5_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 0,
SCALE_HW_CALIB_DEFAULT)
- [ADC5_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 3,
+ [ADC5_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 1,
SCALE_HW_CALIB_DEFAULT)
- [ADC5_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 3,
+ [ADC5_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 1,
SCALE_HW_CALIB_DEFAULT)
- [ADC5_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 1,
+ [ADC5_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 0,
SCALE_HW_CALIB_PMIC_THERM)
- [ADC5_USB_IN_I] = ADC5_CHAN_VOLT("usb_in_i_uv", 1,
+ [ADC5_USB_IN_I] = ADC5_CHAN_VOLT("usb_in_i_uv", 0,
SCALE_HW_CALIB_DEFAULT)
- [ADC5_USB_IN_V_16] = ADC5_CHAN_VOLT("usb_in_v_div_16", 16,
+ [ADC5_USB_IN_V_16] = ADC5_CHAN_VOLT("usb_in_v_div_16", 8,
SCALE_HW_CALIB_DEFAULT)
- [ADC5_CHG_TEMP] = ADC5_CHAN_TEMP("chg_temp", 1,
+ [ADC5_CHG_TEMP] = ADC5_CHAN_TEMP("chg_temp", 0,
SCALE_HW_CALIB_PM5_CHG_TEMP)
/* Charger prescales SBUx and MID_CHG to fit within 1.8V upper unit */
- [ADC5_SBUx] = ADC5_CHAN_VOLT("chg_sbux", 3,
+ [ADC5_SBUx] = ADC5_CHAN_VOLT("chg_sbux", 1,
SCALE_HW_CALIB_DEFAULT)
- [ADC5_MID_CHG_DIV6] = ADC5_CHAN_VOLT("chg_mid_chg", 6,
+ [ADC5_MID_CHG_DIV6] = ADC5_CHAN_VOLT("chg_mid_chg", 3,
SCALE_HW_CALIB_DEFAULT)
- [ADC5_XO_THERM_100K_PU] = ADC5_CHAN_TEMP("xo_therm", 1,
+ [ADC5_XO_THERM_100K_PU] = ADC5_CHAN_TEMP("xo_therm", 0,
SCALE_HW_CALIB_XOTHERM)
- [ADC5_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_100k_pu", 1,
+ [ADC5_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_100k_pu", 0,
SCALE_HW_CALIB_THERM_100K_PULLUP)
- [ADC5_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_100k_pu", 1,
+ [ADC5_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_100k_pu", 0,
SCALE_HW_CALIB_THERM_100K_PULLUP)
- [ADC5_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_100k_pu", 1,
+ [ADC5_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_100k_pu", 0,
SCALE_HW_CALIB_THERM_100K_PULLUP)
- [ADC5_AMUX_THM2] = ADC5_CHAN_TEMP("amux_thm2", 1,
+ [ADC5_AMUX_THM2] = ADC5_CHAN_TEMP("amux_thm2", 0,
SCALE_HW_CALIB_PM5_SMB_TEMP)
};

static const struct adc5_channels adc5_chans_rev2[ADC5_MAX_CHANNEL] = {
- [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 1,
+ [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 0,
SCALE_HW_CALIB_DEFAULT)
- [ADC5_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 1,
+ [ADC5_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 0,
SCALE_HW_CALIB_DEFAULT)
- [ADC5_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 3,
+ [ADC5_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 1,
SCALE_HW_CALIB_DEFAULT)
- [ADC5_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 3,
+ [ADC5_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 1,
SCALE_HW_CALIB_DEFAULT)
- [ADC5_VCOIN] = ADC5_CHAN_VOLT("vcoin", 3,
+ [ADC5_VCOIN] = ADC5_CHAN_VOLT("vcoin", 1,
SCALE_HW_CALIB_DEFAULT)
- [ADC5_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 1,
+ [ADC5_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 0,
SCALE_HW_CALIB_PMIC_THERM)
- [ADC5_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_100k_pu", 1,
+ [ADC5_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_100k_pu", 0,
SCALE_HW_CALIB_THERM_100K_PULLUP)
- [ADC5_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_100k_pu", 1,
+ [ADC5_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_100k_pu", 0,
SCALE_HW_CALIB_THERM_100K_PULLUP)
- [ADC5_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_100k_pu", 1,
+ [ADC5_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_100k_pu", 0,
SCALE_HW_CALIB_THERM_100K_PULLUP)
- [ADC5_AMUX_THM4_100K_PU] = ADC5_CHAN_TEMP("amux_thm4_100k_pu", 1,
+ [ADC5_AMUX_THM4_100K_PU] = ADC5_CHAN_TEMP("amux_thm4_100k_pu", 0,
SCALE_HW_CALIB_THERM_100K_PULLUP)
- [ADC5_AMUX_THM5_100K_PU] = ADC5_CHAN_TEMP("amux_thm5_100k_pu", 1,
+ [ADC5_AMUX_THM5_100K_PU] = ADC5_CHAN_TEMP("amux_thm5_100k_pu", 0,
SCALE_HW_CALIB_THERM_100K_PULLUP)
- [ADC5_XO_THERM_100K_PU] = ADC5_CHAN_TEMP("xo_therm_100k_pu", 1,
+ [ADC5_XO_THERM_100K_PU] = ADC5_CHAN_TEMP("xo_therm_100k_pu", 0,
SCALE_HW_CALIB_THERM_100K_PULLUP)
};

@@ -558,6 +559,9 @@ static int adc5_get_dt_channel_data(struct adc5_chip *adc,
return ret;
}
prop->prescale = ret;
+ } else {
+ prop->prescale =
+ adc->data->adc_chans[prop->channel].prescale_index;
}

ret = of_property_read_u32(node, "qcom,hw-settle-time", &value);
--
2.18.1

2018-12-04 19:58:36

by Matthias Kaehlcke

Subject: Re: [PATCH] iio: adc: qcom-spmi-adc5: Initialize prescale properly

On Tue, Dec 04, 2018 at 11:14:19AM -0800, Evan Green wrote:
> adc5_get_dt_data uses a local, prop, feeds it to adc5_get_dt_channel_data,
> and then puts the result into adc->chan_props. The problem is
> adc5_get_dt_channel_data may not initialize that structure fully, so a
> garbage value is used for prescale if the optional "qcom,pre-scaling" is
> not defined in DT. adc5_read_raw then uses this as an array index,
> generating a crash that looks like this:
>
> [ 6.683186] Unable to handle kernel paging request at virtual address ffffff90e78c7964
> Call trace:
> qcom_vadc_scale_code_voltage_factor+0x74/0x104
> qcom_vadc_scale_hw_calib_die_temp+0x20/0x60
> qcom_adc5_hw_scale+0x78/0xa4
> adc5_read_raw+0x3d0/0x65c
> iio_channel_read+0x240/0x30c
> iio_read_channel_processed+0x10c/0x150
> qpnp_tm_get_temp+0xc0/0x40c
> of_thermal_get_temp+0x7c/0x98
> thermal_zone_get_temp+0xac/0xd8
> thermal_zone_device_update+0xc0/0x38c
> qpnp_tm_probe+0x624/0x81c
> platform_drv_probe+0xe4/0x11c
> really_probe+0x188/0x3fc
> driver_probe_device+0xb8/0x188
> __device_attach_driver+0x114/0x180
> bus_for_each_drv+0xd8/0x118
> __device_attach+0x180/0x27c
> device_initial_probe+0x20/0x2c
> bus_probe_device+0x78/0x124
> deferred_probe_work_func+0xfc/0x138
> process_one_work+0x3d8/0x8b0
> process_scheduled_works+0x48/0x6c
> worker_thread+0x488/0x7cc
> kthread+0x24c/0x264
> ret_from_fork+0x10/0x18
>
> Unfortunately, when I went to add the initializer for this and tried to
> boot it, my machine shut down immediately, complaining that it was
> hotter than the sun. It appears that adc5_chans_pmic and adc5_chans_rev2
> were initializing prescale_index as if it were directly a divisor,
> rather than the index into adc5_prescale_ratios that it is.
>
> Fix the uninitialized value, and change the static initialization to use
> indices into adc5_prescale_ratios.
>
> Signed-off-by: Evan Green <[email protected]>
> ---
>
> drivers/iio/adc/qcom-spmi-adc5.c | 58 +++++++++++++++++---------------
> 1 file changed, 31 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c
> index f9af6b082916b..6a866cc187f7d 100644
> --- a/drivers/iio/adc/qcom-spmi-adc5.c
> +++ b/drivers/iio/adc/qcom-spmi-adc5.c
> @@ -423,6 +423,7 @@ struct adc5_channels {
> enum vadc_scale_fn_type scale_fn_type;
> };
>
> +/* In these definitions, _pre refers to an index into adc5_prescale_ratios. */
> #define ADC5_CHAN(_dname, _type, _mask, _pre, _scale) \
> { \
> .datasheet_name = _dname, \
> @@ -443,63 +444,63 @@ struct adc5_channels {
> _pre, _scale) \
>
> static const struct adc5_channels adc5_chans_pmic[ADC5_MAX_CHANNEL] = {
> - [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 1,
> + [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 0,
> SCALE_HW_CALIB_DEFAULT)
> - [ADC5_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 1,
> + [ADC5_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 0,
> SCALE_HW_CALIB_DEFAULT)
> - [ADC5_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 3,
> + [ADC5_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 1,
> SCALE_HW_CALIB_DEFAULT)
> - [ADC5_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 3,
> + [ADC5_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 1,
> SCALE_HW_CALIB_DEFAULT)
> - [ADC5_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 1,
> + [ADC5_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 0,
> SCALE_HW_CALIB_PMIC_THERM)
> - [ADC5_USB_IN_I] = ADC5_CHAN_VOLT("usb_in_i_uv", 1,
> + [ADC5_USB_IN_I] = ADC5_CHAN_VOLT("usb_in_i_uv", 0,
> SCALE_HW_CALIB_DEFAULT)
> - [ADC5_USB_IN_V_16] = ADC5_CHAN_VOLT("usb_in_v_div_16", 16,
> + [ADC5_USB_IN_V_16] = ADC5_CHAN_VOLT("usb_in_v_div_16", 8,
> SCALE_HW_CALIB_DEFAULT)
> - [ADC5_CHG_TEMP] = ADC5_CHAN_TEMP("chg_temp", 1,
> + [ADC5_CHG_TEMP] = ADC5_CHAN_TEMP("chg_temp", 0,
> SCALE_HW_CALIB_PM5_CHG_TEMP)
> /* Charger prescales SBUx and MID_CHG to fit within 1.8V upper unit */
> - [ADC5_SBUx] = ADC5_CHAN_VOLT("chg_sbux", 3,
> + [ADC5_SBUx] = ADC5_CHAN_VOLT("chg_sbux", 1,
> SCALE_HW_CALIB_DEFAULT)
> - [ADC5_MID_CHG_DIV6] = ADC5_CHAN_VOLT("chg_mid_chg", 6,
> + [ADC5_MID_CHG_DIV6] = ADC5_CHAN_VOLT("chg_mid_chg", 3,
> SCALE_HW_CALIB_DEFAULT)
> - [ADC5_XO_THERM_100K_PU] = ADC5_CHAN_TEMP("xo_therm", 1,
> + [ADC5_XO_THERM_100K_PU] = ADC5_CHAN_TEMP("xo_therm", 0,
> SCALE_HW_CALIB_XOTHERM)
> - [ADC5_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_100k_pu", 1,
> + [ADC5_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_100k_pu", 0,
> SCALE_HW_CALIB_THERM_100K_PULLUP)
> - [ADC5_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_100k_pu", 1,
> + [ADC5_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_100k_pu", 0,
> SCALE_HW_CALIB_THERM_100K_PULLUP)
> - [ADC5_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_100k_pu", 1,
> + [ADC5_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_100k_pu", 0,
> SCALE_HW_CALIB_THERM_100K_PULLUP)
> - [ADC5_AMUX_THM2] = ADC5_CHAN_TEMP("amux_thm2", 1,
> + [ADC5_AMUX_THM2] = ADC5_CHAN_TEMP("amux_thm2", 0,
> SCALE_HW_CALIB_PM5_SMB_TEMP)
> };
>
> static const struct adc5_channels adc5_chans_rev2[ADC5_MAX_CHANNEL] = {
> - [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 1,
> + [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 0,
> SCALE_HW_CALIB_DEFAULT)
> - [ADC5_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 1,
> + [ADC5_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 0,
> SCALE_HW_CALIB_DEFAULT)
> - [ADC5_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 3,
> + [ADC5_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 1,
> SCALE_HW_CALIB_DEFAULT)
> - [ADC5_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 3,
> + [ADC5_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 1,
> SCALE_HW_CALIB_DEFAULT)
> - [ADC5_VCOIN] = ADC5_CHAN_VOLT("vcoin", 3,
> + [ADC5_VCOIN] = ADC5_CHAN_VOLT("vcoin", 1,
> SCALE_HW_CALIB_DEFAULT)
> - [ADC5_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 1,
> + [ADC5_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 0,
> SCALE_HW_CALIB_PMIC_THERM)
> - [ADC5_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_100k_pu", 1,
> + [ADC5_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_100k_pu", 0,
> SCALE_HW_CALIB_THERM_100K_PULLUP)
> - [ADC5_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_100k_pu", 1,
> + [ADC5_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_100k_pu", 0,
> SCALE_HW_CALIB_THERM_100K_PULLUP)
> - [ADC5_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_100k_pu", 1,
> + [ADC5_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_100k_pu", 0,
> SCALE_HW_CALIB_THERM_100K_PULLUP)
> - [ADC5_AMUX_THM4_100K_PU] = ADC5_CHAN_TEMP("amux_thm4_100k_pu", 1,
> + [ADC5_AMUX_THM4_100K_PU] = ADC5_CHAN_TEMP("amux_thm4_100k_pu", 0,
> SCALE_HW_CALIB_THERM_100K_PULLUP)
> - [ADC5_AMUX_THM5_100K_PU] = ADC5_CHAN_TEMP("amux_thm5_100k_pu", 1,
> + [ADC5_AMUX_THM5_100K_PU] = ADC5_CHAN_TEMP("amux_thm5_100k_pu", 0,
> SCALE_HW_CALIB_THERM_100K_PULLUP)
> - [ADC5_XO_THERM_100K_PU] = ADC5_CHAN_TEMP("xo_therm_100k_pu", 1,
> + [ADC5_XO_THERM_100K_PU] = ADC5_CHAN_TEMP("xo_therm_100k_pu", 0,
> SCALE_HW_CALIB_THERM_100K_PULLUP)
> };
>
> @@ -558,6 +559,9 @@ static int adc5_get_dt_channel_data(struct adc5_chip *adc,
> return ret;
> }
> prop->prescale = ret;
> + } else {
> + prop->prescale =
> + adc->data->adc_chans[prop->channel].prescale_index;
> }
>
> ret = of_property_read_u32(node, "qcom,hw-settle-time", &value);

Reviewed-by: Matthias Kaehlcke <[email protected]>

Good as an immediate fix.

In the long term I wonder if it would be better to pass a ratio to
ADC5_CHAN_XYZ and store it in struct adc5_channel_prop, instead of
specifying the index, which is more error prone and errors are harder
to spot. With this adc5_prescale_ratios would still exist, but only be
used for the sanity check of prescaling values from the DT.

2018-12-08 12:07:06

by Jonathan Cameron

Subject: Re: [PATCH] iio: adc: qcom-spmi-adc5: Initialize prescale properly

On Tue, 4 Dec 2018 11:57:32 -0800
Matthias Kaehlcke <[email protected]> wrote:

> On Tue, Dec 04, 2018 at 11:14:19AM -0800, Evan Green wrote:
> > adc5_get_dt_data uses a local, prop, feeds it to adc5_get_dt_channel_data,
> > and then puts the result into adc->chan_props. The problem is
> > adc5_get_dt_channel_data may not initialize that structure fully, so a
> > garbage value is used for prescale if the optional "qcom,pre-scaling" is
> > not defined in DT. adc5_read_raw then uses this as an array index,
> > generating a crash that looks like this:
> >
> > [ 6.683186] Unable to handle kernel paging request at virtual address ffffff90e78c7964
> > Call trace:
> > qcom_vadc_scale_code_voltage_factor+0x74/0x104
> > qcom_vadc_scale_hw_calib_die_temp+0x20/0x60
> > qcom_adc5_hw_scale+0x78/0xa4
> > adc5_read_raw+0x3d0/0x65c
> > iio_channel_read+0x240/0x30c
> > iio_read_channel_processed+0x10c/0x150
> > qpnp_tm_get_temp+0xc0/0x40c
> > of_thermal_get_temp+0x7c/0x98
> > thermal_zone_get_temp+0xac/0xd8
> > thermal_zone_device_update+0xc0/0x38c
> > qpnp_tm_probe+0x624/0x81c
> > platform_drv_probe+0xe4/0x11c
> > really_probe+0x188/0x3fc
> > driver_probe_device+0xb8/0x188
> > __device_attach_driver+0x114/0x180
> > bus_for_each_drv+0xd8/0x118
> > __device_attach+0x180/0x27c
> > device_initial_probe+0x20/0x2c
> > bus_probe_device+0x78/0x124
> > deferred_probe_work_func+0xfc/0x138
> > process_one_work+0x3d8/0x8b0
> > process_scheduled_works+0x48/0x6c
> > worker_thread+0x488/0x7cc
> > kthread+0x24c/0x264
> > ret_from_fork+0x10/0x18
> >
> > Unfortunately, when I went to add the initializer for this and tried to
> > boot it, my machine shut down immediately, complaining that it was
> > hotter than the sun. It appears that adc5_chans_pmic and adc5_chans_rev2
> > were initializing prescale_index as if it were directly a divisor,
> > rather than the index into adc5_prescale_ratios that it is.
> >
> > Fix the uninitialized value, and change the static initialization to use
> > indices into adc5_prescale_ratios.
> >
> > Signed-off-by: Evan Green <[email protected]>
> > ---
> >
> > drivers/iio/adc/qcom-spmi-adc5.c | 58 +++++++++++++++++---------------
> > 1 file changed, 31 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c
> > index f9af6b082916b..6a866cc187f7d 100644
> > --- a/drivers/iio/adc/qcom-spmi-adc5.c
> > +++ b/drivers/iio/adc/qcom-spmi-adc5.c
> > @@ -423,6 +423,7 @@ struct adc5_channels {
> > enum vadc_scale_fn_type scale_fn_type;
> > };
> >
> > +/* In these definitions, _pre refers to an index into adc5_prescale_ratios. */
> > #define ADC5_CHAN(_dname, _type, _mask, _pre, _scale) \
> > { \
> > .datasheet_name = _dname, \
> > @@ -443,63 +444,63 @@ struct adc5_channels {
> > _pre, _scale) \
> >
> > static const struct adc5_channels adc5_chans_pmic[ADC5_MAX_CHANNEL] = {
> > - [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 1,
> > + [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 0,
> > SCALE_HW_CALIB_DEFAULT)
> > - [ADC5_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 1,
> > + [ADC5_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 0,
> > SCALE_HW_CALIB_DEFAULT)
> > - [ADC5_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 3,
> > + [ADC5_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 1,
> > SCALE_HW_CALIB_DEFAULT)
> > - [ADC5_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 3,
> > + [ADC5_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 1,
> > SCALE_HW_CALIB_DEFAULT)
> > - [ADC5_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 1,
> > + [ADC5_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 0,
> > SCALE_HW_CALIB_PMIC_THERM)
> > - [ADC5_USB_IN_I] = ADC5_CHAN_VOLT("usb_in_i_uv", 1,
> > + [ADC5_USB_IN_I] = ADC5_CHAN_VOLT("usb_in_i_uv", 0,
> > SCALE_HW_CALIB_DEFAULT)
> > - [ADC5_USB_IN_V_16] = ADC5_CHAN_VOLT("usb_in_v_div_16", 16,
> > + [ADC5_USB_IN_V_16] = ADC5_CHAN_VOLT("usb_in_v_div_16", 8,
> > SCALE_HW_CALIB_DEFAULT)
> > - [ADC5_CHG_TEMP] = ADC5_CHAN_TEMP("chg_temp", 1,
> > + [ADC5_CHG_TEMP] = ADC5_CHAN_TEMP("chg_temp", 0,
> > SCALE_HW_CALIB_PM5_CHG_TEMP)
> > /* Charger prescales SBUx and MID_CHG to fit within 1.8V upper unit */
> > - [ADC5_SBUx] = ADC5_CHAN_VOLT("chg_sbux", 3,
> > + [ADC5_SBUx] = ADC5_CHAN_VOLT("chg_sbux", 1,
> > SCALE_HW_CALIB_DEFAULT)
> > - [ADC5_MID_CHG_DIV6] = ADC5_CHAN_VOLT("chg_mid_chg", 6,
> > + [ADC5_MID_CHG_DIV6] = ADC5_CHAN_VOLT("chg_mid_chg", 3,
> > SCALE_HW_CALIB_DEFAULT)
> > - [ADC5_XO_THERM_100K_PU] = ADC5_CHAN_TEMP("xo_therm", 1,
> > + [ADC5_XO_THERM_100K_PU] = ADC5_CHAN_TEMP("xo_therm", 0,
> > SCALE_HW_CALIB_XOTHERM)
> > - [ADC5_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_100k_pu", 1,
> > + [ADC5_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_100k_pu", 0,
> > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > - [ADC5_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_100k_pu", 1,
> > + [ADC5_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_100k_pu", 0,
> > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > - [ADC5_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_100k_pu", 1,
> > + [ADC5_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_100k_pu", 0,
> > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > - [ADC5_AMUX_THM2] = ADC5_CHAN_TEMP("amux_thm2", 1,
> > + [ADC5_AMUX_THM2] = ADC5_CHAN_TEMP("amux_thm2", 0,
> > SCALE_HW_CALIB_PM5_SMB_TEMP)
> > };
> >
> > static const struct adc5_channels adc5_chans_rev2[ADC5_MAX_CHANNEL] = {
> > - [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 1,
> > + [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 0,
> > SCALE_HW_CALIB_DEFAULT)
> > - [ADC5_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 1,
> > + [ADC5_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 0,
> > SCALE_HW_CALIB_DEFAULT)
> > - [ADC5_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 3,
> > + [ADC5_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 1,
> > SCALE_HW_CALIB_DEFAULT)
> > - [ADC5_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 3,
> > + [ADC5_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 1,
> > SCALE_HW_CALIB_DEFAULT)
> > - [ADC5_VCOIN] = ADC5_CHAN_VOLT("vcoin", 3,
> > + [ADC5_VCOIN] = ADC5_CHAN_VOLT("vcoin", 1,
> > SCALE_HW_CALIB_DEFAULT)
> > - [ADC5_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 1,
> > + [ADC5_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 0,
> > SCALE_HW_CALIB_PMIC_THERM)
> > - [ADC5_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_100k_pu", 1,
> > + [ADC5_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_100k_pu", 0,
> > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > - [ADC5_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_100k_pu", 1,
> > + [ADC5_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_100k_pu", 0,
> > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > - [ADC5_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_100k_pu", 1,
> > + [ADC5_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_100k_pu", 0,
> > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > - [ADC5_AMUX_THM4_100K_PU] = ADC5_CHAN_TEMP("amux_thm4_100k_pu", 1,
> > + [ADC5_AMUX_THM4_100K_PU] = ADC5_CHAN_TEMP("amux_thm4_100k_pu", 0,
> > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > - [ADC5_AMUX_THM5_100K_PU] = ADC5_CHAN_TEMP("amux_thm5_100k_pu", 1,
> > + [ADC5_AMUX_THM5_100K_PU] = ADC5_CHAN_TEMP("amux_thm5_100k_pu", 0,
> > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > - [ADC5_XO_THERM_100K_PU] = ADC5_CHAN_TEMP("xo_therm_100k_pu", 1,
> > + [ADC5_XO_THERM_100K_PU] = ADC5_CHAN_TEMP("xo_therm_100k_pu", 0,
> > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > };
> >
> > @@ -558,6 +559,9 @@ static int adc5_get_dt_channel_data(struct adc5_chip *adc,
> > return ret;
> > }
> > prop->prescale = ret;
> > + } else {
> > + prop->prescale =
> > + adc->data->adc_chans[prop->channel].prescale_index;
> > }
> >
> > ret = of_property_read_u32(node, "qcom,hw-settle-time", &value);
>
> Reviewed-by: Matthias Kaehlcke <[email protected]>
>
> Good as an immediate fix.
>
> In the long term I wonder if it would be better to pass a ratio to
> ADC5_CHAN_XYZ and store it in struct adc5_channel_prop, instead of
> specifying the index, which is more error prone and errors are harder
> to spot. With this adc5_prescale_ratios would still exist, but only be
> used for the sanity check of prescaling values from the DT.

I am going to let this sit for a little longer to let Siddartha have
a chance to comment as well. We are getting late this cycle
so this may end up going upstream at the start of the next one
(as it'll be marked for stable that shouldn't matter much).

Poke me if I seem to have forgotten it in a week or so.

Thanks,

Jonathan

2018-12-12 18:44:33

by Jonathan Cameron

Subject: Re: [PATCH] iio: adc: qcom-spmi-adc5: Initialize prescale properly

On Sat, 8 Dec 2018 12:05:15 +0000
Jonathan Cameron <[email protected]> wrote:

> On Tue, 4 Dec 2018 11:57:32 -0800
> Matthias Kaehlcke <[email protected]> wrote:
>
> > On Tue, Dec 04, 2018 at 11:14:19AM -0800, Evan Green wrote:
> > > adc5_get_dt_data uses a local, prop, feeds it to adc5_get_dt_channel_data,
> > > and then puts the result into adc->chan_props. The problem is
> > > adc5_get_dt_channel_data may not initialize that structure fully, so a
> > > garbage value is used for prescale if the optional "qcom,pre-scaling" is
> > > not defined in DT. adc5_read_raw then uses this as an array index,
> > > generating a crash that looks like this:
> > >
> > > [ 6.683186] Unable to handle kernel paging request at virtual address ffffff90e78c7964
> > > Call trace:
> > > qcom_vadc_scale_code_voltage_factor+0x74/0x104
> > > qcom_vadc_scale_hw_calib_die_temp+0x20/0x60
> > > qcom_adc5_hw_scale+0x78/0xa4
> > > adc5_read_raw+0x3d0/0x65c
> > > iio_channel_read+0x240/0x30c
> > > iio_read_channel_processed+0x10c/0x150
> > > qpnp_tm_get_temp+0xc0/0x40c
> > > of_thermal_get_temp+0x7c/0x98
> > > thermal_zone_get_temp+0xac/0xd8
> > > thermal_zone_device_update+0xc0/0x38c
> > > qpnp_tm_probe+0x624/0x81c
> > > platform_drv_probe+0xe4/0x11c
> > > really_probe+0x188/0x3fc
> > > driver_probe_device+0xb8/0x188
> > > __device_attach_driver+0x114/0x180
> > > bus_for_each_drv+0xd8/0x118
> > > __device_attach+0x180/0x27c
> > > device_initial_probe+0x20/0x2c
> > > bus_probe_device+0x78/0x124
> > > deferred_probe_work_func+0xfc/0x138
> > > process_one_work+0x3d8/0x8b0
> > > process_scheduled_works+0x48/0x6c
> > > worker_thread+0x488/0x7cc
> > > kthread+0x24c/0x264
> > > ret_from_fork+0x10/0x18
> > >
> > > Unfortunately, when I went to add the initializer for this and tried to
> > > boot it, my machine shut down immediately, complaining that it was
> > > hotter than the sun. It appears that adc5_chans_pmic and adc5_chans_rev2
> > > were initializing prescale_index as if it were directly a divisor,
> > > rather than the index into adc5_prescale_ratios that it is.
> > >
> > > Fix the uninitialized value, and change the static initialization to use
> > > indices into adc5_prescale_ratios.
> > >
> > > Signed-off-by: Evan Green <[email protected]>
> > > ---
> > >
> > > drivers/iio/adc/qcom-spmi-adc5.c | 58 +++++++++++++++++---------------
> > > 1 file changed, 31 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c
> > > index f9af6b082916b..6a866cc187f7d 100644
> > > --- a/drivers/iio/adc/qcom-spmi-adc5.c
> > > +++ b/drivers/iio/adc/qcom-spmi-adc5.c
> > > @@ -423,6 +423,7 @@ struct adc5_channels {
> > > enum vadc_scale_fn_type scale_fn_type;
> > > };
> > >
> > > +/* In these definitions, _pre refers to an index into adc5_prescale_ratios. */
> > > #define ADC5_CHAN(_dname, _type, _mask, _pre, _scale) \
> > > { \
> > > .datasheet_name = _dname, \
> > > @@ -443,63 +444,63 @@ struct adc5_channels {
> > > _pre, _scale) \
> > >
> > > static const struct adc5_channels adc5_chans_pmic[ADC5_MAX_CHANNEL] = {
> > > - [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 1,
> > > + [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 0,
> > > SCALE_HW_CALIB_DEFAULT)
> > > - [ADC5_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 1,
> > > + [ADC5_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 0,
> > > SCALE_HW_CALIB_DEFAULT)
> > > - [ADC5_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 3,
> > > + [ADC5_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 1,
> > > SCALE_HW_CALIB_DEFAULT)
> > > - [ADC5_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 3,
> > > + [ADC5_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 1,
> > > SCALE_HW_CALIB_DEFAULT)
> > > - [ADC5_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 1,
> > > + [ADC5_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 0,
> > > SCALE_HW_CALIB_PMIC_THERM)
> > > - [ADC5_USB_IN_I] = ADC5_CHAN_VOLT("usb_in_i_uv", 1,
> > > + [ADC5_USB_IN_I] = ADC5_CHAN_VOLT("usb_in_i_uv", 0,
> > > SCALE_HW_CALIB_DEFAULT)
> > > - [ADC5_USB_IN_V_16] = ADC5_CHAN_VOLT("usb_in_v_div_16", 16,
> > > + [ADC5_USB_IN_V_16] = ADC5_CHAN_VOLT("usb_in_v_div_16", 8,
> > > SCALE_HW_CALIB_DEFAULT)
> > > - [ADC5_CHG_TEMP] = ADC5_CHAN_TEMP("chg_temp", 1,
> > > + [ADC5_CHG_TEMP] = ADC5_CHAN_TEMP("chg_temp", 0,
> > > SCALE_HW_CALIB_PM5_CHG_TEMP)
> > > /* Charger prescales SBUx and MID_CHG to fit within 1.8V upper unit */
> > > - [ADC5_SBUx] = ADC5_CHAN_VOLT("chg_sbux", 3,
> > > + [ADC5_SBUx] = ADC5_CHAN_VOLT("chg_sbux", 1,
> > > SCALE_HW_CALIB_DEFAULT)
> > > - [ADC5_MID_CHG_DIV6] = ADC5_CHAN_VOLT("chg_mid_chg", 6,
> > > + [ADC5_MID_CHG_DIV6] = ADC5_CHAN_VOLT("chg_mid_chg", 3,
> > > SCALE_HW_CALIB_DEFAULT)
> > > - [ADC5_XO_THERM_100K_PU] = ADC5_CHAN_TEMP("xo_therm", 1,
> > > + [ADC5_XO_THERM_100K_PU] = ADC5_CHAN_TEMP("xo_therm", 0,
> > > SCALE_HW_CALIB_XOTHERM)
> > > - [ADC5_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_100k_pu", 1,
> > > + [ADC5_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_100k_pu", 0,
> > > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > > - [ADC5_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_100k_pu", 1,
> > > + [ADC5_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_100k_pu", 0,
> > > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > > - [ADC5_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_100k_pu", 1,
> > > + [ADC5_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_100k_pu", 0,
> > > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > > - [ADC5_AMUX_THM2] = ADC5_CHAN_TEMP("amux_thm2", 1,
> > > + [ADC5_AMUX_THM2] = ADC5_CHAN_TEMP("amux_thm2", 0,
> > > SCALE_HW_CALIB_PM5_SMB_TEMP)
> > > };
> > >
> > > static const struct adc5_channels adc5_chans_rev2[ADC5_MAX_CHANNEL] = {
> > > - [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 1,
> > > + [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 0,
> > > SCALE_HW_CALIB_DEFAULT)
> > > - [ADC5_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 1,
> > > + [ADC5_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 0,
> > > SCALE_HW_CALIB_DEFAULT)
> > > - [ADC5_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 3,
> > > + [ADC5_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 1,
> > > SCALE_HW_CALIB_DEFAULT)
> > > - [ADC5_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 3,
> > > + [ADC5_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 1,
> > > SCALE_HW_CALIB_DEFAULT)
> > > - [ADC5_VCOIN] = ADC5_CHAN_VOLT("vcoin", 3,
> > > + [ADC5_VCOIN] = ADC5_CHAN_VOLT("vcoin", 1,
> > > SCALE_HW_CALIB_DEFAULT)
> > > - [ADC5_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 1,
> > > + [ADC5_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 0,
> > > SCALE_HW_CALIB_PMIC_THERM)
> > > - [ADC5_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_100k_pu", 1,
> > > + [ADC5_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_100k_pu", 0,
> > > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > > - [ADC5_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_100k_pu", 1,
> > > + [ADC5_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_100k_pu", 0,
> > > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > > - [ADC5_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_100k_pu", 1,
> > > + [ADC5_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_100k_pu", 0,
> > > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > > - [ADC5_AMUX_THM4_100K_PU] = ADC5_CHAN_TEMP("amux_thm4_100k_pu", 1,
> > > + [ADC5_AMUX_THM4_100K_PU] = ADC5_CHAN_TEMP("amux_thm4_100k_pu", 0,
> > > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > > - [ADC5_AMUX_THM5_100K_PU] = ADC5_CHAN_TEMP("amux_thm5_100k_pu", 1,
> > > + [ADC5_AMUX_THM5_100K_PU] = ADC5_CHAN_TEMP("amux_thm5_100k_pu", 0,
> > > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > > - [ADC5_XO_THERM_100K_PU] = ADC5_CHAN_TEMP("xo_therm_100k_pu", 1,
> > > + [ADC5_XO_THERM_100K_PU] = ADC5_CHAN_TEMP("xo_therm_100k_pu", 0,
> > > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > > };
> > >
> > > @@ -558,6 +559,9 @@ static int adc5_get_dt_channel_data(struct adc5_chip *adc,
> > > return ret;
> > > }
> > > prop->prescale = ret;
> > > + } else {
> > > + prop->prescale =
> > > + adc->data->adc_chans[prop->channel].prescale_index;
> > > }
> > >
> > > ret = of_property_read_u32(node, "qcom,hw-settle-time", &value);
> >
> > Reviewed-by: Matthias Kaehlcke <[email protected]>
> >
> > Good as an immediate fix.
> >
> > In the long term I wonder if it would be better to pass a ratio to
> > ADC5_CHAN_XYZ and store it in struct adc5_channel_prop, instead of
> > specifying the index, which is more error prone and errors are harder
> > to spot. With this adc5_prescale_ratios would still exist, but only be
> > used for the sanity check of prescaling values from the DT.
>
> I am going to let this sit for a little longer to let Siddartha have
> a chance to comment as well. We are getting late this cycle
> so this may end up going upstream at the start of the next one
> (as it'll be marked for stable that shouldn't matter much).
>
> Poke me if I seem to have forgotten it in a week or so.
I've applied this to the togreg branch of iio.git so it'll go in
during the next merge window (hopefully). Marked for stable.

Thanks,

Jonathan

>
> Thanks,
>
> Jonathan
>

2018-12-12 20:17:57

by Evan Green

Subject: Re: [PATCH] iio: adc: qcom-spmi-adc5: Initialize prescale properly

On Wed, Dec 12, 2018 at 10:41 AM Jonathan Cameron
<[email protected]> wrote:
>
> On Sat, 8 Dec 2018 12:05:15 +0000
> Jonathan Cameron <[email protected]> wrote:
>
> > On Tue, 4 Dec 2018 11:57:32 -0800
> > Matthias Kaehlcke <[email protected]> wrote:
> >
> > > On Tue, Dec 04, 2018 at 11:14:19AM -0800, Evan Green wrote:
> > > > adc5_get_dt_data uses a local, prop, feeds it to adc5_get_dt_channel_data,
> > > > and then puts the result into adc->chan_props. The problem is
> > > > adc5_get_dt_channel_data may not initialize that structure fully, so a
> > > > garbage value is used for prescale if the optional "qcom,pre-scaling" is
> > > > not defined in DT. adc5_read_raw then uses this as an array index,
> > > > generating a crash that looks like this:
> > > >
> > > > [ 6.683186] Unable to handle kernel paging request at virtual address ffffff90e78c7964
> > > > Call trace:
> > > > qcom_vadc_scale_code_voltage_factor+0x74/0x104
> > > > qcom_vadc_scale_hw_calib_die_temp+0x20/0x60
> > > > qcom_adc5_hw_scale+0x78/0xa4
> > > > adc5_read_raw+0x3d0/0x65c
> > > > iio_channel_read+0x240/0x30c
> > > > iio_read_channel_processed+0x10c/0x150
> > > > qpnp_tm_get_temp+0xc0/0x40c
> > > > of_thermal_get_temp+0x7c/0x98
> > > > thermal_zone_get_temp+0xac/0xd8
> > > > thermal_zone_device_update+0xc0/0x38c
> > > > qpnp_tm_probe+0x624/0x81c
> > > > platform_drv_probe+0xe4/0x11c
> > > > really_probe+0x188/0x3fc
> > > > driver_probe_device+0xb8/0x188
> > > > __device_attach_driver+0x114/0x180
> > > > bus_for_each_drv+0xd8/0x118
> > > > __device_attach+0x180/0x27c
> > > > device_initial_probe+0x20/0x2c
> > > > bus_probe_device+0x78/0x124
> > > > deferred_probe_work_func+0xfc/0x138
> > > > process_one_work+0x3d8/0x8b0
> > > > process_scheduled_works+0x48/0x6c
> > > > worker_thread+0x488/0x7cc
> > > > kthread+0x24c/0x264
> > > > ret_from_fork+0x10/0x18
> > > >
> > > > Unfortunately, when I went to add the initializer for this and tried to
> > > > boot it, my machine shut down immediately, complaining that it was
> > > > hotter than the sun. It appears that adc5_chans_pmic and adc5_chans_rev2
> > > > were initializing prescale_index as if it were directly a divisor,
> > > > rather than the index into adc5_prescale_ratios that it is.
> > > >
> > > > Fix the uninitialized value, and change the static initialization to use
> > > > indices into adc5_prescale_ratios.
> > > >
> > > > Signed-off-by: Evan Green <[email protected]>
> > > > ---
> > > >
> > > > drivers/iio/adc/qcom-spmi-adc5.c | 58 +++++++++++++++++---------------
> > > > 1 file changed, 31 insertions(+), 27 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c
> > > > index f9af6b082916b..6a866cc187f7d 100644
> > > > --- a/drivers/iio/adc/qcom-spmi-adc5.c
> > > > +++ b/drivers/iio/adc/qcom-spmi-adc5.c
> > > > @@ -423,6 +423,7 @@ struct adc5_channels {
> > > > enum vadc_scale_fn_type scale_fn_type;
> > > > };
> > > >
> > > > +/* In these definitions, _pre refers to an index into adc5_prescale_ratios. */
> > > > #define ADC5_CHAN(_dname, _type, _mask, _pre, _scale) \
> > > > { \
> > > > .datasheet_name = _dname, \
> > > > @@ -443,63 +444,63 @@ struct adc5_channels {
> > > > _pre, _scale) \
> > > >
> > > > static const struct adc5_channels adc5_chans_pmic[ADC5_MAX_CHANNEL] = {
> > > > - [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 1,
> > > > + [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 0,
> > > > SCALE_HW_CALIB_DEFAULT)
> > > > - [ADC5_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 1,
> > > > + [ADC5_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 0,
> > > > SCALE_HW_CALIB_DEFAULT)
> > > > - [ADC5_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 3,
> > > > + [ADC5_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 1,
> > > > SCALE_HW_CALIB_DEFAULT)
> > > > - [ADC5_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 3,
> > > > + [ADC5_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 1,
> > > > SCALE_HW_CALIB_DEFAULT)
> > > > - [ADC5_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 1,
> > > > + [ADC5_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 0,
> > > > SCALE_HW_CALIB_PMIC_THERM)
> > > > - [ADC5_USB_IN_I] = ADC5_CHAN_VOLT("usb_in_i_uv", 1,
> > > > + [ADC5_USB_IN_I] = ADC5_CHAN_VOLT("usb_in_i_uv", 0,
> > > > SCALE_HW_CALIB_DEFAULT)
> > > > - [ADC5_USB_IN_V_16] = ADC5_CHAN_VOLT("usb_in_v_div_16", 16,
> > > > + [ADC5_USB_IN_V_16] = ADC5_CHAN_VOLT("usb_in_v_div_16", 8,
> > > > SCALE_HW_CALIB_DEFAULT)
> > > > - [ADC5_CHG_TEMP] = ADC5_CHAN_TEMP("chg_temp", 1,
> > > > + [ADC5_CHG_TEMP] = ADC5_CHAN_TEMP("chg_temp", 0,
> > > > SCALE_HW_CALIB_PM5_CHG_TEMP)
> > > > /* Charger prescales SBUx and MID_CHG to fit within 1.8V upper unit */
> > > > - [ADC5_SBUx] = ADC5_CHAN_VOLT("chg_sbux", 3,
> > > > + [ADC5_SBUx] = ADC5_CHAN_VOLT("chg_sbux", 1,
> > > > SCALE_HW_CALIB_DEFAULT)
> > > > - [ADC5_MID_CHG_DIV6] = ADC5_CHAN_VOLT("chg_mid_chg", 6,
> > > > + [ADC5_MID_CHG_DIV6] = ADC5_CHAN_VOLT("chg_mid_chg", 3,
> > > > SCALE_HW_CALIB_DEFAULT)
> > > > - [ADC5_XO_THERM_100K_PU] = ADC5_CHAN_TEMP("xo_therm", 1,
> > > > + [ADC5_XO_THERM_100K_PU] = ADC5_CHAN_TEMP("xo_therm", 0,
> > > > SCALE_HW_CALIB_XOTHERM)
> > > > - [ADC5_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_100k_pu", 1,
> > > > + [ADC5_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_100k_pu", 0,
> > > > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > > > - [ADC5_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_100k_pu", 1,
> > > > + [ADC5_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_100k_pu", 0,
> > > > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > > > - [ADC5_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_100k_pu", 1,
> > > > + [ADC5_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_100k_pu", 0,
> > > > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > > > - [ADC5_AMUX_THM2] = ADC5_CHAN_TEMP("amux_thm2", 1,
> > > > + [ADC5_AMUX_THM2] = ADC5_CHAN_TEMP("amux_thm2", 0,
> > > > SCALE_HW_CALIB_PM5_SMB_TEMP)
> > > > };
> > > >
> > > > static const struct adc5_channels adc5_chans_rev2[ADC5_MAX_CHANNEL] = {
> > > > - [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 1,
> > > > + [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 0,
> > > > SCALE_HW_CALIB_DEFAULT)
> > > > - [ADC5_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 1,
> > > > + [ADC5_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 0,
> > > > SCALE_HW_CALIB_DEFAULT)
> > > > - [ADC5_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 3,
> > > > + [ADC5_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 1,
> > > > SCALE_HW_CALIB_DEFAULT)
> > > > - [ADC5_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 3,
> > > > + [ADC5_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 1,
> > > > SCALE_HW_CALIB_DEFAULT)
> > > > - [ADC5_VCOIN] = ADC5_CHAN_VOLT("vcoin", 3,
> > > > + [ADC5_VCOIN] = ADC5_CHAN_VOLT("vcoin", 1,
> > > > SCALE_HW_CALIB_DEFAULT)
> > > > - [ADC5_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 1,
> > > > + [ADC5_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 0,
> > > > SCALE_HW_CALIB_PMIC_THERM)
> > > > - [ADC5_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_100k_pu", 1,
> > > > + [ADC5_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_100k_pu", 0,
> > > > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > > > - [ADC5_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_100k_pu", 1,
> > > > + [ADC5_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_100k_pu", 0,
> > > > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > > > - [ADC5_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_100k_pu", 1,
> > > > + [ADC5_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_100k_pu", 0,
> > > > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > > > - [ADC5_AMUX_THM4_100K_PU] = ADC5_CHAN_TEMP("amux_thm4_100k_pu", 1,
> > > > + [ADC5_AMUX_THM4_100K_PU] = ADC5_CHAN_TEMP("amux_thm4_100k_pu", 0,
> > > > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > > > - [ADC5_AMUX_THM5_100K_PU] = ADC5_CHAN_TEMP("amux_thm5_100k_pu", 1,
> > > > + [ADC5_AMUX_THM5_100K_PU] = ADC5_CHAN_TEMP("amux_thm5_100k_pu", 0,
> > > > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > > > - [ADC5_XO_THERM_100K_PU] = ADC5_CHAN_TEMP("xo_therm_100k_pu", 1,
> > > > + [ADC5_XO_THERM_100K_PU] = ADC5_CHAN_TEMP("xo_therm_100k_pu", 0,
> > > > SCALE_HW_CALIB_THERM_100K_PULLUP)
> > > > };
> > > >
> > > > @@ -558,6 +559,9 @@ static int adc5_get_dt_channel_data(struct adc5_chip *adc,
> > > > return ret;
> > > > }
> > > > prop->prescale = ret;
> > > > + } else {
> > > > + prop->prescale =
> > > > + adc->data->adc_chans[prop->channel].prescale_index;
> > > > }
> > > >
> > > > ret = of_property_read_u32(node, "qcom,hw-settle-time", &value);
> > >
> > > Reviewed-by: Matthias Kaehlcke <[email protected]>
> > >
> > > Good as an immediate fix.
> > >
> > > In the long term I wonder if it would be better to pass a ratio to
> > > ADC5_CHAN_XYZ and store it in struct adc5_channel_prop, instead of
> > > specifying the index, which is more error prone and errors are harder
> > > to spot. With this adc5_prescale_ratios would still exist, but only be
> > > used for the sanity check of prescaling values from the DT.
> >
> > I am going to let this sit for a little longer to let Siddartha have
> > a chance to comment as well. We are getting late this cycle
> > so this may end up going upstream at the start of the next one
> > (as it'll be marked for stable that shouldn't matter much).
> >
> > Poke me if I seem to have forgotten it in a week or so.
> I've applied this to the togreg branch of iio.git so it'll go in
> during the next merge window (hopefully). Marked for stable.
>

Thanks, Jonathan!