diff mbox series

mmc: core Drop BLK_BOUNCE_HIGH

Message ID 20240125-mmc-no-blk-bounce-high-v1-1-d0f92a30e085@linaro.org
State New
Headers show
Series mmc: core Drop BLK_BOUNCE_HIGH | expand

Commit Message

Linus Walleij Jan. 25, 2024, 8:50 a.m. UTC
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,

Comments

Arnd Bergmann Jan. 25, 2024, 9:47 a.m. UTC | #1
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
Ulf Hansson Jan. 30, 2024, 12:06 p.m. UTC | #2
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>
>
Linus Walleij Feb. 9, 2024, 11:41 p.m. UTC | #3
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
Arnd Bergmann Feb. 10, 2024, 11:58 a.m. UTC | #4
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
Linus Walleij Feb. 10, 2024, 7:38 p.m. UTC | #5
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
Arnd Bergmann Feb. 10, 2024, 9:34 p.m. UTC | #6
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 mbox series

Patch

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)