linux-kernel - Re: [PATCH V2] mmc: mmc_test: Pass different sg lists for non-blocking requests

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <158224835519.184098.16667027079485274979@swboyd.mtv.corp.google.com>
Date:   Thu, 20 Feb 2020 17:25:55 -0800
From:   Stephen Boyd <swboyd@...omium.org>
To:     Veerabhadrarao Badiganti <vbadigan@...eaurora.org>,
        robh+dt@...nel.org, ulf.hansson@...aro.org
Cc:     asutoshd@...eaurora.org, stummala@...eaurora.org,
        sayalil@...eaurora.org, cang@...eaurora.org,
        rampraka@...eaurora.org, dianders@...gle.com,
        linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org,
        Veerabhadrarao Badiganti <vbadigan@...eaurora.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Allison Randal <allison@...utok.net>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH V2] mmc: mmc_test: Pass different sg lists for non-blocking requests

Quoting Veerabhadrarao Badiganti (2020-02-19 01:44:31)
> Supply a separate sg list for each of the request in non-blocking
> IO test cases where two requests will be issued at same time.
> 
> Otherwise, sg memory may get unmapped when a request is done while
> same memory is being accessed by controller from the other request,
> and it leads to iommu errors with below call stack:
> 
>         __arm_lpae_unmap+0x2e0/0x478
>         arm_lpae_unmap+0x54/0x70
>         arm_smmu_unmap+0x64/0xa4
>         __iommu_unmap+0xb8/0x1f0
>         iommu_unmap_fast+0x38/0x48
>         __iommu_dma_unmap+0x88/0x108
>         iommu_dma_unmap_sg+0x90/0xa4
>         sdhci_post_req+0x5c/0x78
>         mmc_test_start_areq+0x10c/0x120 [mmc_test]
>         mmc_test_area_io_seq+0x150/0x264 [mmc_test]
>         mmc_test_rw_multiple+0x174/0x1c0 [mmc_test]
>         mmc_test_rw_multiple_sg_len+0x44/0x6c [mmc_test]
>         mmc_test_profile_sglen_wr_nonblock_perf+0x6c/0x94 [mmc_test]
>         mtf_test_write+0x238/0x3cc [mmc_test]
> 
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@...eaurora.org>
> 
> ---

Reviewed-by: Stephen Boyd <swboyd@...omium.org>

> Changes since V1:
>         - Freeing-up sg_areq memory.
>         - Added check to ensure sg length is equal for both the sg-lists
>           supplied in case of non-blocking requests.
> ---
>  drivers/mmc/core/mmc_test.c | 42 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/core/mmc_test.c b/drivers/mmc/core/mmc_test.c
> index 492dd45..f8f884a 100644
> --- a/drivers/mmc/core/mmc_test.c
> +++ b/drivers/mmc/core/mmc_test.c
> @@ -1411,6 +1417,22 @@ static int mmc_test_area_map(struct mmc_test_card *test, unsigned long sz,
>                 err = mmc_test_map_sg(t->mem, sz, t->sg, 1, t->max_segs,
>                                       t->max_seg_sz, &t->sg_len, min_sg_len);
>         }
> +
> +       if (err || !nonblock)
> +               goto err;
> +
> +       if (max_scatter) {
> +               err = mmc_test_map_sg_max_scatter(t->mem, sz, t->sg_areq,
> +                                                 t->max_segs, t->max_seg_sz,
> +                                                 &sg_len);
> +       } else {
> +               err = mmc_test_map_sg(t->mem, sz, t->sg_areq, 1, t->max_segs,

'repeat' is always set to 1. Why not remove that argument and update the
code? As a follow up patch.

> +                                     t->max_seg_sz, &sg_len, min_sg_len);
> +       }
> +       if (!err && sg_len != t->sg_len)
> +               err = -EINVAL;
> +
> +err:
>         if (err)
>                 pr_info("%s: Failed to map sg list\n",
>                        mmc_hostname(test->card->host));
> @@ -1458,15 +1480,16 @@ static int mmc_test_area_io_seq(struct mmc_test_card *test, unsigned long sz,
>                         sz = max_tfr;
>         }
>  
> -       ret = mmc_test_area_map(test, sz, max_scatter, min_sg_len);
> +       ret = mmc_test_area_map(test, sz, max_scatter, min_sg_len, nonblock);
>         if (ret)
>                 return ret;
>  
>         if (timed)
>                 ktime_get_ts64(&ts1);
>         if (nonblock)
> -               ret = mmc_test_nonblock_transfer(test, t->sg, t->sg_len,
> -                                dev_addr, t->blocks, 512, write, count);
> +               ret = mmc_test_nonblock_transfer(test, t->sg, t->sg_areq,
> +                                t->sg_len, dev_addr, t->blocks, 512, write,
> +                                count);

This is only called one time so it may be simpler to pass 't' instead of
pick it apart and pass it as many arguments. Not a problem in this
patch, besides that we're now passing even more arguments here making
this harder to read. Also, the blksz could be hardcoded in the function
instead of passing it as 512.

>         else
>                 for (i = 0; i < count && ret == 0; i++) {
>                         ret = mmc_test_area_transfer(test, dev_addr, write);
> @@ -1584,6 +1608,12 @@ static int mmc_test_area_init(struct mmc_test_card *test, int erase, int fill)
>                 goto out_free;
>         }
>  
> +       t->sg_areq = kmalloc_array(t->max_segs, sizeof(*t->sg), GFP_KERNEL);

It's more idiomatic to use sizeof(*t->sq_areq) here.

> +       if (!t->sg_areq) {
> +               ret = -ENOMEM;
> +               goto out_free;
> +       }
> +
>         t->dev_addr = mmc_test_capacity(test->card) / 2;
>         t->dev_addr -= t->dev_addr % (t->max_sz >> 9);
>