linux-kernel - Re: [PATCH v5] ufs: core: Add HID support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7e034db7e46a33287f59159401affd58a81abad6.camel@gmail.com>
Date: Wed, 21 May 2025 16:56:50 +0200
From: Bean Huo <huobean@...il.com>
To: Huan Tang <tanghuan@...o.com>, bvanassche@....org
Cc: James.Bottomley@...senPartnership.com, alim.akhtar@...sung.com, 
 angelogioacchino.delregno@...labora.com, avri.altman@....com, 
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
 linux-mediatek@...ts.infradead.org, linux-scsi@...r.kernel.org, 
 luhongfei@...o.com, manivannan.sadhasivam@...aro.org,
 martin.petersen@...cle.com,  matthias.bgg@...il.com,
 opensource.kernel@...o.com, peter.wang@...iatek.com, 
 quic_nguyenb@...cinc.com, wenxing.cheng@...o.com
Subject: Re: [PATCH v5] ufs: core: Add HID support

On Wed, 2025-05-21 at 22:32 +0800, Huan Tang wrote:
> > > > +static const char *ufs_hid_state_to_string(enum ufs_hid_state state)
> > > > +{
> > > > +       switch (state) {
> > > > +       case HID_IDLE:
> > > > +               return "idle";
> > > > +       case ANALYSIS_IN_PROGRESS:
> > > > +               return "analysis_in_progress";
> > > > +       case DEFRAG_REQUIRED:
> > > > +               return "defrag_required";
> > > > +       case DEFRAG_IN_PROGRESS:
> > > > +               return "defrag_in_progress";
> > > > +       case DEFRAG_COMPLETED:
> > > > +               return "defrag_completed";
> > > > +       case DEFRAG_NOT_REQUIRED:
> > > > +               return "defrag_not_required";
> > > > +       default:
> > > > +               return "unknown";
> > > > +       }
> > > > +}
> > > 
> > > The enum ufs_hid_state values are contiguous and start from 0, maybe change switch-state to :
> > > 
> > > #define NUM_UFS_HID_STATES 6
> > > static const char *ufs_hid_states[NUM_UFS_HID_STATES] = {
> > >     "idle",
> > >      "analysis_in_progress",
> > >      "defrag_required",
> > >      "defrag_in_progress",
> > >      "defrag_completed",
> > >      "defrag_not_required"
> > > };
> 
> > If this change is made, please use the designated initializer syntax
> > ([label] = "text"). This will make it easier to verify that the
> > enumeration labels and the strings match.
> 
> Hi bart and bean sir,
> 
> Thank you for your comments and guidance！
> 
> What do you think of the following changes?
> 
> ufs.h
>  +/* bHIDState attribute values */                                               
>  +enum ufs_hid_state {                                                           
>  +       HID_IDLE                = 0,                                            
>  +       ANALYSIS_IN_PROGRESS    = 1,                                            
>  +       DEFRAG_REQUIRED         = 2,                                            
>  +       DEFRAG_IN_PROGRESS      = 3,                                            
>  +       DEFRAG_COMPLETED        = 4,                                            
>  +       DEFRAG_NOT_REQUIRED     = 5,                                            
>  +       NUM_UFS_HID_STATES      = 6,                                            
>  +}; 
> 
> ufs-sysfs.c
>  +static const char * const ufs_hid_states[] = {                                 
>  +       [HID_IDLE]              = "idle",                                       
>  +       [ANALYSIS_IN_PROGRESS]  = "analysis_in_progress",                       
>  +       [DEFRAG_REQUIRED]       = "defrag_required",                            
>  +       [DEFRAG_IN_PROGRESS]    = "defrag_in_progress",                         
>  +       [DEFRAG_COMPLETED]      = "defrag_completed",                           
>  +       [DEFRAG_NOT_REQUIRED]   = "defrag_not_required",                        
>  +};                                                                             
>  +                                                                               
>  +static const char *ufs_hid_state_to_string(enum ufs_hid_state state)           
>  +{                                                                              
>  +       if (state < NUM_UFS_HID_STATES)                                         
>  +               return ufs_hid_states[state];                                   
>  +                                                                               
>  +       return "unknown";                                                       
>  +} 
> 
> ...
> 
> +static ssize_t state_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       struct ufs_hba *hba = dev_get_drvdata(dev);
> +       u32 value;
> +       int ret;
> +
> +       ret = hid_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +                       QUERY_ATTR_IDN_HID_STATE, &value);
> +       if (ret)
> +               return ret;
> +
> +       return sysfs_emit(buf, "%s\n", ufs_hid_state_to_string(value));
> +}
> 
> Thanks
> Huan
> 
> 
> 


looks good to me, with this change, please add my reviewed-by tag in the next version.

Kind regards,
Bean