linux-kernel - RE: [PATCH v5 2/2] mmc: core: Support packed command for eMMC4.5 device

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]

Date:	Mon, 30 Apr 2012 09:31:09 +0900
From:	Seungwon Jeon <tgih.jun@...sung.com>
To:	merez@...eaurora.org
Cc:	linux-mmc@...r.kernel.org, 'Chris Ball' <cjb@...top.org>,
	linux-kernel@...r.kernel.org
Subject: RE: [PATCH v5 2/2] mmc: core: Support packed command for eMMC4.5 device

Hi Maya,

Maya Erez <merez@...eaurora.org> wrote:
> 
> Hi Jeon,
> 
> Any update for splitting between the read and write packing?
I'll work soon.
> I also have a few more comments:
> 
> > +static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct request
> *req)
> > +{
> > +	struct request_queue *q = mq->queue;
> > +	struct mmc_card *card = mq->card;
> > +	struct request *cur = req, *next = NULL;
> > +	struct mmc_blk_data *md = mq->data;
> > +	bool en_rel_wr = card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN;
> +	unsigned int req_sectors = 0, phys_segments = 0;
> > +	unsigned int max_blk_count, max_phys_segs;
> > +	u8 put_back = 0;
> > +	u8 max_packed_rw = 0;
> > +	u8 reqs = 0;
> > +
> > +	mq->mqrq_cur->packed_num = MMC_PACKED_N_ZERO;
> > +
> > +	if (!(md->flags & MMC_BLK_CMD23) ||
> > +			!card->ext_csd.packed_event_en)
> > +		goto no_packed;
> > +
> > +	if ((rq_data_dir(cur) == READ) &&
> > +			(card->host->caps2 & MMC_CAP2_PACKED_RD))
> > +		max_packed_rw = card->ext_csd.max_packed_reads;
> > +	else if ((rq_data_dir(cur) == WRITE) &&
> > +			(card->host->caps2 & MMC_CAP2_PACKED_WR))
> > +		max_packed_rw = card->ext_csd.max_packed_writes;
> > +
> > +	if (max_packed_rw == 0)
> > +		goto no_packed;
> > +
> > +	if (mmc_req_rel_wr(cur) &&
> > +			(md->flags & MMC_BLK_REL_WR) &&
> > +			!en_rel_wr) {
> > +		goto no_packed;
> > +	}
> > +
> > +	max_blk_count = min(card->host->max_blk_count,
> > +			card->host->max_req_size >> 9);
> > +	if (unlikely(max_blk_count > 0xffff))
> > +		max_blk_count = 0xffff;
> > +
> > +	max_phys_segs = queue_max_segments(q);
> > +	req_sectors += blk_rq_sectors(cur);
> > +	phys_segments += req->nr_phys_segments;
> It would be best to change req to cur. This is the only place you use req,
> in all other places you refer to cur.
Good point.
> 
> > @@ -1291,10 +1657,42 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue
> *mq, struct request *rqc)
> >  			 * A block was successfully transferred.
> >  			 */
> >  			mmc_blk_reset_success(md, type);
> > -			spin_lock_irq(&md->lock);
> > -			ret = __blk_end_request(req, 0,
> > +
> > +			if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
> > +				int idx = mq_rq->packed_fail_idx, i = 0;
> > +				ret = 0;
> > +				while (!list_empty(&mq_rq->packed_list)) {
> > +					prq = list_entry_rq(
> > +						mq_rq->packed_list.next);
> > +					if (idx == i) {
> > +						/* retry from error index */
> > +						mq_rq->packed_num -= idx;
> > +						mq_rq->req = prq;
> > +						ret = 1;
> > +						break;
> > +					}
> > +					list_del_init(&prq->queuelist);
> > +					spin_lock_irq(&md->lock);
> > +					__blk_end_request(prq, 0,
> > +							blk_rq_bytes(prq));
> > +					spin_unlock_irq(&md->lock);
> > +					i++;
> > +				}
> > +				if (mq_rq->packed_num == MMC_PACKED_N_SINGLE) {
> > +					prq = list_entry_rq(
> > +						mq_rq->packed_list.next);
> You already get the prq inside the while. There is no need to do it again.
Right, but if while loop isn't taken, then prq can be used uninitialized.
Though that case wouldn't happen actually, we don't want to see the compiling error.
> 
> 
> > @@ -1329,6 +1727,8 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue
> *mq,
> > struct request *rqc)
> >  				break;
> >  			if (err == -ENODEV)
> >  				goto cmd_abort;
> > +			if (mq_rq->packed_cmd != MMC_PACKED_NONE)
> > +				break;
> This can cause an endless loop in case of MMC_BLK_DATA_ERR. The same
> packed command will be sent over and over again without a beaking point.
Yes. It may be possible in case of twice MMC_BLK_DATA_ERR.

Thanks
Seungwon Jeon.
> 
> Thanks,
> Maya Erez
> Consultant for Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
> 
> 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/