linux-kernel - Re: [PATCH] mmc: dw_mmc: Fix IDMAC operation with pages bigger than 4K

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] [day] [month] [year] [list]
Message-ID: <614692b4-1dbe-31b8-a34d-cb6db1909bb7@w6rz.net>
Date: Sun, 15 Sep 2024 23:09:55 -0700
From: Ron Economos <re@...z.net>
To: Sam Protsenko <semen.protsenko@...aro.org>,
 Jaehoon Chung <jh80.chung@...sung.com>, Ulf Hansson
 <ulf.hansson@...aro.org>, Arnd Bergmann <arnd@...db.de>,
 Christoph Hellwig <hch@....de>
Cc: Chris Ball <cjb@...top.org>, Will Newton <will.newton@...il.com>,
 Matt Fleming <matt@...sole-pimps.org>, Christian Brauner
 <brauner@...nel.org>, Jens Axboe <axboe@...nel.dk>,
 Sumit Semwal <sumit.semwal@...aro.org>,
 Dan Carpenter <dan.carpenter@...aro.org>,
 Anders Roxell <anders.roxell@...aro.org>,
 Naresh Kamboju <naresh.kamboju@...aro.org>, linux-mmc@...r.kernel.org,
 linux-block <linux-block@...r.kernel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mmc: dw_mmc: Fix IDMAC operation with pages bigger than
 4K

On 3/6/24 3:20 PM, Sam Protsenko wrote:
> Commit 616f87661792 ("mmc: pass queue_limits to blk_mq_alloc_disk") [1]
> revealed the long living issue in dw_mmc.c driver, existing since the
> time when it was first introduced in commit f95f3850f7a9 ("mmc: dw_mmc:
> Add Synopsys DesignWare mmc host driver."), also making kernel boot
> broken on platforms using dw_mmc driver with 16K or 64K pages enabled,
> with this message in dmesg:
>
>      mmcblk: probe of mmc0:0001 failed with error -22
>
> That's happening because mmc_blk_probe() fails when it calls
> blk_validate_limits() consequently, which returns the error due to
> failed max_segment_size check in this code:
>
>      /*
>       * The maximum segment size has an odd historic 64k default that
>       * drivers probably should override.  Just like the I/O size we
>       * require drivers to at least handle a full page per segment.
>       */
>      ...
>      if (WARN_ON_ONCE(lim->max_segment_size < PAGE_SIZE))
>          return -EINVAL;
>
> In case when IDMAC (Internal DMA Controller) is used, dw_mmc.c always
> sets .max_seg_size to 4 KiB:
>
>      mmc->max_seg_size = 0x1000;
>
> The comment in the code above explains why it's incorrect. Arnd
> suggested setting .max_seg_size to .max_req_size to fix it, which is
> also what some other drivers are doing:
>
>     $ grep -rl 'max_seg_size.*=.*max_req_size' drivers/mmc/host/ | \
>       wc -l
>     18
>
> This change is not only fixing the boot with 16K/64K pages, but also
> leads to a better MMC performance. The linear write performance was
> tested on E850-96 board (eMMC only), before commit [1] (where it's
> possible to boot with 16K/64K pages without this fix, to be able to do
> a comparison). It was tested with this command:
>
>      # dd if=/dev/zero of=somefile bs=1M count=500 oflag=sync
>
> Test results are as follows:
>
>    - 4K pages,  .max_seg_size = 4 KiB:                   94.2 MB/s
>    - 4K pages,  .max_seg_size = .max_req_size = 512 KiB: 96.9 MB/s
>    - 16K pages, .max_seg_size = 4 KiB:                   126 MB/s
>    - 16K pages, .max_seg_size = .max_req_size = 2 MiB:   128 MB/s
>    - 64K pages, .max_seg_size = 4 KiB:                   138 MB/s
>    - 64K pages, .max_seg_size = .max_req_size = 8 MiB:   138 MB/s
>
> Unfortunately, SD card controller is not enabled in E850-96 yet, so it
> wasn't possible for me to run the test on some cheap SD cards to check
> this patch's impact on those. But it's possible that this change might
> also reduce the writes count, thus improving SD/eMMC longevity.
>
> All credit for the analysis and the suggested solution goes to Arnd.
>
> [1] https://lore.kernel.org/all/20240215070300.2200308-18-hch@lst.de/
>
> Fixes: f95f3850f7a9 ("mmc: dw_mmc: Add Synopsys DesignWare mmc host driver.")
> Suggested-by: Arnd Bergmann <arnd@...db.de>
> Reported-by: Linux Kernel Functional Testing <lkft@...aro.org>
> Closes: https://lore.kernel.org/all/CA+G9fYtddf2Fd3be+YShHP6CmSDNcn0ptW8qg+stUKW+Cn0rjQ@mail.gmail.com/
> Signed-off-by: Sam Protsenko <semen.protsenko@...aro.org>
> ---
>   drivers/mmc/host/dw_mmc.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 8e2d676b9239..cccd5633ff40 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -2951,8 +2951,8 @@ static int dw_mci_init_slot(struct dw_mci *host)
>   	if (host->use_dma == TRANS_MODE_IDMAC) {
>   		mmc->max_segs = host->ring_size;
>   		mmc->max_blk_size = 65535;
> -		mmc->max_seg_size = 0x1000;
> -		mmc->max_req_size = mmc->max_seg_size * host->ring_size;
> +		mmc->max_req_size = DW_MCI_DESC_DATA_LENGTH * host->ring_size;
> +		mmc->max_seg_size = mmc->max_req_size;
>   		mmc->max_blk_count = mmc->max_req_size / 512;
>   	} else if (host->use_dma == TRANS_MODE_EDMAC) {
>   		mmc->max_segs = 64;

Unfortunately, this patch creates side effects for the RISC-V StarFive 
VisionFive2 board (which uses the DesignWare controller). The following 
swiotlb buffer warnings are produced at boot:

2024-09-15T17:54:27.194101-07:00 visionfive kernel: dwmmc_starfive 
16020000.mmc: swiotlb buffer is full (sz: 352256 bytes), total 32768 
(slots), used 222 (slots)
2024-09-15T17:54:27.194107-07:00 visionfive kernel: dwmmc_starfive 
16020000.mmc: swiotlb buffer is full (sz: 352256 bytes), total 32768 
(slots), used 222 (slots)
2024-09-15T17:54:27.194123-07:00 visionfive kernel: dwmmc_starfive 
16020000.mmc: swiotlb buffer is full (sz: 524288 bytes), total 32768 
(slots), used 110 (slots)
2024-09-15T17:54:27.194129-07:00 visionfive kernel: dwmmc_starfive 
16020000.mmc: swiotlb buffer is full (sz: 524288 bytes), total 32768 
(slots), used 110 (slots)