Micron Confidential
> -----Original Message-----
> From: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> Sent: Wednesday, March 10, 2021 9:54 AM
> To: Ulf Hansson <ulf.hansson@xxxxxxxxxx>; Luca Porzio <porzio@xxxxxxxxx>
> Cc: linux-mmc@xxxxxxxxxxxxxxx; Zhan Liu (zliua) <zliua@xxxxxxxxxx>; Luca
> Porzio (lporzio) <lporzio@xxxxxxxxxx>
> Subject: [EXT] Re: [RFC PATCH 2/2] Make cmdq_en attribute writeable
>
> CAUTION: EXTERNAL EMAIL. Do not click links or open attachments unless
> you recognize the sender and were expecting this message.
>
>
> On 2/03/21 12:46 pm, Ulf Hansson wrote:
> > + Adrian
> >
> > On Mon, 15 Feb 2021 at 01:33, Luca Porzio <porzio@xxxxxxxxx> wrote:
> >>
> >> cmdq_en attribute in sysfs now can now be written.
> >> When 0 is written:
> >> CMDQ is disabled and kept disabled across device reboots.
> >> When 1 is written:
> >> CMDQ mode is instantly reneabled (if supported).
> >>
> >> Signed-off-by: Luca Porzio <lporzio@xxxxxxxxxx>
> >> Signed-off-by: Zhan Liu <zliua@xxxxxxxxxx>
> >
> > Luca, thanks for your patch! I am about to start to review this.
> >
> > I have also looped in Adrian to get his opinions.
> >
> > Get back to you soon!
> >
> > Kind regards
> > Uffe
> >
> >> ---
> >> drivers/mmc/core/mmc.c | 152 ++++++++++++++++++++++++++++++-
> --------
> >> include/linux/mmc/card.h | 1 +
> >> 2 files changed, 118 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index
> >> 0d80b72ddde8..5c7d5bac5c00 100644
> >> --- a/drivers/mmc/core/mmc.c
> >> +++ b/drivers/mmc/core/mmc.c
> >> @@ -794,7 +794,120 @@ MMC_DEV_ATTR(enhanced_rpmb_supported,
> "%#x\n",
> >> MMC_DEV_ATTR(rel_sectors, "%#x\n", card->ext_csd.rel_sectors);
> >> MMC_DEV_ATTR(ocr, "0x%08x\n", card->ocr); MMC_DEV_ATTR(rca,
> >> "0x%04x\n", card->rca); -MMC_DEV_ATTR(cmdq_en, "%d\n",
> >> card->ext_csd.cmdq_en);
> >> +
> >> +
> >> +/* Setup command queue mode and CQE if underling hw supports it
> >> + * and assuming force_disable_cmdq has not been set.
> >> + */
> >> +static int mmc_cmdq_setup(struct mmc_host *host, struct mmc_card
> >> +*card) {
> >> + int err;
> >> +
> >> + /* Check HW support */
> >> + if (!card->ext_csd.cmdq_support || !(host->caps2 &
> MMC_CAP2_CQE))
> >> + card->force_disable_cmdq = true;
> >> +
> >> + /* Enable/Disable CMDQ mode */
> >> + if (!card->ext_csd.cmdq_en && !card->force_disable_cmdq) {
> >> + err = mmc_cmdq_enable(card);
> >> + if (err && err != -EBADMSG)
> >> + return err;
> >> + if (err) {
> >> + pr_warn("%s: Enabling CMDQ failed\n",
> >> + mmc_hostname(card->host));
> >> + card->ext_csd.cmdq_support = false;
> >> + card->ext_csd.cmdq_depth = 0;
> >> + }
> >> +
> >> + } else if (card->ext_csd.cmdq_en && card->force_disable_cmdq) {
> >> + err = mmc_cmdq_disable(card);
> >> + if (err) {
> >> + pr_warn("%s: Disabling CMDQ failed, error %d\n",
> >> + mmc_hostname(card->host), err);
> >> + err = 0;
> >> + }
> >> + }
> >> +
> >> + /*
> >> + * In some cases (e.g. RPMB or mmc_test), the Command Queue
> must be
> >> + * disabled for a time, so a flag is needed to indicate to re-enable the
> >> + * Command Queue.
> >> + */
> >> + card->reenable_cmdq = card->ext_csd.cmdq_en;
> >> +
> >> + /* Enable/Disable Host CQE */
> >> + if (!card->force_disable_cmdq) {
> >> +
> >> + if (host->cqe_ops && !host->cqe_enabled) {
> >> + err = host->cqe_ops->cqe_enable(host, card);
> >> + if (!err) {
> >> + host->cqe_enabled = true;
>
> Re-initializing the card is also a recovery path for the block driver.
> Changing host->cqe_enabled during a recovery reset, creates an unexpected
> dependency for the block driver. That should not be necessary, and given
> that cqhci does memory allocation as part of enabling, it is better not to
> disable / re-enable if it can be helped.
>
Adrian,
This patch does nothing if the CMDQ maintains the same status (even
across reboots). It only take decision if CMDQ state is to be changed, thus
the occurrence you mention should not happen.
> From a design point of view, it is really the block driver that should control
> the use of command queuing rather than expecting it to cope with changes
> from a lower level.
>
I see this is also related with comment from Ulf. I will propose a new patch
which addresses this one. Thanks for your comment here.
> >> +
> >> + if (card->ext_csd.cmdq_en) {
> >> + pr_info("%s: Command Queue Engine enabled\n",
> >> + mmc_hostname(host));
> >> + } else {
> >> + host->hsq_enabled = true;
> >> + pr_info("%s: Host Software Queue enabled\n",
> >> + mmc_hostname(host));
> >> + }
> >> + }
> >> + }
> >> +
> >> + } else {
> >> +
> >> + if (host->cqe_enabled) {
> >> + host->cqe_ops->cqe_disable(host);
> >> + host->cqe_enabled = false;
> >> + pr_info("%s: Command Queue Engine disabled\n",
> >> + mmc_hostname(host));
> >> + }
> >> +
> >> + host->hsq_enabled = false;
>
> This looks quite wrong for hsq which is currently not used with cmdq.
>
HSQ is really trying to speed up command adoption but in my use case
(ie. multi cmd list with as low as possible interference from other sources)
HSQ could mix commands and make problems, so I decided to stay safe
and prefer stability against speed of execution.
Do you agree? Any suggestion?
Thanks,
Luca
