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

[Seungwon Jeon]

Hi Maya,

Maya Erez <merez@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/