RE: [PATCH v1 1/2] scsi: ufs: Support WriteBooster on Samsung UFS devices

[Stanley Chu]

Hi Avri,

On Sat, 2020-05-30 at 20:37 +0000, Avri Altman wrote:
> > @@ -2801,11 +2801,17 @@ int ufshcd_query_flag(struct ufs_hba *hba, enum
> > query_opcode opcode,
> >  {
> >         struct ufs_query_req *request = NULL;
> >         struct ufs_query_res *response = NULL;
> > -       int err, selector = 0;
> > +       int err;
> >         int timeout = QUERY_REQ_TIMEOUT;
> > +       u8 selector = 0;
> > 
> >         BUG_ON(!hba);
> > 
> > +       if (hba->dev_quirks & UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR) {
> > +               if (ufshcd_is_wb_flags(idn))
> > +                       selector = 1;
> > +       }
> > +
> Why not make the caller set the applicable selector,
> Instead of checking this for every flag?

This way have the minimum modification efforts and places compared to
other ways. However it looks a little wired because the selector control
is better assigned by users. I will submit next version with changing
the way selector assigned for comparison.

> 
> >         ufshcd_hold(hba, false);
> >         mutex_lock(&hba->dev_cmd.lock);
> >         ufshcd_init_query(hba, &request, &response, opcode, idn, index,
> > @@ -2882,6 +2888,11 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum
> > query_opcode opcode,
> >                 goto out;
> >         }
> > 
> > +       if (hba->dev_quirks & UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR) {
> > +               if (ufshcd_is_wb_attrs(idn))
> > +                       selector = 1;
> > +       }
> > +
> Same here
> 
> >         mutex_lock(&hba->dev_cmd.lock);
> >         ufshcd_init_query(hba, &request, &response, opcode, idn, index,
> >                         selector);
> > @@ -3042,6 +3053,11 @@ int ufshcd_query_descriptor_retry(struct ufs_hba
> > *hba,
> >         int err;
> >         int retries;
> > 
> > +       if (hba->dev_quirks & UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR) {
> > +               if (ufshcd_is_wb_desc(idn, index))
> > +                       selector = 1;
> > +       }
> > +
> And here.
> But this can't be true - 
> Are you setting the selector = 1 for reading any field for those descriptors?
> Shouldn't it be for the wb specific fields?

Yes, thanks for remind this.
I shall assign selector = 1 for WB related fields only in descriptors.

>  
> 
> >         for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) {
> >                 err = __ufshcd_query_descriptor(hba, opcode, idn, index,
> >                                                 selector, desc_buf, buf_len);
> > @@ -6907,8 +6923,10 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
> >         size_t buff_len;
> >         u8 model_index;
> >         u8 *desc_buf;
> > +       u8 retry_cnt = 0;
> >         struct ufs_dev_info *dev_info = &hba->dev_info;
> > 
> > +retry:
> >         buff_len = max_t(size_t, hba->desc_size.dev_desc,
> >                          QUERY_DESC_MAX_SIZE + 1);
> >         desc_buf = kmalloc(buff_len, GFP_KERNEL);
> > @@ -6948,6 +6966,29 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
> > 
> >         ufs_fixup_device_setup(hba);
> > 
> > +       if (!retry_cnt && (hba->dev_quirks &
> > +               UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR)) {
> If you only want to enter this clause once - you should use something other than retry_cnt,
> Which the reader expects to performs retries....

OK! I will fix this label by using another more comprehensible name.
> 
> Also, this is becoming too wired - 
> From your commit log I get that for specific Samsung devices,
> You need to query wb descriptor fields/attributes/flags using selectore = 1.
> But what it has to do with descriptor sizes?

Sorry to not mention clearly in the commit log.

Here driver needs to update the descriptor size to a "longer size" which
includes the "hidden WB related fields" which can be "found" by selector
= 1.

If descriptor size is not updated, any query can only get the fields
offset within current descriptor size even if selector = 1, and
out-of-boundary desc_buf[] access will happen in
ufshcd_read_desc_param().

PS. The check of "param_offset" to prevent possible out-of-boundary
desc_buf[] access can be patched as well.


Thanks,
Stanley Chu