From mboxrd@z Thu Jan 1 00:00:00 1970 From: Asutosh Das Subject: Re: [PATCH 2/5] mmc: queue: initialization of command-queue support Date: Fri, 16 Jan 2015 09:10:32 +0530 Message-ID: <54B88830.5010404@codeaurora.org> References: <20141202115728.GA27647@asutoshd-ics.in.qualcomm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from smtp.codeaurora.org ([198.145.11.231]:35731 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752962AbbAPDkh (ORCPT ); Thu, 15 Jan 2015 22:40:37 -0500 In-Reply-To: Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Ulf Hansson Cc: linux-mmc , "linux-arm-msm@vger.kernel.org" On 1/15/2015 4:07 PM, Ulf Hansson wrote: > On 2 December 2014 at 12:57, Asutosh Das wrote: >> Add command queue initialization support. This is >> applicable for emmc devices supporting cmd-queueing. >> >> Signed-off-by: Asutosh Das >> Signed-off-by: Konstantin Dorfman >> --- >> drivers/mmc/card/queue.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++ >> drivers/mmc/card/queue.h | 6 ++++ >> include/linux/mmc/card.h | 2 ++ >> include/linux/mmc/host.h | 1 + >> 4 files changed, 83 insertions(+) >> >> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c >> index 3e049c1..c99e385 100644 >> --- a/drivers/mmc/card/queue.c >> +++ b/drivers/mmc/card/queue.c >> @@ -403,6 +403,80 @@ void mmc_packed_clean(struct mmc_queue *mq) >> mqrq_prev->packed = NULL; >> } >> >> +static void mmc_cmdq_softirq_done(struct request *rq) >> +{ >> + struct mmc_queue *mq = rq->q->queuedata; >> + >> + mq->cmdq_complete_fn(rq); >> +} >> + >> +int mmc_cmdq_init(struct mmc_queue *mq, struct mmc_card *card) >> +{ >> + int i, ret = 0; >> + /* one slot is reserved for dcmd requests */ >> + int q_depth = card->ext_csd.cmdq_depth - 1; >> + >> + card->cmdq_init = false; >> + if (!(card->host->caps2 & MMC_CAP2_CMD_QUEUE)) { >> + ret = -ENOTSUPP; >> + goto out; >> + } > > I think it's preferable to do above check outside of this function. Thanks. True, will do that. > >> + >> + mq->mqrq_cmdq = kzalloc( >> + sizeof(struct mmc_queue_req) * q_depth, GFP_KERNEL); >> + if (!mq->mqrq_cmdq) { >> + pr_warn("%s: unable to allocate mqrq's for q_depth %d\n", >> + mmc_card_name(card), q_depth); > > No need to print this, done from kzalloc(); Thanks. Will remove it. > >> + ret = -ENOMEM; >> + goto out; >> + } >> + >> + for (i = 0; i < q_depth; i++) { >> + /* TODO: reduce the sg allocation by delaying them */ > > As I stated in my earlier reply in the $subject patchset, I will only > accept good quality code. > Have TODO comments will be out of the question. > >> + mq->mqrq_cmdq[i].sg = mmc_alloc_sg(card->host->max_segs, &ret); >> + if (ret) { >> + pr_warn("%s: unable to allocate cmdq sg of size %d\n", >> + mmc_card_name(card), >> + card->host->max_segs); > > No need to print this, done from kzalloc(); > >> + goto free_mqrq_sg; >> + } > > Hmm, it seems like a lot of pre-allocating is done here, so I > certainly think your TODO comment is valid. You just need to fix it. > :-) Thanks :). Will fix the pre-allocation in the next set. > >> + } >> + >> + ret = blk_queue_init_tags(mq->queue, q_depth, NULL); >> + if (ret) { >> + pr_warn("%s: unable to allocate cmdq tags %d\n", >> + mmc_card_name(card), q_depth); > > I don't think this print is needed. Likely it's already handled by > blk_queue_init_tags(). Thanks. I think there are no error prints in blk_queue_init_tags, however, I'll re-check and remove if it is already handled in there. > >> + goto free_mqrq_sg; >> + } >> + >> + blk_queue_softirq_done(mq->queue, mmc_cmdq_softirq_done); >> + card->cmdq_init = true; >> + goto out; >> + >> +free_mqrq_sg: >> + for (i = 0; i < q_depth; i++) >> + kfree(mq->mqrq_cmdq[i].sg); >> + kfree(mq->mqrq_cmdq); >> + mq->mqrq_cmdq = NULL; >> +out: >> + return ret; >> +} >> + >> +void mmc_cmdq_clean(struct mmc_queue *mq, struct mmc_card *card) >> +{ >> + int i; >> + int q_depth = card->ext_csd.cmdq_depth - 1; >> + >> + blk_free_tags(mq->queue->queue_tags); >> + mq->queue->queue_tags = NULL; >> + blk_queue_free_tags(mq->queue); >> + >> + for (i = 0; i < q_depth; i++) >> + kfree(mq->mqrq_cmdq[i].sg); >> + kfree(mq->mqrq_cmdq); >> + mq->mqrq_cmdq = NULL; >> +} >> + >> /** >> * mmc_queue_suspend - suspend a MMC request queue >> * @mq: MMC queue to suspend >> diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h >> index 5752d50..36a8d64 100644 >> --- a/drivers/mmc/card/queue.h >> +++ b/drivers/mmc/card/queue.h >> @@ -52,11 +52,14 @@ struct mmc_queue { >> #define MMC_QUEUE_NEW_REQUEST (1 << 1) >> >> int (*issue_fn)(struct mmc_queue *, struct request *); >> + int (*cmdq_issue_fn)(struct mmc_queue *, struct request *); > > I don't find this callback being used in this patch. Thanks. I'll check and move it to appropriate patch-set. > >> + void (*cmdq_complete_fn)(struct request *); >> void *data; >> struct request_queue *queue; >> struct mmc_queue_req mqrq[2]; >> struct mmc_queue_req *mqrq_cur; >> struct mmc_queue_req *mqrq_prev; >> + struct mmc_queue_req *mqrq_cmdq; >> }; >> >> extern int mmc_init_queue(struct mmc_queue *, struct mmc_card *, spinlock_t *, >> @@ -73,4 +76,7 @@ extern void mmc_queue_bounce_post(struct mmc_queue_req *); >> extern int mmc_packed_init(struct mmc_queue *, struct mmc_card *); >> extern void mmc_packed_clean(struct mmc_queue *); >> >> +extern int mmc_cmdq_init(struct mmc_queue *mq, struct mmc_card *card); >> +extern void mmc_cmdq_clean(struct mmc_queue *mq, struct mmc_card *card); >> + >> #endif >> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h >> index b87a27c..41f368d 100644 >> --- a/include/linux/mmc/card.h >> +++ b/include/linux/mmc/card.h >> @@ -309,6 +309,7 @@ struct mmc_card { >> struct dentry *debugfs_root; >> struct mmc_part part[MMC_NUM_PHY_PARTITION]; /* physical partitions */ >> unsigned int nr_parts; >> + bool cmdq_init; /* cmdq init done */ >> }; >> >> /* >> @@ -545,4 +546,5 @@ extern void mmc_unregister_driver(struct mmc_driver *); >> extern void mmc_fixup_device(struct mmc_card *card, >> const struct mmc_fixup *table); >> >> +extern void mmc_blk_cmdq_req_done(struct mmc_request *mrq); >> #endif /* LINUX_MMC_CARD_H */ >> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h >> index cb61ea4..f0edb36 100644 >> --- a/include/linux/mmc/host.h >> +++ b/include/linux/mmc/host.h >> @@ -278,6 +278,7 @@ struct mmc_host { >> #define MMC_CAP2_PACKED_CMD (MMC_CAP2_PACKED_RD | \ >> MMC_CAP2_PACKED_WR) >> #define MMC_CAP2_NO_PRESCAN_POWERUP (1 << 14) /* Don't power up before scan */ >> +#define MMC_CAP2_CMD_QUEUE (1 << 15) /* support eMMC command queue */ >> >> mmc_pm_flag_t pm_caps; /* supported pm features */ >> >> -- >> 1.8.2.1 >> > > Please run checkpatch on all your patches. This one had warnings you > shall resolve and the rest in the series has so as well. Thanks. I had run checkpatch, but perhaps missed some warning. Sorry about that. I'll run it again through the series and re-post. > > Moreover, I can't apply this patch on my next branch so it needs a > rebase. Can you please do that so I am able to test it. > Sure. Will do that. > Kind regards > Uffe > -- > 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 > -- Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.