[PATCH 00/32 v2] staging: gasket: sundry fixes and fixups

[PATCH 00/32 v2] staging: gasket: sundry fixes and fixups

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Various fixes mainly from the chromium review of the gasket and apex
drivers.  More to come.

Various (32):
  staging: gasket: remove X86 Kconfig restriction
  staging: gasket: fix typo in apex_enter_reset
  staging: gasket: fix typo in gasket_core.h comments
  staging: gasket: whitespace fix in gasket_page_table_init
  staging: gasket: remove driver registration on class creation failure
  staging: gasket: hold mutex on gasket driver unregistration
  staging: gasket: Return EBUSY on mapping create when already in use
  staging: gasket: Remove stale pointers on error allocating attr array
  staging: gasket: convert gasket_mmap_has_permissions to bool return
  staging: gasket: fix gasket_wait_with_reschedule timeout return code
  staging: gasket: gasket_wait_with_reschedule use msleep
  staging: gasket: gasket_wait_with_reschedule return when condition hit
  staging: gasket: gasket_wait_with_reschedule use 32 bits of retry
    count
  staging: gasket: bail out of reset sequence on device callback error
  staging: gasket: drop gasket_cdev_get_info, use container_of
  staging: gasket: always allow root open for write
  staging: gasket: annotate ioctl arg with __user
  staging: gasket: gasket_enable_dev remove unnecessary variable
  staging: gasket: remove code for no physical device
  staging: gasket: fix class create bug handling
  staging: gasket: remove unnecessary code in coherent allocator
  staging: gasket: don't treat no device reset callback as an error
  staging: gasket: gasket_mmap return error instead of valid BAR index
  staging: gasket: apex_clock_gating simplify logic, reduce indentation
  staging: gasket: apex_ioctl_check_permissions use bool return type
  staging: gasket: gasket page table functions use bool return type
  staging: gasket: remove else clause after return in if clause
  staging: gasket: fix comment syntax in apex.h
  staging: gasket: remove unnecessary parens in page table code
  staging: gasket: gasket_mmap use PAGE_MASK
  staging: gasket: remove extra parens in gasket_write_mappable_regions
  staging: gasket: fix multi-line comment syntax in gasket_core.h

 drivers/staging/gasket/Kconfig             |   2 +-
 drivers/staging/gasket/apex.h              |   7 +-
 drivers/staging/gasket/apex_driver.c       |  79 ++++++++--------
 drivers/staging/gasket/gasket_core.c       | 105 +++++++++------------
 drivers/staging/gasket/gasket_core.h       |  11 ++-
 drivers/staging/gasket/gasket_ioctl.c      |  91 +++++++++---------
 drivers/staging/gasket/gasket_ioctl.h      |   4 +-
 drivers/staging/gasket/gasket_page_table.c |  67 +++++++------
 drivers/staging/gasket/gasket_page_table.h |   8 +-
 drivers/staging/gasket/gasket_sysfs.c      |   4 +-
 10 files changed, 177 insertions(+), 201 deletions(-)

-- 
2.18.0.203.gfac676dfb9-goog

Patches changed from version 1:
  staging: gasket: typo and whitespace cleanups
     Split into new separate patches:
        staging: gasket: whitespace fix in gasket_page_table_init
        staging: gasket: fix typo in gasket_core.h comments
        staging: gasket: fix typo in apex_enter_reset
  staging: gasket: device registration error and unregister fixups
     Split into new separate patches:
        staging: gasket: remove driver registration on class creation failure
	staging: gasket: hold mutex on gasket driver unregistration
  staging: gasket: sysfs mapping creation fixups
     Split into new separate patches:
        staging: gasket: Return EBUSY on mapping create when already in use
        staging: gasket: Remove stale pointers on error allocating attr array
  staging: gasket: gasket_wait_with_reschedule fixups
     Split into new separate patches:
        staging: gasket: fix gasket_wait_with_reschedule timeout return code
	staging: gasket: gasket_wait_with_reschedule use msleep
	staging: gasket: gasket_wait_with_reschedule return when condition hit
  staging: gasket: gasket_open use container_of()
      Replaced by new patch:
        staging: gasket: drop gasket_cdev_get_info, use container_of
  staging: gasket: always allow root open for write
     Add Reviewed-by from Dmitry Torokhov.
  staging: gasket: gasket_enable_dev fixups
     Split into new separate patches:
        staging: gasket: gasket_enable_dev remove unnecessary variable
	staging: gasket: remove code for no physical device
  staging: gasket: fix class create bug handling
     Add Reviewed-by from Dmitry Torokhov.
  staging: gasket: gasket core error handling fixups
     Split into new separate patches:
        staging: gasket: don't treat no device reset callback as an error
	staging: gasket: gasket_mmap return error instead of valid BAR index
  staging: gasket: various cleanups
     Split into new separate patches:
        staging: gasket: apex_clock_gating simplify logic, reduce indentation
	staging: gasket: apex_ioctl_check_permissions use bool return type
	staging: gasket: gasket page table functions use bool return type
	staging: gasket: remove else clause after return in if clause
	staging: gasket: fix comment syntax in apex.h
	staging: gasket: remove unnecessary parens in page table code
	staging: gasket: gasket_mmap use PAGE_MASK
	staging: gasket: remove extra parens in gasket_write_mappable_regions
	staging: gasket: fix multi-line comment syntax in gasket_core.h

Patches dropped from version 1:
  MAINTAINERS: Add maintainer for drivers/staging/gasket
     Need to arrange a positive handoff from existing maintainer, not
     available now.
  staging: gasket: fix deadlock in pci driver unregister path
     Wrapped PCI calls broken, needs a rewrite.
  staging: gasket: don't release coherent mappings
     Not necessary.

Patches unchanged from version 1:
  staging: gasket: remove X86 Kconfig restriction
  staging: gasket: convert gasket_mmap_has_permissions to bool return
  staging: gasket: bail out of reset sequence on device callback error
  staging: gasket: always allow root open for write
  staging: gasket: annotate ioctl arg with __user   
  staging: gasket: remove unnecessary code in coherent allocator

[PATCH 01/32] staging: gasket: remove X86 Kconfig restriction

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

The gasket and apex drivers are to be used on other architectures
besides X86.

Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/gasket/Kconfig b/drivers/staging/gasket/Kconfig
index c836389c1402d..ef40c4c75e0f2 100644
--- a/drivers/staging/gasket/Kconfig
+++ b/drivers/staging/gasket/Kconfig
@@ -2,7 +2,7 @@ menu "Gasket devices"
 
 config STAGING_GASKET_FRAMEWORK
 	tristate "Gasket framework"
-	depends on PCI && X86_64
+	depends on PCI
 	help
 	  This framework supports Gasket-compatible devices, such as Apex.
 	  It is required for any of the following module(s).
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 01/32] staging: gasket: remove X86 Kconfig restriction

[Greg Kroah-Hartman]

On Mon, Jul 16, 2018 at 07:08:55PM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> The gasket and apex drivers are to be used on other architectures
> besides X86.
> 
> Signed-off-by: Todd Poynor <toddpoynor@google.com>
> ---
>  drivers/staging/gasket/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/gasket/Kconfig b/drivers/staging/gasket/Kconfig
> index c836389c1402d..ef40c4c75e0f2 100644
> --- a/drivers/staging/gasket/Kconfig
> +++ b/drivers/staging/gasket/Kconfig
> @@ -2,7 +2,7 @@ menu "Gasket devices"
>  
>  config STAGING_GASKET_FRAMEWORK
>  	tristate "Gasket framework"
> -	depends on PCI && X86_64
> +	depends on PCI

I was waiting to see how badly 0-day barfed on this one, now dropping it
from my queue :)

thanks,

greg k-h

[PATCH 02/32] staging: gasket: fix typo in apex_enter_reset

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Fix typo in log message.

Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 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 cca4cf491a583..a31937dfff836 100644
--- a/drivers/staging/gasket/apex_driver.c
+++ b/drivers/staging/gasket/apex_driver.c
@@ -488,7 +488,7 @@ static int apex_enter_reset(struct gasket_dev *gasket_dev, uint type)
 					APEX_BAR2_REG_USER_HIB_DMA_PAUSED, 1, 1,
 					APEX_RESET_DELAY, APEX_RESET_RETRY)) {
 		gasket_log_error(gasket_dev,
-				 "DMAs did not quiece within timeout (%d ms)",
+				 "DMAs did not quiesce within timeout (%d ms)",
 				 APEX_RESET_RETRY * APEX_RESET_DELAY);
 		return -EINVAL;
 	}
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 03/32] staging: gasket: fix typo in gasket_core.h comments

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Grammar fixup in gasket_core.h comments describing struct
gasket_interrupt_desc.

Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/gasket/gasket_core.h b/drivers/staging/gasket/gasket_core.h
index e51acadc0fc4f..94a5537bff2c2 100644
--- a/drivers/staging/gasket/gasket_core.h
+++ b/drivers/staging/gasket/gasket_core.h
@@ -56,7 +56,7 @@ enum gasket_interrupt_type {
 
 /* Used to describe a Gasket interrupt. Contains an interrupt index, a register,
  * and packing data for that interrupt. The register and packing data
- * fields is relevant only for PCI_MSIX interrupt type and can be
+ * fields are relevant only for PCI_MSIX interrupt type and can be
  * set to 0 for everything else.
  */
 struct gasket_interrupt_desc {
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 04/32] staging: gasket: whitespace fix in gasket_page_table_init

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Tab replaced with space.

Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_page_table.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/gasket/gasket_page_table.c b/drivers/staging/gasket/gasket_page_table.c
index dcd52e141f95d..36a560c87af36 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -347,7 +347,7 @@ int gasket_page_table_init(
 	    pg_tbl->config.mode == GASKET_PAGE_TABLE_MODE_SIMPLE) {
 		pg_tbl->num_simple_entries = total_entries;
 		pg_tbl->num_extended_entries = 0;
-		pg_tbl->extended_flag =	1ull << page_table_config->extended_bit;
+		pg_tbl->extended_flag = 1ull << page_table_config->extended_bit;
 	} else {
 		pg_tbl->num_simple_entries = 0;
 		pg_tbl->num_extended_entries = total_entries;
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 05/32] staging: gasket: remove driver registration on class creation failure

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

If class_create() fails, remove the gasket driver from the global
registration table.

Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index b69b630f1b79b..cbadab7544c81 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -335,7 +335,8 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
 	if (IS_ERR_OR_NULL(internal->class)) {
 		gasket_nodev_error("Cannot register %s class [ret=%ld]",
 				   driver_desc->name, PTR_ERR(internal->class));
-		return PTR_ERR(internal->class);
+		ret = PTR_ERR(internal->class);
+		goto unregister_gasket_driver;
 	}
 
 	/*
@@ -369,6 +370,7 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
 fail1:
 	class_destroy(internal->class);
 
+unregister_gasket_driver:
 	g_descs[desc_idx].driver_desc = NULL;
 	return ret;
 }
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 06/32] staging: gasket: hold mutex on gasket driver unregistration

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Take the global mutex on driver unregistration updates for proper
ordering of updates and consistent access procedures.

Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index cbadab7544c81..2ff328652356a 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -371,7 +371,9 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
 	class_destroy(internal->class);
 
 unregister_gasket_driver:
+	mutex_lock(&g_mutex);
 	g_descs[desc_idx].driver_desc = NULL;
+	mutex_unlock(&g_mutex);
 	return ret;
 }
 EXPORT_SYMBOL(gasket_register_device);
@@ -408,7 +410,9 @@ void gasket_unregister_device(const struct gasket_driver_desc *driver_desc)
 	class_destroy(internal_desc->class);
 
 	/* Finally, effectively "remove" the driver. */
+	mutex_lock(&g_mutex);
 	g_descs[desc_idx].driver_desc = NULL;
+	mutex_unlock(&g_mutex);
 
 	gasket_nodev_info("removed %s driver", driver_desc->name);
 }
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 07/32] staging: gasket: Return EBUSY on mapping create when already in use

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

gasket_sysfs_create_mapping() return EBUSY if sysfs mapping already in
use.

Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/gasket/gasket_sysfs.c b/drivers/staging/gasket/gasket_sysfs.c
index e3d770630961b..dd4d3aaa57e2f 100644
--- a/drivers/staging/gasket/gasket_sysfs.c
+++ b/drivers/staging/gasket/gasket_sysfs.c
@@ -194,7 +194,7 @@ int gasket_sysfs_create_mapping(
 			"0x%p.", device);
 		put_mapping(mapping);
 		mutex_unlock(&function_mutex);
-		return -EINVAL;
+		return -EBUSY;
 	}
 
 	/* Find the first empty entry in the array. */
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 08/32] staging: gasket: Remove stale pointers on error allocating attr array

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

If gasket_sysfs_create_mapping() hits errors allocating the attribute
array, remove stale pointers to device info from the mapping object.

Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_sysfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/gasket/gasket_sysfs.c b/drivers/staging/gasket/gasket_sysfs.c
index dd4d3aaa57e2f..1c5f7502e0d5e 100644
--- a/drivers/staging/gasket/gasket_sysfs.c
+++ b/drivers/staging/gasket/gasket_sysfs.c
@@ -225,6 +225,8 @@ int gasket_sysfs_create_mapping(
 	mapping->attribute_count = 0;
 	if (!mapping->attributes) {
 		gasket_nodev_error("Unable to allocate sysfs attribute array.");
+		mapping->device = NULL;
+		mapping->gasket_dev = NULL;
 		mutex_unlock(&mapping->mutex);
 		mutex_unlock(&function_mutex);
 		return -ENOMEM;
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 09/32] staging: gasket: convert gasket_mmap_has_permissions to bool return

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

gasket_mmap_has_permissions() should return a boolean value.

Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 2ff328652356a..248d717e1df65 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -1241,19 +1241,19 @@ static int gasket_release(struct inode *inode, struct file *file)
  * that the provided descriptor/range is of adequate size to hold the range to
  * be mapped.
  */
-static int gasket_mmap_has_permissions(
+static bool gasket_mmap_has_permissions(
 	struct gasket_dev *gasket_dev, struct vm_area_struct *vma,
 	int bar_permissions)
 {
 	int requested_permissions;
 	/* Always allow sysadmin to access. */
 	if (capable(CAP_SYS_ADMIN))
-		return 1;
+		return true;
 
 	/* Never allow non-sysadmins to access to a dead device. */
 	if (gasket_dev->status != GASKET_STATUS_ALIVE) {
 		gasket_log_info(gasket_dev, "Device is dead.");
-		return 0;
+		return false;
 	}
 
 	/* Make sure that no wrong flags are set. */
@@ -1265,7 +1265,7 @@ static int gasket_mmap_has_permissions(
 			"Attempting to map a region with requested permissions "
 			"0x%x, but region has permissions 0x%x.",
 			requested_permissions, bar_permissions);
-		return 0;
+		return false;
 	}
 
 	/* Do not allow a non-owner to write. */
@@ -1275,10 +1275,10 @@ static int gasket_mmap_has_permissions(
 			gasket_dev,
 			"Attempting to mmap a region for write without owning "
 			"device.");
-		return 0;
+		return false;
 	}
 
-	return 1;
+	return true;
 }
 
 /*
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 10/32] staging: gasket: fix gasket_wait_with_reschedule timeout return code

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Return -ETIMEDOUT, not -EINVAL, on timeout, including callers.

Reported-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/apex_driver.c | 8 ++++----
 drivers/staging/gasket/gasket_core.c | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
index a31937dfff836..3a83c3d4d5561 100644
--- a/drivers/staging/gasket/apex_driver.c
+++ b/drivers/staging/gasket/apex_driver.c
@@ -490,7 +490,7 @@ static int apex_enter_reset(struct gasket_dev *gasket_dev, uint type)
 		gasket_log_error(gasket_dev,
 				 "DMAs did not quiesce within timeout (%d ms)",
 				 APEX_RESET_RETRY * APEX_RESET_DELAY);
-		return -EINVAL;
+		return -ETIMEDOUT;
 	}
 
 	/*  - Enable GCB reset (0x1 to rg_rst_gcb) */
@@ -513,7 +513,7 @@ static int apex_enter_reset(struct gasket_dev *gasket_dev, uint type)
 			gasket_dev,
 			"RAM did not shut down within timeout (%d ms)",
 			APEX_RESET_RETRY * APEX_RESET_DELAY);
-		return -EINVAL;
+		return -ETIMEDOUT;
 	}
 
 	return 0;
@@ -562,7 +562,7 @@ static int apex_quit_reset(struct gasket_dev *gasket_dev, uint type)
 			gasket_dev,
 			"RAM did not enable within timeout (%d ms)",
 			APEX_RESET_RETRY * APEX_RESET_DELAY);
-		return -EINVAL;
+		return -ETIMEDOUT;
 	}
 
 	/*    - Wait for Reset complete. */
@@ -574,7 +574,7 @@ static int apex_quit_reset(struct gasket_dev *gasket_dev, uint type)
 			gasket_dev,
 			"GCB did not leave reset within timeout (%d ms)",
 			APEX_RESET_RETRY * APEX_RESET_DELAY);
-		return -EINVAL;
+		return -ETIMEDOUT;
 	}
 
 	if (!allow_hw_clock_gating) {
diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 248d717e1df65..803566229bfcb 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -2106,7 +2106,7 @@ int gasket_wait_with_reschedule(
 			"%s timeout: reg %llx timeout (%llu ms)",
 			__func__,
 			offset, max_retries * delay_ms);
-		return -EINVAL;
+		return -ETIMEDOUT;
 	}
 	return 0;
 }
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 11/32] staging: gasket: gasket_wait_with_reschedule use msleep

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Replace schedule_timeout() call with msleep().

Reported-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 803566229bfcb..442543573f6e9 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -14,6 +14,7 @@
 #include "gasket_page_table.h"
 #include "gasket_sysfs.h"
 
+#include <linux/delay.h>
 #include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/of.h>
@@ -2097,7 +2098,7 @@ int gasket_wait_with_reschedule(
 		tmp = gasket_dev_read_64(gasket_dev, bar, offset);
 		if ((tmp & mask) == val)
 			break;
-		schedule_timeout(msecs_to_jiffies(delay_ms));
+		msleep(delay_ms);
 		retries++;
 	}
 	if (retries == max_retries) {
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 11/32] staging: gasket: gasket_wait_with_reschedule use msleep

[Dan Carpenter]

On Mon, Jul 16, 2018 at 07:09:05PM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> Replace schedule_timeout() call with msleep().
> 

Why does it matter?  I genuinely don't know the rules here.  The commit
message should normally say if there is a user visible effect or if it's
just a clean up or whatever.

regards,
dan carpenter

[PATCH 12/32] staging: gasket: gasket_wait_with_reschedule return when condition hit

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Return right away instead of break out of while and then return.

Reported-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 442543573f6e9..85116cc46f311 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -2097,7 +2097,7 @@ int gasket_wait_with_reschedule(
 	while (retries < max_retries) {
 		tmp = gasket_dev_read_64(gasket_dev, bar, offset);
 		if ((tmp & mask) == val)
-			break;
+			return 0;
 		msleep(delay_ms);
 		retries++;
 	}
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 12/32] staging: gasket: gasket_wait_with_reschedule return when condition hit

[Dan Carpenter]

On Mon, Jul 16, 2018 at 07:09:06PM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> Return right away instead of break out of while and then return.
> 

You could remove the (retries == max_retries) condition since that's
always true now and pull that code in one tab.  You could do it in one
patch because they're closely related so it's One Thing [tm].

regads,
dan carpenter

Re: [PATCH 12/32] staging: gasket: gasket_wait_with_reschedule return when condition hit

[Dan Carpenter]

On Tue, Jul 17, 2018 at 01:13:29PM +0300, Dan Carpenter wrote:
> On Mon, Jul 16, 2018 at 07:09:06PM -0700, Todd Poynor wrote:
> > From: Todd Poynor <toddpoynor@google.com>
> > 
> > Return right away instead of break out of while and then return.
> > 
>

Btw, I wasn't going to complain about this but since Greg is being extra
critical about commit messages that don't explain *why* it just creates
an environment why I also feel free to complain...  I couldn't
understand what this commit does without looking at the code in context.

A possible commit message here is:

[PATCH 12/32] staging: gasket: small cleanup in gasket_wait_with_reschedule()

gasket_wait_with_reschedule() is a little more clear if we just return
directly.

regards,
dan carpenter

Re: [PATCH 12/32] staging: gasket: gasket_wait_with_reschedule return when condition hit

[Todd Poynor]

On Tue, Jul 17, 2018 at 4:12 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Tue, Jul 17, 2018 at 01:13:29PM +0300, Dan Carpenter wrote:
>> On Mon, Jul 16, 2018 at 07:09:06PM -0700, Todd Poynor wrote:
>> > From: Todd Poynor <toddpoynor@google.com>
>> >
>> > Return right away instead of break out of while and then return.
>> >
>>
>
> Btw, I wasn't going to complain about this but since Greg is being extra
> critical about commit messages that don't explain *why* it just creates
> an environment why I also feel free to complain...  I couldn't
> understand what this commit does without looking at the code in context.
>
> A possible commit message here is:
>
> [PATCH 12/32] staging: gasket: small cleanup in gasket_wait_with_reschedule()
>
> gasket_wait_with_reschedule() is a little more clear if we just return
> directly.

I've updated the text for v3, thanks -- Todd

>
> regards,
> dan carpenter
>



-- 
Todd

Re: [PATCH 12/32] staging: gasket: gasket_wait_with_reschedule return when condition hit

[Todd Poynor]

On Tue, Jul 17, 2018 at 3:13 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Mon, Jul 16, 2018 at 07:09:06PM -0700, Todd Poynor wrote:
>> From: Todd Poynor <toddpoynor@google.com>
>>
>> Return right away instead of break out of while and then return.
>>
>
> You could remove the (retries == max_retries) condition since that's
> always true now and pull that code in one tab.  You could do it in one
> patch because they're closely related so it's One Thing [tm].

Ah yes, also removed the impossible-to-reach "return 0" at the end,
thanks -- Todd

>
> regads,
> dan carpenter

[PATCH 13/32] staging: gasket: gasket_wait_with_reschedule use 32 bits of retry count

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Don't need a 64-bit retry counter.

Reported-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 4 ++--
 drivers/staging/gasket/gasket_core.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 85116cc46f311..a7f696d8e1026 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -2089,9 +2089,9 @@ struct device *gasket_get_device(struct gasket_dev *dev)
  **/
 int gasket_wait_with_reschedule(
 	struct gasket_dev *gasket_dev, int bar, u64 offset, u64 mask, u64 val,
-	u64 max_retries, u64 delay_ms)
+	uint max_retries, u64 delay_ms)
 {
-	u64 retries = 0;
+	uint retries = 0;
 	u64 tmp;
 
 	while (retries < max_retries) {
diff --git a/drivers/staging/gasket/gasket_core.h b/drivers/staging/gasket/gasket_core.h
index 94a5537bff2c2..50ad0c8853183 100644
--- a/drivers/staging/gasket/gasket_core.h
+++ b/drivers/staging/gasket/gasket_core.h
@@ -702,6 +702,6 @@ struct device *gasket_get_device(struct gasket_dev *dev);
 /* Helper function, Asynchronous waits on a given set of bits. */
 int gasket_wait_with_reschedule(
 	struct gasket_dev *gasket_dev, int bar, u64 offset, u64 mask, u64 val,
-	u64 max_retries, u64 delay_ms);
+	uint max_retries, u64 delay_ms);
 
 #endif /* __GASKET_CORE_H__ */
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 14/32] staging: gasket: bail out of reset sequence on device callback error

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

If device reset callback returns an error, error out at the gasket
level.

Reported-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index a7f696d8e1026..4808bac93f983 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -1875,9 +1875,11 @@ int gasket_reset_nolock(struct gasket_dev *gasket_dev, uint reset_type)
 
 	/* Perform a device reset of the requested type. */
 	ret = driver_desc->device_reset_cb(gasket_dev, reset_type);
-	if (ret)
+	if (ret) {
 		gasket_log_error(
 			gasket_dev, "Device reset cb returned %d.", ret);
+		return ret;
+	}
 
 	/* Reinitialize the page tables and interrupt framework. */
 	for (i = 0; i < driver_desc->num_page_tables; ++i)
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 15/32] staging: gasket: drop gasket_cdev_get_info, use container_of

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Remove gasket_cdev_get_info(), use container_of() directly instead,
drop unnecessary NULL checks.

Reported-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 26 ++++----------------------
 1 file changed, 4 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 4808bac93f983..ae20b7ff9569b 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -233,18 +233,6 @@ static inline int gasket_check_and_invoke_callback_nolock(
 	return ret;
 }
 
-/*
- * Retrieve device-specific data via cdev pointer.
- * @cdev_ptr: Character device pointer associated with the device.
- *
- * This function returns the pointer to the device-specific data allocated in
- * add_dev_cb for the device associated with cdev_ptr.
- */
-static struct gasket_cdev_info *gasket_cdev_get_info(struct cdev *cdev_ptr)
-{
-	return container_of(cdev_ptr, struct gasket_cdev_info, cdev);
-}
-
 /*
  * Returns nonzero if the gasket_cdev_info is owned by the current thread group
  * ID.
@@ -1095,12 +1083,9 @@ static int gasket_open(struct inode *inode, struct file *filp)
 	const struct gasket_driver_desc *driver_desc;
 	struct gasket_ownership *ownership;
 	char task_name[TASK_COMM_LEN];
-	struct gasket_cdev_info *dev_info = gasket_cdev_get_info(inode->i_cdev);
+	struct gasket_cdev_info *dev_info =
+	    container_of(inode->i_cdev, struct gasket_cdev_info, cdev);
 
-	if (!dev_info) {
-		gasket_nodev_error("Unable to retrieve device data");
-		return -EINVAL;
-	}
 	gasket_dev = dev_info->gasket_dev_ptr;
 	driver_desc = gasket_dev->internal_desc->driver_desc;
 	ownership = &dev_info->ownership;
@@ -1182,11 +1167,8 @@ static int gasket_release(struct inode *inode, struct file *file)
 	const struct gasket_driver_desc *driver_desc;
 	char task_name[TASK_COMM_LEN];
 	struct gasket_cdev_info *dev_info =
-		(struct gasket_cdev_info *)gasket_cdev_get_info(inode->i_cdev);
-	if (!dev_info) {
-		gasket_nodev_error("Unable to retrieve device data");
-		return -EINVAL;
-	}
+		container_of(inode->i_cdev, struct gasket_cdev_info, cdev);
+
 	gasket_dev = dev_info->gasket_dev_ptr;
 	driver_desc = gasket_dev->internal_desc->driver_desc;
 	ownership = &dev_info->ownership;
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 16/32] staging: gasket: always allow root open for write

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Always allow root to open device for writing.

Drop special-casing of ioctl permissions for root vs. owner.

Reported-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
Reviewed-by: Dmitry Torokhov <dtor@chromium.org>
---
 drivers/staging/gasket/apex_driver.c  |  9 +++------
 drivers/staging/gasket/gasket_core.c  |  8 +++++---
 drivers/staging/gasket/gasket_ioctl.c | 15 ++++++---------
 3 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
index 3a83c3d4d5561..6fd09c45a3df6 100644
--- a/drivers/staging/gasket/apex_driver.c
+++ b/drivers/staging/gasket/apex_driver.c
@@ -630,13 +630,10 @@ static bool is_gcb_in_reset(struct gasket_dev *gasket_dev)
 static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
 {
 	struct gasket_dev *gasket_dev = filp->private_data;
-	int root = capable(CAP_SYS_ADMIN);
-	int is_owner = gasket_dev->dev_info.ownership.is_owned &&
-		       current->tgid == gasket_dev->dev_info.ownership.owner;
+	fmode_t write;
 
-	if (root || is_owner)
-		return 1;
-	return 0;
+	write = filp->f_mode & FMODE_WRITE;
+	return write;
 }
 
 /*
diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index ae20b7ff9569b..9d1bb3caf6de2 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -1085,6 +1085,7 @@ static int gasket_open(struct inode *inode, struct file *filp)
 	char task_name[TASK_COMM_LEN];
 	struct gasket_cdev_info *dev_info =
 	    container_of(inode->i_cdev, struct gasket_cdev_info, cdev);
+	int is_root = capable(CAP_SYS_ADMIN);
 
 	gasket_dev = dev_info->gasket_dev_ptr;
 	driver_desc = gasket_dev->internal_desc->driver_desc;
@@ -1098,7 +1099,7 @@ static int gasket_open(struct inode *inode, struct file *filp)
 		"Attempting to open with tgid %u (%s) (f_mode: 0%03o, "
 		"fmode_write: %d is_root: %u)",
 		current->tgid, task_name, filp->f_mode,
-		(filp->f_mode & FMODE_WRITE), capable(CAP_SYS_ADMIN));
+		(filp->f_mode & FMODE_WRITE), is_root);
 
 	/* Always allow non-writing accesses. */
 	if (!(filp->f_mode & FMODE_WRITE)) {
@@ -1112,8 +1113,9 @@ static int gasket_open(struct inode *inode, struct file *filp)
 		gasket_dev, "Current owner open count (owning tgid %u): %d.",
 		ownership->owner, ownership->write_open_count);
 
-	/* Opening a node owned by another TGID is an error (even root.) */
-	if (ownership->is_owned && ownership->owner != current->tgid) {
+	/* Opening a node owned by another TGID is an error (unless root) */
+	if (ownership->is_owned && ownership->owner != current->tgid &&
+	    !is_root) {
 		gasket_log_error(
 			gasket_dev,
 			"Process %u is opening a node held by %u.",
diff --git a/drivers/staging/gasket/gasket_ioctl.c b/drivers/staging/gasket/gasket_ioctl.c
index 0c2f85cf54480..17431d14e6ef1 100644
--- a/drivers/staging/gasket/gasket_ioctl.c
+++ b/drivers/staging/gasket/gasket_ioctl.c
@@ -171,7 +171,7 @@ long gasket_is_supported_ioctl(uint cmd)
  */
 static uint gasket_ioctl_check_permissions(struct file *filp, uint cmd)
 {
-	uint alive, root, device_owner;
+	uint alive;
 	fmode_t read, write;
 	struct gasket_dev *gasket_dev = (struct gasket_dev *)filp->private_data;
 
@@ -183,33 +183,30 @@ static uint gasket_ioctl_check_permissions(struct file *filp, uint cmd)
 			alive, gasket_dev->status);
 	}
 
-	root = capable(CAP_SYS_ADMIN);
 	read = filp->f_mode & FMODE_READ;
 	write = filp->f_mode & FMODE_WRITE;
-	device_owner = (gasket_dev->dev_info.ownership.is_owned &&
-			current->tgid == gasket_dev->dev_info.ownership.owner);
 
 	switch (cmd) {
 	case GASKET_IOCTL_RESET:
 	case GASKET_IOCTL_CLEAR_INTERRUPT_COUNTS:
-		return root || (write && device_owner);
+		return write;
 
 	case GASKET_IOCTL_PAGE_TABLE_SIZE:
 	case GASKET_IOCTL_SIMPLE_PAGE_TABLE_SIZE:
 	case GASKET_IOCTL_NUMBER_PAGE_TABLES:
-		return root || read;
+		return read;
 
 	case GASKET_IOCTL_PARTITION_PAGE_TABLE:
 	case GASKET_IOCTL_CONFIG_COHERENT_ALLOCATOR:
-		return alive && (root || (write && device_owner));
+		return alive && write;
 
 	case GASKET_IOCTL_MAP_BUFFER:
 	case GASKET_IOCTL_UNMAP_BUFFER:
-		return alive && (root || (write && device_owner));
+		return alive && write;
 
 	case GASKET_IOCTL_CLEAR_EVENTFD:
 	case GASKET_IOCTL_SET_EVENTFD:
-		return alive && (root || (write && device_owner));
+		return alive && write;
 	}
 
 	return 0; /* unknown permissions */
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 16/32] staging: gasket: always allow root open for write

[Dan Carpenter]

On Mon, Jul 16, 2018 at 07:09:10PM -0700, Todd Poynor wrote:
> --- a/drivers/staging/gasket/apex_driver.c
> +++ b/drivers/staging/gasket/apex_driver.c
> @@ -630,13 +630,10 @@ static bool is_gcb_in_reset(struct gasket_dev *gasket_dev)
>  static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)

This function name is a bit of out of date.

>  {
>  	struct gasket_dev *gasket_dev = filp->private_data;
> -	int root = capable(CAP_SYS_ADMIN);
> -	int is_owner = gasket_dev->dev_info.ownership.is_owned &&
> -		       current->tgid == gasket_dev->dev_info.ownership.owner;
> +	fmode_t write;
>  
> -	if (root || is_owner)
> -		return 1;
> -	return 0;
> +	write = filp->f_mode & FMODE_WRITE;
> +	return write;

This doesn't match the comment because it returns 0x2 or zero.

	return !!(filp->f_mode & FMODE_WRITE);

>  }
>  
>  /*
> diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
> index ae20b7ff9569b..9d1bb3caf6de2 100644
> --- a/drivers/staging/gasket/gasket_core.c
> +++ b/drivers/staging/gasket/gasket_core.c
> @@ -1085,6 +1085,7 @@ static int gasket_open(struct inode *inode, struct file *filp)
>  	char task_name[TASK_COMM_LEN];
>  	struct gasket_cdev_info *dev_info =
>  	    container_of(inode->i_cdev, struct gasket_cdev_info, cdev);
> +	int is_root = capable(CAP_SYS_ADMIN);
>  
>  	gasket_dev = dev_info->gasket_dev_ptr;
>  	driver_desc = gasket_dev->internal_desc->driver_desc;
> @@ -1098,7 +1099,7 @@ static int gasket_open(struct inode *inode, struct file *filp)
>  		"Attempting to open with tgid %u (%s) (f_mode: 0%03o, "
>  		"fmode_write: %d is_root: %u)",
>  		current->tgid, task_name, filp->f_mode,
> -		(filp->f_mode & FMODE_WRITE), capable(CAP_SYS_ADMIN));
> +		(filp->f_mode & FMODE_WRITE), is_root);
>  
>  	/* Always allow non-writing accesses. */
>  	if (!(filp->f_mode & FMODE_WRITE)) {
> @@ -1112,8 +1113,9 @@ static int gasket_open(struct inode *inode, struct file *filp)
>  		gasket_dev, "Current owner open count (owning tgid %u): %d.",
>  		ownership->owner, ownership->write_open_count);
>  
> -	/* Opening a node owned by another TGID is an error (even root.) */
> -	if (ownership->is_owned && ownership->owner != current->tgid) {
> +	/* Opening a node owned by another TGID is an error (unless root) */
> +	if (ownership->is_owned && ownership->owner != current->tgid &&
> +	    !is_root) {
>  		gasket_log_error(
>  			gasket_dev,
>  			"Process %u is opening a node held by %u.",
> diff --git a/drivers/staging/gasket/gasket_ioctl.c b/drivers/staging/gasket/gasket_ioctl.c
> index 0c2f85cf54480..17431d14e6ef1 100644
> --- a/drivers/staging/gasket/gasket_ioctl.c
> +++ b/drivers/staging/gasket/gasket_ioctl.c
> @@ -171,7 +171,7 @@ long gasket_is_supported_ioctl(uint cmd)
>   */
>  static uint gasket_ioctl_check_permissions(struct file *filp, uint cmd)
>  {
> -	uint alive, root, device_owner;
> +	uint alive;
>  	fmode_t read, write;
>  	struct gasket_dev *gasket_dev = (struct gasket_dev *)filp->private_data;
>  
> @@ -183,33 +183,30 @@ static uint gasket_ioctl_check_permissions(struct file *filp, uint cmd)
>  			alive, gasket_dev->status);
>  	}
>  
> -	root = capable(CAP_SYS_ADMIN);
>  	read = filp->f_mode & FMODE_READ;
>  	write = filp->f_mode & FMODE_WRITE;

write = !!(filp->f_mode & FMODE_WRITE);

> -	device_owner = (gasket_dev->dev_info.ownership.is_owned &&
> -			current->tgid == gasket_dev->dev_info.ownership.owner);
>  
>  	switch (cmd) {
>  	case GASKET_IOCTL_RESET:
>  	case GASKET_IOCTL_CLEAR_INTERRUPT_COUNTS:
> -		return root || (write && device_owner);
> +		return write;

regards,
dan carpenter

Re: [PATCH 16/32] staging: gasket: always allow root open for write

[Joe Perches]

On Tue, 2018-07-17 at 13:22 +0300, Dan Carpenter wrote:
> On Mon, Jul 16, 2018 at 07:09:10PM -0700, Todd Poynor wrote:
> > --- a/drivers/staging/gasket/apex_driver.c
> > +++ b/drivers/staging/gasket/apex_driver.c
> > @@ -630,13 +630,10 @@ static bool is_gcb_in_reset(struct gasket_dev *gasket_dev)
> >  static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
> 
> This function name is a bit of out of date.
> 
> >  {
> >  	struct gasket_dev *gasket_dev = filp->private_data;
> > -	int root = capable(CAP_SYS_ADMIN);
> > -	int is_owner = gasket_dev->dev_info.ownership.is_owned &&
> > -		       current->tgid == gasket_dev->dev_info.ownership.owner;
> > +	fmode_t write;
> >  
> > -	if (root || is_owner)
> > -		return 1;
> > -	return 0;
> > +	write = filp->f_mode & FMODE_WRITE;
> > +	return write;
> 
> This doesn't match the comment because it returns 0x2 or zero.
> 
> 	return !!(filp->f_mode & FMODE_WRITE);

Or maybe change the functions to return bool

Re: [PATCH 16/32] staging: gasket: always allow root open for write

[Todd Poynor]

On Tue, Jul 17, 2018 at 11:50 AM, Joe Perches <joe@perches.com> wrote:
> On Tue, 2018-07-17 at 13:22 +0300, Dan Carpenter wrote:
>> On Mon, Jul 16, 2018 at 07:09:10PM -0700, Todd Poynor wrote:
>> > --- a/drivers/staging/gasket/apex_driver.c
>> > +++ b/drivers/staging/gasket/apex_driver.c
>> > @@ -630,13 +630,10 @@ static bool is_gcb_in_reset(struct gasket_dev *gasket_dev)
>> >  static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
>>
>> This function name is a bit of out of date.
>>
>> >  {
>> >     struct gasket_dev *gasket_dev = filp->private_data;
>> > -   int root = capable(CAP_SYS_ADMIN);
>> > -   int is_owner = gasket_dev->dev_info.ownership.is_owned &&
>> > -                  current->tgid == gasket_dev->dev_info.ownership.owner;
>> > +   fmode_t write;
>> >
>> > -   if (root || is_owner)
>> > -           return 1;
>> > -   return 0;
>> > +   write = filp->f_mode & FMODE_WRITE;
>> > +   return write;
>>
>> This doesn't match the comment because it returns 0x2 or zero.

Will fix, I noticed this earlier and forgot to address it.

>>
>>       return !!(filp->f_mode & FMODE_WRITE);
>
> Or maybe change the functions to return bool

And there's another patch farther down in the stack that does that,
too, will keep them separate and fix up this intermediate patch for
now.

Thanks -- Todd

[PATCH 17/32] staging: gasket: annotate ioctl arg with __user

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

For sparse checking.

Reported-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/apex_driver.c  | 12 ++---
 drivers/staging/gasket/gasket_core.c  |  6 ++-
 drivers/staging/gasket/gasket_core.h  |  4 +-
 drivers/staging/gasket/gasket_ioctl.c | 72 ++++++++++++++-------------
 drivers/staging/gasket/gasket_ioctl.h |  4 +-
 5 files changed, 52 insertions(+), 46 deletions(-)

diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
index 6fd09c45a3df6..6a22fa9a9c045 100644
--- a/drivers/staging/gasket/apex_driver.c
+++ b/drivers/staging/gasket/apex_driver.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2018 Google, Inc.
  */
 
+#include <linux/compiler.h>
 #include <linux/delay.h>
 #include <linux/fs.h>
 #include <linux/init.h>
@@ -142,9 +143,9 @@ static int apex_get_status(struct gasket_dev *gasket_dev);
 
 static uint apex_ioctl_check_permissions(struct file *file, uint cmd);
 
-static long apex_ioctl(struct file *file, uint cmd, ulong arg);
+static long apex_ioctl(struct file *file, uint cmd, void __user *arg);
 
-static long apex_clock_gating(struct gasket_dev *gasket_dev, ulong arg);
+static long apex_clock_gating(struct gasket_dev *gasket_dev, void __user *arg);
 
 static int apex_enter_reset(struct gasket_dev *gasket_dev, uint type);
 
@@ -629,7 +630,6 @@ static bool is_gcb_in_reset(struct gasket_dev *gasket_dev)
  */
 static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
 {
-	struct gasket_dev *gasket_dev = filp->private_data;
 	fmode_t write;
 
 	write = filp->f_mode & FMODE_WRITE;
@@ -639,7 +639,7 @@ static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
 /*
  * Apex-specific ioctl handler.
  */
-static long apex_ioctl(struct file *filp, uint cmd, ulong arg)
+static long apex_ioctl(struct file *filp, uint cmd, void __user *arg)
 {
 	struct gasket_dev *gasket_dev = filp->private_data;
 
@@ -659,7 +659,7 @@ static long apex_ioctl(struct file *filp, uint cmd, ulong arg)
  * @gasket_dev: Gasket device pointer.
  * @arg: User ioctl arg, in this case to a apex_gate_clock_ioctl struct.
  */
-static long apex_clock_gating(struct gasket_dev *gasket_dev, ulong arg)
+static long apex_clock_gating(struct gasket_dev *gasket_dev, void __user *arg)
 {
 	struct apex_gate_clock_ioctl ibuf;
 
@@ -667,7 +667,7 @@ static long apex_clock_gating(struct gasket_dev *gasket_dev, ulong arg)
 		return 0;
 
 	if (allow_sw_clock_gating) {
-		if (copy_from_user(&ibuf, (void __user *)arg, sizeof(ibuf)))
+		if (copy_from_user(&ibuf, arg, sizeof(ibuf)))
 			return -EFAULT;
 
 		gasket_log_error(
diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 9d1bb3caf6de2..9f974f179e378 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -14,6 +14,7 @@
 #include "gasket_page_table.h"
 #include "gasket_sysfs.h"
 
+#include <linux/compiler.h>
 #include <linux/delay.h>
 #include <linux/fs.h>
 #include <linux/init.h>
@@ -1823,14 +1824,15 @@ static long gasket_ioctl(struct file *filp, uint cmd, ulong arg)
 		 * check_and_invoke_callback.
 		 */
 		if (driver_desc->ioctl_handler_cb)
-			return driver_desc->ioctl_handler_cb(filp, cmd, arg);
+			return driver_desc->ioctl_handler_cb(
+			    filp, cmd, (void __user *)arg);
 
 		gasket_log_error(
 			gasket_dev, "Received unknown ioctl 0x%x", cmd);
 		return -EINVAL;
 	}
 
-	return gasket_handle_ioctl(filp, cmd, arg);
+	return gasket_handle_ioctl(filp, cmd, (void __user *)arg);
 }
 
 int gasket_reset(struct gasket_dev *gasket_dev, uint reset_type)
diff --git a/drivers/staging/gasket/gasket_core.h b/drivers/staging/gasket/gasket_core.h
index 50ad0c8853183..68b4d2ac9fd6c 100644
--- a/drivers/staging/gasket/gasket_core.h
+++ b/drivers/staging/gasket/gasket_core.h
@@ -315,7 +315,7 @@ struct gasket_dev {
 
 /* Type of the ioctl permissions check callback. See below. */
 typedef int (*gasket_ioctl_permissions_cb_t)(
-	struct file *filp, uint cmd, ulong arg);
+	struct file *filp, uint cmd, void __user *arg);
 
 /*
  * Device type descriptor.
@@ -549,7 +549,7 @@ struct gasket_driver_desc {
 	 * return -EINVAL. Should return an error status (either -EINVAL or
 	 * the error result of the ioctl being handled).
 	 */
-	long (*ioctl_handler_cb)(struct file *filp, uint cmd, ulong arg);
+	long (*ioctl_handler_cb)(struct file *filp, uint cmd, void __user *arg);
 
 	/*
 	 * device_status_cb: Callback to determine device health.
diff --git a/drivers/staging/gasket/gasket_ioctl.c b/drivers/staging/gasket/gasket_ioctl.c
index 17431d14e6ef1..531c360949a64 100644
--- a/drivers/staging/gasket/gasket_ioctl.c
+++ b/drivers/staging/gasket/gasket_ioctl.c
@@ -7,6 +7,7 @@
 #include "gasket_interrupt.h"
 #include "gasket_logging.h"
 #include "gasket_page_table.h"
+#include <linux/compiler.h>
 #include <linux/fs.h>
 #include <linux/uaccess.h>
 
@@ -23,17 +24,18 @@
 #endif
 
 static uint gasket_ioctl_check_permissions(struct file *filp, uint cmd);
-static int gasket_set_event_fd(struct gasket_dev *dev, ulong arg);
+static int gasket_set_event_fd(struct gasket_dev *dev, void __user *arg);
 static int gasket_read_page_table_size(
-	struct gasket_dev *gasket_dev, ulong arg);
+	struct gasket_dev *gasket_dev, void __user *arg);
 static int gasket_read_simple_page_table_size(
-	struct gasket_dev *gasket_dev, ulong arg);
+	struct gasket_dev *gasket_dev, void __user *arg);
 static int gasket_partition_page_table(
-	struct gasket_dev *gasket_dev, ulong arg);
-static int gasket_map_buffers(struct gasket_dev *gasket_dev, ulong arg);
-static int gasket_unmap_buffers(struct gasket_dev *gasket_dev, ulong arg);
+	struct gasket_dev *gasket_dev, void __user *arg);
+static int gasket_map_buffers(struct gasket_dev *gasket_dev, void __user *arg);
+static int gasket_unmap_buffers(struct gasket_dev *gasket_dev,
+				void __user *arg);
 static int gasket_config_coherent_allocator(
-	struct gasket_dev *gasket_dev, ulong arg);
+	struct gasket_dev *gasket_dev, void __user *arg);
 
 /*
  * standard ioctl dispatch function.
@@ -43,9 +45,10 @@ static int gasket_config_coherent_allocator(
  *
  * Standard ioctl dispatcher; forwards operations to individual handlers.
  */
-long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg)
+long gasket_handle_ioctl(struct file *filp, uint cmd, void __user *arg)
 {
 	struct gasket_dev *gasket_dev;
+	ulong intarg = (ulong)arg;
 	int retval;
 
 	gasket_dev = (struct gasket_dev *)filp->private_data;
@@ -74,16 +77,16 @@ long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg)
 	 */
 	switch (cmd) {
 	case GASKET_IOCTL_RESET:
-		trace_gasket_ioctl_integer_data(arg);
-		retval = gasket_reset(gasket_dev, arg);
+		trace_gasket_ioctl_integer_data(intarg);
+		retval = gasket_reset(gasket_dev, intarg);
 		break;
 	case GASKET_IOCTL_SET_EVENTFD:
 		retval = gasket_set_event_fd(gasket_dev, arg);
 		break;
 	case GASKET_IOCTL_CLEAR_EVENTFD:
-		trace_gasket_ioctl_integer_data(arg);
+		trace_gasket_ioctl_integer_data(intarg);
 		retval = gasket_interrupt_clear_eventfd(
-			gasket_dev->interrupt_data, (int)arg);
+			gasket_dev->interrupt_data, (int)intarg);
 		break;
 	case GASKET_IOCTL_PARTITION_PAGE_TABLE:
 		trace_gasket_ioctl_integer_data(arg);
@@ -91,7 +94,7 @@ long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg)
 		break;
 	case GASKET_IOCTL_NUMBER_PAGE_TABLES:
 		trace_gasket_ioctl_integer_data(gasket_dev->num_page_tables);
-		if (copy_to_user((void __user *)arg,
+		if (copy_to_user(arg,
 				 &gasket_dev->num_page_tables,
 				 sizeof(uint64_t)))
 			retval = -EFAULT;
@@ -217,11 +220,11 @@ static uint gasket_ioctl_check_permissions(struct file *filp, uint cmd)
  * @gasket_dev: Pointer to the current gasket_dev we're using.
  * @arg: Pointer to gasket_interrupt_eventfd struct in userspace.
  */
-static int gasket_set_event_fd(struct gasket_dev *gasket_dev, ulong arg)
+static int gasket_set_event_fd(struct gasket_dev *gasket_dev, void __user *arg)
 {
 	struct gasket_interrupt_eventfd die;
 
-	if (copy_from_user(&die, (void __user *)arg,
+	if (copy_from_user(&die, arg,
 			   sizeof(struct gasket_interrupt_eventfd))) {
 		return -EFAULT;
 	}
@@ -237,13 +240,13 @@ static int gasket_set_event_fd(struct gasket_dev *gasket_dev, ulong arg)
  * @gasket_dev: Pointer to the current gasket_dev we're using.
  * @arg: Pointer to gasket_page_table_ioctl struct in userspace.
  */
-static int gasket_read_page_table_size(struct gasket_dev *gasket_dev, ulong arg)
+static int gasket_read_page_table_size(struct gasket_dev *gasket_dev,
+				       void __user *arg)
 {
 	int ret = 0;
 	struct gasket_page_table_ioctl ibuf;
 
-	if (copy_from_user(&ibuf, (void __user *)arg,
-			   sizeof(struct gasket_page_table_ioctl)))
+	if (copy_from_user(&ibuf, arg, sizeof(struct gasket_page_table_ioctl)))
 		return -EFAULT;
 
 	if (ibuf.page_table_index >= gasket_dev->num_page_tables)
@@ -256,7 +259,7 @@ static int gasket_read_page_table_size(struct gasket_dev *gasket_dev, ulong arg)
 		ibuf.page_table_index, ibuf.size, ibuf.host_address,
 		ibuf.device_address);
 
-	if (copy_to_user((void __user *)arg, &ibuf, sizeof(ibuf)))
+	if (copy_to_user(arg, &ibuf, sizeof(ibuf)))
 		return -EFAULT;
 
 	return ret;
@@ -268,13 +271,12 @@ static int gasket_read_page_table_size(struct gasket_dev *gasket_dev, ulong arg)
  * @arg: Pointer to gasket_page_table_ioctl struct in userspace.
  */
 static int gasket_read_simple_page_table_size(
-	struct gasket_dev *gasket_dev, ulong arg)
+	struct gasket_dev *gasket_dev, void __user *arg)
 {
 	int ret = 0;
 	struct gasket_page_table_ioctl ibuf;
 
-	if (copy_from_user(&ibuf, (void __user *)arg,
-			   sizeof(struct gasket_page_table_ioctl)))
+	if (copy_from_user(&ibuf, arg, sizeof(struct gasket_page_table_ioctl)))
 		return -EFAULT;
 
 	if (ibuf.page_table_index >= gasket_dev->num_page_tables)
@@ -287,7 +289,7 @@ static int gasket_read_simple_page_table_size(
 		ibuf.page_table_index, ibuf.size, ibuf.host_address,
 		ibuf.device_address);
 
-	if (copy_to_user((void __user *)arg, &ibuf, sizeof(ibuf)))
+	if (copy_to_user(arg, &ibuf, sizeof(ibuf)))
 		return -EFAULT;
 
 	return ret;
@@ -298,14 +300,14 @@ static int gasket_read_simple_page_table_size(
  * @gasket_dev: Pointer to the current gasket_dev we're using.
  * @arg: Pointer to gasket_page_table_ioctl struct in userspace.
  */
-static int gasket_partition_page_table(struct gasket_dev *gasket_dev, ulong arg)
+static int gasket_partition_page_table(struct gasket_dev *gasket_dev,
+				       void __user *arg)
 {
 	int ret;
 	struct gasket_page_table_ioctl ibuf;
 	uint max_page_table_size;
 
-	if (copy_from_user(&ibuf, (void __user *)arg,
-			   sizeof(struct gasket_page_table_ioctl)))
+	if (copy_from_user(&ibuf, arg, sizeof(struct gasket_page_table_ioctl)))
 		return -EFAULT;
 
 	trace_gasket_ioctl_page_table_data(
@@ -339,12 +341,12 @@ static int gasket_partition_page_table(struct gasket_dev *gasket_dev, ulong arg)
  * @gasket_dev: Pointer to the current gasket_dev we're using.
  * @arg: Pointer to a gasket_page_table_ioctl struct in userspace.
  */
-static int gasket_map_buffers(struct gasket_dev *gasket_dev, ulong arg)
+static int gasket_map_buffers(struct gasket_dev *gasket_dev,
+			      void __user *arg)
 {
 	struct gasket_page_table_ioctl ibuf;
 
-	if (copy_from_user(&ibuf, (void __user *)arg,
-			   sizeof(struct gasket_page_table_ioctl)))
+	if (copy_from_user(&ibuf, arg, sizeof(struct gasket_page_table_ioctl)))
 		return -EFAULT;
 
 	trace_gasket_ioctl_page_table_data(
@@ -369,12 +371,12 @@ static int gasket_map_buffers(struct gasket_dev *gasket_dev, ulong arg)
  * @gasket_dev: Pointer to the current gasket_dev we're using.
  * @arg: Pointer to a gasket_page_table_ioctl struct in userspace.
  */
-static int gasket_unmap_buffers(struct gasket_dev *gasket_dev, ulong arg)
+static int gasket_unmap_buffers(struct gasket_dev *gasket_dev,
+				void __user *arg)
 {
 	struct gasket_page_table_ioctl ibuf;
 
-	if (copy_from_user(&ibuf, (void __user *)arg,
-			   sizeof(struct gasket_page_table_ioctl)))
+	if (copy_from_user(&ibuf, arg, sizeof(struct gasket_page_table_ioctl)))
 		return -EFAULT;
 
 	trace_gasket_ioctl_page_table_data(
@@ -402,12 +404,12 @@ static int gasket_unmap_buffers(struct gasket_dev *gasket_dev, ulong arg)
  * @arg: Pointer to a gasket_coherent_alloc_config_ioctl struct in userspace.
  */
 static int gasket_config_coherent_allocator(
-	struct gasket_dev *gasket_dev, ulong arg)
+	struct gasket_dev *gasket_dev, void __user *arg)
 {
 	int ret;
 	struct gasket_coherent_alloc_config_ioctl ibuf;
 
-	if (copy_from_user(&ibuf, (void __user *)arg,
+	if (copy_from_user(&ibuf, arg,
 			   sizeof(struct gasket_coherent_alloc_config_ioctl)))
 		return -EFAULT;
 
@@ -431,7 +433,7 @@ static int gasket_config_coherent_allocator(
 			gasket_dev, ibuf.size, &ibuf.dma_address,
 			ibuf.page_table_index);
 	}
-	if (copy_to_user((void __user *)arg, &ibuf, sizeof(ibuf)))
+	if (copy_to_user(arg, &ibuf, sizeof(ibuf)))
 		return -EFAULT;
 
 	return ret;
diff --git a/drivers/staging/gasket/gasket_ioctl.h b/drivers/staging/gasket/gasket_ioctl.h
index 461fab27a3e52..2c6bd3e6ba2c9 100644
--- a/drivers/staging/gasket/gasket_ioctl.h
+++ b/drivers/staging/gasket/gasket_ioctl.h
@@ -5,6 +5,8 @@
 
 #include "gasket_core.h"
 
+#include <linux/compiler.h>
+
 /*
  * Handle Gasket common ioctls.
  * @filp: Pointer to the ioctl's file.
@@ -13,7 +15,7 @@
  *
  * Returns 0 on success and nonzero on failure.
  */
-long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg);
+long gasket_handle_ioctl(struct file *filp, uint cmd, void __user *arg);
 
 /*
  * Determines if an ioctl is part of the standard Gasket framework.
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 17/32] staging: gasket: annotate ioctl arg with __user

[Dan Carpenter]

On Mon, Jul 16, 2018 at 07:09:11PM -0700, Todd Poynor wrote:
> @@ -629,7 +630,6 @@ static bool is_gcb_in_reset(struct gasket_dev *gasket_dev)
>   */
>  static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
>  {
> -	struct gasket_dev *gasket_dev = filp->private_data;
>  	fmode_t write;
>  
>  	write = filp->f_mode & FMODE_WRITE;

This should have gone in the earlier patch.  When you spot something
like that then create a separate patch for it and use `git rebase -i` to
fold it back into the original.

regards,
dan carpenter

Re: [PATCH 17/32] staging: gasket: annotate ioctl arg with __user

[Todd Poynor]

On Tue, Jul 17, 2018 at 3:26 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Mon, Jul 16, 2018 at 07:09:11PM -0700, Todd Poynor wrote:
>> @@ -629,7 +630,6 @@ static bool is_gcb_in_reset(struct gasket_dev *gasket_dev)
>>   */
>>  static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
>>  {
>> -     struct gasket_dev *gasket_dev = filp->private_data;
>>       fmode_t write;
>>
>>       write = filp->f_mode & FMODE_WRITE;
>
> This should have gone in the earlier patch.  When you spot something
> like that then create a separate patch for it and use `git rebase -i` to
> fold it back into the original.

Oopsie, thanks.

>
> regards,
> dan carpenter
>



-- 
Todd

[PATCH 18/32] staging: gasket: gasket_enable_dev remove unnecessary variable

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Remove unnecessary variable, pass constant param instead.

Reported-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 9f974f179e378..8ceafcb213413 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -899,7 +899,6 @@ static int gasket_enable_dev(
 {
 	int tbl_idx;
 	int ret;
-	bool has_dma_ops;
 	struct device *ddev;
 	const struct gasket_driver_desc *driver_desc =
 		internal_desc->driver_desc;
@@ -918,8 +917,6 @@ static int gasket_enable_dev(
 		return ret;
 	}
 
-	has_dma_ops = true;
-
 	for (tbl_idx = 0; tbl_idx < driver_desc->num_page_tables; tbl_idx++) {
 		gasket_log_debug(
 			gasket_dev, "Initializing page table %d.", tbl_idx);
@@ -937,7 +934,7 @@ static int gasket_enable_dev(
 			&gasket_dev->bar_data[
 				driver_desc->page_table_bar_index],
 			&driver_desc->page_table_configs[tbl_idx],
-			ddev, gasket_dev->pci_dev, has_dma_ops);
+			ddev, gasket_dev->pci_dev, true);
 		if (ret) {
 			gasket_log_error(
 				gasket_dev,
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 19/32] staging: gasket: remove code for no physical device

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

gasket_enable_dev code for enabling a gasket device with no physical PCI
device registered shouldn't be necessary.

Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 8ceafcb213413..07c6fa3623f77 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -899,7 +899,6 @@ static int gasket_enable_dev(
 {
 	int tbl_idx;
 	int ret;
-	struct device *ddev;
 	const struct gasket_driver_desc *driver_desc =
 		internal_desc->driver_desc;
 
@@ -920,21 +919,12 @@ static int gasket_enable_dev(
 	for (tbl_idx = 0; tbl_idx < driver_desc->num_page_tables; tbl_idx++) {
 		gasket_log_debug(
 			gasket_dev, "Initializing page table %d.", tbl_idx);
-		if (gasket_dev->pci_dev) {
-			ddev = &gasket_dev->pci_dev->dev;
-		} else {
-			gasket_log_error(
-				gasket_dev,
-				"%s with no physical device!!", __func__);
-			WARN_ON(1);
-			ddev = NULL;
-		}
 		ret = gasket_page_table_init(
 			&gasket_dev->page_table[tbl_idx],
 			&gasket_dev->bar_data[
 				driver_desc->page_table_bar_index],
 			&driver_desc->page_table_configs[tbl_idx],
-			ddev, gasket_dev->pci_dev, true);
+			&gasket_dev->pci_dev->dev, gasket_dev->pci_dev, true);
 		if (ret) {
 			gasket_log_error(
 				gasket_dev,
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 20/32] staging: gasket: fix class create bug handling

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

class_create() never returns NULL, and this driver should never return
PTR_ERR(NULL) anyway.

Reported-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
Reviewed-by: Dmitry Torokhov <dtor@chromium.org>
---
 drivers/staging/gasket/gasket_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 07c6fa3623f77..7ea6551626524 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -322,7 +322,7 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
 	internal->class =
 		class_create(driver_desc->module, driver_desc->name);
 
-	if (IS_ERR_OR_NULL(internal->class)) {
+	if (IS_ERR(internal->class)) {
 		gasket_nodev_error("Cannot register %s class [ret=%ld]",
 				   driver_desc->name, PTR_ERR(internal->class));
 		ret = PTR_ERR(internal->class);
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 21/32] staging: gasket: remove unnecessary code in coherent allocator

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Remove extraneous statement in gasket_config_coherent_allocator()

Reported-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_ioctl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/gasket/gasket_ioctl.c b/drivers/staging/gasket/gasket_ioctl.c
index 531c360949a64..9ecf9a4ed1a53 100644
--- a/drivers/staging/gasket/gasket_ioctl.c
+++ b/drivers/staging/gasket/gasket_ioctl.c
@@ -419,10 +419,8 @@ static int gasket_config_coherent_allocator(
 	if (ibuf.page_table_index >= gasket_dev->num_page_tables)
 		return -EFAULT;
 
-	if (ibuf.size > PAGE_SIZE * MAX_NUM_COHERENT_PAGES) {
-		ibuf.size = PAGE_SIZE * MAX_NUM_COHERENT_PAGES;
+	if (ibuf.size > PAGE_SIZE * MAX_NUM_COHERENT_PAGES)
 		return -ENOMEM;
-	}
 
 	if (ibuf.enable == 0) {
 		ret = gasket_free_coherent_memory(
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 22/32] staging: gasket: don't treat no device reset callback as an error

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

It is not an error for a device to not have a reset callback registered.

Signed-off-by: Simon Que <sque@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 7ea6551626524..131ae2462edc1 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -1840,11 +1840,8 @@ int gasket_reset_nolock(struct gasket_dev *gasket_dev, uint reset_type)
 	const struct gasket_driver_desc *driver_desc;
 
 	driver_desc = gasket_dev->internal_desc->driver_desc;
-	if (!driver_desc->device_reset_cb) {
-		gasket_log_error(
-			gasket_dev, "No device reset callback was registered.");
-		return -EINVAL;
-	}
+	if (!driver_desc->device_reset_cb)
+		return 0;
 
 	/* Perform a device reset of the requested type. */
 	ret = driver_desc->device_reset_cb(gasket_dev, reset_type);
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 23/32] staging: gasket: gasket_mmap return error instead of valid BAR index

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

When offset to be mapped matches both a BAR region and a coherent mapped
region return an error as intended, not the BAR index.

Signed-off-by: Simon Que <sque@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 131ae2462edc1..1d979772217bf 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -1630,7 +1630,7 @@ static int gasket_mmap(struct file *filp, struct vm_area_struct *vma)
 			"0x%lx",
 			raw_offset);
 		trace_gasket_mmap_exit(bar_index);
-		return bar_index;
+		return -EINVAL;
 	}
 
 	vma->vm_private_data = gasket_dev;
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 24/32] staging: gasket: apex_clock_gating simplify logic, reduce indentation

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Collapse together two checks and return immediately, avoid conditional
indentation for most of function code.

Reported-by: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Simon Que <sque@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/apex_driver.c | 44 +++++++++++++---------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
index 6a22fa9a9c045..a1fec53d152b9 100644
--- a/drivers/staging/gasket/apex_driver.c
+++ b/drivers/staging/gasket/apex_driver.c
@@ -663,33 +663,31 @@ static long apex_clock_gating(struct gasket_dev *gasket_dev, void __user *arg)
 {
 	struct apex_gate_clock_ioctl ibuf;
 
-	if (bypass_top_level)
+	if (bypass_top_level || !allow_sw_clock_gating)
 		return 0;
 
-	if (allow_sw_clock_gating) {
-		if (copy_from_user(&ibuf, arg, sizeof(ibuf)))
-			return -EFAULT;
+	if (copy_from_user(&ibuf, arg, sizeof(ibuf)))
+		return -EFAULT;
 
-		gasket_log_error(
-			gasket_dev, "%s %llu", __func__, ibuf.enable);
+	gasket_log_error(
+	    gasket_dev, "%s %llu", __func__, ibuf.enable);
 
-		if (ibuf.enable) {
-			/* Quiesce AXI, gate GCB clock. */
-			gasket_read_modify_write_32(
-				gasket_dev, APEX_BAR_INDEX,
-				APEX_BAR2_REG_AXI_QUIESCE, 0x1, 1, 16);
-			gasket_read_modify_write_32(
-				gasket_dev, APEX_BAR_INDEX,
-				APEX_BAR2_REG_GCB_CLOCK_GATE, 0x1, 2, 18);
-		} else {
-			/* Un-gate GCB clock, un-quiesce AXI. */
-			gasket_read_modify_write_32(
-				gasket_dev, APEX_BAR_INDEX,
-				APEX_BAR2_REG_GCB_CLOCK_GATE, 0x0, 2, 18);
-			gasket_read_modify_write_32(
-				gasket_dev, APEX_BAR_INDEX,
-				APEX_BAR2_REG_AXI_QUIESCE, 0x0, 1, 16);
-		}
+	if (ibuf.enable) {
+		/* Quiesce AXI, gate GCB clock. */
+		gasket_read_modify_write_32(
+		    gasket_dev, APEX_BAR_INDEX,
+		    APEX_BAR2_REG_AXI_QUIESCE, 0x1, 1, 16);
+		gasket_read_modify_write_32(
+		    gasket_dev, APEX_BAR_INDEX,
+		    APEX_BAR2_REG_GCB_CLOCK_GATE, 0x1, 2, 18);
+	} else {
+		/* Un-gate GCB clock, un-quiesce AXI. */
+		gasket_read_modify_write_32(
+		    gasket_dev, APEX_BAR_INDEX,
+		    APEX_BAR2_REG_GCB_CLOCK_GATE, 0x0, 2, 18);
+		gasket_read_modify_write_32(
+		    gasket_dev, APEX_BAR_INDEX,
+		    APEX_BAR2_REG_AXI_QUIESCE, 0x0, 1, 16);
 	}
 	return 0;
 }
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 25/32] staging: gasket: apex_ioctl_check_permissions use bool return type

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Convert from int return to bool.

Reported-by: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Simon Que <sque@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/apex_driver.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
index a1fec53d152b9..81288aae02d88 100644
--- a/drivers/staging/gasket/apex_driver.c
+++ b/drivers/staging/gasket/apex_driver.c
@@ -141,7 +141,7 @@ static int apex_reset(struct gasket_dev *gasket_dev, uint type);
 
 static int apex_get_status(struct gasket_dev *gasket_dev);
 
-static uint apex_ioctl_check_permissions(struct file *file, uint cmd);
+static bool apex_ioctl_check_permissions(struct file *file, uint cmd);
 
 static long apex_ioctl(struct file *file, uint cmd, void __user *arg);
 
@@ -626,9 +626,9 @@ static bool is_gcb_in_reset(struct gasket_dev *gasket_dev)
  * @file: File pointer from ioctl.
  * @cmd: ioctl command.
  *
- * Returns 1 if the current user may execute this ioctl, and 0 otherwise.
+ * Returns true if the current user may execute this ioctl, and false otherwise.
  */
-static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
+static bool apex_ioctl_check_permissions(struct file *filp, uint cmd)
 {
 	fmode_t write;
 
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 26/32] staging: gasket: gasket page table functions use bool return type

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Convert from int to bool return type for gasket page table functions
that return values used as booleans.

Reported-by: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Simon Que <sque@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_page_table.c | 58 +++++++++++-----------
 drivers/staging/gasket/gasket_page_table.h |  8 +--
 2 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/drivers/staging/gasket/gasket_page_table.c b/drivers/staging/gasket/gasket_page_table.c
index 36a560c87af36..2a27db658a4e4 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -262,16 +262,16 @@ static void gasket_perform_unmapping(
 static void gasket_free_extended_subtable(
 	struct gasket_page_table *pg_tbl, struct gasket_page_table_entry *pte,
 	u64 __iomem *att_reg);
-static int gasket_release_page(struct page *page);
+static bool gasket_release_page(struct page *page);
 
 /* Other/utility declarations */
-static inline int gasket_addr_is_simple(
+static inline bool gasket_addr_is_simple(
 	struct gasket_page_table *pg_tbl, ulong addr);
-static int gasket_is_simple_dev_addr_bad(
+static bool gasket_is_simple_dev_addr_bad(
 	struct gasket_page_table *pg_tbl, ulong dev_addr, uint num_pages);
-static int gasket_is_extended_dev_addr_bad(
+static bool gasket_is_extended_dev_addr_bad(
 	struct gasket_page_table *pg_tbl, ulong dev_addr, uint num_pages);
-static int gasket_is_pte_range_free(
+static bool gasket_is_pte_range_free(
 	struct gasket_page_table_entry *pte, uint num_entries);
 static void gasket_page_table_garbage_collect_nolock(
 	struct gasket_page_table *pg_tbl);
@@ -558,7 +558,7 @@ int gasket_page_table_lookup_page(
 }
 
 /* See gasket_page_table.h for description. */
-int gasket_page_table_are_addrs_bad(
+bool gasket_page_table_are_addrs_bad(
 	struct gasket_page_table *pg_tbl, ulong host_addr, ulong dev_addr,
 	ulong bytes)
 {
@@ -567,7 +567,7 @@ int gasket_page_table_are_addrs_bad(
 			pg_tbl,
 			"host mapping address 0x%lx must be page aligned",
 			host_addr);
-		return 1;
+		return true;
 	}
 
 	return gasket_page_table_is_dev_addr_bad(pg_tbl, dev_addr, bytes);
@@ -575,7 +575,7 @@ int gasket_page_table_are_addrs_bad(
 EXPORT_SYMBOL(gasket_page_table_are_addrs_bad);
 
 /* See gasket_page_table.h for description. */
-int gasket_page_table_is_dev_addr_bad(
+bool gasket_page_table_is_dev_addr_bad(
 	struct gasket_page_table *pg_tbl, ulong dev_addr, ulong bytes)
 {
 	uint num_pages = bytes / PAGE_SIZE;
@@ -584,7 +584,7 @@ int gasket_page_table_is_dev_addr_bad(
 		gasket_pg_tbl_error(
 			pg_tbl,
 			"mapping size 0x%lX must be page aligned", bytes);
-		return 1;
+		return true;
 	}
 
 	if (num_pages == 0) {
@@ -592,7 +592,7 @@ int gasket_page_table_is_dev_addr_bad(
 			pg_tbl,
 			"requested mapping is less than one page: %lu / %lu",
 			bytes, PAGE_SIZE);
-		return 1;
+		return true;
 	}
 
 	if (gasket_addr_is_simple(pg_tbl, dev_addr))
@@ -1285,23 +1285,23 @@ static void gasket_free_extended_subtable(
 /*
  * Safely return a page to the OS.
  * @page: The page to return to the OS.
- * Returns 1 if the page was released, 0 if it was
+ * Returns true if the page was released, false if it was
  * ignored.
  */
-static int gasket_release_page(struct page *page)
+static bool gasket_release_page(struct page *page)
 {
 	if (!page)
-		return 0;
+		return false;
 
 	if (!PageReserved(page))
 		SetPageDirty(page);
 	put_page(page);
 
-	return 1;
+	return true;
 }
 
 /* Evaluates to nonzero if the specified virtual address is simple. */
-static inline int gasket_addr_is_simple(
+static inline bool gasket_addr_is_simple(
 	struct gasket_page_table *pg_tbl, ulong addr)
 {
 	return !((addr) & (pg_tbl)->extended_flag);
@@ -1317,7 +1317,7 @@ static inline int gasket_addr_is_simple(
  * address to/from page + offset) and that the requested page range starts and
  * ends within the set of currently-partitioned simple pages.
  */
-static int gasket_is_simple_dev_addr_bad(
+static bool gasket_is_simple_dev_addr_bad(
 	struct gasket_page_table *pg_tbl, ulong dev_addr, uint num_pages)
 {
 	ulong page_offset = dev_addr & (PAGE_SIZE - 1);
@@ -1328,7 +1328,7 @@ static int gasket_is_simple_dev_addr_bad(
 		pg_tbl, 1, page_index, page_offset) != dev_addr) {
 		gasket_pg_tbl_error(
 			pg_tbl, "address is invalid, 0x%lX", dev_addr);
-		return 1;
+		return true;
 	}
 
 	if (page_index >= pg_tbl->num_simple_entries) {
@@ -1336,7 +1336,7 @@ static int gasket_is_simple_dev_addr_bad(
 			pg_tbl,
 			"starting slot at %lu is too large, max is < %u",
 			page_index, pg_tbl->num_simple_entries);
-		return 1;
+		return true;
 	}
 
 	if (page_index + num_pages > pg_tbl->num_simple_entries) {
@@ -1344,10 +1344,10 @@ static int gasket_is_simple_dev_addr_bad(
 			pg_tbl,
 			"ending slot at %lu is too large, max is <= %u",
 			page_index + num_pages, pg_tbl->num_simple_entries);
-		return 1;
+		return true;
 	}
 
-	return 0;
+	return false;
 }
 
 /*
@@ -1359,7 +1359,7 @@ static int gasket_is_simple_dev_addr_bad(
  * @dev_addr: The device address to which the pages will be mapped.
  * @num_pages: The number of second-level/sub pages in the range to consider.
  */
-static int gasket_is_extended_dev_addr_bad(
+static bool gasket_is_extended_dev_addr_bad(
 	struct gasket_page_table *pg_tbl, ulong dev_addr, uint num_pages)
 {
 	/* Starting byte index of dev_addr into the first mapped page */
@@ -1373,7 +1373,7 @@ static int gasket_is_extended_dev_addr_bad(
 	if (addr >> (GASKET_EXTENDED_LVL0_WIDTH + GASKET_EXTENDED_LVL0_SHIFT)) {
 		gasket_pg_tbl_error(pg_tbl, "device address out of bound, 0x%p",
 				    (void *)dev_addr);
-		return 1;
+		return true;
 	}
 
 	/* Find the starting sub-page index in the space of all sub-pages. */
@@ -1391,7 +1391,7 @@ static int gasket_is_extended_dev_addr_bad(
 		pg_tbl, 0, page_global_idx, page_offset) != dev_addr) {
 		gasket_pg_tbl_error(
 			pg_tbl, "address is invalid, 0x%p", (void *)dev_addr);
-		return 1;
+		return true;
 	}
 
 	if (page_lvl0_idx >= pg_tbl->num_extended_entries) {
@@ -1399,7 +1399,7 @@ static int gasket_is_extended_dev_addr_bad(
 			pg_tbl,
 			"starting level 0 slot at %lu is too large, max is < "
 			"%u", page_lvl0_idx, pg_tbl->num_extended_entries);
-		return 1;
+		return true;
 	}
 
 	if (page_lvl0_idx + num_lvl0_pages > pg_tbl->num_extended_entries) {
@@ -1408,10 +1408,10 @@ static int gasket_is_extended_dev_addr_bad(
 			"ending level 0 slot at %lu is too large, max is <= %u",
 			page_lvl0_idx + num_lvl0_pages,
 			pg_tbl->num_extended_entries);
-		return 1;
+		return true;
 	}
 
-	return 0;
+	return false;
 }
 
 /*
@@ -1425,17 +1425,17 @@ static int gasket_is_extended_dev_addr_bad(
  *
  * The page table mutex must be held before this call.
  */
-static int gasket_is_pte_range_free(
+static bool gasket_is_pte_range_free(
 	struct gasket_page_table_entry *ptes, uint num_entries)
 {
 	int i;
 
 	for (i = 0; i < num_entries; i++) {
 		if (ptes[i].status != PTE_FREE)
-			return 0;
+			return false;
 	}
 
-	return 1;
+	return true;
 }
 
 /*
diff --git a/drivers/staging/gasket/gasket_page_table.h b/drivers/staging/gasket/gasket_page_table.h
index 720ce2bc2c012..0e8afdb8c1139 100644
--- a/drivers/staging/gasket/gasket_page_table.h
+++ b/drivers/staging/gasket/gasket_page_table.h
@@ -162,9 +162,9 @@ int gasket_page_table_lookup_page(
  * specified by both addresses and the size are valid for mapping pages into
  * device memory.
  *
- * Returns 1 if true - if the mapping is bad, 0 otherwise.
+ * Returns true if the mapping is bad, false otherwise.
  */
-int gasket_page_table_are_addrs_bad(
+bool gasket_page_table_are_addrs_bad(
 	struct gasket_page_table *page_table, ulong host_addr, ulong dev_addr,
 	ulong bytes);
 
@@ -178,9 +178,9 @@ int gasket_page_table_are_addrs_bad(
  * specified by the device address and the size is valid for mapping pages into
  * device memory.
  *
- * Returns 1 if true - if the address is bad, 0 otherwise.
+ * Returns true if the address is bad, false otherwise.
  */
-int gasket_page_table_is_dev_addr_bad(
+bool gasket_page_table_is_dev_addr_bad(
 	struct gasket_page_table *page_table, ulong dev_addr, ulong bytes);
 
 /*
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 27/32] staging: gasket: remove else clause after return in if clause

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Else after return is unnecessary and may cause static code checkers to
complain.

Reported-by: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Simon Que <sque@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 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 2a27db658a4e4..080bf78c22c3b 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -598,9 +598,8 @@ bool gasket_page_table_is_dev_addr_bad(
 	if (gasket_addr_is_simple(pg_tbl, dev_addr))
 		return gasket_is_simple_dev_addr_bad(
 			pg_tbl, dev_addr, num_pages);
-	else
-		return gasket_is_extended_dev_addr_bad(
-			pg_tbl, dev_addr, num_pages);
+	return gasket_is_extended_dev_addr_bad(
+		pg_tbl, dev_addr, num_pages);
 }
 EXPORT_SYMBOL(gasket_page_table_is_dev_addr_bad);
 
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 27/32] staging: gasket: remove else clause after return in if clause

[Dan Carpenter]

On Mon, Jul 16, 2018 at 07:09:21PM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> Else after return is unnecessary and may cause static code checkers to
> complain.
> 
> Reported-by: Guenter Roeck <groeck@chromium.org>
> Signed-off-by: Simon Que <sque@chromium.org>
> Signed-off-by: Todd Poynor <toddpoynor@google.com>
> ---
>  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 2a27db658a4e4..080bf78c22c3b 100644
> --- a/drivers/staging/gasket/gasket_page_table.c
> +++ b/drivers/staging/gasket/gasket_page_table.c
> @@ -598,9 +598,8 @@ bool gasket_page_table_is_dev_addr_bad(
>  	if (gasket_addr_is_simple(pg_tbl, dev_addr))
>  		return gasket_is_simple_dev_addr_bad(
>  			pg_tbl, dev_addr, num_pages);
> -	else
> -		return gasket_is_extended_dev_addr_bad(
> -			pg_tbl, dev_addr, num_pages);
> +	return gasket_is_extended_dev_addr_bad(
> +		pg_tbl, dev_addr, num_pages);

This isn't the normal way to break up a line.  It might even cause
problems if you run with checkpatch.pl --strict.  And anyway this fits
on one line:

	return gasket_is_extended_dev_addr_bad(pg_tbl, dev_addr, num_pages);

But normally we would do:

	return gasket_is_extended_dev_addr_bad(pg_tbl, dev_addr, num_pages,
					       wha'ever, blah, blah, blah);

There are times when that looks a bit ugly if the variable names are
long so we could do:

	return gasket_is_extended_dev_addr_bad(
				long_variable_name_asdfasdfaasdf,
				pg_tbl, dev_addr);

But generally try to get a couple tabs in from the start of the function
name.

regards,
dan carpenter

Re: [PATCH 27/32] staging: gasket: remove else clause after return in if clause

[Todd Poynor]

On Tue, Jul 17, 2018 at 3:37 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Mon, Jul 16, 2018 at 07:09:21PM -0700, Todd Poynor wrote:
>> From: Todd Poynor <toddpoynor@google.com>
>>
>> Else after return is unnecessary and may cause static code checkers to
>> complain.
>>
>> Reported-by: Guenter Roeck <groeck@chromium.org>
>> Signed-off-by: Simon Que <sque@chromium.org>
>> Signed-off-by: Todd Poynor <toddpoynor@google.com>
>> ---
>>  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 2a27db658a4e4..080bf78c22c3b 100644
>> --- a/drivers/staging/gasket/gasket_page_table.c
>> +++ b/drivers/staging/gasket/gasket_page_table.c
>> @@ -598,9 +598,8 @@ bool gasket_page_table_is_dev_addr_bad(
>>       if (gasket_addr_is_simple(pg_tbl, dev_addr))
>>               return gasket_is_simple_dev_addr_bad(
>>                       pg_tbl, dev_addr, num_pages);
>> -     else
>> -             return gasket_is_extended_dev_addr_bad(
>> -                     pg_tbl, dev_addr, num_pages);
>> +     return gasket_is_extended_dev_addr_bad(
>> +             pg_tbl, dev_addr, num_pages);
>
> This isn't the normal way to break up a line.  It might even cause
> problems if you run with checkpatch.pl --strict.  And anyway this fits
> on one line:
>
>         return gasket_is_extended_dev_addr_bad(pg_tbl, dev_addr, num_pages);
>
> But normally we would do:
>
>         return gasket_is_extended_dev_addr_bad(pg_tbl, dev_addr, num_pages,
>                                                wha'ever, blah, blah, blah);
>
> There are times when that looks a bit ugly if the variable names are
> long so we could do:
>
>         return gasket_is_extended_dev_addr_bad(
>                                 long_variable_name_asdfasdfaasdf,
>                                 pg_tbl, dev_addr);
>
> But generally try to get a couple tabs in from the start of the function
> name.

Got it, fixed in v3, thanks -- Todd

>
> regards,
> dan carpenter
>

[PATCH 28/32] staging: gasket: fix comment syntax in apex.h

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Use kernel-style multi-line comment syntax.

Reported-by: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Simon Que <sque@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/apex.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/gasket/apex.h b/drivers/staging/gasket/apex.h
index 4ef264106f503..d89cc2387b7d4 100644
--- a/drivers/staging/gasket/apex.h
+++ b/drivers/staging/gasket/apex.h
@@ -22,9 +22,10 @@
 
 #define APEX_EXTENDED_SHIFT 63 /* Extended address bit position. */
 
-/* Addresses are 2^3=8 bytes each. */
-/* page in second level page table */
-/* holds APEX_PAGE_SIZE/8 addresses  */
+/*
+ * Addresses are 2^3=8 bytes each. Page in second level page table holds
+ * APEX_PAGE_SIZE/8 addresses.
+ */
 #define APEX_ADDR_SHIFT 3
 #define APEX_LEVEL_SHIFT (APEX_PAGE_SHIFT - APEX_ADDR_SHIFT)
 #define APEX_LEVEL_SIZE BIT(APEX_LEVEL_SHIFT)
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 29/32] staging: gasket: remove unnecessary parens in page table code

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

gasket_alloc_coherent_memory() extra parentheses in statement.

Reported-by: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Simon Que <sque@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_page_table.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/gasket/gasket_page_table.c b/drivers/staging/gasket/gasket_page_table.c
index 080bf78c22c3b..9283dbef20ec9 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -1640,7 +1640,7 @@ int gasket_alloc_coherent_memory(struct gasket_dev *gasket_dev, u64 size,
 	dma_addr_t handle;
 	void *mem;
 	int j;
-	unsigned int num_pages = (size + PAGE_SIZE - 1) / (PAGE_SIZE);
+	unsigned int num_pages = (size + PAGE_SIZE - 1) / PAGE_SIZE;
 	const struct gasket_driver_desc *driver_desc =
 		gasket_get_driver_desc(gasket_dev);
 
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 30/32] staging: gasket: gasket_mmap use PAGE_MASK

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Instead of math on PAGE_SIZE.

Reported-by: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Simon Que <sque@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 1d979772217bf..01740a8184594 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -1594,7 +1594,7 @@ static int gasket_mmap(struct file *filp, struct vm_area_struct *vma)
 	}
 	driver_desc = gasket_dev->internal_desc->driver_desc;
 
-	if (vma->vm_start & (PAGE_SIZE - 1)) {
+	if (vma->vm_start & ~PAGE_MASK) {
 		gasket_log_error(
 			gasket_dev, "Base address not page-aligned: 0x%p\n",
 			(void *)vma->vm_start);
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 30/32] staging: gasket: gasket_mmap use PAGE_MASK

[Dan Carpenter]

On Mon, Jul 16, 2018 at 07:09:24PM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> Instead of math on PAGE_SIZE.
> 

The commit message is sort of distinct from the first line.  The first
line is like a title, not the start of a sentence.  On like marc.info
when you're looking at patches, the Subject and the commit message are
sort of far apart so it looks weird.  It looks weird on my email as
well.

Sorry, we're really hitting you with a bunch of nit-picky stuff.  If you
were a one off contributer I'd just let a lot of this stuff slide but
since you're going to be a maintainer for a while then it's good to
learn all these micro rules at the begining.

regards,
dan carpenter

[PATCH 31/32] staging: gasket: remove extra parens in gasket_write_mappable_regions

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Remove unneeded parentheses around subexpressions.

Reported-by: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Simon Que <sque@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 01740a8184594..fd024f2a282a8 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -1893,7 +1893,7 @@ static ssize_t gasket_write_mappable_regions(
 	if (bar_desc.permissions == GASKET_NOMAP)
 		return 0;
 	for (i = 0;
-	     (i < bar_desc.num_mappable_regions) && (total_written < PAGE_SIZE);
+	     i < bar_desc.num_mappable_regions && total_written < PAGE_SIZE;
 	     i++) {
 		min_addr = bar_desc.mappable_regions[i].start -
 			   driver_desc->legacy_mmap_address_offset;
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 32/32] staging: gasket: fix multi-line comment syntax in gasket_core.h

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Use consistent kernel-style multi-line comment syntax.

Reported-by: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Simon Que <sque@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/gasket/gasket_core.h b/drivers/staging/gasket/gasket_core.h
index 68b4d2ac9fd6c..c23764e8b8d91 100644
--- a/drivers/staging/gasket/gasket_core.h
+++ b/drivers/staging/gasket/gasket_core.h
@@ -54,7 +54,8 @@ enum gasket_interrupt_type {
 	PLATFORM_WIRE = 2,
 };
 
-/* Used to describe a Gasket interrupt. Contains an interrupt index, a register,
+/*
+ * Used to describe a Gasket interrupt. Contains an interrupt index, a register,
  * and packing data for that interrupt. The register and packing data
  * fields are relevant only for PCI_MSIX interrupt type and can be
  * set to 0 for everything else.
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 00/32 v2] staging: gasket: sundry fixes and fixups

[Greg Kroah-Hartman]

On Mon, Jul 16, 2018 at 07:08:54PM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> Various fixes mainly from the chromium review of the gasket and apex
> drivers.  More to come.

What changed from v1?

You have to list it somewhere...

thanks,

greg k-h

Re: [PATCH 00/32 v2] staging: gasket: sundry fixes and fixups

[Todd Poynor]

On Tue, Jul 17, 2018 at 4:16 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jul 16, 2018 at 07:08:54PM -0700, Todd Poynor wrote:
> > From: Todd Poynor <toddpoynor@google.com>
> >
> > Various fixes mainly from the chromium review of the gasket and apex
> > drivers.  More to come.
>
> What changed from v1?
>
> You have to list it somewhere...

I have a list of each changed, dropped, and unchanged patch from v1
here, was hoping it's in the right place for that info, does it need
to be elsewhere farther up?

>
> thanks,
>
> greg k-h

Re: [PATCH 00/32 v2] staging: gasket: sundry fixes and fixups

[Todd Poynor]

On Tue, Jul 17, 2018 at 4:25 AM Todd Poynor <toddpoynor@google.com> wrote:
>
> On Tue, Jul 17, 2018 at 4:16 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Jul 16, 2018 at 07:08:54PM -0700, Todd Poynor wrote:
> > > From: Todd Poynor <toddpoynor@google.com>
> > >
> > > Various fixes mainly from the chromium review of the gasket and apex
> > > drivers.  More to come.
> >
> > What changed from v1?
> >
> > You have to list it somewhere...
>
> I have a list of each changed, dropped, and unchanged patch from v1 here, was hoping it's in the right place for that info, does it need to be elsewhere farther up?

Oops, I see I need to move it, will fix in v3.

>
> >
> > thanks,
> >
> > greg k-h

Re: [PATCH 00/32 v2] staging: gasket: sundry fixes and fixups

[Greg KH]

On Tue, Jul 17, 2018 at 04:25:36AM -0700, Todd Poynor wrote:
> On Tue, Jul 17, 2018 at 4:16 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Jul 16, 2018 at 07:08:54PM -0700, Todd Poynor wrote:
> > > From: Todd Poynor <toddpoynor@google.com>
> > >
> > > Various fixes mainly from the chromium review of the gasket and apex
> > > drivers.  More to come.
> >
> > What changed from v1?
> >
> > You have to list it somewhere...
> 
> I have a list of each changed, dropped, and unchanged patch from v1
> here, was hoping it's in the right place for that info, does it need
> to be elsewhere farther up?

Ah, yeah, it was below the diffstat, I didn't even notice it there,
sorry about that.  Putting it higher up is nice, but not essencial.

thanks,

greg k-h

Re: [PATCH 00/32 v2] staging: gasket: sundry fixes and fixups

[Greg Kroah-Hartman]

On Mon, Jul 16, 2018 at 07:08:54PM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> Various fixes mainly from the chromium review of the gasket and apex
> drivers.  More to come.

I think you lost the reviewed-by tags that the v1 series had, why?

thanks,

greg k-h

Re: [PATCH 00/32 v2] staging: gasket: sundry fixes and fixups

[Todd Poynor]

On Tue, Jul 17, 2018 at 4:16 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jul 16, 2018 at 07:08:54PM -0700, Todd Poynor wrote:
> > From: Todd Poynor <toddpoynor@google.com>
> >
> > Various fixes mainly from the chromium review of the gasket and apex
> > drivers.  More to come.
>
> I think you lost the reviewed-by tags that the v1 series had, why?

I received 2 Reviewed-By tags for v1 but only preserved one of them in
v2, failed to update the other one, will fix.

>
> thanks,
>
> greg k-h

[PATCH 00/32 v3] staging: gasket: sundry fixes and fixups

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Various fixes mainly from the chromium review of the gasket and apex
drivers.  More to come.

Todd Poynor (32):
  staging: gasket: remove X86 Kconfig restriction
  staging: gasket: fix typo in apex_enter_reset
  staging: gasket: fix typo in gasket_core.h comments
  staging: gasket: whitespace fix in gasket_page_table_init
  staging: gasket: remove driver registration on class creation failure
  staging: gasket: hold mutex on gasket driver unregistration
  staging: gasket: Return EBUSY on mapping create when already in use
  staging: gasket: Remove stale pointers on error allocating attr array
  staging: gasket: convert gasket_mmap_has_permissions to bool return
  staging: gasket: fix gasket_wait_with_reschedule timeout return code
  staging: gasket: gasket_wait_with_reschedule use msleep
  staging: gasket: gasket_wait_with_reschedule simplify logic
  staging: gasket: gasket_wait_with_reschedule use 32 bits of retry
    count
  staging: gasket: bail out of reset sequence on device callback error
  staging: gasket: drop gasket_cdev_get_info, use container_of
  staging: gasket: always allow root open for write
  staging: gasket: annotate ioctl arg with __user
  staging: gasket: gasket_enable_dev remove unnecessary variable
  staging: gasket: remove code for no physical device
  staging: gasket: fix class create bug handling
  staging: gasket: remove unnecessary code in coherent allocator
  staging: gasket: don't treat no device reset callback as an error
  staging: gasket: gasket_mmap return error instead of valid BAR index
  staging: gasket: apex_clock_gating simplify logic, reduce indentation
  staging: gasket: apex_ioctl_check_permissions use bool return type
  staging: gasket: gasket page table functions use bool return type
  staging: gasket: remove else clause after return in if clause
  staging: gasket: fix comment syntax in apex.h
  staging: gasket: remove unnecessary parens in page table code
  staging: gasket: gasket_mmap use PAGE_MASK
  staging: gasket: remove extra parens in gasket_write_mappable_regions
  staging: gasket: fix multi-line comment syntax in gasket_core.h

Changes from v2:
  staging: gasket: Return EBUSY on mapping create when already in use
     Update commit text for clarity: why EBUSY is better than EINVAL here.
  staging: gasket: gasket_wait_with_reschedule use msleep
     Update commit text for clarity: why msleep is better than
     schedule_timeout here.
  staging: gasket: gasket_wait_with_reschedule return when condition hit
     Renamed: staging: gasket: gasket_wait_with_reschedule simplify logic
     Update commit text for clarity: improved description of change.
     Remove extraneous condition check and a return statement now impossible
     to reach.
  staging: gasket: always allow root open for write
     Add change that accidentally went into the below patch in v2.
     Normalize file mode bitwise ANDs to zero or one values.
     Dropped Reviewed-by from Dmitry Torokhov since this has changed.
  staging: gasket: annotate ioctl arg with __user
     Remove stray change from v2, now moved into the above patch in v3.
  staging: gasket: remove else clause after return in if clause
     Don't break line that doesn't need it after indentation reduced.
  staging: gasket: gasket_mmap use PAGE_MASK
     Update commit text for clarity: why ~PAGE_MASK is better than
     PAGE_SIZE - 1 here.
  staging: gasket: apex_clock_gating simplify logic, reduce indentation
     Unbreak line that no longer needs a break after re-indent.
     
Unchanged from v2:
  staging: gasket: remove X86 Kconfig restriction
  staging: gasket: fix typo in apex_enter_reset
  staging: gasket: fix typo in gasket_core.h comments
  staging: gasket: whitespace fix in gasket_page_table_init
  staging: gasket: remove driver registration on class creation failure
  staging: gasket: hold mutex on gasket driver unregistration
  staging: gasket: Remove stale pointers on error allocating attr array
  staging: gasket: convert gasket_mmap_has_permissions to bool return
  staging: gasket: fix gasket_wait_with_reschedule timeout return code
  staging: gasket: gasket_wait_with_reschedule use 32 bits of retry
    count
  staging: gasket: bail out of reset sequence on device callback error
  staging: gasket: drop gasket_cdev_get_info, use container_of
  staging: gasket: gasket_enable_dev remove unnecessary variable
  staging: gasket: remove code for no physical device
  staging: gasket: fix class create bug handling
  staging: gasket: remove unnecessary code in coherent allocator
  staging: gasket: don't treat no device reset callback as an error
  staging: gasket: gasket_mmap return error instead of valid BAR index
  staging: gasket: apex_ioctl_check_permissions use bool return type
  staging: gasket: gasket page table functions use bool return type
  staging: gasket: fix comment syntax in apex.h
  staging: gasket: remove unnecessary parens in page table code
  staging: gasket: remove extra parens in gasket_write_mappable_regions
  staging: gasket: fix multi-line comment syntax in gasket_core.h

 drivers/staging/gasket/Kconfig             |   2 +-
 drivers/staging/gasket/apex.h              |   7 +-
 drivers/staging/gasket/apex_driver.c       |  77 ++++++--------
 drivers/staging/gasket/gasket_core.c       | 115 ++++++++-------------
 drivers/staging/gasket/gasket_core.h       |  11 +-
 drivers/staging/gasket/gasket_ioctl.c      |  95 +++++++++--------
 drivers/staging/gasket/gasket_ioctl.h      |   4 +-
 drivers/staging/gasket/gasket_page_table.c |  66 ++++++------
 drivers/staging/gasket/gasket_page_table.h |   8 +-
 drivers/staging/gasket/gasket_sysfs.c      |   4 +-
 10 files changed, 177 insertions(+), 212 deletions(-)

-- 
2.18.0.203.gfac676dfb9-goog

[PATCH 17/32] staging: gasket: annotate ioctl arg with __user

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

For sparse checking.

Reported-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Zhongze Hu <frankhu@chromium.org>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
Reviewed-by: Dmitry Torokhov <dtor@chromium.org>
---
 drivers/staging/gasket/apex_driver.c  | 11 ++--
 drivers/staging/gasket/gasket_core.c  |  6 ++-
 drivers/staging/gasket/gasket_core.h  |  4 +-
 drivers/staging/gasket/gasket_ioctl.c | 72 ++++++++++++++-------------
 drivers/staging/gasket/gasket_ioctl.h |  4 +-
 5 files changed, 52 insertions(+), 45 deletions(-)

diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
index 612b3ab803196..c91c5aff5ab9c 100644
--- a/drivers/staging/gasket/apex_driver.c
+++ b/drivers/staging/gasket/apex_driver.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2018 Google, Inc.
  */
 
+#include <linux/compiler.h>
 #include <linux/delay.h>
 #include <linux/fs.h>
 #include <linux/init.h>
@@ -142,9 +143,9 @@ static int apex_get_status(struct gasket_dev *gasket_dev);
 
 static uint apex_ioctl_check_permissions(struct file *file, uint cmd);
 
-static long apex_ioctl(struct file *file, uint cmd, ulong arg);
+static long apex_ioctl(struct file *file, uint cmd, void __user *arg);
 
-static long apex_clock_gating(struct gasket_dev *gasket_dev, ulong arg);
+static long apex_clock_gating(struct gasket_dev *gasket_dev, void __user *arg);
 
 static int apex_enter_reset(struct gasket_dev *gasket_dev, uint type);
 
@@ -635,7 +636,7 @@ static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
 /*
  * Apex-specific ioctl handler.
  */
-static long apex_ioctl(struct file *filp, uint cmd, ulong arg)
+static long apex_ioctl(struct file *filp, uint cmd, void __user *arg)
 {
 	struct gasket_dev *gasket_dev = filp->private_data;
 
@@ -655,7 +656,7 @@ static long apex_ioctl(struct file *filp, uint cmd, ulong arg)
  * @gasket_dev: Gasket device pointer.
  * @arg: User ioctl arg, in this case to a apex_gate_clock_ioctl struct.
  */
-static long apex_clock_gating(struct gasket_dev *gasket_dev, ulong arg)
+static long apex_clock_gating(struct gasket_dev *gasket_dev, void __user *arg)
 {
 	struct apex_gate_clock_ioctl ibuf;
 
@@ -663,7 +664,7 @@ static long apex_clock_gating(struct gasket_dev *gasket_dev, ulong arg)
 		return 0;
 
 	if (allow_sw_clock_gating) {
-		if (copy_from_user(&ibuf, (void __user *)arg, sizeof(ibuf)))
+		if (copy_from_user(&ibuf, arg, sizeof(ibuf)))
 			return -EFAULT;
 
 		gasket_log_error(
diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 947b4fcc76970..ff34af42bbe7c 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -14,6 +14,7 @@
 #include "gasket_page_table.h"
 #include "gasket_sysfs.h"
 
+#include <linux/compiler.h>
 #include <linux/delay.h>
 #include <linux/fs.h>
 #include <linux/init.h>
@@ -1823,14 +1824,15 @@ static long gasket_ioctl(struct file *filp, uint cmd, ulong arg)
 		 * check_and_invoke_callback.
 		 */
 		if (driver_desc->ioctl_handler_cb)
-			return driver_desc->ioctl_handler_cb(filp, cmd, arg);
+			return driver_desc->ioctl_handler_cb(
+			    filp, cmd, (void __user *)arg);
 
 		gasket_log_error(
 			gasket_dev, "Received unknown ioctl 0x%x", cmd);
 		return -EINVAL;
 	}
 
-	return gasket_handle_ioctl(filp, cmd, arg);
+	return gasket_handle_ioctl(filp, cmd, (void __user *)arg);
 }
 
 int gasket_reset(struct gasket_dev *gasket_dev, uint reset_type)
diff --git a/drivers/staging/gasket/gasket_core.h b/drivers/staging/gasket/gasket_core.h
index 50ad0c8853183..68b4d2ac9fd6c 100644
--- a/drivers/staging/gasket/gasket_core.h
+++ b/drivers/staging/gasket/gasket_core.h
@@ -315,7 +315,7 @@ struct gasket_dev {
 
 /* Type of the ioctl permissions check callback. See below. */
 typedef int (*gasket_ioctl_permissions_cb_t)(
-	struct file *filp, uint cmd, ulong arg);
+	struct file *filp, uint cmd, void __user *arg);
 
 /*
  * Device type descriptor.
@@ -549,7 +549,7 @@ struct gasket_driver_desc {
 	 * return -EINVAL. Should return an error status (either -EINVAL or
 	 * the error result of the ioctl being handled).
 	 */
-	long (*ioctl_handler_cb)(struct file *filp, uint cmd, ulong arg);
+	long (*ioctl_handler_cb)(struct file *filp, uint cmd, void __user *arg);
 
 	/*
 	 * device_status_cb: Callback to determine device health.
diff --git a/drivers/staging/gasket/gasket_ioctl.c b/drivers/staging/gasket/gasket_ioctl.c
index d0fa05b8bf1d3..ab2376c32c00b 100644
--- a/drivers/staging/gasket/gasket_ioctl.c
+++ b/drivers/staging/gasket/gasket_ioctl.c
@@ -7,6 +7,7 @@
 #include "gasket_interrupt.h"
 #include "gasket_logging.h"
 #include "gasket_page_table.h"
+#include <linux/compiler.h>
 #include <linux/fs.h>
 #include <linux/uaccess.h>
 
@@ -23,17 +24,18 @@
 #endif
 
 static uint gasket_ioctl_check_permissions(struct file *filp, uint cmd);
-static int gasket_set_event_fd(struct gasket_dev *dev, ulong arg);
+static int gasket_set_event_fd(struct gasket_dev *dev, void __user *arg);
 static int gasket_read_page_table_size(
-	struct gasket_dev *gasket_dev, ulong arg);
+	struct gasket_dev *gasket_dev, void __user *arg);
 static int gasket_read_simple_page_table_size(
-	struct gasket_dev *gasket_dev, ulong arg);
+	struct gasket_dev *gasket_dev, void __user *arg);
 static int gasket_partition_page_table(
-	struct gasket_dev *gasket_dev, ulong arg);
-static int gasket_map_buffers(struct gasket_dev *gasket_dev, ulong arg);
-static int gasket_unmap_buffers(struct gasket_dev *gasket_dev, ulong arg);
+	struct gasket_dev *gasket_dev, void __user *arg);
+static int gasket_map_buffers(struct gasket_dev *gasket_dev, void __user *arg);
+static int gasket_unmap_buffers(struct gasket_dev *gasket_dev,
+				void __user *arg);
 static int gasket_config_coherent_allocator(
-	struct gasket_dev *gasket_dev, ulong arg);
+	struct gasket_dev *gasket_dev, void __user *arg);
 
 /*
  * standard ioctl dispatch function.
@@ -43,9 +45,10 @@ static int gasket_config_coherent_allocator(
  *
  * Standard ioctl dispatcher; forwards operations to individual handlers.
  */
-long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg)
+long gasket_handle_ioctl(struct file *filp, uint cmd, void __user *arg)
 {
 	struct gasket_dev *gasket_dev;
+	ulong intarg = (ulong)arg;
 	int retval;
 
 	gasket_dev = (struct gasket_dev *)filp->private_data;
@@ -74,16 +77,16 @@ long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg)
 	 */
 	switch (cmd) {
 	case GASKET_IOCTL_RESET:
-		trace_gasket_ioctl_integer_data(arg);
-		retval = gasket_reset(gasket_dev, arg);
+		trace_gasket_ioctl_integer_data(intarg);
+		retval = gasket_reset(gasket_dev, intarg);
 		break;
 	case GASKET_IOCTL_SET_EVENTFD:
 		retval = gasket_set_event_fd(gasket_dev, arg);
 		break;
 	case GASKET_IOCTL_CLEAR_EVENTFD:
-		trace_gasket_ioctl_integer_data(arg);
+		trace_gasket_ioctl_integer_data(intarg);
 		retval = gasket_interrupt_clear_eventfd(
-			gasket_dev->interrupt_data, (int)arg);
+			gasket_dev->interrupt_data, (int)intarg);
 		break;
 	case GASKET_IOCTL_PARTITION_PAGE_TABLE:
 		trace_gasket_ioctl_integer_data(arg);
@@ -91,7 +94,7 @@ long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg)
 		break;
 	case GASKET_IOCTL_NUMBER_PAGE_TABLES:
 		trace_gasket_ioctl_integer_data(gasket_dev->num_page_tables);
-		if (copy_to_user((void __user *)arg,
+		if (copy_to_user(arg,
 				 &gasket_dev->num_page_tables,
 				 sizeof(uint64_t)))
 			retval = -EFAULT;
@@ -217,11 +220,11 @@ static uint gasket_ioctl_check_permissions(struct file *filp, uint cmd)
  * @gasket_dev: Pointer to the current gasket_dev we're using.
  * @arg: Pointer to gasket_interrupt_eventfd struct in userspace.
  */
-static int gasket_set_event_fd(struct gasket_dev *gasket_dev, ulong arg)
+static int gasket_set_event_fd(struct gasket_dev *gasket_dev, void __user *arg)
 {
 	struct gasket_interrupt_eventfd die;
 
-	if (copy_from_user(&die, (void __user *)arg,
+	if (copy_from_user(&die, arg,
 			   sizeof(struct gasket_interrupt_eventfd))) {
 		return -EFAULT;
 	}
@@ -237,13 +240,13 @@ static int gasket_set_event_fd(struct gasket_dev *gasket_dev, ulong arg)
  * @gasket_dev: Pointer to the current gasket_dev we're using.
  * @arg: Pointer to gasket_page_table_ioctl struct in userspace.
  */
-static int gasket_read_page_table_size(struct gasket_dev *gasket_dev, ulong arg)
+static int gasket_read_page_table_size(struct gasket_dev *gasket_dev,
+				       void __user *arg)
 {
 	int ret = 0;
 	struct gasket_page_table_ioctl ibuf;
 
-	if (copy_from_user(&ibuf, (void __user *)arg,
-			   sizeof(struct gasket_page_table_ioctl)))
+	if (copy_from_user(&ibuf, arg, sizeof(struct gasket_page_table_ioctl)))
 		return -EFAULT;
 
 	if (ibuf.page_table_index >= gasket_dev->num_page_tables)
@@ -256,7 +259,7 @@ static int gasket_read_page_table_size(struct gasket_dev *gasket_dev, ulong arg)
 		ibuf.page_table_index, ibuf.size, ibuf.host_address,
 		ibuf.device_address);
 
-	if (copy_to_user((void __user *)arg, &ibuf, sizeof(ibuf)))
+	if (copy_to_user(arg, &ibuf, sizeof(ibuf)))
 		return -EFAULT;
 
 	return ret;
@@ -268,13 +271,12 @@ static int gasket_read_page_table_size(struct gasket_dev *gasket_dev, ulong arg)
  * @arg: Pointer to gasket_page_table_ioctl struct in userspace.
  */
 static int gasket_read_simple_page_table_size(
-	struct gasket_dev *gasket_dev, ulong arg)
+	struct gasket_dev *gasket_dev, void __user *arg)
 {
 	int ret = 0;
 	struct gasket_page_table_ioctl ibuf;
 
-	if (copy_from_user(&ibuf, (void __user *)arg,
-			   sizeof(struct gasket_page_table_ioctl)))
+	if (copy_from_user(&ibuf, arg, sizeof(struct gasket_page_table_ioctl)))
 		return -EFAULT;
 
 	if (ibuf.page_table_index >= gasket_dev->num_page_tables)
@@ -287,7 +289,7 @@ static int gasket_read_simple_page_table_size(
 		ibuf.page_table_index, ibuf.size, ibuf.host_address,
 		ibuf.device_address);
 
-	if (copy_to_user((void __user *)arg, &ibuf, sizeof(ibuf)))
+	if (copy_to_user(arg, &ibuf, sizeof(ibuf)))
 		return -EFAULT;
 
 	return ret;
@@ -298,14 +300,14 @@ static int gasket_read_simple_page_table_size(
  * @gasket_dev: Pointer to the current gasket_dev we're using.
  * @arg: Pointer to gasket_page_table_ioctl struct in userspace.
  */
-static int gasket_partition_page_table(struct gasket_dev *gasket_dev, ulong arg)
+static int gasket_partition_page_table(struct gasket_dev *gasket_dev,
+				       void __user *arg)
 {
 	int ret;
 	struct gasket_page_table_ioctl ibuf;
 	uint max_page_table_size;
 
-	if (copy_from_user(&ibuf, (void __user *)arg,
-			   sizeof(struct gasket_page_table_ioctl)))
+	if (copy_from_user(&ibuf, arg, sizeof(struct gasket_page_table_ioctl)))
 		return -EFAULT;
 
 	trace_gasket_ioctl_page_table_data(
@@ -339,12 +341,12 @@ static int gasket_partition_page_table(struct gasket_dev *gasket_dev, ulong arg)
  * @gasket_dev: Pointer to the current gasket_dev we're using.
  * @arg: Pointer to a gasket_page_table_ioctl struct in userspace.
  */
-static int gasket_map_buffers(struct gasket_dev *gasket_dev, ulong arg)
+static int gasket_map_buffers(struct gasket_dev *gasket_dev,
+			      void __user *arg)
 {
 	struct gasket_page_table_ioctl ibuf;
 
-	if (copy_from_user(&ibuf, (void __user *)arg,
-			   sizeof(struct gasket_page_table_ioctl)))
+	if (copy_from_user(&ibuf, arg, sizeof(struct gasket_page_table_ioctl)))
 		return -EFAULT;
 
 	trace_gasket_ioctl_page_table_data(
@@ -369,12 +371,12 @@ static int gasket_map_buffers(struct gasket_dev *gasket_dev, ulong arg)
  * @gasket_dev: Pointer to the current gasket_dev we're using.
  * @arg: Pointer to a gasket_page_table_ioctl struct in userspace.
  */
-static int gasket_unmap_buffers(struct gasket_dev *gasket_dev, ulong arg)
+static int gasket_unmap_buffers(struct gasket_dev *gasket_dev,
+				void __user *arg)
 {
 	struct gasket_page_table_ioctl ibuf;
 
-	if (copy_from_user(&ibuf, (void __user *)arg,
-			   sizeof(struct gasket_page_table_ioctl)))
+	if (copy_from_user(&ibuf, arg, sizeof(struct gasket_page_table_ioctl)))
 		return -EFAULT;
 
 	trace_gasket_ioctl_page_table_data(
@@ -402,12 +404,12 @@ static int gasket_unmap_buffers(struct gasket_dev *gasket_dev, ulong arg)
  * @arg: Pointer to a gasket_coherent_alloc_config_ioctl struct in userspace.
  */
 static int gasket_config_coherent_allocator(
-	struct gasket_dev *gasket_dev, ulong arg)
+	struct gasket_dev *gasket_dev, void __user *arg)
 {
 	int ret;
 	struct gasket_coherent_alloc_config_ioctl ibuf;
 
-	if (copy_from_user(&ibuf, (void __user *)arg,
+	if (copy_from_user(&ibuf, arg,
 			   sizeof(struct gasket_coherent_alloc_config_ioctl)))
 		return -EFAULT;
 
@@ -431,7 +433,7 @@ static int gasket_config_coherent_allocator(
 			gasket_dev, ibuf.size, &ibuf.dma_address,
 			ibuf.page_table_index);
 	}
-	if (copy_to_user((void __user *)arg, &ibuf, sizeof(ibuf)))
+	if (copy_to_user(arg, &ibuf, sizeof(ibuf)))
 		return -EFAULT;
 
 	return ret;
diff --git a/drivers/staging/gasket/gasket_ioctl.h b/drivers/staging/gasket/gasket_ioctl.h
index 461fab27a3e52..2c6bd3e6ba2c9 100644
--- a/drivers/staging/gasket/gasket_ioctl.h
+++ b/drivers/staging/gasket/gasket_ioctl.h
@@ -5,6 +5,8 @@
 
 #include "gasket_core.h"
 
+#include <linux/compiler.h>
+
 /*
  * Handle Gasket common ioctls.
  * @filp: Pointer to the ioctl's file.
@@ -13,7 +15,7 @@
  *
  * Returns 0 on success and nonzero on failure.
  */
-long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg);
+long gasket_handle_ioctl(struct file *filp, uint cmd, void __user *arg);
 
 /*
  * Determines if an ioctl is part of the standard Gasket framework.
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH 17/32] staging: gasket: annotate ioctl arg with __user

[Greg Kroah-Hartman]

On Tue, Jul 17, 2018 at 01:56:57PM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> For sparse checking.

Close, but you can do better :)

> 
> Reported-by: Dmitry Torokhov <dtor@chromium.org>
> Signed-off-by: Zhongze Hu <frankhu@chromium.org>
> Signed-off-by: Todd Poynor <toddpoynor@google.com>
> Reviewed-by: Dmitry Torokhov <dtor@chromium.org>
> ---
>  drivers/staging/gasket/apex_driver.c  | 11 ++--
>  drivers/staging/gasket/gasket_core.c  |  6 ++-
>  drivers/staging/gasket/gasket_core.h  |  4 +-
>  drivers/staging/gasket/gasket_ioctl.c | 72 ++++++++++++++-------------
>  drivers/staging/gasket/gasket_ioctl.h |  4 +-
>  5 files changed, 52 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
> index 612b3ab803196..c91c5aff5ab9c 100644
> --- a/drivers/staging/gasket/apex_driver.c
> +++ b/drivers/staging/gasket/apex_driver.c
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2018 Google, Inc.
>   */
>  
> +#include <linux/compiler.h>
>  #include <linux/delay.h>
>  #include <linux/fs.h>
>  #include <linux/init.h>
> @@ -142,9 +143,9 @@ static int apex_get_status(struct gasket_dev *gasket_dev);
>  
>  static uint apex_ioctl_check_permissions(struct file *file, uint cmd);
>  
> -static long apex_ioctl(struct file *file, uint cmd, ulong arg);
> +static long apex_ioctl(struct file *file, uint cmd, void __user *arg);
> 
> -static long apex_clock_gating(struct gasket_dev *gasket_dev, ulong arg);
> +static long apex_clock_gating(struct gasket_dev *gasket_dev, void __user *arg);

Make this a __user pointer to the correct struct type you are handling
here.  You know what the type is, use it.

>  static int apex_enter_reset(struct gasket_dev *gasket_dev, uint type);
>  
> @@ -635,7 +636,7 @@ static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
>  /*
>   * Apex-specific ioctl handler.
>   */
> -static long apex_ioctl(struct file *filp, uint cmd, ulong arg)
> +static long apex_ioctl(struct file *filp, uint cmd, void __user *arg)
>  {
>  	struct gasket_dev *gasket_dev = filp->private_data;
>  
> @@ -655,7 +656,7 @@ static long apex_ioctl(struct file *filp, uint cmd, ulong arg)
>   * @gasket_dev: Gasket device pointer.
>   * @arg: User ioctl arg, in this case to a apex_gate_clock_ioctl struct.
>   */
> -static long apex_clock_gating(struct gasket_dev *gasket_dev, ulong arg)
> +static long apex_clock_gating(struct gasket_dev *gasket_dev, void __user *arg)

As above, this should be a different type.

>  {
>  	struct apex_gate_clock_ioctl ibuf;
>  
> @@ -663,7 +664,7 @@ static long apex_clock_gating(struct gasket_dev *gasket_dev, ulong arg)
>  		return 0;
>  
>  	if (allow_sw_clock_gating) {
> -		if (copy_from_user(&ibuf, (void __user *)arg, sizeof(ibuf)))
> +		if (copy_from_user(&ibuf, arg, sizeof(ibuf)))
>  			return -EFAULT;
>  
>  		gasket_log_error(
> diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
> index 947b4fcc76970..ff34af42bbe7c 100644
> --- a/drivers/staging/gasket/gasket_core.c
> +++ b/drivers/staging/gasket/gasket_core.c
> @@ -14,6 +14,7 @@
>  #include "gasket_page_table.h"
>  #include "gasket_sysfs.h"
>  
> +#include <linux/compiler.h>
>  #include <linux/delay.h>
>  #include <linux/fs.h>
>  #include <linux/init.h>
> @@ -1823,14 +1824,15 @@ static long gasket_ioctl(struct file *filp, uint cmd, ulong arg)
>  		 * check_and_invoke_callback.
>  		 */
>  		if (driver_desc->ioctl_handler_cb)
> -			return driver_desc->ioctl_handler_cb(filp, cmd, arg);
> +			return driver_desc->ioctl_handler_cb(
> +			    filp, cmd, (void __user *)arg);

You can use a temp variable and then only have to cast things once then,
instead of twice, if you care.  Not a big deal.

>  
>  		gasket_log_error(
>  			gasket_dev, "Received unknown ioctl 0x%x", cmd);

This is a fun way to cause a DoS on your system, you should fix this up
in later changes.

>  		return -EINVAL;
>  	}
>  
> -	return gasket_handle_ioctl(filp, cmd, arg);
> +	return gasket_handle_ioctl(filp, cmd, (void __user *)arg);
>  }
>  
>  int gasket_reset(struct gasket_dev *gasket_dev, uint reset_type)
> diff --git a/drivers/staging/gasket/gasket_core.h b/drivers/staging/gasket/gasket_core.h
> index 50ad0c8853183..68b4d2ac9fd6c 100644
> --- a/drivers/staging/gasket/gasket_core.h
> +++ b/drivers/staging/gasket/gasket_core.h
> @@ -315,7 +315,7 @@ struct gasket_dev {
>  
>  /* Type of the ioctl permissions check callback. See below. */
>  typedef int (*gasket_ioctl_permissions_cb_t)(
> -	struct file *filp, uint cmd, ulong arg);
> +	struct file *filp, uint cmd, void __user *arg);
>  
>  /*
>   * Device type descriptor.
> @@ -549,7 +549,7 @@ struct gasket_driver_desc {
>  	 * return -EINVAL. Should return an error status (either -EINVAL or
>  	 * the error result of the ioctl being handled).
>  	 */
> -	long (*ioctl_handler_cb)(struct file *filp, uint cmd, ulong arg);
> +	long (*ioctl_handler_cb)(struct file *filp, uint cmd, void __user *arg);

Why are you not using the typedef above?

>  
>  	/*
>  	 * device_status_cb: Callback to determine device health.
> diff --git a/drivers/staging/gasket/gasket_ioctl.c b/drivers/staging/gasket/gasket_ioctl.c
> index d0fa05b8bf1d3..ab2376c32c00b 100644
> --- a/drivers/staging/gasket/gasket_ioctl.c
> +++ b/drivers/staging/gasket/gasket_ioctl.c
> @@ -7,6 +7,7 @@
>  #include "gasket_interrupt.h"
>  #include "gasket_logging.h"
>  #include "gasket_page_table.h"
> +#include <linux/compiler.h>
>  #include <linux/fs.h>
>  #include <linux/uaccess.h>
>  
> @@ -23,17 +24,18 @@
>  #endif
>  
>  static uint gasket_ioctl_check_permissions(struct file *filp, uint cmd);
> -static int gasket_set_event_fd(struct gasket_dev *dev, ulong arg);
> +static int gasket_set_event_fd(struct gasket_dev *dev, void __user *arg);
>  static int gasket_read_page_table_size(
> -	struct gasket_dev *gasket_dev, ulong arg);
> +	struct gasket_dev *gasket_dev, void __user *arg);
>  static int gasket_read_simple_page_table_size(
> -	struct gasket_dev *gasket_dev, ulong arg);
> +	struct gasket_dev *gasket_dev, void __user *arg);
>  static int gasket_partition_page_table(
> -	struct gasket_dev *gasket_dev, ulong arg);
> -static int gasket_map_buffers(struct gasket_dev *gasket_dev, ulong arg);
> -static int gasket_unmap_buffers(struct gasket_dev *gasket_dev, ulong arg);
> +	struct gasket_dev *gasket_dev, void __user *arg);
> +static int gasket_map_buffers(struct gasket_dev *gasket_dev, void __user *arg);
> +static int gasket_unmap_buffers(struct gasket_dev *gasket_dev,
> +				void __user *arg);
>  static int gasket_config_coherent_allocator(
> -	struct gasket_dev *gasket_dev, ulong arg);
> +	struct gasket_dev *gasket_dev, void __user *arg);


For most of these you know the real type, please just use it.

>  /*
>   * standard ioctl dispatch function.
> @@ -43,9 +45,10 @@ static int gasket_config_coherent_allocator(
>   *
>   * Standard ioctl dispatcher; forwards operations to individual handlers.
>   */
> -long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg)
> +long gasket_handle_ioctl(struct file *filp, uint cmd, void __user *arg)
>  {
>  	struct gasket_dev *gasket_dev;
> +	ulong intarg = (ulong)arg;

Ick, really?  That's a horrible name for a variable, especially as it
doesn't describe what it is...

and "unsigned long" please, ulong is not a "real" kernel type.

>  	int retval;
>  
>  	gasket_dev = (struct gasket_dev *)filp->private_data;
> @@ -74,16 +77,16 @@ long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg)
>  	 */
>  	switch (cmd) {
>  	case GASKET_IOCTL_RESET:
> -		trace_gasket_ioctl_integer_data(arg);
> -		retval = gasket_reset(gasket_dev, arg);
> +		trace_gasket_ioctl_integer_data(intarg);
> +		retval = gasket_reset(gasket_dev, intarg);
>  		break;
>  	case GASKET_IOCTL_SET_EVENTFD:
>  		retval = gasket_set_event_fd(gasket_dev, arg);
>  		break;
>  	case GASKET_IOCTL_CLEAR_EVENTFD:
> -		trace_gasket_ioctl_integer_data(arg);
> +		trace_gasket_ioctl_integer_data(intarg);
>  		retval = gasket_interrupt_clear_eventfd(
> -			gasket_dev->interrupt_data, (int)arg);
> +			gasket_dev->interrupt_data, (int)intarg);

See, look at that cast, why would you ever think to cast a variabled
with "int" in the name of it, to an int?  Naming is hard :(


>  		break;
>  	case GASKET_IOCTL_PARTITION_PAGE_TABLE:
>  		trace_gasket_ioctl_integer_data(arg);
> @@ -91,7 +94,7 @@ long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg)
>  		break;
>  	case GASKET_IOCTL_NUMBER_PAGE_TABLES:
>  		trace_gasket_ioctl_integer_data(gasket_dev->num_page_tables);
> -		if (copy_to_user((void __user *)arg,
> +		if (copy_to_user(arg,
>  				 &gasket_dev->num_page_tables,

Nit, you can move this line up one now.

I'll stop now, you get the idea.  This should be broken up a lot.

thanks,

greg k-h

Re: [PATCH 17/32] staging: gasket: annotate ioctl arg with __user

[Todd Poynor]

On Thu, Jul 19, 2018 at 2:37 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Jul 17, 2018 at 01:56:57PM -0700, Todd Poynor wrote:
>> From: Todd Poynor <toddpoynor@google.com>
>>
>> For sparse checking.
>
> Close, but you can do better :)
>
>>
>> Reported-by: Dmitry Torokhov <dtor@chromium.org>
>> Signed-off-by: Zhongze Hu <frankhu@chromium.org>
>> Signed-off-by: Todd Poynor <toddpoynor@google.com>
>> Reviewed-by: Dmitry Torokhov <dtor@chromium.org>
>> ---
>>  drivers/staging/gasket/apex_driver.c  | 11 ++--
>>  drivers/staging/gasket/gasket_core.c  |  6 ++-
>>  drivers/staging/gasket/gasket_core.h  |  4 +-
>>  drivers/staging/gasket/gasket_ioctl.c | 72 ++++++++++++++-------------
>>  drivers/staging/gasket/gasket_ioctl.h |  4 +-
>>  5 files changed, 52 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
>> index 612b3ab803196..c91c5aff5ab9c 100644
>> --- a/drivers/staging/gasket/apex_driver.c
>> +++ b/drivers/staging/gasket/apex_driver.c
>> @@ -5,6 +5,7 @@
>>   * Copyright (C) 2018 Google, Inc.
>>   */
>>
>> +#include <linux/compiler.h>
>>  #include <linux/delay.h>
>>  #include <linux/fs.h>
>>  #include <linux/init.h>
>> @@ -142,9 +143,9 @@ static int apex_get_status(struct gasket_dev *gasket_dev);
>>
>>  static uint apex_ioctl_check_permissions(struct file *file, uint cmd);
>>
>> -static long apex_ioctl(struct file *file, uint cmd, ulong arg);
>> +static long apex_ioctl(struct file *file, uint cmd, void __user *arg);
>>
>> -static long apex_clock_gating(struct gasket_dev *gasket_dev, ulong arg);
>> +static long apex_clock_gating(struct gasket_dev *gasket_dev, void __user *arg);
>
> Make this a __user pointer to the correct struct type you are handling
> here.  You know what the type is, use it.

Got it.

>
>>  static int apex_enter_reset(struct gasket_dev *gasket_dev, uint type);
>>
>> @@ -635,7 +636,7 @@ static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
>>  /*
>>   * Apex-specific ioctl handler.
>>   */
>> -static long apex_ioctl(struct file *filp, uint cmd, ulong arg)
>> +static long apex_ioctl(struct file *filp, uint cmd, void __user *arg)
>>  {
>>       struct gasket_dev *gasket_dev = filp->private_data;
>>
>> @@ -655,7 +656,7 @@ static long apex_ioctl(struct file *filp, uint cmd, ulong arg)
>>   * @gasket_dev: Gasket device pointer.
>>   * @arg: User ioctl arg, in this case to a apex_gate_clock_ioctl struct.
>>   */
>> -static long apex_clock_gating(struct gasket_dev *gasket_dev, ulong arg)
>> +static long apex_clock_gating(struct gasket_dev *gasket_dev, void __user *arg)
>
> As above, this should be a different type.
>
>>  {
>>       struct apex_gate_clock_ioctl ibuf;
>>
>> @@ -663,7 +664,7 @@ static long apex_clock_gating(struct gasket_dev *gasket_dev, ulong arg)
>>               return 0;
>>
>>       if (allow_sw_clock_gating) {
>> -             if (copy_from_user(&ibuf, (void __user *)arg, sizeof(ibuf)))
>> +             if (copy_from_user(&ibuf, arg, sizeof(ibuf)))
>>                       return -EFAULT;
>>
>>               gasket_log_error(
>> diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
>> index 947b4fcc76970..ff34af42bbe7c 100644
>> --- a/drivers/staging/gasket/gasket_core.c
>> +++ b/drivers/staging/gasket/gasket_core.c
>> @@ -14,6 +14,7 @@
>>  #include "gasket_page_table.h"
>>  #include "gasket_sysfs.h"
>>
>> +#include <linux/compiler.h>
>>  #include <linux/delay.h>
>>  #include <linux/fs.h>
>>  #include <linux/init.h>
>> @@ -1823,14 +1824,15 @@ static long gasket_ioctl(struct file *filp, uint cmd, ulong arg)
>>                * check_and_invoke_callback.
>>                */
>>               if (driver_desc->ioctl_handler_cb)
>> -                     return driver_desc->ioctl_handler_cb(filp, cmd, arg);
>> +                     return driver_desc->ioctl_handler_cb(
>> +                         filp, cmd, (void __user *)arg);
>
> You can use a temp variable and then only have to cast things once then,
> instead of twice, if you care.  Not a big deal.

But then I have to name it, and I suck at that. :P  I'm trying
switching to "arg" for ulong/int arguments and "argp" for pointer
arguments.

>
>>
>>               gasket_log_error(
>>                       gasket_dev, "Received unknown ioctl 0x%x", cmd);
>
> This is a fun way to cause a DoS on your system, you should fix this up
> in later changes.

Yeah there's another patch coming soon that converts a bunch of errors
and infos to debugs.

>
>>               return -EINVAL;
>>       }
>>
>> -     return gasket_handle_ioctl(filp, cmd, arg);
>> +     return gasket_handle_ioctl(filp, cmd, (void __user *)arg);
>>  }
>>
>>  int gasket_reset(struct gasket_dev *gasket_dev, uint reset_type)
>> diff --git a/drivers/staging/gasket/gasket_core.h b/drivers/staging/gasket/gasket_core.h
>> index 50ad0c8853183..68b4d2ac9fd6c 100644
>> --- a/drivers/staging/gasket/gasket_core.h
>> +++ b/drivers/staging/gasket/gasket_core.h
>> @@ -315,7 +315,7 @@ struct gasket_dev {
>>
>>  /* Type of the ioctl permissions check callback. See below. */
>>  typedef int (*gasket_ioctl_permissions_cb_t)(
>> -     struct file *filp, uint cmd, ulong arg);
>> +     struct file *filp, uint cmd, void __user *arg);
>>
>>  /*
>>   * Device type descriptor.
>> @@ -549,7 +549,7 @@ struct gasket_driver_desc {
>>        * return -EINVAL. Should return an error status (either -EINVAL or
>>        * the error result of the ioctl being handled).
>>        */
>> -     long (*ioctl_handler_cb)(struct file *filp, uint cmd, ulong arg);
>> +     long (*ioctl_handler_cb)(struct file *filp, uint cmd, void __user *arg);
>
> Why are you not using the typedef above?

There's a typedef for the permissions check callback, but not for the
handler callback.  It's a bit confusing, so I tried adding a typedef
for the handler, but now checkpatch is spanking me for adding new
typedefs -- maybe I should drop the existing typedef?

>
>>
>>       /*
>>        * device_status_cb: Callback to determine device health.
>> diff --git a/drivers/staging/gasket/gasket_ioctl.c b/drivers/staging/gasket/gasket_ioctl.c
>> index d0fa05b8bf1d3..ab2376c32c00b 100644
>> --- a/drivers/staging/gasket/gasket_ioctl.c
>> +++ b/drivers/staging/gasket/gasket_ioctl.c
>> @@ -7,6 +7,7 @@
>>  #include "gasket_interrupt.h"
>>  #include "gasket_logging.h"
>>  #include "gasket_page_table.h"
>> +#include <linux/compiler.h>
>>  #include <linux/fs.h>
>>  #include <linux/uaccess.h>
>>
>> @@ -23,17 +24,18 @@
>>  #endif
>>
>>  static uint gasket_ioctl_check_permissions(struct file *filp, uint cmd);
>> -static int gasket_set_event_fd(struct gasket_dev *dev, ulong arg);
>> +static int gasket_set_event_fd(struct gasket_dev *dev, void __user *arg);
>>  static int gasket_read_page_table_size(
>> -     struct gasket_dev *gasket_dev, ulong arg);
>> +     struct gasket_dev *gasket_dev, void __user *arg);
>>  static int gasket_read_simple_page_table_size(
>> -     struct gasket_dev *gasket_dev, ulong arg);
>> +     struct gasket_dev *gasket_dev, void __user *arg);
>>  static int gasket_partition_page_table(
>> -     struct gasket_dev *gasket_dev, ulong arg);
>> -static int gasket_map_buffers(struct gasket_dev *gasket_dev, ulong arg);
>> -static int gasket_unmap_buffers(struct gasket_dev *gasket_dev, ulong arg);
>> +     struct gasket_dev *gasket_dev, void __user *arg);
>> +static int gasket_map_buffers(struct gasket_dev *gasket_dev, void __user *arg);
>> +static int gasket_unmap_buffers(struct gasket_dev *gasket_dev,
>> +                             void __user *arg);
>>  static int gasket_config_coherent_allocator(
>> -     struct gasket_dev *gasket_dev, ulong arg);
>> +     struct gasket_dev *gasket_dev, void __user *arg);
>
>
> For most of these you know the real type, please just use it.

Roger.

>
>>  /*
>>   * standard ioctl dispatch function.
>> @@ -43,9 +45,10 @@ static int gasket_config_coherent_allocator(
>>   *
>>   * Standard ioctl dispatcher; forwards operations to individual handlers.
>>   */
>> -long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg)
>> +long gasket_handle_ioctl(struct file *filp, uint cmd, void __user *arg)
>>  {
>>       struct gasket_dev *gasket_dev;
>> +     ulong intarg = (ulong)arg;
>
> Ick, really?  That's a horrible name for a variable, especially as it
> doesn't describe what it is...
>
> and "unsigned long" please, ulong is not a "real" kernel type.
>
>>       int retval;
>>
>>       gasket_dev = (struct gasket_dev *)filp->private_data;
>> @@ -74,16 +77,16 @@ long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg)
>>        */
>>       switch (cmd) {
>>       case GASKET_IOCTL_RESET:
>> -             trace_gasket_ioctl_integer_data(arg);
>> -             retval = gasket_reset(gasket_dev, arg);
>> +             trace_gasket_ioctl_integer_data(intarg);
>> +             retval = gasket_reset(gasket_dev, intarg);
>>               break;
>>       case GASKET_IOCTL_SET_EVENTFD:
>>               retval = gasket_set_event_fd(gasket_dev, arg);
>>               break;
>>       case GASKET_IOCTL_CLEAR_EVENTFD:
>> -             trace_gasket_ioctl_integer_data(arg);
>> +             trace_gasket_ioctl_integer_data(intarg);
>>               retval = gasket_interrupt_clear_eventfd(
>> -                     gasket_dev->interrupt_data, (int)arg);
>> +                     gasket_dev->interrupt_data, (int)intarg);
>
> See, look at that cast, why would you ever think to cast a variabled
> with "int" in the name of it, to an int?  Naming is hard :(

Yeah now that I look at it that was a pretty horrible name.  I hate
naming things.

>
>
>>               break;
>>       case GASKET_IOCTL_PARTITION_PAGE_TABLE:
>>               trace_gasket_ioctl_integer_data(arg);
>> @@ -91,7 +94,7 @@ long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg)
>>               break;
>>       case GASKET_IOCTL_NUMBER_PAGE_TABLES:
>>               trace_gasket_ioctl_integer_data(gasket_dev->num_page_tables);
>> -             if (copy_to_user((void __user *)arg,
>> +             if (copy_to_user(arg,
>>                                &gasket_dev->num_page_tables,
>
> Nit, you can move this line up one now.

OK.

>
> I'll stop now, you get the idea.  This should be broken up a lot.

I'll split up in some fashion.

Thanks -- Todd

>
> thanks,
>
> greg k-h

Re: [PATCH 17/32] staging: gasket: annotate ioctl arg with __user

[Greg Kroah-Hartman]

On Thu, Jul 19, 2018 at 07:44:53PM -0700, Todd Poynor wrote:
> >>  /* Type of the ioctl permissions check callback. See below. */
> >>  typedef int (*gasket_ioctl_permissions_cb_t)(
> >> -     struct file *filp, uint cmd, ulong arg);
> >> +     struct file *filp, uint cmd, void __user *arg);
> >>
> >>  /*
> >>   * Device type descriptor.
> >> @@ -549,7 +549,7 @@ struct gasket_driver_desc {
> >>        * return -EINVAL. Should return an error status (either -EINVAL or
> >>        * the error result of the ioctl being handled).
> >>        */
> >> -     long (*ioctl_handler_cb)(struct file *filp, uint cmd, ulong arg);
> >> +     long (*ioctl_handler_cb)(struct file *filp, uint cmd, void __user *arg);
> >
> > Why are you not using the typedef above?
> 
> There's a typedef for the permissions check callback, but not for the
> handler callback.  It's a bit confusing, so I tried adding a typedef
> for the handler, but now checkpatch is spanking me for adding new
> typedefs -- maybe I should drop the existing typedef?

No, typedefs for function pointers is just fine, checkpatch should not
be complaining about that.  And even if it is, it's just a guideline,
not a hard rule that you have to abide by everything it says :)

thanks,

greg k-h

Re: [PATCH 17/32] staging: gasket: annotate ioctl arg with __user

[Dan Carpenter]

On Thu, Jul 19, 2018 at 07:44:53PM -0700, Todd Poynor wrote:
> >> @@ -549,7 +549,7 @@ struct gasket_driver_desc {
> >>        * return -EINVAL. Should return an error status (either -EINVAL or
> >>        * the error result of the ioctl being handled).
> >>        */
> >> -     long (*ioctl_handler_cb)(struct file *filp, uint cmd, ulong arg);
> >> +     long (*ioctl_handler_cb)(struct file *filp, uint cmd, void __user *arg);
> >
> > Why are you not using the typedef above?
> 
> There's a typedef for the permissions check callback, but not for the
> handler callback.  It's a bit confusing, so I tried adding a typedef
> for the handler, but now checkpatch is spanking me for adding new
> typedefs -- maybe I should drop the existing typedef?
> 

Someone should update checkpatch to not complain about function typedefs
because those are obviously OK.  It wouldn't have even occured to me
that people were avoiding them because of checkpatch...

regards,
dan carpenter