lkml.org 
[lkml]   [2020]   [Oct]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH] firmware: gsmi: Drop the use of dma_pool_* API functions
    On Wed, 21 Oct 2020 at 10:51, Greg Kroah-Hartman
    <gregkh@linuxfoundation.org> 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@kernel.org> wrote:
    > > >
    > > > On Wed, 21 Oct 2020 at 07:18, Greg Kroah-Hartman
    > > > <gregkh@linuxfoundation.org> 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?
    >

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


    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);
    \
     
     \ /
      Last update: 2020-10-21 11:36    [W:4.271 / U:0.508 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site