[PATCH 0/7] i2c: improve error messages when registering an adapter

[PATCH 0/7] i2c: improve error messages when registering an adapter

[Wolfram Sang]

The goal of this series is to have proper error messages whenever something in
i2c_add_adapter() and friends fail. Then, we can hereafter drop all similar
messages in drivers which is largely duplicated code and often not very helpful
because they are too generic.

To achieve this goal, we make sure that dev_* is used whenever a device is
available. pr_* is used when we have at least an adapter name. If not, we use
WARN to show the user the code path that failed. Error messages were added,
fixed, and made consistent. Also, the first patch fixes a potential leak found
while creating this series. Also, i2c_register_adapter() was a bit refactored
to be easier to handle. Please look at the patch descriptions for details. A
brach is here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git strings/i2c

Looking forward to comments.

   Wolfram

Wolfram Sang (7):
  i2c: free idr when sanity checks in i2c_register_adapter() fail
  i2c: cleanup i2c_register_adapter() by refactoring recovery init
  i2c: improve error messages in i2c_register_adapter()
  i2c: add error message when obtaining idr fails
  i2c: print more info when of_i2c_notify fails
  i2c: print more info when acpi_i2c_space_handler() fails
  i2c: use pr_fmt in the core

 drivers/i2c/i2c-core.c | 124 ++++++++++++++++++++++++++-----------------------
 1 file changed, 66 insertions(+), 58 deletions(-)

-- 
2.8.1

[PATCH 1/7] i2c: free idr when sanity checks in i2c_register_adapter() fail

[Wolfram Sang]

On error, we should give idr back to the pool in any case.

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
Cc: Jean Delvare <jdelvare@suse.de>
---
 drivers/i2c/i2c-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 71ad532be1d891..7e6ff6d4cf659a 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1560,7 +1560,7 @@ static int __process_new_adapter(struct device_driver *d, void *data)
 
 static int i2c_register_adapter(struct i2c_adapter *adap)
 {
-	int res = 0;
+	int res = -EINVAL;
 
 	/* Can't register until after driver model init */
 	if (WARN_ON(!is_registered)) {
@@ -1572,12 +1572,12 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	if (unlikely(adap->name[0] == '\0')) {
 		pr_err("i2c-core: Attempt to register an adapter with "
 		       "no name!\n");
-		return -EINVAL;
+		goto out_list;
 	}
 	if (unlikely(!adap->algo)) {
 		pr_err("i2c-core: Attempt to register adapter '%s' with "
 		       "no algo!\n", adap->name);
-		return -EINVAL;
+		goto out_list;
 	}
 
 	if (!adap->lock_bus) {
-- 
2.8.1

[PATCH 2/7] i2c: cleanup i2c_register_adapter() by refactoring recovery init

[Wolfram Sang]

Move recovery init to a seperate function to let have
i2c_register_adapter() less lines and to avoid goto and a label.
Refactor string handling there for consistency and to save some bytes.

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/i2c-core.c | 76 ++++++++++++++++++++++++++++----------------------
 1 file changed, 42 insertions(+), 34 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 7e6ff6d4cf659a..cc3e3bf986591c 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -666,6 +666,47 @@ int i2c_recover_bus(struct i2c_adapter *adap)
 }
 EXPORT_SYMBOL_GPL(i2c_recover_bus);
 
+static void i2c_init_recovery(struct i2c_adapter *adap)
+{
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+	char *err_str;
+
+	if (!bri)
+		return;
+
+	if (!bri->recover_bus) {
+		err_str = "no recover_bus() found";
+		goto err;
+	}
+
+	/* Generic GPIO recovery */
+	if (bri->recover_bus == i2c_generic_gpio_recovery) {
+		if (!gpio_is_valid(bri->scl_gpio)) {
+			err_str = "invalid SCL gpio";
+			goto err;
+		}
+
+		if (gpio_is_valid(bri->sda_gpio))
+			bri->get_sda = get_sda_gpio_value;
+		else
+			bri->get_sda = NULL;
+
+		bri->get_scl = get_scl_gpio_value;
+		bri->set_scl = set_scl_gpio_value;
+	} else if (bri->recover_bus == i2c_generic_scl_recovery) {
+		/* Generic SCL recovery */
+		if (!bri->set_scl || !bri->get_scl) {
+			err_str = "no {get|set}_scl() found";
+			goto err;
+		}
+	}
+
+	return;
+ err:
+	dev_err(&adap->dev, "Not using recovery: %s\n", err_str);
+	adap->bus_recovery_info = NULL;
+}
+
 static int i2c_device_probe(struct device *dev)
 {
 	struct i2c_client	*client = i2c_verify_client(dev);
@@ -1616,41 +1657,8 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 			 "Failed to create compatibility class link\n");
 #endif
 
-	/* bus recovery specific initialization */
-	if (adap->bus_recovery_info) {
-		struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
-
-		if (!bri->recover_bus) {
-			dev_err(&adap->dev, "No recover_bus() found, not using recovery\n");
-			adap->bus_recovery_info = NULL;
-			goto exit_recovery;
-		}
-
-		/* Generic GPIO recovery */
-		if (bri->recover_bus == i2c_generic_gpio_recovery) {
-			if (!gpio_is_valid(bri->scl_gpio)) {
-				dev_err(&adap->dev, "Invalid SCL gpio, not using recovery\n");
-				adap->bus_recovery_info = NULL;
-				goto exit_recovery;
-			}
-
-			if (gpio_is_valid(bri->sda_gpio))
-				bri->get_sda = get_sda_gpio_value;
-			else
-				bri->get_sda = NULL;
-
-			bri->get_scl = get_scl_gpio_value;
-			bri->set_scl = set_scl_gpio_value;
-		} else if (bri->recover_bus == i2c_generic_scl_recovery) {
-			/* Generic SCL recovery */
-			if (!bri->set_scl || !bri->get_scl) {
-				dev_err(&adap->dev, "No {get|set}_scl() found, not using recovery\n");
-				adap->bus_recovery_info = NULL;
-			}
-		}
-	}
+	i2c_init_recovery(adap);
 
-exit_recovery:
 	/* create pre-declared device nodes */
 	of_i2c_register_devices(adap);
 	acpi_i2c_register_devices(adap);
-- 
2.8.1

[PATCH 3/7] i2c: improve error messages in i2c_register_adapter()

[Wolfram Sang]

Switch to WARN if no adapter name is given, otherwise we won't know who
missed to do that. Add error message if device registration fails.
Update error message for missing algo to match style of the others.

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/i2c-core.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index cc3e3bf986591c..36183eacec105b 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1610,14 +1610,11 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	}
 
 	/* Sanity checks */
-	if (unlikely(adap->name[0] == '\0')) {
-		pr_err("i2c-core: Attempt to register an adapter with "
-		       "no name!\n");
+	if (WARN(!adap->name[0], "i2c adapter has no name"))
 		goto out_list;
-	}
-	if (unlikely(!adap->algo)) {
-		pr_err("i2c-core: Attempt to register adapter '%s' with "
-		       "no algo!\n", adap->name);
+
+	if (!adap->algo) {
+		pr_err("i2c-core: adapter '%s': no algo supplied!\n", adap->name);
 		goto out_list;
 	}
 
@@ -1640,8 +1637,11 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	adap->dev.bus = &i2c_bus_type;
 	adap->dev.type = &i2c_adapter_type;
 	res = device_register(&adap->dev);
-	if (res)
+	if (res) {
+		pr_err("i2c-core: adapter '%s': can't register device (%d)\n",
+			adap->name, res);
 		goto out_list;
+	}
 
 	dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name);
 
-- 
2.8.1

[PATCH 4/7] i2c: add error message when obtaining idr fails

[Wolfram Sang]

Fix some whitespace issues while here.

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/i2c-core.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 36183eacec105b..a94e7ccb9c3067 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1690,13 +1690,12 @@ out_list:
  */
 static int __i2c_add_numbered_adapter(struct i2c_adapter *adap)
 {
-	int	id;
+	int id;
 
 	mutex_lock(&core_lock);
-	id = idr_alloc(&i2c_adapter_idr, adap, adap->nr, adap->nr + 1,
-		       GFP_KERNEL);
+	id = idr_alloc(&i2c_adapter_idr, adap, adap->nr, adap->nr + 1, GFP_KERNEL);
 	mutex_unlock(&core_lock);
-	if (id < 0)
+	if (WARN(id < 0, "couldn't get idr"))
 		return id == -ENOSPC ? -EBUSY : id;
 
 	return i2c_register_adapter(adap);
@@ -1733,7 +1732,7 @@ int i2c_add_adapter(struct i2c_adapter *adapter)
 	id = idr_alloc(&i2c_adapter_idr, adapter,
 		       __i2c_first_dynamic_bus_num, 0, GFP_KERNEL);
 	mutex_unlock(&core_lock);
-	if (id < 0)
+	if (WARN(id < 0, "couldn't get idr"))
 		return id;
 
 	adapter->nr = id;
-- 
2.8.1

[PATCH 5/7] i2c: print more info when of_i2c_notify fails

[Wolfram Sang]

Use dev_err instead of pr_err for more details.

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/i2c/i2c-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index a94e7ccb9c3067..57fe66cc855163 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2109,8 +2109,8 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
 		put_device(&adap->dev);
 
 		if (IS_ERR(client)) {
-			pr_err("%s: failed to create for '%s'\n",
-					__func__, rd->dn->full_name);
+			dev_err(&adap->dev, "failed to create client for '%s'\n",
+				 rd->dn->full_name);
 			return notifier_from_errno(PTR_ERR(client));
 		}
 		break;
-- 
2.8.1

[PATCH 6/7] i2c: print more info when acpi_i2c_space_handler() fails

[Wolfram Sang]

Use a warning loglevel instead of info and switch to dev_* for device
info. Also print which client was accessed.

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/i2c/i2c-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 57fe66cc855163..7f376e9b007ba7 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -400,7 +400,8 @@ acpi_i2c_space_handler(u32 function, acpi_physical_address command,
 		break;
 
 	default:
-		pr_info("protocol(0x%02x) is not supported.\n", accessor_type);
+		dev_warn(&adapter->dev, "protocol 0x%02x not supported for client 0x%02x\n",
+			 accessor_type, client->addr);
 		ret = AE_BAD_PARAMETER;
 		goto err;
 	}
-- 
2.8.1

[PATCH 7/7] i2c: use pr_fmt in the core

[Wolfram Sang]

Now that we revisited all error messages, we can use pr_fmt for the
remaining pr_* messages to ensure consistent output.

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/i2c-core.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 7f376e9b007ba7..bd8be6b6c49246 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -27,6 +27,8 @@
    I2C slave support (c) 2014 by Wolfram Sang <wsa@sang-engineering.com>
  */
 
+#define pr_fmt(fmt) "i2c-core: " fmt
+
 #include <dt-bindings/i2c/i2c.h>
 #include <asm/uaccess.h>
 #include <linux/acpi.h>
@@ -1615,7 +1617,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 		goto out_list;
 
 	if (!adap->algo) {
-		pr_err("i2c-core: adapter '%s': no algo supplied!\n", adap->name);
+		pr_err("adapter '%s': no algo supplied!\n", adap->name);
 		goto out_list;
 	}
 
@@ -1639,8 +1641,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	adap->dev.type = &i2c_adapter_type;
 	res = device_register(&adap->dev);
 	if (res) {
-		pr_err("i2c-core: adapter '%s': can't register device (%d)\n",
-			adap->name, res);
+		pr_err("adapter '%s': can't register device (%d)\n", adap->name, res);
 		goto out_list;
 	}
 
@@ -1831,8 +1832,7 @@ void i2c_del_adapter(struct i2c_adapter *adap)
 	found = idr_find(&i2c_adapter_idr, adap->nr);
 	mutex_unlock(&core_lock);
 	if (found != adap) {
-		pr_debug("i2c-core: attempting to delete unregistered "
-			 "adapter [%s]\n", adap->name);
+		pr_debug("attempting to delete unregistered adapter [%s]\n", adap->name);
 		return;
 	}
 
@@ -1992,7 +1992,7 @@ int i2c_register_driver(struct module *owner, struct i2c_driver *driver)
 	if (res)
 		return res;
 
-	pr_debug("i2c-core: driver [%s] registered\n", driver->driver.name);
+	pr_debug("driver [%s] registered\n", driver->driver.name);
 
 	INIT_LIST_HEAD(&driver->clients);
 	/* Walk the adapters that are already present */
@@ -2019,7 +2019,7 @@ void i2c_del_driver(struct i2c_driver *driver)
 	i2c_for_each_dev(driver, __process_removed_driver);
 
 	driver_unregister(&driver->driver);
-	pr_debug("i2c-core: driver [%s] unregistered\n", driver->driver.name);
+	pr_debug("driver [%s] unregistered\n", driver->driver.name);
 }
 EXPORT_SYMBOL(i2c_del_driver);
 
@@ -2728,7 +2728,7 @@ static int i2c_smbus_check_pec(u8 cpec, struct i2c_msg *msg)
 	cpec = i2c_smbus_msg_pec(cpec, msg);
 
 	if (rpec != cpec) {
-		pr_debug("i2c-core: Bad PEC 0x%02x vs. 0x%02x\n",
+		pr_debug("Bad PEC 0x%02x vs. 0x%02x\n",
 			rpec, cpec);
 		return -EBADMSG;
 	}
-- 
2.8.1

Re: [PATCH 6/7] i2c: print more info when acpi_i2c_space_handler() fails

[Mika Westerberg]

On Sat, Jul 09, 2016 at 01:35:03PM +0900, Wolfram Sang wrote:
> Use a warning loglevel instead of info and switch to dev_* for device
> info. Also print which client was accessed.
> 
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 1/7] i2c: free idr when sanity checks in i2c_register_adapter() fail

[Jean Delvare]

On Sat,  9 Jul 2016 13:34:58 +0900, Wolfram Sang wrote:
> On error, we should give idr back to the pool in any case.
> 
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> Cc: Jean Delvare <jdelvare@suse.de>
> ---
>  drivers/i2c/i2c-core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 71ad532be1d891..7e6ff6d4cf659a 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -1560,7 +1560,7 @@ static int __process_new_adapter(struct device_driver *d, void *data)
>  
>  static int i2c_register_adapter(struct i2c_adapter *adap)
>  {
> -	int res = 0;
> +	int res = -EINVAL;
>  
>  	/* Can't register until after driver model init */
>  	if (WARN_ON(!is_registered)) {
> @@ -1572,12 +1572,12 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>  	if (unlikely(adap->name[0] == '\0')) {
>  		pr_err("i2c-core: Attempt to register an adapter with "
>  		       "no name!\n");
> -		return -EINVAL;
> +		goto out_list;
>  	}
>  	if (unlikely(!adap->algo)) {
>  		pr_err("i2c-core: Attempt to register adapter '%s' with "
>  		       "no algo!\n", adap->name);
> -		return -EINVAL;
> +		goto out_list;
>  	}
>  
>  	if (!adap->lock_bus) {

Good catch.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 0/7] i2c: improve error messages when registering an adapter

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]

On Sat, Jul 09, 2016 at 01:34:57PM +0900, Wolfram Sang wrote:
> The goal of this series is to have proper error messages whenever something in
> i2c_add_adapter() and friends fail. Then, we can hereafter drop all similar
> messages in drivers which is largely duplicated code and often not very helpful
> because they are too generic.
> 
> To achieve this goal, we make sure that dev_* is used whenever a device is
> available. pr_* is used when we have at least an adapter name. If not, we use
> WARN to show the user the code path that failed. Error messages were added,
> fixed, and made consistent. Also, the first patch fixes a potential leak found
> while creating this series. Also, i2c_register_adapter() was a bit refactored
> to be easier to handle. Please look at the patch descriptions for details. A
> brach is here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git strings/i2c
> 
> Looking forward to comments.

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]