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

[Seungwon Jeon]

Maya Erez <merez@xxxxxxxxxxxxxx> wrote:
> -----Original Message-----
> From: linux-mmc-owner@xxxxxxxxxxxxxxx [mailto:linux-mmc-owner@xxxxxxxxxxxxxxx] On Behalf Of
> merez@xxxxxxxxxxxxxx
> Sent: Thursday, May 03, 2012 3:35 AM
> To: Seungwon Jeon
> Cc: merez@xxxxxxxxxxxxxx; linux-mmc@xxxxxxxxxxxxxxx; 'Chris Ball'; linux-kernel@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH v5 2/2] mmc: core: Support packed command for eMMC4.5 device
> 
> >> > @@ -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.
> 
> The loop must be taken since we are inside the case of packed commands so
> the list can't be empty.
> 
> If the compiler complained, you can set prq to be the first request before
> entering the loop instead of setting it again in the if that follows the
> loop. It will probably be more understood.
> If you decide to leave it as is, I would also add the following to the if:
> +                                                mq_rq->req = prq;
> +                                                ret = 1;
> Otherwise it seems like there could be a bug in cases where the loop is
> not taken (since prq is the only one that is set) and the code is less
> understood.
I'll clarify this as you concern.
Thank you for your review.

Best regards,
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/