[RFC PATCH v6 4/5] mmc: tmio: Use dma_max_mapping_size() instead of a workaround
Since the commit 133d624b1cee ("dma: Introduce dma_max_mapping_size()") provides a helper function to get the max mapping size, we can use the function instead of the workaround code for swiotlb. Signed-off-by: Yoshihiro Shimoda --- drivers/mmc/host/tmio_mmc_core.c | 17 - 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c index 130b91c..85bd6aa6 100644 --- a/drivers/mmc/host/tmio_mmc_core.c +++ b/drivers/mmc/host/tmio_mmc_core.c @@ -26,6 +26,7 @@ #include #include +#include #include #include #include @@ -1189,19 +1190,9 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host) mmc->max_blk_size = TMIO_MAX_BLK_SIZE; mmc->max_blk_count = pdata->max_blk_count ? : (PAGE_SIZE / mmc->max_blk_size) * mmc->max_segs; - mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count; - /* -* Since swiotlb has memory size limitation, this will calculate -* the maximum size locally (because we don't have any APIs for it now) -* and check the current max_req_size. And then, this will update -* the max_req_size if needed as a workaround. -*/ - if (swiotlb_max_segment()) { - unsigned int max_size = (1 << IO_TLB_SHIFT) * IO_TLB_SEGSIZE; - - if (mmc->max_req_size > max_size) - mmc->max_req_size = max_size; - } + mmc->max_req_size = min_t(unsigned int, + mmc->max_blk_size * mmc->max_blk_count, + dma_max_mapping_size(&pdev->dev)); mmc->max_seg_size = mmc->max_req_size; if (mmc_can_gpio_ro(mmc)) -- 2.7.4 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v6 4/5] mmc: tmio: Use dma_max_mapping_size() instead of a workaround
On Thu, Jun 13, 2019 at 07:20:14PM +0900, Yoshihiro Shimoda wrote: > Since the commit 133d624b1cee ("dma: Introduce dma_max_mapping_size()") > provides a helper function to get the max mapping size, we can use > the function instead of the workaround code for swiotlb. > > Signed-off-by: Yoshihiro Shimoda I love it! I'd really like to see this code go away. Do I get this right that this patch is kinda independent of the reset of the series? Anyway: Acked-by: Wolfram Sang signature.asc Description: PGP signature
Re: [RFC PATCH v6 4/5] mmc: tmio: Use dma_max_mapping_size() instead of a workaround
Hi Shimoda-san, On Thu, Jun 13, 2019 at 5:37 PM Yoshihiro Shimoda wrote: > Since the commit 133d624b1cee ("dma: Introduce dma_max_mapping_size()") > provides a helper function to get the max mapping size, we can use > the function instead of the workaround code for swiotlb. > > Signed-off-by: Yoshihiro Shimoda Thanks for your patch! > --- a/drivers/mmc/host/tmio_mmc_core.c > +++ b/drivers/mmc/host/tmio_mmc_core.c > @@ -1189,19 +1190,9 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host) > mmc->max_blk_size = TMIO_MAX_BLK_SIZE; > mmc->max_blk_count = pdata->max_blk_count ? : > (PAGE_SIZE / mmc->max_blk_size) * mmc->max_segs; > - mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count; > - /* > -* Since swiotlb has memory size limitation, this will calculate > -* the maximum size locally (because we don't have any APIs for it > now) > -* and check the current max_req_size. And then, this will update > -* the max_req_size if needed as a workaround. > -*/ > - if (swiotlb_max_segment()) { > - unsigned int max_size = (1 << IO_TLB_SHIFT) * IO_TLB_SEGSIZE; > - > - if (mmc->max_req_size > max_size) > - mmc->max_req_size = max_size; > - } > + mmc->max_req_size = min_t(unsigned int, > + mmc->max_blk_size * mmc->max_blk_count, > + dma_max_mapping_size(&pdev->dev)); > mmc->max_seg_size = mmc->max_req_size; I'm always triggered by the use of min_t() and other casts: mmc->max_blk_size and mmc->max_blk_count are both unsigned int. dma_max_mapping_size() returns size_t, which can be 64-bit. 1) Can the multiplication overflow? Probably not, as per commit 2a55c1eac7882232 ("mmc: renesas_sdhi: prevent overflow for max_req_size"), but I thought I'd better ask. 2) In theory, dma_max_mapping_size() can return a number that doesn't fit in 32-bit, and will be truncated (to e.g. 0), leading to max_req_size is zero? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v6 4/5] mmc: tmio: Use dma_max_mapping_size() instead of a workaround
On Thu, Jun 13, 2019 at 10:35:44PM +0200, Geert Uytterhoeven wrote: > I'm always triggered by the use of min_t() and other casts: > mmc->max_blk_size and mmc->max_blk_count are both unsigned int. > dma_max_mapping_size() returns size_t, which can be 64-bit. > > 1) Can the multiplication overflow? > Probably not, as per commit 2a55c1eac7882232 ("mmc: renesas_sdhi: > prevent overflow for max_req_size"), but I thought I'd better ask. > 2) In theory, dma_max_mapping_size() can return a number that doesn't > fit in 32-bit, and will be truncated (to e.g. 0), leading to max_req_size > is zero? This really should use a min_t on size_t. Otherwise the patch looks fine: Reviewed-by: Christoph Hellwig ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v6 4/5] mmc: tmio: Use dma_max_mapping_size() instead of a workaround
Hi Christoph, On Fri, Jun 14, 2019 at 9:18 AM Christoph Hellwig wrote: > On Thu, Jun 13, 2019 at 10:35:44PM +0200, Geert Uytterhoeven wrote: > > I'm always triggered by the use of min_t() and other casts: > > mmc->max_blk_size and mmc->max_blk_count are both unsigned int. > > dma_max_mapping_size() returns size_t, which can be 64-bit. > > > > 1) Can the multiplication overflow? > > Probably not, as per commit 2a55c1eac7882232 ("mmc: renesas_sdhi: > > prevent overflow for max_req_size"), but I thought I'd better ask. > > 2) In theory, dma_max_mapping_size() can return a number that doesn't > > fit in 32-bit, and will be truncated (to e.g. 0), leading to > > max_req_size > > is zero? > > This really should use a min_t on size_t. Otherwise the patch looks > fine: Followed by another min() to make it fit in mmc->max_req_size, which is unsigned int. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [RFC PATCH v6 4/5] mmc: tmio: Use dma_max_mapping_size() instead of a workaround
Hi Wolfram-san, > From: Wolfram Sang, Sent: Friday, June 14, 2019 4:46 AM > > On Thu, Jun 13, 2019 at 07:20:14PM +0900, Yoshihiro Shimoda wrote: > > Since the commit 133d624b1cee ("dma: Introduce dma_max_mapping_size()") > > provides a helper function to get the max mapping size, we can use > > the function instead of the workaround code for swiotlb. > > > > Signed-off-by: Yoshihiro Shimoda > > I love it! I'd really like to see this code go away. Do I get this right > that this patch is kinda independent of the reset of the series? Anyway: Thank you for your suggestion! I think so (because IOMMU and block patches seem to need update). I'll submit 3/5 and 4/5 patches as independent later. > Acked-by: Wolfram Sang Thank you for your Acked-by! Best regards, Yoshihiro Shimoda
RE: [RFC PATCH v6 4/5] mmc: tmio: Use dma_max_mapping_size() instead of a workaround
Hi Geert, Christoph, Thank you for your comments! > From: Geert Uytterhoeven, Sent: Friday, June 14, 2019 4:27 PM > > Hi Christoph, > > On Fri, Jun 14, 2019 at 9:18 AM Christoph Hellwig wrote: > > On Thu, Jun 13, 2019 at 10:35:44PM +0200, Geert Uytterhoeven wrote: > > > I'm always triggered by the use of min_t() and other casts: > > > mmc->max_blk_size and mmc->max_blk_count are both unsigned int. > > > dma_max_mapping_size() returns size_t, which can be 64-bit. > > > > > > 1) Can the multiplication overflow? > > > Probably not, as per commit 2a55c1eac7882232 ("mmc: renesas_sdhi: > > > prevent overflow for max_req_size"), but I thought I'd better ask. Geert-san: I agree. > > > 2) In theory, dma_max_mapping_size() can return a number that doesn't > > > fit in 32-bit, and will be truncated (to e.g. 0), leading to > > > max_req_size > > > is zero? Geert-san: I agree. If dma_max_mapping_size() return 0x1__, it will be truncated to 0 and then max_req_size is set to zero. It is a problem. Also, the second argument "mmc->max_blk_size * mmc->max_blk_count" will not be overflow and then the value is 0x_ or less. So, I also think this should use size_t instead of unsigned int. > > This really should use a min_t on size_t. Otherwise the patch looks > > fine: > > Followed by another min() to make it fit in mmc->max_req_size, which is > unsigned int. Geert-san: I'm afraid, but I cannot understand this means. Is this patch is possible to be upstream? Or, do you have any concern? Best regards, Yoshihiro Shimoda
Re: [RFC PATCH v6 4/5] mmc: tmio: Use dma_max_mapping_size() instead of a workaround
Hi Shimoda-san, On Mon, Jun 17, 2019 at 6:54 AM Yoshihiro Shimoda wrote: > > From: Geert Uytterhoeven, Sent: Friday, June 14, 2019 4:27 PM > > On Fri, Jun 14, 2019 at 9:18 AM Christoph Hellwig wrote: > > > On Thu, Jun 13, 2019 at 10:35:44PM +0200, Geert Uytterhoeven wrote: > > > > I'm always triggered by the use of min_t() and other casts: > > > > mmc->max_blk_size and mmc->max_blk_count are both unsigned int. > > > > dma_max_mapping_size() returns size_t, which can be 64-bit. > > > > > > > > 1) Can the multiplication overflow? > > > > Probably not, as per commit 2a55c1eac7882232 ("mmc: renesas_sdhi: > > > > prevent overflow for max_req_size"), but I thought I'd better ask. > > Geert-san: > > I agree. > > > > > 2) In theory, dma_max_mapping_size() can return a number that doesn't > > > > fit in 32-bit, and will be truncated (to e.g. 0), leading to > > > > max_req_size > > > > is zero? > > Geert-san: > > I agree. If dma_max_mapping_size() return 0x1__, it will be truncated > to 0 > and then max_req_size is set to zero. It is a problem. Also, the second > argument > "mmc->max_blk_size * mmc->max_blk_count" will not be overflow and then the > value is > 0x_ or less. So, I also think this should use size_t instead of > unsigned int. > > > > This really should use a min_t on size_t. Otherwise the patch looks > > > fine: > > > > Followed by another min() to make it fit in mmc->max_req_size, which is > > unsigned int. > > Geert-san: > > I'm afraid, but I cannot understand this means. > Is this patch is possible to be upstream? Or, do you have any concern? Please disregard my last comment: as the value of "mmc->max_blk_size * mmc->max_blk_count" is always 0x_ or less, "min_t(size_t, mmc->max_blk_size * mmc->max_blk_count, dma_max_mapping_size(&pdev->dev))" will always be 0x_ or less, too, so there is no extra step needed to make it fit in mmc->max_req_size. Sorry for the confusion. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [RFC PATCH v6 4/5] mmc: tmio: Use dma_max_mapping_size() instead of a workaround
Hi Geert-san, > From: Geert Uytterhoeven, Sent: Monday, June 17, 2019 3:23 PM > > Hi Shimoda-san, > > On Mon, Jun 17, 2019 at 6:54 AM Yoshihiro Shimoda > wrote: > > > From: Geert Uytterhoeven, Sent: Friday, June 14, 2019 4:27 PM > > > On Fri, Jun 14, 2019 at 9:18 AM Christoph Hellwig wrote: > > > > On Thu, Jun 13, 2019 at 10:35:44PM +0200, Geert Uytterhoeven wrote: > > > > This really should use a min_t on size_t. Otherwise the patch looks > > > > fine: > > > > > > Followed by another min() to make it fit in mmc->max_req_size, which is > > > unsigned int. > > > > Geert-san: > > > > I'm afraid, but I cannot understand this means. > > Is this patch is possible to be upstream? Or, do you have any concern? > > Please disregard my last comment: as the value of "mmc->max_blk_size * > mmc->max_blk_count" is always 0x_ or less, "min_t(size_t, > mmc->max_blk_size * mmc->max_blk_count, dma_max_mapping_size(&pdev->dev))" > will always be 0x_ or less, too, so there is no extra step needed > to make it fit in mmc->max_req_size. Thank you for your prompt reply! I understood it. > Sorry for the confusion. No worries. Best regards, Yoshihiro Shimoda ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu