linux-kernel - Re: [PATCH v2 5/5] scsi: ufs: UFS Host Performance Booster(HPB) driver

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: <e53f3b2c-37ec-2bab-83ff-702dd3b2b813@gmail.com>
Date:   Mon, 27 Apr 2020 00:03:59 +0200
From:   Bean Huo <huobean@...il.com>
To:     Bart Van Assche <bvanassche@....org>, alim.akhtar@...sung.com,
        avri.altman@....com, asutoshd@...eaurora.org, jejb@...ux.ibm.com,
        martin.petersen@...cle.com, stanley.chu@...iatek.com,
        beanhuo@...ron.com, tomas.winkler@...el.com, cang@...eaurora.org
Cc:     linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 5/5] scsi: ufs: UFS Host Performance Booster(HPB)
 driver


On 25.04.20 20:07, Bart Van Assche wrote:
> On 2020-04-16 13:31, huobean@...il.com wrote:
>> +static int ufshpb_execute_cmd(struct ufshpb_lu *hpb, unsigned char *cmd)
>> +{
>> +	struct scsi_sense_hdr sshdr;
>> +	struct scsi_device *sdp;
>> +	struct ufs_hba *hba;
>> +	int retries;
>> +	int ret = 0;
>> +
>> +	hba = hpb->hba;
>> +
>> +	sdp = hba->sdev_ufs_lu[hpb->lun];
>> +	if (!sdp) {
>> +		hpb_warn("%s UFSHPB cannot find scsi_device\n",  __func__);
>> +		return -ENODEV;
>> +	}
>> +
>> +	ret = scsi_device_get(sdp);
>> +	if (!ret && !scsi_device_online(sdp)) {
>> +		ret = -ENODEV;
>> +		scsi_device_put(sdp);
>> +		return ret;
>> +	}
>> +
>> +	for (retries = 3; retries > 0; --retries) {
>> +		ret = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
>> +				   msecs_to_jiffies(30000), 3, 0, RQF_PM, NULL);
>> +		if (ret == 0)
>> +			break;
>> +	}
>> +
>> +	if (ret) {
>> +		if (driver_byte(ret) == DRIVER_SENSE &&
>> +		    scsi_sense_valid(&sshdr)) {
>> +			switch (sshdr.sense_key) {
>> +			case ILLEGAL_REQUEST:
>> +				hpb_err("ILLEGAL REQUEST asc=0x%x ascq=0x%x\n",
>> +					sshdr.asc, sshdr.ascq);
>> +				break;
>> +			default:
>> +				hpb_err("Unknown return code = %x\n", ret);
>> +				break;
>> +			}
>> +		}
>> +	}
>> +	scsi_device_put(sdp);
>> +
>> +	return ret;
>> +}
> If scsi_execute() would be changed into asynchronous SCSI command
> submission, can ufshpb_execute_cmd() be called from inside the UFS
> .queue_rq() callback instead of from workqueue context?
>
> The scsi_device_get() call looks misplaced. I think that call should
> happen before schedule_work() is called.
>
>> +static int ufshpb_l2p_load_req(struct ufshpb_lu *hpb, struct request_queue *q,
>> +			       struct ufshpb_map_req *map_req)
>> +{
>> +	struct scsi_request *scsi_rq;
>> +	unsigned char cmd[16] = { };
>> +	struct request *req;
>> +	struct bio *bio;
>> +	int alloc_len;
>> +	int ret;
>> +
>> +	bio = &map_req->bio;
>> +
>> +	ret = ufshpb_add_bio_page(hpb, q, bio, map_req->bvec, map_req->mctx);
>> +	if (ret) {
>> +		hpb_err("ufshpb_add_bio_page() failed with ret %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	alloc_len = hpb->hba->hpb_geo.subregion_entry_sz;
>> +	/*
>> +	 * HPB Sub-Regions are equally sized except for the last one which is
>> +	 * smaller if the last hpb Region is not an integer multiple of
>> +	 * bHPBSubRegionSize.
>> +	 */
>> +	if (map_req->region == (hpb->lu_region_cnt - 1) &&
>> +	    map_req->subregion == (hpb->subregions_in_last_region - 1))
>> +		alloc_len = hpb->last_subregion_entry_size;
>> +
>> +	ufshpb_prepare_read_buf_cmd(cmd, map_req->region, map_req->subregion,
>> +				    alloc_len);
>> +	if (!map_req->req) {
>> +		map_req->req = blk_get_request(q, REQ_OP_SCSI_IN, 0);
>> +		if (IS_ERR(map_req->req))
>> +			return PTR_ERR(map_req->req);
>> +	}
>> +	req = map_req->req;
>> +	scsi_rq = scsi_req(req);
>> +
>> +	blk_rq_append_bio(req, &bio);
>> +
>> +	scsi_req_init(scsi_rq);
>> +
>> +	scsi_rq->cmd_len = COMMAND_SIZE(cmd[0]);
>> +	memcpy(scsi_rq->cmd, cmd, scsi_rq->cmd_len);
>> +	req->timeout = msecs_to_jiffies(30000);
>> +	req->end_io_data = (void *)map_req;
>> +
>> +	hpb_dbg(SCHEDULE_INFO, hpb, "ISSUE READ_BUFFER : (%d-%d) retry = %d\n",
>> +		map_req->region, map_req->subregion, map_req->retry_cnt);
>> +	hpb_trace(hpb, "%s: I RB %d - %d", DRIVER_NAME, map_req->region,
>> +		  map_req->subregion);
>> +
>> +	blk_execute_rq_nowait(q, NULL, req, 1, ufshpb_l2p_load_req_done);
>> +	map_req->req_issue_t = ktime_to_ns(ktime_get());
>> +
>> +	atomic64_inc(&hpb->status.map_req_cnt);
>> +
>> +	return 0;
>> +}
> Same question here: if 'req' would be submitted asynchronously, can
> ufshpb_l2p_load_req() be called directly from inside the UFS .queue_rq()
> callback instead of from workqueue context?
>
> Thanks,
>
> Bart.

Hi, Bart
Thanks very much.
If I understood the Christoph's the second question correctly. Enqueue 
HPB requests to the
scsi_device->request_queueu, and fly back to SCSI schedule, it's 
unacceptable. I don't like this
implementation way either. Also, the latency of the L2P table update is 
higher. I will change it
in the RFC patch.

thanks,
Bean