> > + lrbp->command_type = hba->ufs_version == UFSHCI_VERSION_10 ||
> > + hba->ufs_version == UFSHCI_VERSION_11 ?
> > + UTP_CMD_TYPE_DEV_MANAGE :
> > + UTP_CMD_TYPE_UFS_STORAGE;
>
> I think a good old if/self or even a switch statement would be more
> descriptive here:
>
> switch (hba->ufs_version) {
> case UFSHCI_VERSION_10:
> case UFSHCI_VERSION_11:
> lrbp->command_type = UTP_CMD_TYPE_DEV_MANAGE;
> break;
> default:
> lrbp->command_type = UTP_CMD_TYPE_UFS_STORAGE;
> break;
> }
>
Done.
> > + /* just copy the upiu request as it is */
> > + memcpy(lrbp->ucd_req_ptr, req_upiu,
> GENERAL_UPIU_REQUEST_SIZE);
> > +
> > + if (desc_buff && rw == WRITE) {
> > + descp = (u8 *)lrbp->ucd_req_ptr +
> GENERAL_UPIU_REQUEST_SIZE;
> > + memcpy(descp, desc_buff, *buff_len);
> > + *buff_len = 0;
> > + }
>
> The GENERAL_UPIU_REQUEST_SIZE usage here looks odd. Shouldn't we use
> data structure sizes an pointer arithmetics? E.g.
>
> memcpy(lrbp->ucd_req_ptr, req_upiu, sizeof(*lrbp->ucd_req_ptr));
> if (desc_buff && rw == WRITE) {
> memcpy(lrbp->ucd_req_ptr + 1, desc_buff, *buff_len);
> *buff_len = 0;
> }
>
> preferably with a comment explaining the weird overallocation of
> ucd_req_ptr over the declared data type in the existing driver.
Done.
This is also done elsewhere so will fix those as well in a different patch.
>
> > +
> > + hba->dev_cmd.complete = &wait;
> > +
> > + /* Make sure descriptors are ready before ringing the doorbell */
> > + wmb();
> > + spin_lock_irqsave(hba->host->host_lock, flags);
> > + ufshcd_send_command(hba, tag);
>
> I think the above wmb() is taken care of by the writel() inside
> ufshcd_send_command.
Those barriers were added later (and the one in send_command() last),
explaining that there is a need to verify that the "..descriptors are
actually written to memory before ringing the doorbell..."
See for more details:
ad1a1b9 scsi: ufs: commit descriptors before setting the doorbell
e3dfdc5 scsi: ufs: commit descriptors before setting the doorbell
897efe6 scsi: ufs: add missing memory barriers
>
> > + /* just copy the upiu response as it is */
> > + memcpy(rsp_upiu, lrbp->ucd_rsp_ptr,
> GENERAL_UPIU_REQUEST_SIZE);
>
> sizeof again.
Done.
