Re: [EXT] Re: [RFC PATCH 2/2] Make cmdq_en attribute writeable

[Adrian Hunter <adrian.hunter@intel.com>]

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.