| Message ID | 20240703091651.2820236-2-quic_varada@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,01/10] soc: qcom: cpr3: Fix 'acc_desc' usage | expand |
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@quicinc.com> > > --- > > 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. Thanks Varada
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@quicinc.com> > > > --- > > > 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.
On Fri, Jul 05, 2024 at 06:16:51PM +0300, Dmitry Baryshkov wrote: > 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@quicinc.com> > > > > --- > > > > 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. Ok. Will post next version with that. Thanks Varada
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);
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@quicinc.com> --- 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(-)