[RFC,v6,4/5] mmc: tmio: Use dma_max_mapping_size() instead of a workaround
diff mbox series

Message ID 1560421215-10750-5-git-send-email-yoshihiro.shimoda.uh@renesas.com
State New
Headers show
Series
  • treewide: improve R-Car SDHI performance
Related show

Commit Message

Yoshihiro Shimoda June 13, 2019, 10:20 a.m. UTC
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 <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/mmc/host/tmio_mmc_core.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

Comments

Wolfram Sang June 13, 2019, 7:45 p.m. UTC | #1
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 <yoshihiro.shimoda.uh@renesas.com>

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 <wsa+renesas@sang-engineering.com>
Geert Uytterhoeven June 13, 2019, 8:35 p.m. UTC | #2
Hi Shimoda-san,

On Thu, Jun 13, 2019 at 5:37 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> 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 <yoshihiro.shimoda.uh@renesas.com>

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 -- geert@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
Christoph Hellwig June 14, 2019, 7:18 a.m. UTC | #3
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 <hch@lst.de>
Geert Uytterhoeven June 14, 2019, 7:27 a.m. UTC | #4
Hi Christoph,

On Fri, Jun 14, 2019 at 9:18 AM Christoph Hellwig <hch@lst.de> 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
Yoshihiro Shimoda June 17, 2019, 4:25 a.m. UTC | #5
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 <yoshihiro.shimoda.uh@renesas.com>
> 
> 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 <wsa+renesas@sang-engineering.com>

Thank you for your Acked-by!

Best regards,
Yoshihiro Shimoda
Yoshihiro Shimoda June 17, 2019, 4:54 a.m. UTC | #6
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_0000_0000, 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
0xffff_ffff 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
Geert Uytterhoeven June 17, 2019, 6:23 a.m. UTC | #7
Hi Shimoda-san,

On Mon, Jun 17, 2019 at 6:54 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> 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_0000_0000, 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
> 0xffff_ffff 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 0xffff_ffff or less, "min_t(size_t,
mmc->max_blk_size * mmc->max_blk_count, dma_max_mapping_size(&pdev->dev))"
will always be 0xffff_ffff 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
Yoshihiro Shimoda June 17, 2019, 6:54 a.m. UTC | #8
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
> <yoshihiro.shimoda.uh@renesas.com> 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:
<snip>
> > > > 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 0xffff_ffff or less, "min_t(size_t,
> mmc->max_blk_size * mmc->max_blk_count, dma_max_mapping_size(&pdev->dev))"
> will always be 0xffff_ffff 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

Patch
diff mbox series

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 <linux/delay.h>
 #include <linux/device.h>
+#include <linux/dma-mapping.h>
 #include <linux/highmem.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
@@ -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))