Re: [PATCH v2 2/2] scsi: ufs: core: Add a sysfs entry for ufshcd_state

[Can Guo <can.guo@oss.qualcomm.com>]

Hi Bart,

On 2/26/2026 2:59 AM, Bart Van Assche wrote:
> On 2/24/26 6:29 PM, Can Guo wrote:
>> +What: /sys/bus/platform/drivers/ufshcd/*/ufshcd_state
>> +What:        /sys/bus/platform/devices/*.ufs/ufshcd_state
>> +Date:        February 2026
>> +Contact:    Can Guo <can.guo@oss.qualcomm.com>
>> +Description:
>> +        This attribute shows the state of ufshcd.
>> +
>> +        The attribute is read only.
>
> Please expand "state of ufshcd", e.g. into "state of the UFS host 
> controller driver".
>
>> +static const char * const ufshcd_states[] = {
>> +    [UFSHCD_STATE_RESET]            = "reset",
>> +    [UFSHCD_STATE_OPERATIONAL]        = "operational",
>> +    [UFSHCD_STATE_EH_SCHEDULED_NON_FATAL]    = 
>> "eh_scheduled_non_fatal",
>> +    [UFSHCD_STATE_EH_SCHEDULED_FATAL]    = "eh_scheduled_fatal",
>> +    [UFSHCD_STATE_ERROR]            = "error",
>> +};
>
> Please follow the kernel coding style with regard to spaces around "*".
>
>> +static ssize_t ufshcd_state_show(struct device *dev,
>> +                 struct device_attribute *attr, char *buf)
>> +{
>> +    struct ufs_hba *hba = dev_get_drvdata(dev);
>> +
>> +    return sysfs_emit(buf, "%s\n", ufshcd_states[hba->ufshcd_state]);
>> +}
>
> In the above function, please check that hba->ufshcd_state does not 
> exceed the bounds of the ufshcd_states[] array and also that
> ufshcd_states[hba->ufshcd_state] is not NULL.
>
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index c6c7de7a0603..32a508e1582e 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -7917,6 +7917,8 @@ static void ufshcd_process_probe_result(struct 
>> ufs_hba *hba,
>>           hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
>>       spin_unlock_irqrestore(hba->host->host_lock, flags);
>>   +    sysfs_notify(&hba->dev->kobj, NULL, "ufshcd_state");
>> +
>>       trace_ufshcd_init(hba, ret,
>>                 ktime_to_us(ktime_sub(ktime_get(), probe_start)),
>>                 hba->curr_dev_pwr_mode, hba->uic_link_state);
>
> Shouldn't there be one sysfs_notify(&hba->dev->kobj, NULL, 
> "ufshcd_state") call after every hba->ufshcd_state change?
Thanks for your review. My first thinking was to indicate to userspace
that DME QoS monitor has been reset by host. But on second thought, it
would be much simpler if I just use Bit[0] in dme_qos_nofitication
attribute to communicate that information to userspace, because DME QoS
events are mapped to Bit[3:1], meaning Bit[0] is free anyways. I am
dropping this change in next version.

Thanks,
Can Guo.
>
> Thanks,
>
> Bart.