| Message ID | 20240125-mmc-no-blk-bounce-high-v1-1-d0f92a30e085@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: core Drop BLK_BOUNCE_HIGH | expand |
On Thu, Jan 25, 2024, at 09:50, Linus Walleij wrote: > The MMC core sets BLK_BOUNCE_HIGH for devices where dma_mask > is unassigned. > > For the majority of MMC hosts this path is never taken: the > OF core will unconditionally assign a 32-bit mask to any > OF device, and most MMC hosts are probed from device tree, > see drivers/of/platform.c: > > of_platform_device_create_pdata() > dev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > if (!dev->dev.dma_mask) > dev->dev.dma_mask = &dev->dev.coherent_dma_mask; > > of_amba_device_create() > dev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > dev->dev.dma_mask = &dev->dev.coherent_dma_mask; > > MMC devices that are probed from ACPI or PCI will likewise > have a proper dma_mask assigned. > > The only remaining devices that could have a blank dma_mask > are platform devices instantiated from board files. > > These are mostly used on systems without CONFIG_HIGHMEM > enabled which means the block layer will not bounce, and in > the few cases where it is enabled it is not used anyway: > for example some OMAP2 systems such as Nokia n800/n810 will > create a platform_device and not assign a dma_mask, however > they do not have any highmem, so no bouncing will happen > anyway: the block core checks if max_low_pfn >= max_pfn > and this will always be false. > > Should it turn out there is a platform_device with blank > DMA mask actually using CONFIG_HIGHMEM somewhere out there > we should set dma_mask for it, not do this trickery. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Acked-by: Arnd Bergmann <arnd@arndb.de> I think it's worth mentioning the cb710 example here, which uses a platform device as a child of a PCI device and does not assign a DMA mask nor use DMA. This one will see a change in behavior, meaning that the blockdev buffers are no longer bounced. As far as I can tell, this is fine because the driver appears to correctly use the sg_iter infrastructure for mapping data pages, but it would be good to have this confirmed by Michał Mirosław because this code path has probably never been tested without BLK_BOUNCE_HIGH. Adding Michał to Cc. Arnd
From the block POV: awesome,
Acked-by: Christoph Hellwig <hch@lst.de>
On Thu, 25 Jan 2024 at 09:50, Linus Walleij <linus.walleij@linaro.org> wrote: > > The MMC core sets BLK_BOUNCE_HIGH for devices where dma_mask > is unassigned. > > For the majority of MMC hosts this path is never taken: the > OF core will unconditionally assign a 32-bit mask to any > OF device, and most MMC hosts are probed from device tree, > see drivers/of/platform.c: > > of_platform_device_create_pdata() > dev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > if (!dev->dev.dma_mask) > dev->dev.dma_mask = &dev->dev.coherent_dma_mask; > > of_amba_device_create() > dev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > dev->dev.dma_mask = &dev->dev.coherent_dma_mask; > > MMC devices that are probed from ACPI or PCI will likewise > have a proper dma_mask assigned. > > The only remaining devices that could have a blank dma_mask > are platform devices instantiated from board files. > > These are mostly used on systems without CONFIG_HIGHMEM > enabled which means the block layer will not bounce, and in > the few cases where it is enabled it is not used anyway: > for example some OMAP2 systems such as Nokia n800/n810 will > create a platform_device and not assign a dma_mask, however > they do not have any highmem, so no bouncing will happen > anyway: the block core checks if max_low_pfn >= max_pfn > and this will always be false. > > Should it turn out there is a platform_device with blank > DMA mask actually using CONFIG_HIGHMEM somewhere out there > we should set dma_mask for it, not do this trickery. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Applied for next, thanks! Kind regards Uffe > --- > drivers/mmc/core/queue.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c > index a0a2412f62a7..316415588a77 100644 > --- a/drivers/mmc/core/queue.c > +++ b/drivers/mmc/core/queue.c > @@ -351,8 +351,6 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card) > if (mmc_can_erase(card)) > mmc_queue_setup_discard(mq->queue, card); > > - if (!mmc_dev(host)->dma_mask || !*mmc_dev(host)->dma_mask) > - blk_queue_bounce_limit(mq->queue, BLK_BOUNCE_HIGH); > blk_queue_max_hw_sectors(mq->queue, > min(host->max_blk_count, host->max_req_size / 512)); > if (host->can_dma_map_merge) > > --- > base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d > change-id: 20240124-mmc-no-blk-bounce-high-d84e8898c707 > > Best regards, > -- > Linus Walleij <linus.walleij@linaro.org> >
On Thu, Jan 25, 2024 at 10:47:26AM +0100, Arnd Bergmann wrote: > On Thu, Jan 25, 2024, at 09:50, Linus Walleij wrote: > > The MMC core sets BLK_BOUNCE_HIGH for devices where dma_mask > > is unassigned. > > > > For the majority of MMC hosts this path is never taken: the > > OF core will unconditionally assign a 32-bit mask to any > > OF device, and most MMC hosts are probed from device tree, > > see drivers/of/platform.c: > > > > of_platform_device_create_pdata() > > dev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > > if (!dev->dev.dma_mask) > > dev->dev.dma_mask = &dev->dev.coherent_dma_mask; > > > > of_amba_device_create() > > dev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > > dev->dev.dma_mask = &dev->dev.coherent_dma_mask; > > > > MMC devices that are probed from ACPI or PCI will likewise > > have a proper dma_mask assigned. > > > > The only remaining devices that could have a blank dma_mask > > are platform devices instantiated from board files. > > > > These are mostly used on systems without CONFIG_HIGHMEM > > enabled which means the block layer will not bounce, and in > > the few cases where it is enabled it is not used anyway: > > for example some OMAP2 systems such as Nokia n800/n810 will > > create a platform_device and not assign a dma_mask, however > > they do not have any highmem, so no bouncing will happen > > anyway: the block core checks if max_low_pfn >= max_pfn > > and this will always be false. > > > > Should it turn out there is a platform_device with blank > > DMA mask actually using CONFIG_HIGHMEM somewhere out there > > we should set dma_mask for it, not do this trickery. > > > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > > Acked-by: Arnd Bergmann <arnd@arndb.de> > > I think it's worth mentioning the cb710 example here, which > uses a platform device as a child of a PCI device and > does not assign a DMA mask nor use DMA. > > This one will see a change in behavior, meaning that the > blockdev buffers are no longer bounced. As far as I can > tell, this is fine because the driver appears to correctly > use the sg_iter infrastructure for mapping data pages, > but it would be good to have this confirmed by > Michał Mirosław because this code path has probably never > been tested without BLK_BOUNCE_HIGH. Hi, this driver doesn't do DMA at all, so having DMA mask set or not it should be good as long as the CPU can read/write the buffers. Best Regards Michał Mirosław
On Fri, Feb 9, 2024 at 11:35 PM Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote: > > I think it's worth mentioning the cb710 example here, which > > uses a platform device as a child of a PCI device and > > does not assign a DMA mask nor use DMA. > > > > This one will see a change in behavior, meaning that the > > blockdev buffers are no longer bounced. As far as I can > > tell, this is fine because the driver appears to correctly > > use the sg_iter infrastructure for mapping data pages, > > but it would be good to have this confirmed by > > Michał Mirosław because this code path has probably never > > been tested without BLK_BOUNCE_HIGH. > > Hi, this driver doesn't do DMA at all, so having DMA mask set or not > it should be good as long as the CPU can read/write the buffers. The only difference is where the CPU have to read/write the buffers really, before the change those were all guaranteed to be in lowmem (bounced there by the block core), now they can also be in highmem, but sg_miter will deal with it for sure. Yours, Linus Wallej
On Sat, Feb 10, 2024, at 00:41, Linus Walleij wrote: > On Fri, Feb 9, 2024 at 11:35 PM Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote: > >> > I think it's worth mentioning the cb710 example here, which >> > uses a platform device as a child of a PCI device and >> > does not assign a DMA mask nor use DMA. >> > >> > This one will see a change in behavior, meaning that the >> > blockdev buffers are no longer bounced. As far as I can >> > tell, this is fine because the driver appears to correctly >> > use the sg_iter infrastructure for mapping data pages, >> > but it would be good to have this confirmed by >> > Michał Mirosław because this code path has probably never >> > been tested without BLK_BOUNCE_HIGH. >> >> Hi, this driver doesn't do DMA at all, so having DMA mask set or not >> it should be good as long as the CPU can read/write the buffers. > > The only difference is where the CPU have to read/write the > buffers really, before the change those were all guaranteed to > be in lowmem (bounced there by the block core), now they can > also be in highmem, but sg_miter will deal with it for sure. Yes, that was my point: The sg_miter() code is meant to handle exactly this case with highmem data, but as far as I can tell, that code path has never been tested on 32-bit systems with highmem but without BLK_BOUNCE_HIGH. Arnd
On Sat, Feb 10, 2024 at 12:58 PM Arnd Bergmann <arnd@arndb.de> wrote: > On Sat, Feb 10, 2024, at 00:41, Linus Walleij wrote: > > The only difference is where the CPU have to read/write the > > buffers really, before the change those were all guaranteed to > > be in lowmem (bounced there by the block core), now they can > > also be in highmem, but sg_miter will deal with it for sure. > > Yes, that was my point: The sg_miter() code is meant to > handle exactly this case with highmem data, but as far > as I can tell, that code path has never been tested on > 32-bit systems with highmem but without BLK_BOUNCE_HIGH. It's actually possible to enforce testing of highmem scatterlists to an MMC card (one need to be careful as this is destructive testing!) drivers/mmc/core/mmc_test.c ...but the one relevant target I have is a Kirkwood and it only has 128 MB of memory so highmem won't be exercised. I'll put this into the cover letter on the other series (fixing a bunch of drivers to use sg_miter) though. Yours, Linus Walleij
On Sat, Feb 10, 2024, at 20:38, Linus Walleij wrote: > On Sat, Feb 10, 2024 at 12:58 PM Arnd Bergmann <arnd@arndb.de> wrote: >> On Sat, Feb 10, 2024, at 00:41, Linus Walleij wrote: > >> > The only difference is where the CPU have to read/write the >> > buffers really, before the change those were all guaranteed to >> > be in lowmem (bounced there by the block core), now they can >> > also be in highmem, but sg_miter will deal with it for sure. >> >> Yes, that was my point: The sg_miter() code is meant to >> handle exactly this case with highmem data, but as far >> as I can tell, that code path has never been tested on >> 32-bit systems with highmem but without BLK_BOUNCE_HIGH. > > It's actually possible to enforce testing of highmem scatterlists > to an MMC card (one need to be careful as this is destructive > testing!) > drivers/mmc/core/mmc_test.c > > ...but the one relevant target I have is a Kirkwood and it only > has 128 MB of memory so highmem won't be exercised. I think you can pass a vmalloc= command line option to the kernel that will increase the size of the vmalloc are at the expense of lowmem and give you some highmem instead. Arnd
diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c index a0a2412f62a7..316415588a77 100644 --- a/drivers/mmc/core/queue.c +++ b/drivers/mmc/core/queue.c @@ -351,8 +351,6 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card) if (mmc_can_erase(card)) mmc_queue_setup_discard(mq->queue, card); - if (!mmc_dev(host)->dma_mask || !*mmc_dev(host)->dma_mask) - blk_queue_bounce_limit(mq->queue, BLK_BOUNCE_HIGH); blk_queue_max_hw_sectors(mq->queue, min(host->max_blk_count, host->max_req_size / 512)); if (host->can_dma_map_merge)
The MMC core sets BLK_BOUNCE_HIGH for devices where dma_mask is unassigned. For the majority of MMC hosts this path is never taken: the OF core will unconditionally assign a 32-bit mask to any OF device, and most MMC hosts are probed from device tree, see drivers/of/platform.c: of_platform_device_create_pdata() dev->dev.coherent_dma_mask = DMA_BIT_MASK(32); if (!dev->dev.dma_mask) dev->dev.dma_mask = &dev->dev.coherent_dma_mask; of_amba_device_create() dev->dev.coherent_dma_mask = DMA_BIT_MASK(32); dev->dev.dma_mask = &dev->dev.coherent_dma_mask; MMC devices that are probed from ACPI or PCI will likewise have a proper dma_mask assigned. The only remaining devices that could have a blank dma_mask are platform devices instantiated from board files. These are mostly used on systems without CONFIG_HIGHMEM enabled which means the block layer will not bounce, and in the few cases where it is enabled it is not used anyway: for example some OMAP2 systems such as Nokia n800/n810 will create a platform_device and not assign a dma_mask, however they do not have any highmem, so no bouncing will happen anyway: the block core checks if max_low_pfn >= max_pfn and this will always be false. Should it turn out there is a platform_device with blank DMA mask actually using CONFIG_HIGHMEM somewhere out there we should set dma_mask for it, not do this trickery. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/mmc/core/queue.c | 2 -- 1 file changed, 2 deletions(-) --- base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d change-id: 20240124-mmc-no-blk-bounce-high-d84e8898c707 Best regards,