RE: [PATCH v5 4/4] ufs: core: Remove ufshcd_map_desc_id_to_length function

[Arthur Simchaev]

> Should a check be added here that desc_buf[QUERY_DESC_LENGTH_OFFSET]
> <= QUERY_DESC_MAX_SIZE to protect against firmware bugs? Since
> QUERY_DESC_MAX_SIZE == 255, adding BUILD_BUG_ON(QUERY_DESC_SIZE !=
> 255)
> and a comment may be sufficient.

Sorry - my bad.
In the previous review you mentioned that the patch looks good to you, hence the "Review by".
Regarding your comment, we can do that, although I don't think we should cover for those FW basic bugs.
Please let me know that you prefer.

Regards
Arthur

> -----Original Message-----
> From: Bart Van Assche <bvanassche@xxxxxxx>
> Sent: Monday, December 12, 2022 2:19 AM
> To: Arthur Simchaev <Arthur.Simchaev@xxxxxxx>;
> martin.petersen@xxxxxxxxxx
> Cc: beanhuo@xxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v5 4/4] ufs: core: Remove ufshcd_map_desc_id_to_length
> function
> 
> CAUTION: This email originated from outside of Western Digital. Do not click
> on links or open attachments unless you recognize the sender and know that
> the content is safe.
> 
> 
> On 12/11/22 05:05, Arthur Simchaev wrote:
> > There shouldn't be any restriction of the descriptor size
> > (not the descriptor id for that matter) up to QUERY_DESC_MAX_SIZE.
> > According to the spec, the caller can use any descriptor size,
> > and it is up to the device to return the actual size.
> > Therefore there shouldn't be any sizes hardcoded in the kernel,
> > nor any need to cache it, hence ufshcd_map_desc_id_to_length function is
> > redundant. Always read the descriptors with QUERY_DESC_MAX_SIZE size.
> >
> > Reviewed-by: Bart Van Assche <bvanassche@xxxxxxx>
> 
> I have not yet replied with "Reviewed-by" to this patch so you are not
> yet allowed to add my Reviewed-by tag to this patch.
> 
> > +     /* Update descriptor length */
> > +     buff_len = desc_buf[QUERY_DESC_LENGTH_OFFSET];
> 
> Should a check be added here that desc_buf[QUERY_DESC_LENGTH_OFFSET]
> <= QUERY_DESC_MAX_SIZE to protect against firmware bugs? Since
> QUERY_DESC_MAX_SIZE == 255, adding BUILD_BUG_ON(QUERY_DESC_SIZE !=
> 255)
> and a comment may be sufficient.
> 
> Thanks,
> 
> Bart.