[Linux-stm32] [PATCH 03/13] mmc: mmci: Unwind big if() clause

[Ulf Hansson]

On Wed, 5 Apr 2023 at 08:50, Linus Walleij <linus.walleij at linaro.org> wrote:
>
> This does two things: firsr replace the hard-to-read long
> if-expression:
>
>   if (!host->busy_status && !(status & err_msk) &&
>       (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
>
> With the more readable:
>
>   if (!host->busy_status && !(status & err_msk)) {
>      status = readl(base + MMCISTATUS);
>      if (status & host->variant->busy_detect_flag) {

If I am reading the code below, this isn't what is being done. See more below.

>
> Second notice that the re-read MMCISTATUS register is now
> stored into the status variable, using logic OR because what
> if something else changed too?
>
> While we are at it, explain what the function is doing.
>
> Signed-off-by: Linus Walleij <linus.walleij at linaro.org>
> ---
>  drivers/mmc/host/mmci.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 3e08b2e95550..3c1e11266fa9 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -654,6 +654,13 @@ static u32 ux500v2_get_dctrl_cfg(struct mmci_host *host)
>         return MCI_DPSM_ENABLE | (host->data->blksz << 16);
>  }
>
> +/*
> + * ux500_busy_complete() - this will wait until the busy status
> + * goes off, saving any status that occur in the meantime into
> + * host->busy_status until we know the card is not busy any more.
> + * The function returns true when the busy detection is ended
> + * and we should continue processing the command.
> + */
>  static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk)
>  {
>         void __iomem *base = host->base;
> @@ -671,14 +678,17 @@ static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk)
>          * while, to allow it to be set, but tests indicates that it
>          * isn't needed.
>          */
> -       if (!host->busy_status && !(status & err_msk) &&
> -           (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
> -               writel(readl(base + MMCIMASK0) |
> -                      host->variant->busy_detect_mask,
> -                      base + MMCIMASK0);
> -
> +       if (!host->busy_status && !(status & err_msk)) {
>                 host->busy_status = status & (MCI_CMDSENT | MCI_CMDRESPEND);

I wonder if this change is intentional. According to the commit
message, I assume not.

So, this may lead to that we end up giving host->busy_status a value,
even if the busy_detect_flag bit isn't set in the status register. In
other words, we could end up waiting for busy completion, while we
shouldn't.

> -               return false;
> +               status = readl(base + MMCISTATUS);
> +               if (status & host->variant->busy_detect_flag) {
> +                       writel(readl(base + MMCIMASK0) |
> +                              host->variant->busy_detect_mask,
> +                              base + MMCIMASK0);
> +
> +                       host->busy_status |= status & (MCI_CMDSENT | MCI_CMDRESPEND);
> +                       return false;
> +               }
>         }
>

Kind regards
Uffe