Re: [PATCH] firmware: gsmi: Drop the use of dma_pool_* API functions

[Furquan Shaikh]

On Wed, Oct 21, 2020 at 2:36 AM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
>
> On Wed, 21 Oct 2020 at 10:51, Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Wed, Oct 21, 2020 at 12:37:52AM -0700, Furquan Shaikh wrote:
> > > On Tue, Oct 20, 2020 at 11:37 PM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
> > > >
> > > > On Wed, 21 Oct 2020 at 07:18, Greg Kroah-Hartman
> > > > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > On Tue, Oct 20, 2020 at 10:01:41PM -0700, Furquan Shaikh wrote:
> > > > > > GSMI driver uses dma_pool_* API functions for buffer allocation
> > > > > > because it requires that the SMI buffers are allocated within 32-bit
> > > > > > physical address space. However, this does not work well with IOMMU
> > > > > > since there is no real device and hence no domain associated with the
> > > > > > device.
> > > > > >
> > > > > > Since this is not a real device, it does not require any device
> > > > > > address(IOVA) for the buffer allocations. The only requirement is to
> > > > > > ensure that the physical address allocated to the buffer is within
> > > > > > 32-bit physical address space. This change allocates a page using
> > > > > > `get_zeroed_page()` and passes in GFP_DMA32 flag to ensure that the
> > > > > > page allocation is done in the DMA32 zone. All the buffer allocation
> > > > > > requests for gsmi_buf are then satisfed using this pre-allocated page
> > > > > > for the device.
> > > > >
> > > > > Are you sure that "GFP_DMA32" really does what you think it does?  A
> > > > > "normal" call with GFP_KERNEL" will give you memory that is properly
> > > > > dma-able.
> > > > >
> > > > > We should not be adding new GFP_DMA* users in the kernel in these days,
> > > > > just call dma_alloc*() and you should be fine.
> > > > >
> > > >
> > > > The point seems to be that this is not about DMA at all, and so there
> > > > is no device associated with the DMA allocation.
> > > >
> > > > The other 'master' is the CPU running firmware in an execution mode
> > > > where it can only access the bottom 4 GB of memory, and GFP_DMA32
> > > > happens to allocate from a zone which is guaranteed to be accessible
> > > > to the firmware.
> > >
> > > Ard captured the context and requirement perfectly. GFP_DMA32
> > > satisfies the requirement for allocating memory from a zone which is
> > > accessible to the firmware in SMI mode. This seems to be one of the
> > > common ways  how other drivers and common code in the kernel currently
> > > allocate physical memory below the 4G boundary. Hence, I used the same
> > > mechanism in GSMI driver.
> >
> > Then can you please document this a bit better in the changelog,
> > explaining why this is ok to use this obsolete api, and also in the code
> > itself so that no one tries to clean it up in the future?

I will do that and send out a newer revision of the patch. Thanks!

>
> >
>
> Wouldn't it be simpler to switch to a SLAB cache created with SLAB_CACHE_DMA32?

I considered doing that, but there is some weirdness around use of
GFP_DMA32 in `kmem_cache_zalloc` i.e. you can allocate slab cache
using SLAB_CACHE_DMA32, however you cannot pass in GFP_DMA32 flag when
using `kmem_cache_zalloc`, else it hits GFP_SLAB_BUG_MASK. If the
GFP_DMA32 flag is skipped, then `kmem_cache_zalloc` is happy and
provides an allocation under the 4G boundary.

Since this driver really just needs to allocate space for 3 buffers, I
went with allocating a page and handing out space for the buffers from
the page. If the SLAB_CACHE_DMA32 seems like a cleaner way to do this,
I can update the patch to use that.

Thanks,
Furquan

>
>
>
> I.e., something like the below (whitespace mangling courtesy of gmail)
>
> diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/google/gsmi.c
> index 7d9367b22010..d932284f970c 100644
> --- a/drivers/firmware/google/gsmi.c
> +++ b/drivers/firmware/google/gsmi.c
> @@ -85,7 +85,6 @@
>  struct gsmi_buf {
>         u8 *start;                      /* start of buffer */
>         size_t length;                  /* length of buffer */
> -       dma_addr_t handle;              /* dma allocation handle */
>         u32 address;                    /* physical address of buffer */
>  };
>
> @@ -97,7 +96,7 @@ static struct gsmi_device {
>         spinlock_t lock;                /* serialize access to SMIs */
>         u16 smi_cmd;                    /* SMI command port */
>         int handshake_type;             /* firmware handler interlock type */
> -       struct dma_pool *dma_pool;      /* DMA buffer pool */
> +       struct kmem_cache *mem_pool;    /* buffer pool */
>  } gsmi_dev;
>
>  /* Packed structures for communicating with the firmware */
> @@ -157,8 +156,7 @@ static struct gsmi_buf *gsmi_buf_alloc(void)
>         }
>
>         /* allocate buffer in 32bit address space */
> -       smibuf->start = dma_pool_alloc(gsmi_dev.dma_pool, GFP_KERNEL,
> -                                      &smibuf->handle);
> +       smibuf->start = kmem_cache_zalloc(gsmi_dev.mem_pool, GFP_KERNEL);
>         if (!smibuf->start) {
>                 printk(KERN_ERR "gsmi: failed to allocate name buffer\n");
>                 kfree(smibuf);
> @@ -176,8 +174,7 @@ static void gsmi_buf_free(struct gsmi_buf *smibuf)
>  {
>         if (smibuf) {
>                 if (smibuf->start)
> -                       dma_pool_free(gsmi_dev.dma_pool, smibuf->start,
> -                                     smibuf->handle);
> +                       kmem_cache_free(gsmi_dev.mem_pool, smibuf->start);
>                 kfree(smibuf);
>         }
>  }
> @@ -914,9 +911,10 @@ static __init int gsmi_init(void)
>         spin_lock_init(&gsmi_dev.lock);
>
>         ret = -ENOMEM;
> -       gsmi_dev.dma_pool = dma_pool_create("gsmi", &gsmi_dev.pdev->dev,
> -                                            GSMI_BUF_SIZE, GSMI_BUF_ALIGN, 0);
> -       if (!gsmi_dev.dma_pool)
> +       gsmi_dev.mem_pool = kmem_cache_create("gsmi", GSMI_BUF_SIZE,
> +                                             GSMI_BUF_ALIGN, SLAB_CACHE_DMA32,
> +                                             NULL);
> +       if (!gsmi_dev.mem_pool)
>                 goto out_err;
>
>         /*
> @@ -1032,7 +1030,7 @@ static __init int gsmi_init(void)
>         gsmi_buf_free(gsmi_dev.param_buf);
>         gsmi_buf_free(gsmi_dev.data_buf);
>         gsmi_buf_free(gsmi_dev.name_buf);
> -       dma_pool_destroy(gsmi_dev.dma_pool);
> +       kmem_cache_destroy(gsmi_dev.mem_pool);
>         platform_device_unregister(gsmi_dev.pdev);
>         pr_info("gsmi: failed to load: %d\n", ret);
>  #ifdef CONFIG_PM
> @@ -1057,7 +1055,7 @@ static void __exit gsmi_exit(void)
>         gsmi_buf_free(gsmi_dev.param_buf);
>         gsmi_buf_free(gsmi_dev.data_buf);
>         gsmi_buf_free(gsmi_dev.name_buf);
> -       dma_pool_destroy(gsmi_dev.dma_pool);
> +       kmem_cache_destroy(gsmi_dev.mem_pool);
>         platform_device_unregister(gsmi_dev.pdev);
>  #ifdef CONFIG_PM
>         platform_driver_unregister(&gsmi_driver_info);