On Thu, Jul 04, 2024 at 10:30:50AM GMT, Varadarajan Narayanan wrote: > On Wed, Jul 03, 2024 at 01:46:54PM +0300, Dmitry Baryshkov wrote: > > On Wed, Jul 03, 2024 at 02:46:42PM GMT, Varadarajan Narayanan wrote: > > > cpr3 code assumes that 'acc_desc' is available for SoCs > > > implementing CPR version 4 or less. However, IPQ9574 SoC > > > implements CPRv4 without ACC. This causes NULL pointer accesses > > > resulting in crashes. Hence, check if 'acc_desc' is populated > > > before using it. > > > > > > Signed-off-by: Varadarajan Narayanan <quic_varada@xxxxxxxxxxx> > > > --- > > > v4: Undo the acc_desc validation in probe function as that could > > > affect other SoC. > > > --- > > > drivers/pmdomain/qcom/cpr3.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/pmdomain/qcom/cpr3.c b/drivers/pmdomain/qcom/cpr3.c > > > index c7790a71e74f..6ceb7605f84d 100644 > > > --- a/drivers/pmdomain/qcom/cpr3.c > > > +++ b/drivers/pmdomain/qcom/cpr3.c > > > @@ -2399,12 +2399,12 @@ static int cpr_pd_attach_dev(struct generic_pm_domain *domain, > > > if (ret) > > > goto exit; > > > > > > - if (acc_desc->config) > > > + if (acc_desc && acc_desc->config) > > > regmap_multi_reg_write(drv->tcsr, acc_desc->config, > > > acc_desc->num_regs_per_fuse); > > > > > > /* Enable ACC if required */ > > > - if (acc_desc->enable_mask) > > > + if (acc_desc && acc_desc->enable_mask) > > > regmap_update_bits(drv->tcsr, acc_desc->enable_reg, > > > acc_desc->enable_mask, > > > acc_desc->enable_mask); > > > > Should the same fix be applied to other places which access acc_desc? > > For example cpr_pre_voltage() and cpr_post_voltage() which call > > cpr_set_acc()? > > With this patch alone, if acc_desc is NULL, cpr_probe() will fail > at the start itself because of this check > > if (!data->acc_desc && desc->cpr_type < CTRL_TYPE_CPRH) > return -EINVAL; > > After applying this patch series, cpr_probe will cross the above > check to accomodate IPQ9574. However, the check below will ensure > drv->tcsr is not initialized. > > if (desc->cpr_type < CTRL_TYPE_CPRH && > !of_device_is_compatible(dev->of_node, "qcom,ipq9574-cpr4")) > > cpr_pre_voltage() and cpr_post_voltage() call cpr_set_acc() only > if drv->tcsr is not NULL. Hence acc_desc need not be checked. > > Will add the check to cpr_pre_voltage() and cpr_post_voltage() if > you feel it will make it more robust regardless of the changes to > cpr_probe in future. Please let me know. Having !acc_desc check instead of the of_device_is_compatible would solve the issue. -- With best wishes Dmitry
