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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bdc420e4-3f2e-cf52-eb42-f85e747b2fb1@acm.org>
Date:   Wed, 10 Jun 2020 18:37:18 -0700
From:   Bart Van Assche <bvanassche@....org>
To:     daejun7.park@...sung.com, ALIM AKHTAR <alim.akhtar@...sung.com>,
        "avri.altman@....com" <avri.altman@....com>,
        "jejb@...ux.ibm.com" <jejb@...ux.ibm.com>,
        "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
        "asutoshd@...eaurora.org" <asutoshd@...eaurora.org>,
        "beanhuo@...ron.com" <beanhuo@...ron.com>,
        "stanley.chu@...iatek.com" <stanley.chu@...iatek.com>,
        "cang@...eaurora.org" <cang@...eaurora.org>,
        "tomas.winkler@...el.com" <tomas.winkler@...el.com>
Cc:     "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Sang-yoon Oh <sangyoon.oh@...sung.com>,
        Sung-Jun Park <sungjun07.park@...sung.com>,
        yongmyung lee <ymhungry.lee@...sung.com>,
        Jinyoung CHOI <j-young.choi@...sung.com>,
        Adel Choi <adel.choi@...sung.com>,
        BoRam Shin <boram.shin@...sung.com>
Subject: Re: [RFC PATCH 5/5] scsi: ufs: Prepare HPB read for cached sub-region

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.