On 15/03/21 12:33 am, Luca Porzio (lporzio) wrote:
> 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.
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.
