Re: [RFC PATCH 5/5] scsi: ufs: Prepare HPB read for cached sub-region

[Bart Van Assche <bvanassche@acm.org>]

On 2020-06-04 19:12, Daejun Park wrote:
> +static inline bool ufshpb_is_read_cmd(struct scsi_cmnd *cmd)
> +{
> +	if (cmd->cmnd[0] == READ_10 || cmd->cmnd[0] == READ_16)
> +		return true;
> +
> +	return false;
> +}

Has it been considered to check req_op(cmd->request) instead of checking
the SCSI CDB?

> +static inline bool ufshpb_is_write_discard_cmd(struct scsi_cmnd *cmd)
> +{
> +	if (cmd->cmnd[0] == WRITE_10 || cmd->cmnd[0] == WRITE_16 ||
> +	    cmd->cmnd[0] == UNMAP)
> +		return true;
> +
> +	return false;
> +}

Does the above code depend on how the sd driver translates write and
discard requests? Do UFS devices support the WRITE SAME SCSI command?
Has it been considered to check req_op(cmd->request) instead of checking
the SCSI CDB?

> +static inline bool ufshpb_is_support_chunk(int transfer_len)
> +{
> +	return transfer_len <= HPB_MULTI_CHUNK_HIGH;
> +}

The names used in the above function are mysterious. What is a support
chunk? What does "multi chunk high" mean? Please add a comment.

> +static inline u32 ufshpb_get_lpn(struct scsi_cmnd *cmnd)
> +{
> +	return blk_rq_pos(cmnd->request) >>
> +		(ilog2(cmnd->device->sector_size) - 9);
> +}
>
> +static inline unsigned int ufshpb_get_len(struct scsi_cmnd *cmnd)
> +{
> +	return blk_rq_sectors(cmnd->request) >>
> +		(ilog2(cmnd->device->sector_size) - 9);
> +}

Do the above two functions perhaps each include a duplicate of
sectors_to_logical()?

> +/* routine : READ10 -> HPB_READ  */
> +static void ufshpb_prep_fn(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
> +{
> +	struct ufshpb_lu *hpb;
> +	struct ufshpb_region *rgn;
> +	struct ufshpb_subregion *srgn;
> +	struct scsi_cmnd *cmd = lrbp->cmd;
> +	u32 lpn;
> +	u64 ppn;
> +	unsigned long flags;
> +	int transfer_len, rgn_idx, srgn_idx, srgn_offset;
> +	int err = 0;
> +
> +	hpb = ufshpb_get_hpb_data(cmd);
> +	err = ufshpb_lu_get(hpb);
> +	if (unlikely(err))
> +		return;
> +
> +	WARN_ON(hpb->lun != cmd->device->lun);
        ^^^^^^^
        WARN_ON_ONCE()?

> +	if (!ufshpb_is_write_discard_cmd(cmd) &&
> +	    !ufshpb_is_read_cmd(cmd))
> +		goto put_hpb;
> +
> +	transfer_len = ufshpb_get_len(cmd);
> +	if (unlikely(!transfer_len))
> +		goto put_hpb;
> +
> +	lpn = ufshpb_get_lpn(cmd);
> +	ufshpb_get_pos_from_lpn(hpb, lpn, &rgn_idx, &srgn_idx, &srgn_offset);
> +	rgn = hpb->rgn_tbl + rgn_idx;
> +	srgn = rgn->srgn_tbl + srgn_idx;
> +
> +	/* If commnad type is WRITE and DISCARD, set bitmap as drity */
              ^^^^^^^               ^^^                        ^^^^^
              command?              or?                        dirty?
> +	if (ufshpb_is_write_discard_cmd(cmd)) {
> +		spin_lock_irqsave(&hpb->hpb_state_lock, flags);
> +		ufshpb_set_ppn_dirty(hpb, rgn_idx, srgn_idx, srgn_offset,
> +				 transfer_len);
> +		spin_unlock_irqrestore(&hpb->hpb_state_lock, flags);
> +		goto put_hpb;
> +	}
> +
> +	WARN_ON(!ufshpb_is_read_cmd(cmd));
        ^^^^^^^
        WARN_ON_ONCE()?

> +	if (!ufshpb_is_support_chunk(transfer_len))
> +		goto put_hpb;
> +
> +	spin_lock_irqsave(&hpb->hpb_state_lock, flags);
> +	if (ufshpb_test_ppn_dirty(hpb, rgn_idx, srgn_idx, srgn_offset,
> +				   transfer_len)) {
> +		atomic_inc(&hpb->stats.miss_cnt);
> +		spin_unlock_irqrestore(&hpb->hpb_state_lock, flags);
> +		goto put_hpb;
> +	}
> +
> +	ppn = ufshpb_get_ppn(hpb, srgn->mctx, srgn_offset, &err);
> +	spin_unlock_irqrestore(&hpb->hpb_state_lock, flags);
> +	if (unlikely(err)) {
> +		/*
> +		 * In this case, the region state is active,
> +		 * but the ppn table is not allocated.
> +		 * Make sure that ppn tabie must be allocated on
                                      ^^^^^
                                      table?
> +		 * active state
> +		 */
> +		WARN_ON(true);
> +		dev_err(&hpb->hpb_lu_dev,
> +			"ufshpb_get_ppn failed. err %d\n", err);
> +		goto put_hpb;
> +	}
> +
> +	ufshpb_set_hpb_read_to_upiu(hpb, lrbp, lpn, ppn, transfer_len);
> +
> +	atomic_inc(&hpb->stats.hit_cnt);
> +put_hpb:
> +	ufshpb_lu_put(hpb);
> +}

Thanks,

Bart.