| Message ID | 20210215003249.GA12303@lupo-laptop |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/2] remove field use_cqe in mmc_queue | expand |
+ Adrian On Mon, 15 Feb 2021 at 01:33, Luca Porzio <porzio@gmail.com> 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@micron.com> > Signed-off-by: Zhan Liu <zliua@micron.com> 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; > + > + 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; > + err = 0; > + } > + > + return err; > +} > + > + > +static ssize_t cmdq_en_show(struct device *dev, struct device_attribute *attr, char *buf) > +{ > + struct mmc_card *card = mmc_dev_to_card(dev); > + > + return sprintf(buf, "%d\n", card->ext_csd.cmdq_en); > +} > + > +static ssize_t cmdq_en_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct mmc_card *card = mmc_dev_to_card(dev); > + struct mmc_host *host = card->host; > + unsigned long enable; > + int err; > + > + if (!card || kstrtoul(buf, 0, &enable)) > + return -EINVAL; > + if (!card->ext_csd.cmdq_support) > + return -EOPNOTSUPP; > + > + enable = !!enable; > + if (enable == card->ext_csd.cmdq_en) > + return count; > + > + mmc_get_card(card, NULL); > + card->force_disable_cmdq = !enable; > + err = mmc_cmdq_setup(host, card); > + mmc_put_card(card, NULL); > + > + if (err) > + return err; > + else > + return count; > +} > + > +static DEVICE_ATTR_RW(cmdq_en); > + > > static ssize_t mmc_fwrev_show(struct device *dev, > struct device_attribute *attr, > @@ -1838,40 +1951,9 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, > * Enable Command Queue if supported. Note that Packed Commands cannot > * be used with Command Queue. > */ > - card->ext_csd.cmdq_en = false; > - if (card->ext_csd.cmdq_support && host->caps2 & MMC_CAP2_CQE) { > - err = mmc_cmdq_enable(card); > - if (err && err != -EBADMSG) > - goto free_card; > - 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; > - } > - } > - /* > - * 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; > - > - if (host->cqe_ops && !host->cqe_enabled) { > - err = host->cqe_ops->cqe_enable(host, card); > - if (!err) { > - host->cqe_enabled = true; > - > - 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)); > - } > - } > - } > + err = mmc_cmdq_setup(host, card); > + if (err) > + goto free_card; > > if (host->caps2 & MMC_CAP2_AVOID_3_3V && > host->ios.signal_voltage == MMC_SIGNAL_VOLTAGE_330) { > diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h > index f9ad35dd6012..e554bb0cf722 100644 > --- a/include/linux/mmc/card.h > +++ b/include/linux/mmc/card.h > @@ -272,6 +272,7 @@ struct mmc_card { > #define MMC_QUIRK_BROKEN_HPI (1<<13) /* Disable broken HPI support */ > > bool reenable_cmdq; /* Re-enable Command Queue */ > + bool force_disable_cmdq; /* Keep Command Queue disabled */ > > unsigned int erase_size; /* erase size in sectors */ > unsigned int erase_shift; /* if erase unit is power 2 */ > -- > 2.17.1 >
On Mon, 15 Feb 2021 at 01:33, Luca Porzio <porzio@gmail.com> 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@micron.com> > Signed-off-by: Zhan Liu <zliua@micron.com> > --- > 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 [...] > +static ssize_t cmdq_en_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct mmc_card *card = mmc_dev_to_card(dev); > + struct mmc_host *host = card->host; > + unsigned long enable; > + int err; > + > + if (!card || kstrtoul(buf, 0, &enable)) > + return -EINVAL; > + if (!card->ext_csd.cmdq_support) > + return -EOPNOTSUPP; > + > + enable = !!enable; > + if (enable == card->ext_csd.cmdq_en) > + return count; > + > + mmc_get_card(card, NULL); This means adding a new path for when the host needs to get locked (claimed), which is the opposite direction of what we have been working on for SD/eMMC during the last couple of years. Please have a look at mmc_blk_issue_drv_op(), where you can find how these kinds of requests are being funneled through the mmc block device layer instead. This is the preferred option. That said, I am actually wondering if we perhaps could manage the enable/disable of CQE "automagically" for FFU, along the lines of what we do for RPMB already. In fact, maybe the best/easiest approach is to *always* disable CQE when there is a request being received through the mmc ioctl interface. Of course, then if we disabled CQE we should also re-enable it when the ioctl request has been completed. What do you think? > + card->force_disable_cmdq = !enable; > + err = mmc_cmdq_setup(host, card); > + mmc_put_card(card, NULL); > + > + if (err) > + return err; > + else > + return count; > +} > + > +static DEVICE_ATTR_RW(cmdq_en); > + > [...] Kind regards Uffe
On Tue, 9 Mar 2021 at 14:18, Luca Porzio <porzio@gmail.com> wrote: > > >> >> This means adding a new path for when the host needs to get locked >> (claimed), which is the opposite direction of what we have been >> working on for SD/eMMC during the last couple of years. >> >> Please have a look at mmc_blk_issue_drv_op(), where you can find how >> these kinds of requests are being funneled through the mmc block >> device layer instead. This is the preferred option. >> >> That said, I am actually wondering if we perhaps could manage the >> enable/disable of CQE "automagically" for FFU, along the lines of what >> we do for RPMB already. In fact, maybe the best/easiest approach is to >> *always* disable CQE when there is a request being received through >> the mmc ioctl interface. Of course, then if we disabled CQE we should >> also re-enable it when the ioctl request has been completed. >> >> What do you think? >> >> > > Thanks a lot for your feedback! > > The reason why this is an RFC patch and not a clear patch is exactly what you > say here. > During the FFU (as well as other sequence of commands issued through > multi cmd() ioctl feature, is that sometimes it may require an emmc reboot or > some other legacy command subsequent sequence (= which needs to be > sent with two ioctl() multi_cmd sequences) and the legacy mode need to > survive across reboots or partition changes. The reboot is kind of a separate problem, even if it's related, isn't it? What you propose is to add a sysfs node to let userspace enable/disable CQE. I was under the impression that this was solely to allow the FFU process to be completed, no? Are you saying that we may want CQE to stay disabled beyond the FFU process, to allow the reboot to be completed safely? > Also we need to claim the host to make sure all the pending commands > in the queue are completed successfully before disabling. Yes, of course. It sounds like you may have misinterpreted my proposals. The problem is not that we need to claim the host, but that you add an entirely new path to do it. *If* we conclude that we should add a sysfs node to control CQE, we should create a mmc blk request for it (which will manage the claim for us as well). I suggest you have a closer look at power_ro_lock_store(), which should be the equivalent to what we need to implement here. > > I can rethink the patch to implement a specific iotcl() request which disables > CMDQ if you think that is a better implementation but in the end it will still > require the host claim. > > Any feedback or suggestion is appreciated. To be clear, I am not proposing to add a new ioctl for mmc. Those we have today, should be sufficient I think. However, rather than adding a new sysfs node to control CQE, perhaps we can parse the received ioctl data structure, to find out if the command/request is for FFU and then take specific actions. Along the lines of what we do for mmc_sanitize(), for example. Another option, rather than parsing ioctl data for the FFU command/request, is to always temporarily disable CQE while processing an mmc ioctl request. Would this work? Kind regards Uffe
On 2/03/21 12:46 pm, Ulf Hansson wrote: > + Adrian > > On Mon, 15 Feb 2021 at 01:33, Luca Porzio <porzio@gmail.com> 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@micron.com> >> Signed-off-by: Zhan Liu <zliua@micron.com> > > 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. 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. >> + >> + 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. >> + err = 0; >> + } >> + >> + return err; >> +} >> + >> + >> +static ssize_t cmdq_en_show(struct device *dev, struct device_attribute *attr, char *buf) >> +{ >> + struct mmc_card *card = mmc_dev_to_card(dev); >> + >> + return sprintf(buf, "%d\n", card->ext_csd.cmdq_en); >> +} >> + >> +static ssize_t cmdq_en_store(struct device *dev, struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct mmc_card *card = mmc_dev_to_card(dev); >> + struct mmc_host *host = card->host; >> + unsigned long enable; >> + int err; >> + >> + if (!card || kstrtoul(buf, 0, &enable)) >> + return -EINVAL; >> + if (!card->ext_csd.cmdq_support) >> + return -EOPNOTSUPP; >> + >> + enable = !!enable; >> + if (enable == card->ext_csd.cmdq_en) >> + return count; >> + >> + mmc_get_card(card, NULL); >> + card->force_disable_cmdq = !enable; >> + err = mmc_cmdq_setup(host, card); >> + mmc_put_card(card, NULL); >> + >> + if (err) >> + return err; >> + else >> + return count; >> +} >> + >> +static DEVICE_ATTR_RW(cmdq_en); >> + >> >> static ssize_t mmc_fwrev_show(struct device *dev, >> struct device_attribute *attr, >> @@ -1838,40 +1951,9 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, >> * Enable Command Queue if supported. Note that Packed Commands cannot >> * be used with Command Queue. >> */ >> - card->ext_csd.cmdq_en = false; >> - if (card->ext_csd.cmdq_support && host->caps2 & MMC_CAP2_CQE) { >> - err = mmc_cmdq_enable(card); >> - if (err && err != -EBADMSG) >> - goto free_card; >> - 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; >> - } >> - } >> - /* >> - * 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; >> - >> - if (host->cqe_ops && !host->cqe_enabled) { >> - err = host->cqe_ops->cqe_enable(host, card); >> - if (!err) { >> - host->cqe_enabled = true; >> - >> - 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)); >> - } >> - } >> - } >> + err = mmc_cmdq_setup(host, card); >> + if (err) >> + goto free_card; >> >> if (host->caps2 & MMC_CAP2_AVOID_3_3V && >> host->ios.signal_voltage == MMC_SIGNAL_VOLTAGE_330) { >> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h >> index f9ad35dd6012..e554bb0cf722 100644 >> --- a/include/linux/mmc/card.h >> +++ b/include/linux/mmc/card.h >> @@ -272,6 +272,7 @@ struct mmc_card { >> #define MMC_QUIRK_BROKEN_HPI (1<<13) /* Disable broken HPI support */ >> >> bool reenable_cmdq; /* Re-enable Command Queue */ >> + bool force_disable_cmdq; /* Keep Command Queue disabled */ >> >> unsigned int erase_size; /* erase size in sectors */ >> unsigned int erase_shift; /* if erase unit is power 2 */ >> -- >> 2.17.1 >>
Micron Confidential > > Are you saying that we may want CQE to stay disabled beyond the FFU > process, to allow the reboot to be completed safely? > Yes, it is. Also there are use cases where reboot is part of the sequence e.g. the force_disable_cmdq flag for the tests is a good example. > > Also we need to claim the host to make sure all the pending commands > > in the queue are completed successfully before disabling. > > Yes, of course. It sounds like you may have misinterpreted my proposals. > > The problem is not that we need to claim the host, but that you add an > entirely new path to do it. > > *If* we conclude that we should add a sysfs node to control CQE, we should > create a mmc blk request for it (which will manage the claim for us as well). I > suggest you have a closer look at power_ro_lock_store(), which should be > the equivalent to what we need to implement here. > I understand what you mean and try to rethink the patch taking inspiration from that. > > > > I can rethink the patch to implement a specific iotcl() request which > > disables CMDQ if you think that is a better implementation but in the > > end it will still require the host claim. > > > > Any feedback or suggestion is appreciated. > > To be clear, I am not proposing to add a new ioctl for mmc. Those we have > today, should be sufficient I think. > > However, rather than adding a new sysfs node to control CQE, perhaps we > can parse the received ioctl data structure, to find out if the > command/request is for FFU and then take specific actions. Along the lines of > what we do for mmc_sanitize(), for example. > > Another option, rather than parsing ioctl data for the FFU command/request, > is to always temporarily disable CQE while processing an mmc ioctl request. > > Would this work? > I think you are right: we should always disable CMDQ for all ioctl requests as of today I do not know a case where CMDQ on is useful for multi cmd lists. What do you suggest: 1 - mimic the power_ro_lock_store and let CMDQ be off across reboots 2 - disable cmdq for multi cmd lists I would prefer 1 because it covers most cases but 2 could be an option I can investigate. > Kind regards > Uffe Cheers! Luca Micron Confidential
Micron Confidential > -----Original Message----- > From: Adrian Hunter <adrian.hunter@intel.com> > Sent: Wednesday, March 10, 2021 9:54 AM > To: Ulf Hansson <ulf.hansson@linaro.org>; Luca Porzio <porzio@gmail.com> > Cc: linux-mmc@vger.kernel.org; Zhan Liu (zliua) <zliua@micron.com>; Luca > Porzio (lporzio) <lporzio@micron.com> > 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@gmail.com> 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@micron.com> > >> Signed-off-by: Zhan Liu <zliua@micron.com> > > > > 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
On 15/03/21 12:33 am, Luca Porzio (lporzio) wrote: > Micron Confidential > > > >> -----Original Message----- >> From: Adrian Hunter <adrian.hunter@intel.com> >> Sent: Wednesday, March 10, 2021 9:54 AM >> To: Ulf Hansson <ulf.hansson@linaro.org>; Luca Porzio <porzio@gmail.com> >> Cc: linux-mmc@vger.kernel.org; Zhan Liu (zliua) <zliua@micron.com>; Luca >> Porzio (lporzio) <lporzio@micron.com> >> 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@gmail.com> 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@micron.com> >>>> Signed-off-by: Zhan Liu <zliua@micron.com> >>> >>> 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. It does if there are errors. > >> 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? I just meant, do not break HSQ.
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; + + 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; + err = 0; + } + + return err; +} + + +static ssize_t cmdq_en_show(struct device *dev, struct device_attribute *attr, char *buf) +{ + struct mmc_card *card = mmc_dev_to_card(dev); + + return sprintf(buf, "%d\n", card->ext_csd.cmdq_en); +} + +static ssize_t cmdq_en_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct mmc_card *card = mmc_dev_to_card(dev); + struct mmc_host *host = card->host; + unsigned long enable; + int err; + + if (!card || kstrtoul(buf, 0, &enable)) + return -EINVAL; + if (!card->ext_csd.cmdq_support) + return -EOPNOTSUPP; + + enable = !!enable; + if (enable == card->ext_csd.cmdq_en) + return count; + + mmc_get_card(card, NULL); + card->force_disable_cmdq = !enable; + err = mmc_cmdq_setup(host, card); + mmc_put_card(card, NULL); + + if (err) + return err; + else + return count; +} + +static DEVICE_ATTR_RW(cmdq_en); + static ssize_t mmc_fwrev_show(struct device *dev, struct device_attribute *attr, @@ -1838,40 +1951,9 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, * Enable Command Queue if supported. Note that Packed Commands cannot * be used with Command Queue. */ - card->ext_csd.cmdq_en = false; - if (card->ext_csd.cmdq_support && host->caps2 & MMC_CAP2_CQE) { - err = mmc_cmdq_enable(card); - if (err && err != -EBADMSG) - goto free_card; - 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; - } - } - /* - * 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; - - if (host->cqe_ops && !host->cqe_enabled) { - err = host->cqe_ops->cqe_enable(host, card); - if (!err) { - host->cqe_enabled = true; - - 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)); - } - } - } + err = mmc_cmdq_setup(host, card); + if (err) + goto free_card; if (host->caps2 & MMC_CAP2_AVOID_3_3V && host->ios.signal_voltage == MMC_SIGNAL_VOLTAGE_330) { diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h index f9ad35dd6012..e554bb0cf722 100644 --- a/include/linux/mmc/card.h +++ b/include/linux/mmc/card.h @@ -272,6 +272,7 @@ struct mmc_card { #define MMC_QUIRK_BROKEN_HPI (1<<13) /* Disable broken HPI support */ bool reenable_cmdq; /* Re-enable Command Queue */ + bool force_disable_cmdq; /* Keep Command Queue disabled */ unsigned int erase_size; /* erase size in sectors */ unsigned int erase_shift; /* if erase unit is power 2 */