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.
In addition to that, all the code for managing the dma_pool for GSMI
platform device is dropped.
Signed-off-by: Furquan Shaikh <[email protected]>
Reviewed-by: Prashant Malani <[email protected]>
---
drivers/firmware/google/gsmi.c | 100 +++++++++++++++++++++++----------
1 file changed, 71 insertions(+), 29 deletions(-)
diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/google/gsmi.c
index 7d9367b22010..054c47346900 100644
--- a/drivers/firmware/google/gsmi.c
+++ b/drivers/firmware/google/gsmi.c
@@ -17,7 +17,6 @@
#include <linux/string.h>
#include <linux/spinlock.h>
#include <linux/dma-mapping.h>
-#include <linux/dmapool.h>
#include <linux/fs.h>
#include <linux/slab.h>
#include <linux/ioctl.h>
@@ -46,7 +45,6 @@
#define DRIVER_VERSION "1.0"
#define GSMI_GUID_SIZE 16
#define GSMI_BUF_SIZE 1024
-#define GSMI_BUF_ALIGN sizeof(u64)
#define GSMI_CALLBACK 0xef
/* SMI return codes */
@@ -85,10 +83,15 @@
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 */
};
+struct gsmi_page {
+ unsigned long base_address; /* Base address of allocated page */
+ size_t alloc_size; /* Size of allocation */
+ size_t alloc_used; /* Amount of allocation that is used */
+};
+
static struct gsmi_device {
struct platform_device *pdev; /* platform device */
struct gsmi_buf *name_buf; /* variable name buffer */
@@ -97,7 +100,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 gsmi_page *page_info; /* page allocated for GSMI buffers */
} gsmi_dev;
/* Packed structures for communicating with the firmware */
@@ -146,9 +149,57 @@ MODULE_PARM_DESC(spincount,
static bool s0ix_logging_enable;
module_param(s0ix_logging_enable, bool, 0600);
+static struct gsmi_page *gsmi_page_alloc(void)
+{
+ struct gsmi_page *page_info;
+
+ page_info = kzalloc(sizeof(*page_info), GFP_KERNEL);
+ if (!page_info) {
+ printk(KERN_ERR "gsmi: out of memory\n");
+ return NULL;
+ }
+
+ page_info->base_address = get_zeroed_page(GFP_KERNEL | GFP_DMA32);
+ if (!page_info->base_address) {
+ printk(KERN_ERR "gsmi: failed to allocate page for buffers\n");
+ return NULL;
+ }
+
+ /* Ensure the entire buffer is allocated within 32bit address space */
+ if (virt_to_phys((void *)(page_info->base_address + PAGE_SIZE - 1))
+ >> 32) {
+ printk(KERN_ERR "gsmi: allocation not within 32-bit physical address space\n");
+ free_page(page_info->base_address);
+ kfree(page_info);
+ return NULL;
+ }
+
+ page_info->alloc_size = PAGE_SIZE;
+
+ return page_info;
+}
+
+static unsigned long gsmi_page_allocate_buffer(void)
+{
+ struct gsmi_page *page_info = gsmi_dev.page_info;
+ unsigned long buf_offset = page_info->alloc_used;
+
+ if (buf_offset + GSMI_BUF_SIZE > page_info->alloc_size) {
+ printk(KERN_ERR "gsmi: out of space for buffer allocation\n");
+ return 0;
+ }
+
+ page_info->alloc_used = buf_offset + GSMI_BUF_SIZE;
+ return page_info->base_address + buf_offset;
+}
+
static struct gsmi_buf *gsmi_buf_alloc(void)
{
struct gsmi_buf *smibuf;
+ u8 *buf_addr = (u8 *)gsmi_page_allocate_buffer();
+
+ if (!buf_addr)
+ return NULL;
smibuf = kzalloc(sizeof(*smibuf), GFP_KERNEL);
if (!smibuf) {
@@ -156,14 +207,7 @@ static struct gsmi_buf *gsmi_buf_alloc(void)
return NULL;
}
- /* allocate buffer in 32bit address space */
- smibuf->start = dma_pool_alloc(gsmi_dev.dma_pool, GFP_KERNEL,
- &smibuf->handle);
- if (!smibuf->start) {
- printk(KERN_ERR "gsmi: failed to allocate name buffer\n");
- kfree(smibuf);
- return NULL;
- }
+ smibuf->start = buf_addr;
/* fill in the buffer handle */
smibuf->length = GSMI_BUF_SIZE;
@@ -172,13 +216,15 @@ static struct gsmi_buf *gsmi_buf_alloc(void)
return smibuf;
}
-static void gsmi_buf_free(struct gsmi_buf *smibuf)
+static void gsmi_free_alloc(void)
{
- if (smibuf) {
- if (smibuf->start)
- dma_pool_free(gsmi_dev.dma_pool, smibuf->start,
- smibuf->handle);
- kfree(smibuf);
+ kfree(gsmi_dev.param_buf);
+ kfree(gsmi_dev.data_buf);
+ kfree(gsmi_dev.name_buf);
+
+ if (gsmi_dev.page_info) {
+ free_page(gsmi_dev.page_info->base_address);
+ kfree(gsmi_dev.page_info);
}
}
@@ -914,10 +960,12 @@ 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.page_info = gsmi_page_alloc();
+ if (!gsmi_dev.page_info) {
+ printk(KERN_ERR "gsmi: failed to allocate page\n");
goto out_err;
+ }
/*
* pre-allocate buffers because sometimes we are called when
@@ -1029,10 +1077,7 @@ static __init int gsmi_init(void)
sysfs_remove_bin_file(gsmi_kobj, &eventlog_bin_attr);
out_err:
kobject_put(gsmi_kobj);
- 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);
+ gsmi_free_alloc();
platform_device_unregister(gsmi_dev.pdev);
pr_info("gsmi: failed to load: %d\n", ret);
#ifdef CONFIG_PM
@@ -1054,10 +1099,7 @@ static void __exit gsmi_exit(void)
sysfs_remove_files(gsmi_kobj, gsmi_attrs);
sysfs_remove_bin_file(gsmi_kobj, &eventlog_bin_attr);
kobject_put(gsmi_kobj);
- 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);
+ gsmi_free_alloc();
platform_device_unregister(gsmi_dev.pdev);
#ifdef CONFIG_PM
platform_driver_unregister(&gsmi_driver_info);
--
2.29.0.rc1.297.gfa9743e501-goog
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.
thanks,
greg k-h
On Tue, Oct 20, 2020 at 11:37 PM Ard Biesheuvel <[email protected]> wrote:
>
> On Wed, 21 Oct 2020 at 07:18, Greg Kroah-Hartman
> <[email protected]> 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.
Thanks,
Furquan
On Wed, Oct 21, 2020 at 12:37:52AM -0700, Furquan Shaikh wrote:
> On Tue, Oct 20, 2020 at 11:37 PM Ard Biesheuvel <[email protected]> wrote:
> >
> > On Wed, 21 Oct 2020 at 07:18, Greg Kroah-Hartman
> > <[email protected]> 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?
thanks,
greg k-h
On Wed, 21 Oct 2020 at 10:51, Greg Kroah-Hartman
<[email protected]> 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 <[email protected]> wrote:
> > >
> > > On Wed, 21 Oct 2020 at 07:18, Greg Kroah-Hartman
> > > <[email protected]> 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);
On Wed, 21 Oct 2020 at 07:18, Greg Kroah-Hartman
<[email protected]> 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.
Hi Furquan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on linux/master v5.9 next-20201021]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Furquan-Shaikh/firmware-gsmi-Drop-the-use-of-dma_pool_-API-functions/20201021-130247
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git c4d6fe7311762f2e03b3c27ad38df7c40c80cc93
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/66886f5e6d40e829b8a48ab2dbb6615b906290a5
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Furquan-Shaikh/firmware-gsmi-Drop-the-use-of-dma_pool_-API-functions/20201021-130247
git checkout 66886f5e6d40e829b8a48ab2dbb6615b906290a5
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/firmware/google/gsmi.c: In function 'gsmi_page_alloc':
>> drivers/firmware/google/gsmi.c:170:6: warning: right shift count >= width of type [-Wshift-count-overflow]
170 | >> 32) {
| ^~
vim +170 drivers/firmware/google/gsmi.c
151
152 static struct gsmi_page *gsmi_page_alloc(void)
153 {
154 struct gsmi_page *page_info;
155
156 page_info = kzalloc(sizeof(*page_info), GFP_KERNEL);
157 if (!page_info) {
158 printk(KERN_ERR "gsmi: out of memory\n");
159 return NULL;
160 }
161
162 page_info->base_address = get_zeroed_page(GFP_KERNEL | GFP_DMA32);
163 if (!page_info->base_address) {
164 printk(KERN_ERR "gsmi: failed to allocate page for buffers\n");
165 return NULL;
166 }
167
168 /* Ensure the entire buffer is allocated within 32bit address space */
169 if (virt_to_phys((void *)(page_info->base_address + PAGE_SIZE - 1))
> 170 >> 32) {
171 printk(KERN_ERR "gsmi: allocation not within 32-bit physical address space\n");
172 free_page(page_info->base_address);
173 kfree(page_info);
174 return NULL;
175 }
176
177 page_info->alloc_size = PAGE_SIZE;
178
179 return page_info;
180 }
181
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Wed, Oct 21, 2020 at 2:36 AM Ard Biesheuvel <[email protected]> wrote:
>
> On Wed, 21 Oct 2020 at 10:51, Greg Kroah-Hartman
> <[email protected]> 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 <[email protected]> wrote:
> > > >
> > > > On Wed, 21 Oct 2020 at 07:18, Greg Kroah-Hartman
> > > > <[email protected]> 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);