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.
>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
>>
