Hi Asutosh,
May I ask whether you have tested the code on hardware platform?
If you have not tested it yet, do you have a schedule for testing?
-----Original Message-----
From: Asutosh Das [mailto:[email protected]]
Sent: 2014年12月7日 21:59
To: Ziji Hu
Cc: [email protected]; [email protected]
Subject: Re: [PATCH 3/5] mmc: card: Add eMMC command queuing support in mmc
block layer
Hi Ziji
Thanks for your comments.
I'm adding linux-mmc to this as well.
On 8 December 2014 at 07:47, Hu Ziji <[email protected]> wrote:
>
> Asutosh Das <asutoshd <at> codeaurora.org> writes:
>
> > +static inline bool mmc_cmdq_should_pull_reqs(struct mmc_host *host,
> > + struct mmc_cmdq_context_info
> > +*ctx) {
> > + spin_lock_bh(&ctx->cmdq_ctx_lock);
> > + if (ctx->active_dcmd || ctx->rpmb_in_wait) {
> > + if ((ctx->curr_state != CMDQ_STATE_HALT) ||
> > + (ctx->curr_state != CMDQ_STATE_ERR)) {
>
> Because CMDQ_STATE_HALT == 0 and
> CMDQ_STATE_ERR == 1, it is impossible that condition ((ctx->curr_state
> == CMDQ_STATE_HALT) && (ctx-
> >curr_state != CMDQ_STATE_ERR)) is satisfied. Thus the above
> >if-condition will
> always be true.
>
Agree.
>
> > + pr_debug("%s: %s: skip pulling reqs: dcmd: %d rpmb:
> > %d state:
> %d\n",
> > + mmc_hostname(host), __func__,
> > ctx->active_dcmd,
> > + ctx->rpmb_in_wait, ctx->curr_state);
> > + spin_unlock_bh(&ctx->cmdq_ctx_lock);
> > + return false;
> > + }
> > + } else {
> > + spin_unlock_bh(&ctx->cmdq_ctx_lock);
> > + return true;
> > + }
> > +}
>
> > +/**
> > + * mmc_cmdq_halt - halt/un-halt the command queue engine
> > + * <at> host: host instance
> > + * <at> halt: true - halt, un-halt otherwise
> > + *
> > + * Host halts the command queue engine. It should complete
> > + * the ongoing transfer and release the SD bus.
> > + * All legacy SD commands can be sent upon successful
> > + * completion of this function.
> > + * Returns 0 on success, negative otherwise
> > + */
> > +int mmc_cmdq_halt(struct mmc_host *host, bool halt) {
> > + int err = 0;
> > +
> > + if ((halt && (host->cmdq_ctx.curr_state & CMDQ_STATE_HALT)) ||
> > + (!halt && !(host->cmdq_ctx.curr_state & CMDQ_STATE_HALT)))
> > + return 1;
> > +
> > + if (host->cmdq_ops->halt) {
> > + err = host->cmdq_ops->halt(host, halt);
> > + if (!err && halt)
> > + host->cmdq_ctx.curr_state |= CMDQ_STATE_HALT;
> > + else if (!err && !halt)
> > + host->cmdq_ctx.curr_state &= ~CMDQ_STATE_HALT;
>
> Since CMDQ_STATE_HALE == 0, ORed with CMDQ_STATE_HALT
> and ANDed with ~CMDQ_STATE_HALT will neither modify the value of
> curr_state. Thus CMDQ_STATE_HALT as 0 cannot indicate the halt state of cmd
> queue.
Agree.
>
>
> > + }
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(mmc_cmdq_halt);
>
> > +enum cmdq_states {
> > + CMDQ_STATE_HALT,
> > + CMDQ_STATE_ERR,
> > +};
>
> According to above definition, CMDQ_STATE_HALT is 0x0 and
> CMDQ_STATE_ERR is 0x1.
>
I will address these comments in the next patch.
>
>
> Hi Asutosh,
>
> I think that the definition of CMDQ_STATE_HALT might fail to
> indicate halt status correctly.
> Could you check my comments please?
> If I misunderstand your source code, please correct me.
>
> Best regards,
> Ziji
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-arm-msm" in the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks & Regards
~/Asutosh Das
