[PATCH 0/8] staging: gasket: cleanups du jour

[PATCH 0/8] staging: gasket: cleanups du jour

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

More cleanups for the gasket+apex drivers.

Todd Poynor (8):
  staging: gasket: apex: enable power save mode by default
  staging: gasket: core: print driver version code at registration time
  staging: gasket: core: move driver loaded log after error cases
  staging: gasket: core: device register debug log cleanups
  staging: gasket: core: add subsystem and device info to error logs
  staging: gasket: remove "reset type" param from framework
  staging: gasket: apex: drop reset type param
  Revert "staging: gasket: core: hold reference to pci_dev while used"

 drivers/staging/gasket/apex_driver.c      | 22 ++++-----
 drivers/staging/gasket/gasket.h           |  4 +-
 drivers/staging/gasket/gasket_constants.h |  2 +-
 drivers/staging/gasket/gasket_core.c      | 56 +++++++++++------------
 drivers/staging/gasket/gasket_core.h      | 13 ++----
 drivers/staging/gasket/gasket_ioctl.c     |  3 +-
 6 files changed, 43 insertions(+), 57 deletions(-)

-- 
2.18.0.597.ga71716f1ad-goog

[PATCH 1/8] staging: gasket: apex: enable power save mode by default

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Set default value of allow_power_save parameter to enable power save
mode, which is expected to be the state usually desired.

Signed-off-by: Marty Faltesek <mfaltesek@google.com>
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 dfbff47b46086..7fd0dd609d037 100644
--- a/drivers/staging/gasket/apex_driver.c
+++ b/drivers/staging/gasket/apex_driver.c
@@ -233,7 +233,7 @@ static struct gasket_interrupt_desc apex_interrupts[] = {
 
 
 /* Allows device to enter power save upon driver close(). */
-static int allow_power_save;
+static int allow_power_save = 1;
 
 /* Allows SW based clock gating. */
 static int allow_sw_clock_gating;
-- 
2.18.0.597.ga71716f1ad-goog

[PATCH 2/8] staging: gasket: core: print driver version code at registration time

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Print the driver version code in the kernel log at gasket driver
registration time for informational purposes.  Add "gasket:" prefix to
make clear it is the gasket framework logging this information.

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

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index f76f4a0ecbac6..e550c9060dcd2 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -1759,8 +1759,9 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
 	}
 	mutex_unlock(&g_mutex);
 
-	pr_info("Loaded %s driver, framework version %s\n",
-		driver_desc->name, GASKET_FRAMEWORK_VERSION);
+	pr_info("gasket: Loaded %s driver version %s, framework version %s\n",
+		driver_desc->name, driver_desc->driver_version,
+		GASKET_FRAMEWORK_VERSION);
 
 	if (desc_idx == -1) {
 		pr_err("Too many Gasket drivers loaded: %d\n",
-- 
2.18.0.597.ga71716f1ad-goog

[PATCH 3/8] staging: gasket: core: move driver loaded log after error cases

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Gasket could claim to have loaded a driver and then print an error
indicating it actually did not.  Move the driver registration message
after the last error check.  Replace the existing "loaded successfully"
message with this instead.

Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index e550c9060dcd2..2160c2de78e77 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -1759,10 +1759,6 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
 	}
 	mutex_unlock(&g_mutex);
 
-	pr_info("gasket: Loaded %s driver version %s, framework version %s\n",
-		driver_desc->name, driver_desc->driver_version,
-		GASKET_FRAMEWORK_VERSION);
-
 	if (desc_idx == -1) {
 		pr_err("Too many Gasket drivers loaded: %d\n",
 		       GASKET_FRAMEWORK_DESC_MAX);
@@ -1810,7 +1806,10 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
 		goto fail2;
 	}
 
-	pr_info("Driver registered successfully.\n");
+	pr_info("gasket: Loaded %s driver version %s, framework version %s\n",
+		driver_desc->name, driver_desc->driver_version,
+		GASKET_FRAMEWORK_VERSION);
+
 	return 0;
 
 fail2:
-- 
2.18.0.597.ga71716f1ad-goog

[PATCH 4/8] staging: gasket: core: device register debug log cleanups

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

At device/driver registration time, convert a not-very-informative
info message to a more informative dbeug message, drop some not overly
helpful debug messages.

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 2160c2de78e77..36c077fffc41c 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -1736,7 +1736,8 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
 	int desc_idx = -1;
 	struct gasket_internal_desc *internal;
 
-	pr_info("Initializing Gasket framework device\n");
+	pr_debug("gasket: Loading %s driver version %s\n", driver_desc->name,
+		 driver_desc->driver_version);
 	/* Check for duplicates and find a free slot. */
 	mutex_lock(&g_mutex);
 
@@ -1765,8 +1766,6 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
 		return -EBUSY;
 	}
 
-	/* Internal structure setup. */
-	pr_debug("Performing initial internal structure setup.\n");
 	internal = &g_descs[desc_idx];
 	mutex_init(&internal->mutex);
 	memset(internal->devs, 0, sizeof(struct gasket_dev *) * GASKET_DEV_MAX);
@@ -1789,7 +1788,6 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
 	 * Not using pci_register_driver() (without underscores), as it
 	 * depends on KBUILD_MODNAME, and this is a shared file.
 	 */
-	pr_debug("Registering PCI driver.\n");
 	ret = __pci_register_driver(&internal->pci, driver_desc->module,
 				    driver_desc->name);
 	if (ret) {
@@ -1797,7 +1795,6 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
 		goto fail1;
 	}
 
-	pr_debug("Registering char driver.\n");
 	ret = register_chrdev_region(MKDEV(driver_desc->major,
 					   driver_desc->minor), GASKET_DEV_MAX,
 				     driver_desc->name);
-- 
2.18.0.597.ga71716f1ad-goog

[PATCH 5/8] staging: gasket: core: add subsystem and device info to error logs

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Identify gasket as the subsystem printing various error messages.
Add the driver name to appropriate messages to indicate which driver
has a problem.

Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket_core.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 36c077fffc41c..19331feb9b09f 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -160,7 +160,7 @@ static int gasket_find_dev_slot(struct gasket_internal_desc *internal_desc,
 	for (i = 0; i < GASKET_DEV_MAX; i++) {
 		if (internal_desc->devs[i] &&
 		    strcmp(internal_desc->devs[i]->kobj_name, kobj_name) == 0) {
-			pr_err("Duplicate device %s\n", kobj_name);
+			pr_err("gasket: Duplicate device %s\n", kobj_name);
 			mutex_unlock(&internal_desc->mutex);
 			return -EBUSY;
 		}
@@ -173,7 +173,8 @@ static int gasket_find_dev_slot(struct gasket_internal_desc *internal_desc,
 	}
 
 	if (i == GASKET_DEV_MAX) {
-		pr_err("Too many registered devices; max %d\n", GASKET_DEV_MAX);
+		pr_err("gasket: Too many registered devices, max %d\n",
+		       GASKET_DEV_MAX);
 		mutex_unlock(&internal_desc->mutex);
 		return -EBUSY;
 	}
@@ -208,7 +209,7 @@ static int gasket_alloc_dev(struct gasket_internal_desc *internal_desc,
 
 	gasket_dev = *pdev = kzalloc(sizeof(*gasket_dev), GFP_KERNEL);
 	if (!gasket_dev) {
-		pr_err("no memory for device\n");
+		pr_err("gasket: no memory for device %s\n",  kobj_name);
 		return -ENOMEM;
 	}
 	internal_desc->devs[dev_idx] = gasket_dev;
@@ -1464,7 +1465,7 @@ static int gasket_pci_probe(struct pci_dev *pci_dev,
 	internal_desc = lookup_internal_desc(pci_dev);
 	mutex_unlock(&g_mutex);
 	if (!internal_desc) {
-		pr_err("PCI probe called for unknown driver type\n");
+		pr_err("gasket: PCI probe called for unknown driver type\n");
 		return -ENODEV;
 	}
 
@@ -1476,7 +1477,7 @@ static int gasket_pci_probe(struct pci_dev *pci_dev,
 		return ret;
 	gasket_dev->pci_dev = pci_dev_get(pci_dev);
 	if (IS_ERR_OR_NULL(gasket_dev->dev_info.device)) {
-		pr_err("Cannot create %s device %s [ret = %ld]\n",
+		pr_err("gasket: Cannot create %s device %s [ret = %ld]\n",
 		       driver_desc->name, gasket_dev->dev_info.name,
 		       PTR_ERR(gasket_dev->dev_info.device));
 		ret = -ENODEV;
@@ -1523,7 +1524,7 @@ static int gasket_pci_probe(struct pci_dev *pci_dev,
 
 	ret = gasket_enable_dev(internal_desc, gasket_dev);
 	if (ret) {
-		pr_err("cannot setup %s device\n", driver_desc->name);
+		pr_err("gasket: cannot setup %s device\n", driver_desc->name);
 		gasket_disable_dev(gasket_dev);
 		goto fail5;
 	}
@@ -1743,7 +1744,7 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
 
 	for (i = 0; i < GASKET_FRAMEWORK_DESC_MAX; i++) {
 		if (g_descs[i].driver_desc == driver_desc) {
-			pr_err("%s driver already loaded/registered\n",
+			pr_err("gasket: %s driver already loaded/registered\n",
 			       driver_desc->name);
 			mutex_unlock(&g_mutex);
 			return -EBUSY;
@@ -1761,7 +1762,7 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
 	mutex_unlock(&g_mutex);
 
 	if (desc_idx == -1) {
-		pr_err("Too many Gasket drivers loaded: %d\n",
+		pr_err("gasket: too many drivers loaded, max %d\n",
 		       GASKET_FRAMEWORK_DESC_MAX);
 		return -EBUSY;
 	}
@@ -1778,7 +1779,7 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
 		class_create(driver_desc->module, driver_desc->name);
 
 	if (IS_ERR(internal->class)) {
-		pr_err("Cannot register %s class [ret=%ld]\n",
+		pr_err("gasket: Cannot register %s class [ret=%ld]\n",
 		       driver_desc->name, PTR_ERR(internal->class));
 		ret = PTR_ERR(internal->class);
 		goto unregister_gasket_driver;
@@ -1791,7 +1792,8 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
 	ret = __pci_register_driver(&internal->pci, driver_desc->module,
 				    driver_desc->name);
 	if (ret) {
-		pr_err("cannot register pci driver [ret=%d]\n", ret);
+		pr_err("gasket: cannot register %s pci driver [ret=%d]\n",
+		       driver_desc->name, ret);
 		goto fail1;
 	}
 
@@ -1799,7 +1801,8 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
 					   driver_desc->minor), GASKET_DEV_MAX,
 				     driver_desc->name);
 	if (ret) {
-		pr_err("cannot register char driver [ret=%d]\n", ret);
+		pr_err("gasket: cannot register %s char driver [ret=%d]\n",
+		       driver_desc->name, ret);
 		goto fail2;
 	}
 
@@ -1840,7 +1843,7 @@ void gasket_unregister_device(const struct gasket_driver_desc *driver_desc)
 	mutex_unlock(&g_mutex);
 
 	if (!internal_desc) {
-		pr_err("request to unregister unknown desc: %s, %d:%d\n",
+		pr_err("gasket: request to unregister unknown desc: %s, %d:%d\n",
 		       driver_desc->name, driver_desc->major,
 		       driver_desc->minor);
 		return;
-- 
2.18.0.597.ga71716f1ad-goog

[PATCH 6/8] staging: gasket: remove "reset type" param from framework

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

The "type of reset" parameter to the gasket device reset APIs isn't
required by the only gasket device submitted upstream, apex.

The framework documents the param as private to the device driver and a
pass-through at the gasket layer, but the gasket core calls the device
driver with a hardcoded reset type of zero, which is not documented as
having a predefined meaning.

In light of all this, remove the reset type parameter from the
framework.  Remove the reset ioctl reset type parameter, and bump the
framework version number to reflect the interface change.

Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/gasket.h           |  4 ++--
 drivers/staging/gasket/gasket_constants.h |  2 +-
 drivers/staging/gasket/gasket_core.c      | 11 +++++------
 drivers/staging/gasket/gasket_core.h      | 13 +++----------
 drivers/staging/gasket/gasket_ioctl.c     |  3 +--
 5 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/gasket/gasket.h b/drivers/staging/gasket/gasket.h
index 9f709f0c5a2bb..a0f065c517a52 100644
--- a/drivers/staging/gasket/gasket.h
+++ b/drivers/staging/gasket/gasket.h
@@ -52,8 +52,8 @@ struct gasket_coherent_alloc_config_ioctl {
 /* Base number for all Gasket-common IOCTLs */
 #define GASKET_IOCTL_BASE 0xDC
 
-/* Reset the device using the specified reset type. */
-#define GASKET_IOCTL_RESET _IOW(GASKET_IOCTL_BASE, 0, unsigned long)
+/* Reset the device. */
+#define GASKET_IOCTL_RESET _IO(GASKET_IOCTL_BASE, 0)
 
 /* Associate the specified [event]fd with the specified interrupt. */
 #define GASKET_IOCTL_SET_EVENTFD                                               \
diff --git a/drivers/staging/gasket/gasket_constants.h b/drivers/staging/gasket/gasket_constants.h
index 82ed3f21e8aed..50d87c7b178c2 100644
--- a/drivers/staging/gasket/gasket_constants.h
+++ b/drivers/staging/gasket/gasket_constants.h
@@ -3,7 +3,7 @@
 #ifndef __GASKET_CONSTANTS_H__
 #define __GASKET_CONSTANTS_H__
 
-#define GASKET_FRAMEWORK_VERSION "1.1.1"
+#define GASKET_FRAMEWORK_VERSION "1.1.2"
 
 /*
  * The maximum number of simultaneous device types supported by the framework.
diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 19331feb9b09f..99994e30b154b 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -1295,7 +1295,7 @@ static int gasket_release(struct inode *inode, struct file *file)
 			ownership->owner = 0;
 
 			/* Forces chip reset before we unmap the page tables. */
-			driver_desc->device_reset_cb(gasket_dev, 0);
+			driver_desc->device_reset_cb(gasket_dev);
 
 			for (i = 0; i < driver_desc->num_page_tables; ++i) {
 				gasket_page_table_unmap_all(gasket_dev->page_table[i]);
@@ -1623,18 +1623,18 @@ const char *gasket_num_name_lookup(uint num,
 }
 EXPORT_SYMBOL(gasket_num_name_lookup);
 
-int gasket_reset(struct gasket_dev *gasket_dev, uint reset_type)
+int gasket_reset(struct gasket_dev *gasket_dev)
 {
 	int ret;
 
 	mutex_lock(&gasket_dev->mutex);
-	ret = gasket_reset_nolock(gasket_dev, reset_type);
+	ret = gasket_reset_nolock(gasket_dev);
 	mutex_unlock(&gasket_dev->mutex);
 	return ret;
 }
 EXPORT_SYMBOL(gasket_reset);
 
-int gasket_reset_nolock(struct gasket_dev *gasket_dev, uint reset_type)
+int gasket_reset_nolock(struct gasket_dev *gasket_dev)
 {
 	int ret;
 	int i;
@@ -1644,8 +1644,7 @@ int gasket_reset_nolock(struct gasket_dev *gasket_dev, uint reset_type)
 	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);
+	ret = driver_desc->device_reset_cb(gasket_dev);
 	if (ret) {
 		dev_dbg(gasket_dev->dev, "Device reset cb returned %d.\n",
 			ret);
diff --git a/drivers/staging/gasket/gasket_core.h b/drivers/staging/gasket/gasket_core.h
index 713bf42de41a4..67f5960943a8a 100644
--- a/drivers/staging/gasket/gasket_core.h
+++ b/drivers/staging/gasket/gasket_core.h
@@ -580,17 +580,12 @@ struct gasket_driver_desc {
 	/*
 	 * device_reset_cb: Reset the hardware in question.
 	 * @dev: Pointer to the gasket_dev structure for this device.
-	 * @type: Integer representing reset type. (All
-	 * Gasket resets have an integer representing their type
-	 * defined in (device)_ioctl.h; the specific resets are
-	 * device-dependent, but are handled in the device-specific
-	 * callback anyways.)
 	 *
 	 * Called by reset ioctls. This function should not
 	 * lock the gasket_dev mutex. It should return 0 on success
 	 * and an error on failure.
 	 */
-	int (*device_reset_cb)(struct gasket_dev *dev, uint reset_type);
+	int (*device_reset_cb)(struct gasket_dev *dev);
 };
 
 /*
@@ -615,15 +610,13 @@ void gasket_unregister_device(const struct gasket_driver_desc *desc);
 /*
  * Reset the Gasket device.
  * @gasket_dev: Gasket device struct.
- * @reset_type: Uint representing requested reset type. Should be
- * valid in the underlying callback.
  *
  * Calls device_reset_cb. Returns 0 on success and an error code othewrise.
  * gasket_reset_nolock will not lock the mutex, gasket_reset will.
  *
  */
-int gasket_reset(struct gasket_dev *gasket_dev, uint reset_type);
-int gasket_reset_nolock(struct gasket_dev *gasket_dev, uint reset_type);
+int gasket_reset(struct gasket_dev *gasket_dev);
+int gasket_reset_nolock(struct gasket_dev *gasket_dev);
 
 /*
  * Memory management functions. These will likely be spun off into their own
diff --git a/drivers/staging/gasket/gasket_ioctl.c b/drivers/staging/gasket/gasket_ioctl.c
index d3397cc74e69f..0ca48e688818f 100644
--- a/drivers/staging/gasket/gasket_ioctl.c
+++ b/drivers/staging/gasket/gasket_ioctl.c
@@ -304,8 +304,7 @@ long gasket_handle_ioctl(struct file *filp, uint cmd, void __user *argp)
 	 */
 	switch (cmd) {
 	case GASKET_IOCTL_RESET:
-		trace_gasket_ioctl_integer_data(arg);
-		retval = gasket_reset(gasket_dev, arg);
+		retval = gasket_reset(gasket_dev);
 		break;
 	case GASKET_IOCTL_SET_EVENTFD:
 		retval = gasket_set_event_fd(gasket_dev, argp);
-- 
2.18.0.597.ga71716f1ad-goog

[PATCH 7/8] staging: gasket: apex: drop reset type param

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

Apex doesn't implement different types of resets based on the reset type
param passed through the gasket layer or from userspace via the
gasket_reset ioctl.  The reset type is dropped from the gasket framework
in a previous patch due to a lack of present need and non-conforming use
of this parameter by the framework.  Drop the parameter from the apex
driver as well.

Signed-off-by: Todd Poynor <toddpoynor@google.com>
---
 drivers/staging/gasket/apex_driver.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
index 7fd0dd609d037..42cef68eb4c19 100644
--- a/drivers/staging/gasket/apex_driver.c
+++ b/drivers/staging/gasket/apex_driver.c
@@ -56,10 +56,6 @@
 
 #define APEX_EXTENDED_SHIFT 63 /* Extended address bit position. */
 
-enum apex_reset_types {
-	APEX_CHIP_REINIT_RESET = 3,
-};
-
 /* Check reset 120 times */
 #define APEX_RESET_RETRY 120
 /* Wait 100 ms between checks. Total 12 sec wait maximum. */
@@ -258,7 +254,7 @@ static int apex_get_status(struct gasket_dev *gasket_dev)
 }
 
 /* Enter GCB reset state. */
-static int apex_enter_reset(struct gasket_dev *gasket_dev, uint type)
+static int apex_enter_reset(struct gasket_dev *gasket_dev)
 {
 	if (bypass_top_level)
 		return 0;
@@ -313,7 +309,7 @@ static int apex_enter_reset(struct gasket_dev *gasket_dev, uint type)
 }
 
 /* Quit GCB reset state. */
-static int apex_quit_reset(struct gasket_dev *gasket_dev, uint type)
+static int apex_quit_reset(struct gasket_dev *gasket_dev)
 {
 	u32 val0, val1;
 
@@ -413,7 +409,7 @@ static int apex_device_cleanup(struct gasket_dev *gasket_dev)
 		__func__, gasket_dev, hib_error, scalar_error);
 
 	if (allow_power_save)
-		ret = apex_enter_reset(gasket_dev, APEX_CHIP_REINIT_RESET);
+		ret = apex_enter_reset(gasket_dev);
 
 	return ret;
 }
@@ -429,7 +425,7 @@ static bool is_gcb_in_reset(struct gasket_dev *gasket_dev)
 }
 
 /* Reset the hardware, then quit reset.  Called on device open. */
-static int apex_reset(struct gasket_dev *gasket_dev, uint type)
+static int apex_reset(struct gasket_dev *gasket_dev)
 {
 	int ret;
 
@@ -442,11 +438,11 @@ static int apex_reset(struct gasket_dev *gasket_dev, uint type)
 		 */
 		dev_dbg(gasket_dev->dev, "%s: toggle reset\n", __func__);
 
-		ret = apex_enter_reset(gasket_dev, type);
+		ret = apex_enter_reset(gasket_dev);
 		if (ret)
 			return ret;
 	}
-	ret = apex_quit_reset(gasket_dev, type);
+	ret = apex_quit_reset(gasket_dev);
 
 	return ret;
 }
@@ -456,7 +452,7 @@ static int apex_add_dev_cb(struct gasket_dev *gasket_dev)
 	ulong page_table_ready, msix_table_ready;
 	int retries = 0;
 
-	apex_reset(gasket_dev, 0);
+	apex_reset(gasket_dev);
 
 	while (retries < APEX_RESET_RETRY) {
 		page_table_ready =
@@ -611,7 +607,7 @@ static int apex_sysfs_setup_cb(struct gasket_dev *gasket_dev)
 /* On device open, perform a core reinit reset. */
 static int apex_device_open_cb(struct gasket_dev *gasket_dev)
 {
-	return gasket_reset_nolock(gasket_dev, APEX_CHIP_REINIT_RESET);
+	return gasket_reset_nolock(gasket_dev);
 }
 
 static const struct pci_device_id apex_pci_ids[] = {
-- 
2.18.0.597.ga71716f1ad-goog

[PATCH 8/8] Revert "staging: gasket: core: hold reference to pci_dev while used"

[Todd Poynor]

From: Todd Poynor <toddpoynor@google.com>

There's no need to take an additional reference on the pci_dev structure
for the pointer copy saved in gasket data structures.

This reverts commit 8dd8a48b9a7dae5493494a8603adddfdf1914716.

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

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 99994e30b154b..8c5574305f13b 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -253,7 +253,6 @@ static void gasket_free_dev(struct gasket_dev *gasket_dev)
 	internal_desc->devs[gasket_dev->dev_idx] = NULL;
 	mutex_unlock(&internal_desc->mutex);
 	put_device(gasket_dev->dev);
-	pci_dev_put(gasket_dev->pci_dev);
 	kfree(gasket_dev);
 }
 
@@ -1475,7 +1474,7 @@ static int gasket_pci_probe(struct pci_dev *pci_dev,
 	ret = gasket_alloc_dev(internal_desc, parent, &gasket_dev, kobj_name);
 	if (ret)
 		return ret;
-	gasket_dev->pci_dev = pci_dev_get(pci_dev);
+	gasket_dev->pci_dev = pci_dev;
 	if (IS_ERR_OR_NULL(gasket_dev->dev_info.device)) {
 		pr_err("gasket: Cannot create %s device %s [ret = %ld]\n",
 		       driver_desc->name, gasket_dev->dev_info.name,
-- 
2.18.0.597.ga71716f1ad-goog

Re: [PATCH 2/8] staging: gasket: core: print driver version code at registration time

[Greg Kroah-Hartman]

On Thu, Aug 02, 2018 at 01:42:39AM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> Print the driver version code in the kernel log at gasket driver
> registration time for informational purposes.  Add "gasket:" prefix to
> make clear it is the gasket framework logging this information.
> 
> Signed-off-by: Todd Poynor <toddpoynor@google.com>
> ---
>  drivers/staging/gasket/gasket_core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
> index f76f4a0ecbac6..e550c9060dcd2 100644
> --- a/drivers/staging/gasket/gasket_core.c
> +++ b/drivers/staging/gasket/gasket_core.c
> @@ -1759,8 +1759,9 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
>  	}
>  	mutex_unlock(&g_mutex);
>  
> -	pr_info("Loaded %s driver, framework version %s\n",
> -		driver_desc->name, GASKET_FRAMEWORK_VERSION);
> +	pr_info("gasket: Loaded %s driver version %s, framework version %s\n",
> +		driver_desc->name, driver_desc->driver_version,
> +		GASKET_FRAMEWORK_VERSION);

This is what pr_fmt is for, please just set that properly and all will
be fine.

But really, just delete this line, drivers, if all is going well, should
not emit any noise to the kernel log.  Especially for something like
doing a 'modprobe', which is really what is happening here.  Just drop
the noisy messages please.

Also, GASKET_FRAMEWORK_VERSION means nothing now, it needs to be dropped :)

thanks,

greg k-h

Re: [PATCH 3/8] staging: gasket: core: move driver loaded log after error cases

[Greg Kroah-Hartman]

On Thu, Aug 02, 2018 at 01:42:40AM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> Gasket could claim to have loaded a driver and then print an error
> indicating it actually did not.  Move the driver registration message
> after the last error check.  Replace the existing "loaded successfully"
> message with this instead.
> 
> Signed-off-by: Todd Poynor <toddpoynor@google.com>
> ---
>  drivers/staging/gasket/gasket_core.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
> index e550c9060dcd2..2160c2de78e77 100644
> --- a/drivers/staging/gasket/gasket_core.c
> +++ b/drivers/staging/gasket/gasket_core.c
> @@ -1759,10 +1759,6 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
>  	}
>  	mutex_unlock(&g_mutex);
>  
> -	pr_info("gasket: Loaded %s driver version %s, framework version %s\n",
> -		driver_desc->name, driver_desc->driver_version,
> -		GASKET_FRAMEWORK_VERSION);
> -
>  	if (desc_idx == -1) {
>  		pr_err("Too many Gasket drivers loaded: %d\n",
>  		       GASKET_FRAMEWORK_DESC_MAX);
> @@ -1810,7 +1806,10 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
>  		goto fail2;
>  	}
>  
> -	pr_info("Driver registered successfully.\n");

Same here, just drop this line.

thanks,

greg k-h

Re: [PATCH 4/8] staging: gasket: core: device register debug log cleanups

[Greg Kroah-Hartman]

On Thu, Aug 02, 2018 at 01:42:41AM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> At device/driver registration time, convert a not-very-informative
> info message to a more informative dbeug message, drop some not overly
> helpful debug messages.
> 
> 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 2160c2de78e77..36c077fffc41c 100644
> --- a/drivers/staging/gasket/gasket_core.c
> +++ b/drivers/staging/gasket/gasket_core.c
> @@ -1736,7 +1736,8 @@ int gasket_register_device(const struct gasket_driver_desc *driver_desc)
>  	int desc_idx = -1;
>  	struct gasket_internal_desc *internal;
>  
> -	pr_info("Initializing Gasket framework device\n");
> +	pr_debug("gasket: Loading %s driver version %s\n", driver_desc->name,

Again, pr_fmt is your friend :)

thanks,

greg k-h

Re: [PATCH 5/8] staging: gasket: core: add subsystem and device info to error logs

[Greg Kroah-Hartman]

On Thu, Aug 02, 2018 at 01:42:42AM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> Identify gasket as the subsystem printing various error messages.
> Add the driver name to appropriate messages to indicate which driver
> has a problem.
> 
> Signed-off-by: Todd Poynor <toddpoynor@google.com>
> ---
>  drivers/staging/gasket/gasket_core.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
> index 36c077fffc41c..19331feb9b09f 100644
> --- a/drivers/staging/gasket/gasket_core.c
> +++ b/drivers/staging/gasket/gasket_core.c
> @@ -160,7 +160,7 @@ static int gasket_find_dev_slot(struct gasket_internal_desc *internal_desc,
>  	for (i = 0; i < GASKET_DEV_MAX; i++) {
>  		if (internal_desc->devs[i] &&
>  		    strcmp(internal_desc->devs[i]->kobj_name, kobj_name) == 0) {
> -			pr_err("Duplicate device %s\n", kobj_name);
> +			pr_err("gasket: Duplicate device %s\n", kobj_name);

same here with the prefix.

thanks,

greg k-h

Re: [PATCH 8/8] Revert "staging: gasket: core: hold reference to pci_dev while used"

[Greg Kroah-Hartman]

On Thu, Aug 02, 2018 at 01:42:45AM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
> 
> There's no need to take an additional reference on the pci_dev structure
> for the pointer copy saved in gasket data structures.
> 
> This reverts commit 8dd8a48b9a7dae5493494a8603adddfdf1914716.

Hint, when dealing with sha1 numbers in the kernel, we like the
following format to make it a bit easier to understand:
	8dd8a48b9a7d ("staging: gasket: core: hold reference to pci_dev while used")
which can be generated by doing:
	git show -s --abbrev-commit --abbrev=12 --pretty=format:"%h (\"%s\")%n"

Can you fix this up and resend?

thanks,

greg k-h