| Message ID | 1560421215-10750-5-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | treewide: improve R-Car SDHI performance | expand |
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>
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
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>
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
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
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
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
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
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))
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(-)