Re: [PATCH V2 1/2] ufs: core: Configure only active lanes during link

[Palash Kambar]

On 3/27/2026 3:01 PM, Shawn Lin wrote:
> Hi Palash
> 
> 在 2026/03/27 星期五 17:03, palash.kambar@xxxxxxxxxxxxxxxx 写道:
>> From: Palash Kambar <palash.kambar@xxxxxxxxxxxxxxxx>
>>
>> The number of connected lanes detected during UFS link startup can be
>> fewer than the lanes specified in the device tree. The current driver
>> logic attempts to configure all lanes defined in the device tree,
>> regardless of their actual availability. This mismatch may cause
>> failures during power mode changes.
>>
>> Hence, add check to identify only the lanes that were successfully
>> discovered during link startup, to warn on power mode change errors
>> caused by mismatched lane counts.
> 
> The logic of your patch is clear, but I believe there is a slight
> inconsistency between the commit message and the current code
> implementation. The patch currently returns -ENOLINK immediately when a
> lane mismatch is detected. This causes the Link Startup process to
> terminate instantly, preventing the UFS device from completing
> initialization. Consequently, ufshcd_change_power_mode() will never be
> executed, there is nothing about warning on power mode change errors.
> 
> How about "to prevents potential failures in subsequent power mode
> changes by failing the initialization early"  or something similart?
> 

Sure Shawn, will update the commit text.

>>
>> Signed-off-by: Palash Kambar <palash.kambar@xxxxxxxxxxxxxxxx>
>> ---
>>   drivers/ufs/core/ufshcd.c | 39 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index 31950fc51a4c..cc291cae79f0 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -5035,6 +5035,40 @@ void ufshcd_update_evt_hist(struct ufs_hba *hba, u32 id, u32 val)
>>   }
>>   EXPORT_SYMBOL_GPL(ufshcd_update_evt_hist);
>>   +static int ufshcd_validate_link_params(struct ufs_hba *hba)
>> +{
>> +    int ret = 0;
>> +    int val = 0;
>> +
>> +    ret = ufshcd_dme_get(hba,
>> +                 UIC_ARG_MIB(PA_CONNECTEDTXDATALANES), &val);
>> +    if (ret)
>> +        goto out;
>> +
>> +    if (val != hba->lanes_per_direction) {
>> +        dev_err(hba->dev, "Tx lane mismatch [config,reported] [%d,%d]\n",
>> +            hba->lanes_per_direction, val);
>> +        ret = -ENOLINK;
>> +        goto out;
>> +    }
>> +
>> +    val = 0;
>> +
> 
> ufshcd_dme_get() returns 0 on success, non-zero value on failure.
> Perhaps you could remove this "val = 0".
>

Hi Shawn, the "val" used here holds the value of attribute returned
I have used "ret" to hold the return from ufshcd_dme_get()

 
>> +    ret = ufshcd_dme_get(hba,
>> +                 UIC_ARG_MIB(PA_CONNECTEDRXDATALANES), &val);
>> +    if (ret)
>> +        goto out;
>> +
>> +    if (val != hba->lanes_per_direction) {
>> +        dev_err(hba->dev, "Rx lane mismatch [config,reported] [%d,%d]\n",
>> +            hba->lanes_per_direction, val);
>> +        ret = -ENOLINK;
>> +    }
>> +
>> +out:
>> +    return ret;
>> +}
>> +
>>   /**
>>    * ufshcd_link_startup - Initialize unipro link startup
>>    * @hba: per adapter instance
>> @@ -5108,6 +5142,11 @@ static int ufshcd_link_startup(struct ufs_hba *hba)
>>               goto out;
>>       }
>>   +    /* Check successfully detected lanes */
>> +    ret = ufshcd_validate_link_params(hba);
>> +    if (ret)
>> +        goto out;
>> +
>>       /* Include any host controller configuration via UIC commands */
>>       ret = ufshcd_vops_link_startup_notify(hba, POST_CHANGE);
>>       if (ret)