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
