From: Todd Poynor <[email protected]>
Various fixes for gasket/apex drivers.
Nick Ewalt (8):
staging: gasket: page_table: remove unnecessary PTE status set to free
staging: gasket: page_table: rearrange gasket_page_table_entry
staging: gasket: page table: fixup error path allocating coherent mem
staging: gasket: page_table: fix comment in components_to_dev_address
staging: gasket: page_table: simplify gasket_components_to_dev_address
staging: gasket: apex: fix sysfs_show
staging: gasket: sysfs: fix attribute release comment
staging: gasket: Update device virtual address comment
Todd Poynor (3):
staging: gasket: core: debug log updates
staging: gasket: page table: return valid error code on map fail
staging: gasket: page table: remove dead code in coherent mem alloc
drivers/staging/gasket/apex_driver.c | 2 +-
drivers/staging/gasket/gasket_core.c | 9 +--
drivers/staging/gasket/gasket_page_table.c | 65 +++++++++-------------
drivers/staging/gasket/gasket_sysfs.h | 4 +-
4 files changed, 33 insertions(+), 47 deletions(-)
--
2.19.0.605.g01d371f741-goog
From: Todd Poynor <[email protected]>
Add debug logs for device enable/disable events, remove logs for
callbacks (the called functions can generate their own logs if needed).
Signed-off-by: Todd Poynor <[email protected]>
---
drivers/staging/gasket/gasket_core.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index f230bec76ae4e..62a7515915e59 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -109,8 +109,6 @@ check_and_invoke_callback(struct gasket_dev *gasket_dev,
{
int ret = 0;
- dev_dbg(gasket_dev->dev, "check_and_invoke_callback %p\n",
- cb_function);
if (cb_function) {
mutex_lock(&gasket_dev->mutex);
ret = cb_function(gasket_dev);
@@ -126,11 +124,8 @@ gasket_check_and_invoke_callback_nolock(struct gasket_dev *gasket_dev,
{
int ret = 0;
- if (cb_function) {
- dev_dbg(gasket_dev->dev,
- "Invoking device-specific callback.\n");
+ if (cb_function)
ret = cb_function(gasket_dev);
- }
return ret;
}
@@ -633,6 +628,7 @@ void gasket_disable_device(struct gasket_dev *gasket_dev)
gasket_dev->internal_desc->driver_desc;
int i;
+ dev_dbg(gasket_dev->dev, "disabling device\n");
/* Only delete the device if it has been successfully added. */
if (gasket_dev->dev_info.cdev_added)
cdev_del(&gasket_dev->dev_info.cdev);
@@ -1357,6 +1353,7 @@ int gasket_enable_device(struct gasket_dev *gasket_dev)
const struct gasket_driver_desc *driver_desc =
gasket_dev->internal_desc->driver_desc;
+ dev_dbg(gasket_dev->dev, "enabling device\n");
ret = gasket_interrupt_init(gasket_dev);
if (ret) {
dev_err(gasket_dev->dev,
--
2.19.0.605.g01d371f741-goog
From: Todd Poynor <[email protected]>
gasket_alloc_coherent_memory() has some unnecessary code related to out
of memory checking that will never hit the condition checked, remove.
Signed-off-by: Todd Poynor <[email protected]>
---
drivers/staging/gasket/gasket_page_table.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/staging/gasket/gasket_page_table.c b/drivers/staging/gasket/gasket_page_table.c
index 2e1de8ad4a2c6..985a3a93499d5 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -1316,7 +1316,6 @@ int gasket_alloc_coherent_memory(struct gasket_dev *gasket_dev, u64 size,
GFP_KERNEL);
if (!gasket_dev->page_table[index]->coherent_pages)
goto nomem;
- *dma_address = 0;
gasket_dev->coherent_buffer.length_bytes =
PAGE_SIZE * (num_pages);
@@ -1331,15 +1330,12 @@ int gasket_alloc_coherent_memory(struct gasket_dev *gasket_dev, u64 size,
(u64)mem + j * PAGE_SIZE;
}
- if (*dma_address == 0)
- goto nomem;
return 0;
nomem:
- if (mem) {
+ if (mem)
dma_free_coherent(gasket_get_device(gasket_dev),
num_pages * PAGE_SIZE, mem, handle);
- }
kfree(gasket_dev->page_table[index]->coherent_pages);
gasket_dev->page_table[index]->coherent_pages = NULL;
--
2.19.0.605.g01d371f741-goog
From: Nick Ewalt <[email protected]>
Correctly clean up data structure state in gasket_alloc_coherent_memory
error path, to ensure no double free on the stale pointer value.
Signed-off-by: Nick Ewalt <[email protected]>
Signed-off-by: Todd Poynor <[email protected]>
---
drivers/staging/gasket/gasket_page_table.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/gasket/gasket_page_table.c b/drivers/staging/gasket/gasket_page_table.c
index 9c2f8671216b5..a88f2ae0cee8b 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -1332,9 +1332,13 @@ int gasket_alloc_coherent_memory(struct gasket_dev *gasket_dev, u64 size,
return 0;
nomem:
- if (mem)
+ if (mem) {
dma_free_coherent(gasket_get_device(gasket_dev),
num_pages * PAGE_SIZE, mem, handle);
+ gasket_dev->coherent_buffer.length_bytes = 0;
+ gasket_dev->coherent_buffer.virt_base = NULL;
+ gasket_dev->coherent_buffer.phys_base = 0;
+ }
kfree(gasket_dev->page_table[index]->coherent_pages);
gasket_dev->page_table[index]->coherent_pages = NULL;
--
2.19.0.605.g01d371f741-goog
From: Nick Ewalt <[email protected]>
Add that number of page table entries and extended address bit offset
are configurable. Update example virtual address format to be more
consistent with typical usage.
Signed-off-by: Nick Ewalt <[email protected]>
Signed-off-by: Todd Poynor <[email protected]>
---
drivers/staging/gasket/gasket_page_table.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/gasket/gasket_page_table.c b/drivers/staging/gasket/gasket_page_table.c
index c2fbab74194f8..5b398b7ba81d3 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -10,10 +10,18 @@
*
* This file assumes 4kB pages throughout; can be factored out when necessary.
*
- * Address format is as follows:
+ * There is a configurable number of page table entries, as well as a
+ * configurable bit index for the extended address flag. Both of these are
+ * specified in gasket_page_table_init through the page_table_config parameter.
+ *
+ * The following example assumes:
+ * page_table_config->total_entries = 8192
+ * page_table_config->extended_bit = 63
+ *
+ * Address format:
* Simple addresses - those whose containing pages are directly placed in the
* device's address translation registers - are laid out as:
- * [ 63 - 40: Unused | 39 - 28: 0 | 27 - 12: page index | 11 - 0: page offset ]
+ * [ 63 - 25: 0 | 24 - 12: page index | 11 - 0: page offset ]
* page index: The index of the containing page in the device's address
* translation registers.
* page offset: The index of the address into the containing page.
@@ -21,7 +29,7 @@
* Extended address - those whose containing pages are contained in a second-
* level page table whose address is present in the device's address translation
* registers - are laid out as:
- * [ 63 - 40: Unused | 39: flag | 38 - 37: 0 | 36 - 21: dev/level 0 index |
+ * [ 63: flag | 62 - 34: 0 | 33 - 21: dev/level 0 index |
* 20 - 12: host/level 1 index | 11 - 0: page offset ]
* flag: Marker indicating that this is an extended address. Always 1.
* dev index: The index of the first-level page in the device's extended
--
2.19.0.605.g01d371f741-goog
From: Nick Ewalt <[email protected]>
sysfs_show was incorrectly extracting the sysfs_attribute_type from the
gasket_sysfs_attribute. This prevented dispatch from working properly.
Signed-off-by: Nick Ewalt <[email protected]>
Signed-off-by: Todd Poynor <[email protected]>
---
drivers/staging/gasket/apex_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
index 6dca3b177863c..3c7a13a4798ef 100644
--- a/drivers/staging/gasket/apex_driver.c
+++ b/drivers/staging/gasket/apex_driver.c
@@ -526,7 +526,7 @@ static ssize_t sysfs_show(struct device *device, struct device_attribute *attr,
return -ENODEV;
}
- type = (enum sysfs_attribute_type)gasket_sysfs_get_attr(device, attr);
+ type = (enum sysfs_attribute_type)gasket_attr->data.attr_type;
switch (type) {
case ATTR_KERNEL_HIB_PAGE_TABLE_SIZE:
ret = scnprintf(buf, PAGE_SIZE, "%u\n",
--
2.19.0.605.g01d371f741-goog
From: Nick Ewalt <[email protected]>
Comments for gasket_sysfs_get_attr() incorrectly describe reference
release procedure.
Signed-off-by: Nick Ewalt <[email protected]>
Signed-off-by: Todd Poynor <[email protected]>
---
drivers/staging/gasket/gasket_sysfs.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/gasket/gasket_sysfs.h b/drivers/staging/gasket/gasket_sysfs.h
index f32eaf89e056b..151e8edd28ea6 100644
--- a/drivers/staging/gasket/gasket_sysfs.h
+++ b/drivers/staging/gasket/gasket_sysfs.h
@@ -152,8 +152,8 @@ void gasket_sysfs_put_device_data(struct device *device,
* Returns the Gasket sysfs attribute associated with the kernel device
* attribute and device structure itself. Upon success, this call will take a
* reference to internal sysfs data that must be released with a call to
- * gasket_sysfs_get_device_data. While this reference is held, the underlying
- * device sysfs information/structure will remain valid/will not be deleted.
+ * gasket_sysfs_put_attr. While this reference is held, the underlying device
+ * sysfs information/structure will remain valid/will not be deleted.
*/
struct gasket_sysfs_attribute *
gasket_sysfs_get_attr(struct device *device, struct device_attribute *attr);
--
2.19.0.605.g01d371f741-goog
From: Nick Ewalt <[email protected]>
Comments in components_to_dev_address() describing examples are
inconsistent, fix these to be accurate.
Signed-off-by: Nick Ewalt <[email protected]>
Signed-off-by: Todd Poynor <[email protected]>
---
drivers/staging/gasket/gasket_page_table.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/gasket/gasket_page_table.c b/drivers/staging/gasket/gasket_page_table.c
index a88f2ae0cee8b..ec9359576ea7c 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -687,13 +687,13 @@ static inline bool gasket_addr_is_simple(struct gasket_page_table *pg_tbl,
* Convert (simple, page, offset) into a device address.
* Examples:
* Simple page 0, offset 32:
- * Input (0, 0, 32), Output 0x20
+ * Input (1, 0, 32), Output 0x20
* Simple page 1000, offset 511:
- * Input (0, 1000, 512), Output 0x3E81FF
+ * Input (1, 1000, 511), Output 0x3E81FF
* Extended page 0, offset 32:
* Input (0, 0, 32), Output 0x8000000020
* Extended page 1000, offset 511:
- * Input (1, 1000, 512), Output 0x8003E81FF
+ * Input (0, 1000, 511), Output 0x8003E81FF
*/
static ulong gasket_components_to_dev_address(struct gasket_page_table *pg_tbl,
int is_simple, uint page_index,
--
2.19.0.605.g01d371f741-goog
From: Nick Ewalt <[email protected]>
Rearrange gasket_page_table entry to reduce padding slop.
Signed-off-by: Nick Ewalt <[email protected]>
Signed-off-by: Todd Poynor <[email protected]>
---
drivers/staging/gasket/gasket_page_table.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/gasket/gasket_page_table.c b/drivers/staging/gasket/gasket_page_table.c
index d2e115d2dba30..9c2f8671216b5 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -103,12 +103,6 @@ struct gasket_page_table_entry {
/* The status of this entry/slot: free or in use. */
enum pte_status status;
- /* Address of the page in DMA space. */
- dma_addr_t dma_addr;
-
- /* Linux page descriptor for the page described by this structure. */
- struct page *page;
-
/*
* Index for alignment into host vaddrs.
* When a user specifies a host address for a mapping, that address may
@@ -119,6 +113,12 @@ struct gasket_page_table_entry {
*/
int offset;
+ /* Address of the page in DMA space. */
+ dma_addr_t dma_addr;
+
+ /* Linux page descriptor for the page described by this structure. */
+ struct page *page;
+
/*
* If this is an extended and first-level entry, sublevel points
* to the second-level entries underneath this entry.
--
2.19.0.605.g01d371f741-goog
From: Nick Ewalt <[email protected]>
Remove unnecessary ptes[i].status update in gasket_perform_unmapping.
The vaaue will be cleared in the following memset.
Signed-off-by: Nick Ewalt <[email protected]>
Signed-off-by: Todd Poynor <[email protected]>
---
drivers/staging/gasket/gasket_page_table.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/staging/gasket/gasket_page_table.c b/drivers/staging/gasket/gasket_page_table.c
index 985a3a93499d5..d2e115d2dba30 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -623,7 +623,6 @@ static void gasket_perform_unmapping(struct gasket_page_table *pg_tbl,
if (gasket_release_page(ptes[i].page))
--pg_tbl->num_active_pages;
}
- ptes[i].status = PTE_FREE;
/* and clear the PTE. */
memset(&ptes[i], 0, sizeof(struct gasket_page_table_entry));
--
2.19.0.605.g01d371f741-goog
From: Nick Ewalt <[email protected]>
Refactor gasket_components_to_dev_address to be faster and easier to
understand. The old implementation was unnecessarily complex and masked
the page_index for simple addresses but not extended ones. It makes the
most sense for this function to perform no such masking.
Signed-off-by: Nick Ewalt <[email protected]>
Signed-off-by: Todd Poynor <[email protected]>
---
drivers/staging/gasket/gasket_page_table.c | 21 ++-------------------
1 file changed, 2 insertions(+), 19 deletions(-)
diff --git a/drivers/staging/gasket/gasket_page_table.c b/drivers/staging/gasket/gasket_page_table.c
index ec9359576ea7c..c2fbab74194f8 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -699,26 +699,9 @@ static ulong gasket_components_to_dev_address(struct gasket_page_table *pg_tbl,
int is_simple, uint page_index,
uint offset)
{
- ulong lvl0_index, lvl1_index;
+ ulong dev_addr = (page_index << GASKET_SIMPLE_PAGE_SHIFT) | offset;
- if (is_simple) {
- /* Return simple addresses directly. */
- lvl0_index = page_index & (pg_tbl->config.total_entries - 1);
- return (lvl0_index << GASKET_SIMPLE_PAGE_SHIFT) | offset;
- }
-
- /*
- * This could be compressed into fewer statements, but
- * A) the compiler should optimize it
- * B) this is not slow
- * C) this is an uncommon operation
- * D) this is actually readable this way.
- */
- lvl0_index = page_index / GASKET_PAGES_PER_SUBTABLE;
- lvl1_index = page_index & (GASKET_PAGES_PER_SUBTABLE - 1);
- return (pg_tbl)->extended_flag |
- (lvl0_index << GASKET_EXTENDED_LVL0_SHIFT) |
- (lvl1_index << GASKET_EXTENDED_LVL1_SHIFT) | offset;
+ return is_simple ? dev_addr : (pg_tbl->extended_flag | dev_addr);
}
/*
--
2.19.0.605.g01d371f741-goog
From: Todd Poynor <[email protected]>
Return -EINVAL on mapping failures, instead of -1, which triggers a
checkpatch error.
Signed-off-by: Todd Poynor <[email protected]>
---
drivers/staging/gasket/gasket_page_table.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/gasket/gasket_page_table.c b/drivers/staging/gasket/gasket_page_table.c
index 964146f0df526..2e1de8ad4a2c6 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -514,13 +514,12 @@ static int gasket_perform_mapping(struct gasket_page_table *pg_tbl,
(void *)page_to_pfn(page),
(void *)page_to_phys(page));
- /* clean up */
if (gasket_release_page(ptes[i].page))
--pg_tbl->num_active_pages;
memset(&ptes[i], 0,
sizeof(struct gasket_page_table_entry));
- return -1;
+ return -EINVAL;
}
}
@@ -1165,7 +1164,7 @@ int gasket_page_table_lookup_page(
*ppage = NULL;
*poffset = 0;
mutex_unlock(&pg_tbl->mutex);
- return -1;
+ return -EINVAL;
}
/* See gasket_page_table.h for description. */
--
2.19.0.605.g01d371f741-goog
On Sun, Oct 14, 2018 at 09:59:16PM -0700, Todd Poynor wrote:
> From: Todd Poynor <[email protected]>
>
> Various fixes for gasket/apex drivers.
For some reason you seem to have ignored/missed this patch:
Subject: [PATCH v3] staging: gasket: Fix sparse "incorrect type in assignment" warnings.
that was sent last week.
Any specific reason?
thanks,
greg k-h
On Sun, Oct 14, 2018 at 09:59:17PM -0700, Todd Poynor wrote:
> From: Todd Poynor <[email protected]>
>
> Add debug logs for device enable/disable events,
Why?
What is going to need this?
> remove logs for
> callbacks (the called functions can generate their own logs if needed).
That's a different thing than "adding" them, so shouldn't this really be
2 patches? If it was, I could have accepted this patch already, and
ignored the "add new logs" one :)
thanks,
greg k-h
On Sun, Oct 14, 2018 at 09:59:21PM -0700, Todd Poynor wrote:
> From: Nick Ewalt <[email protected]>
>
> Rearrange gasket_page_table entry to reduce padding slop.
In the future, defining exactly what "slop" is here would be nice :)
thanks,
greg k-h
On Mon, Oct 15, 2018 at 12:33 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Sun, Oct 14, 2018 at 09:59:16PM -0700, Todd Poynor wrote:
> > From: Todd Poynor <[email protected]>
> >
> > Various fixes for gasket/apex drivers.
>
> For some reason you seem to have ignored/missed this patch:
> Subject: [PATCH v3] staging: gasket: Fix sparse "incorrect type in assignment" warnings.
> that was sent last week.
>
> Any specific reason?
Coming off a 2 week vacation, I'm behind on everything. I saw you had
commented already, but I'll take a look as well, sure.
>
> thanks,
>
> greg k-h
--
Todd
On Sun, Oct 14, 2018 at 09:59:16PM -0700, Todd Poynor wrote:
> From: Todd Poynor <[email protected]>
>
> Various fixes for gasket/apex drivers.
I still can't find any explanation in the staging tree or in your
comments what gasket even is.
On Mon, Oct 15, 2018 at 03:29:49AM -0700, Christoph Hellwig wrote:
> On Sun, Oct 14, 2018 at 09:59:16PM -0700, Todd Poynor wrote:
> > From: Todd Poynor <[email protected]>
> >
> > Various fixes for gasket/apex drivers.
>
> I still can't find any explanation in the staging tree or in your
> comments what gasket even is.
No one really knows, it will all eventually be deleted and they will
just use the UIO interface in the end, but it's fun to watch them
work themselves toward that over time :)
greg k-h
On Mon, Oct 15, 2018 at 12:34 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Sun, Oct 14, 2018 at 09:59:17PM -0700, Todd Poynor wrote:
> > From: Todd Poynor <[email protected]>
> >
> > Add debug logs for device enable/disable events,
>
> Why?
>
> What is going to need this?
As one of the few people actually developing for Apex chips (but this
may be more soon), I ran into a situation where I cared about this.
But I usually cobble together custom debugging for development
situations, and generally don't get debug logs for
released-in-the-wild drivers, so I'm fine not including these or any
other debug logs.
It sounds like debug logs face a pretty high bar for acceptance. I'm
happy to send a patch removing all of 'em from gasket/apex if that's
preferred.
> > remove logs for
> > callbacks (the called functions can generate their own logs if needed).
>
> That's a different thing than "adding" them, so shouldn't this really be
> 2 patches? If it was, I could have accepted this patch already, and
> ignored the "add new logs" one :)
Sure, the callbacks most frequently occur during disable/enable events
and were linked in my brain, but will send a new patch to just remove
the useless callback logs.
>
> thanks,
>
> greg k-h