| Message ID | 20241203024005.391654-1-quic_yrangana@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] soc: qcom: ice: Prevent UFS probe deferral on ICE probe failure | expand |
On 3.12.2024 3:40 AM, Yuvaraj Ranganathan wrote: > When the ICE key programming interface is unavailable, the ice create > function fails, causing the probe to set NULL as the driver data. As a > result, when the UFS driver reads the ICE driver data and encounters a > NULL, leading to the deferral of the UFS probe and preventing the device > from booting to the shell. > > To address this issue, modify the behavior to return an "operation not > supported" error when the ICE key programming interface is unavailable. > Additionally, mark this error in a global variable. When the UFS driver > attempts to read the ICE driver data, it will check for this error and > return it, rather than deferring the probe. > > Signed-off-by: Yuvaraj Ranganathan <quic_yrangana@quicinc.com> > --- > drivers/soc/qcom/ice.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c > index 393d2d1d275f..160916cb8fb0 100644 > --- a/drivers/soc/qcom/ice.c > +++ b/drivers/soc/qcom/ice.c > @@ -41,6 +41,8 @@ > #define qcom_ice_readl(engine, reg) \ > readl((engine)->base + (reg)) > > +static bool qcom_ice_create_error; So you could drop this.. > + > struct qcom_ice { > struct device *dev; > void __iomem *base; > @@ -215,7 +217,7 @@ static struct qcom_ice *qcom_ice_create(struct device *dev, > > if (!qcom_scm_ice_available()) { > dev_warn(dev, "ICE SCM interface not found\n"); > - return NULL; > + return ERR_PTR(-EOPNOTSUPP); > } > > engine = devm_kzalloc(dev, sizeof(*engine), GFP_KERNEL); > @@ -303,6 +305,9 @@ struct qcom_ice *of_qcom_ice_get(struct device *dev) > return ERR_PTR(-EPROBE_DEFER); > } > > + if (qcom_ice_create_error) > + return ERR_PTR(-EOPNOTSUPP); > + > ice = platform_get_drvdata(pdev); > if (!ice) { ..and check for || IS_ERR(ice) here if I'm reading things right Konrad
On 12/5/2024 10:54 PM, Konrad Dybcio wrote: > On 3.12.2024 3:40 AM, Yuvaraj Ranganathan wrote: >> When the ICE key programming interface is unavailable, the ice create >> function fails, causing the probe to set NULL as the driver data. As a >> result, when the UFS driver reads the ICE driver data and encounters a >> NULL, leading to the deferral of the UFS probe and preventing the device >> from booting to the shell. >> >> To address this issue, modify the behavior to return an "operation not >> supported" error when the ICE key programming interface is unavailable. >> Additionally, mark this error in a global variable. When the UFS driver >> attempts to read the ICE driver data, it will check for this error and >> return it, rather than deferring the probe. >> >> Signed-off-by: Yuvaraj Ranganathan <quic_yrangana@quicinc.com> >> --- >> drivers/soc/qcom/ice.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c >> index 393d2d1d275f..160916cb8fb0 100644 >> --- a/drivers/soc/qcom/ice.c >> +++ b/drivers/soc/qcom/ice.c >> @@ -41,6 +41,8 @@ >> #define qcom_ice_readl(engine, reg) \ >> readl((engine)->base + (reg)) >> >> +static bool qcom_ice_create_error; > > So you could drop this.. > >> + >> struct qcom_ice { >> struct device *dev; >> void __iomem *base; >> @@ -215,7 +217,7 @@ static struct qcom_ice *qcom_ice_create(struct device *dev, >> >> if (!qcom_scm_ice_available()) { >> dev_warn(dev, "ICE SCM interface not found\n"); >> - return NULL; >> + return ERR_PTR(-EOPNOTSUPP); >> } >> >> engine = devm_kzalloc(dev, sizeof(*engine), GFP_KERNEL); >> @@ -303,6 +305,9 @@ struct qcom_ice *of_qcom_ice_get(struct device *dev) >> return ERR_PTR(-EPROBE_DEFER); >> } >> >> + if (qcom_ice_create_error) >> + return ERR_PTR(-EOPNOTSUPP); >> + >> ice = platform_get_drvdata(pdev); >> if (!ice) { > > ..and check for || IS_ERR(ice) here > > if I'm reading things right > > Konrad In case of failure, platform_set_drvdata is not invoked and it is causing ice to become NULL on platform_get_drvdata. Adding IS_ERR(ice) can't help unless we set the platform_set_drvdata even on failure.
On 23.12.2024 10:20 AM, Yuvaraj Ranganathan wrote: > On 12/5/2024 10:54 PM, Konrad Dybcio wrote: >> On 3.12.2024 3:40 AM, Yuvaraj Ranganathan wrote: >>> When the ICE key programming interface is unavailable, the ice create >>> function fails, causing the probe to set NULL as the driver data. As a >>> result, when the UFS driver reads the ICE driver data and encounters a >>> NULL, leading to the deferral of the UFS probe and preventing the device >>> from booting to the shell. >>> >>> To address this issue, modify the behavior to return an "operation not >>> supported" error when the ICE key programming interface is unavailable. >>> Additionally, mark this error in a global variable. When the UFS driver >>> attempts to read the ICE driver data, it will check for this error and >>> return it, rather than deferring the probe. >>> >>> Signed-off-by: Yuvaraj Ranganathan <quic_yrangana@quicinc.com> >>> --- >>> drivers/soc/qcom/ice.c | 11 +++++++++-- >>> 1 file changed, 9 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c >>> index 393d2d1d275f..160916cb8fb0 100644 >>> --- a/drivers/soc/qcom/ice.c >>> +++ b/drivers/soc/qcom/ice.c >>> @@ -41,6 +41,8 @@ >>> #define qcom_ice_readl(engine, reg) \ >>> readl((engine)->base + (reg)) >>> >>> +static bool qcom_ice_create_error; >> >> So you could drop this.. >> >>> + >>> struct qcom_ice { >>> struct device *dev; >>> void __iomem *base; >>> @@ -215,7 +217,7 @@ static struct qcom_ice *qcom_ice_create(struct device *dev, >>> >>> if (!qcom_scm_ice_available()) { >>> dev_warn(dev, "ICE SCM interface not found\n"); >>> - return NULL; >>> + return ERR_PTR(-EOPNOTSUPP); >>> } >>> >>> engine = devm_kzalloc(dev, sizeof(*engine), GFP_KERNEL); >>> @@ -303,6 +305,9 @@ struct qcom_ice *of_qcom_ice_get(struct device *dev) >>> return ERR_PTR(-EPROBE_DEFER); >>> } >>> >>> + if (qcom_ice_create_error) >>> + return ERR_PTR(-EOPNOTSUPP); >>> + >>> ice = platform_get_drvdata(pdev); >>> if (!ice) { >> >> ..and check for || IS_ERR(ice) here >> >> if I'm reading things right >> >> Konrad > > In case of failure, platform_set_drvdata is not invoked and it is > causing ice to become NULL on platform_get_drvdata. > Adding IS_ERR(ice) can't help unless we set the platform_set_drvdata > even on failure. Which we should be able to do, given the platform device exists by the time we get there. An additional parameter to create() may be useful to make sure we're not overwriting UFS's drvdata in the legacy fallback case Konrad
On Tue, Dec 03, 2024 at 08:10:05AM +0530, Yuvaraj Ranganathan wrote: > When the ICE key programming interface is unavailable, the ice create > function fails, causing the probe to set NULL as the driver data. As a > result, when the UFS driver reads the ICE driver data and encounters a > NULL, leading to the deferral of the UFS probe and preventing the device > from booting to the shell. > > To address this issue, modify the behavior to return an "operation not > supported" error when the ICE key programming interface is unavailable. > Additionally, mark this error in a global variable. When the UFS driver > attempts to read the ICE driver data, it will check for this error and > return it, rather than deferring the probe. > I'm guessing that the actual test case here is that DeviceTree defines that UFS should use the ICE, but the trusted firmware doesn't implement the ICE API. Your commit message is pretty good, but I would like to see the commit message clarify which case this (DT says there's ICE, but firmware says no) is a valid case. > Signed-off-by: Yuvaraj Ranganathan <quic_yrangana@quicinc.com> > --- > drivers/soc/qcom/ice.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c > index 393d2d1d275f..160916cb8fb0 100644 > --- a/drivers/soc/qcom/ice.c > +++ b/drivers/soc/qcom/ice.c > @@ -41,6 +41,8 @@ > #define qcom_ice_readl(engine, reg) \ > readl((engine)->base + (reg)) > > +static bool qcom_ice_create_error; > + > struct qcom_ice { > struct device *dev; > void __iomem *base; > @@ -215,7 +217,7 @@ static struct qcom_ice *qcom_ice_create(struct device *dev, > > if (!qcom_scm_ice_available()) { > dev_warn(dev, "ICE SCM interface not found\n"); > - return NULL; > + return ERR_PTR(-EOPNOTSUPP); > } > > engine = devm_kzalloc(dev, sizeof(*engine), GFP_KERNEL); > @@ -303,6 +305,9 @@ struct qcom_ice *of_qcom_ice_get(struct device *dev) > return ERR_PTR(-EPROBE_DEFER); > } > > + if (qcom_ice_create_error) > + return ERR_PTR(-EOPNOTSUPP); > + > ice = platform_get_drvdata(pdev); > if (!ice) { > dev_err(dev, "Cannot get ice instance from %s\n", > @@ -336,8 +341,10 @@ static int qcom_ice_probe(struct platform_device *pdev) > } > > engine = qcom_ice_create(&pdev->dev, base); > - if (IS_ERR(engine)) > + if (IS_ERR(engine)) { > + qcom_ice_create_error = true; This would also handle the existing qcom_ice_check_supported() failure better, allowing the UFS device to probe - which may or may not be good, please use the commit message to document what the expected behavior is in these casees. That said, this will do the same for the core_clk = -EPROBE_DEFER, which is not correct - that should remain a EPROBE_DEFER. Regards, Bjorn > return PTR_ERR(engine); > + } > > platform_set_drvdata(pdev, engine); > > -- > 2.34.1 >
diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c index 393d2d1d275f..160916cb8fb0 100644 --- a/drivers/soc/qcom/ice.c +++ b/drivers/soc/qcom/ice.c @@ -41,6 +41,8 @@ #define qcom_ice_readl(engine, reg) \ readl((engine)->base + (reg)) +static bool qcom_ice_create_error; + struct qcom_ice { struct device *dev; void __iomem *base; @@ -215,7 +217,7 @@ static struct qcom_ice *qcom_ice_create(struct device *dev, if (!qcom_scm_ice_available()) { dev_warn(dev, "ICE SCM interface not found\n"); - return NULL; + return ERR_PTR(-EOPNOTSUPP); } engine = devm_kzalloc(dev, sizeof(*engine), GFP_KERNEL); @@ -303,6 +305,9 @@ struct qcom_ice *of_qcom_ice_get(struct device *dev) return ERR_PTR(-EPROBE_DEFER); } + if (qcom_ice_create_error) + return ERR_PTR(-EOPNOTSUPP); + ice = platform_get_drvdata(pdev); if (!ice) { dev_err(dev, "Cannot get ice instance from %s\n", @@ -336,8 +341,10 @@ static int qcom_ice_probe(struct platform_device *pdev) } engine = qcom_ice_create(&pdev->dev, base); - if (IS_ERR(engine)) + if (IS_ERR(engine)) { + qcom_ice_create_error = true; return PTR_ERR(engine); + } platform_set_drvdata(pdev, engine);
When the ICE key programming interface is unavailable, the ice create function fails, causing the probe to set NULL as the driver data. As a result, when the UFS driver reads the ICE driver data and encounters a NULL, leading to the deferral of the UFS probe and preventing the device from booting to the shell. To address this issue, modify the behavior to return an "operation not supported" error when the ICE key programming interface is unavailable. Additionally, mark this error in a global variable. When the UFS driver attempts to read the ICE driver data, it will check for this error and return it, rather than deferring the probe. Signed-off-by: Yuvaraj Ranganathan <quic_yrangana@quicinc.com> --- drivers/soc/qcom/ice.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)