Re: [PATCH v1] soc: qcom: ice: Prevent UFS probe deferral on ICE probe failure

[Yuvaraj Ranganathan <quic_yrangana@quicinc.com>]

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.