lkml.org 
[lkml]   [2012]   [May]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v5 2/2] mmc: core: Support packed command for eMMC4.5 device
Date
Maya Erez <merez@codeaurora.org> wrote:
> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of
> merez@codeaurora.org
> Sent: Thursday, May 03, 2012 3:35 AM
> To: Seungwon Jeon
> Cc: merez@codeaurora.org; linux-mmc@vger.kernel.org; 'Chris Ball'; linux-kernel@vger.kernel.org
> 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html



\
 
 \ /
  Last update: 2012-05-09 02:01    [W:0.053 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site