* [PATCH 1/7] i2c: free idr when sanity checks in i2c_register_adapter() fail
2016-07-09 4:34 [PATCH 0/7] i2c: improve error messages when registering an adapter Wolfram Sang
@ 2016-07-09 4:34 ` Wolfram Sang
2016-07-11 11:51 ` Jean Delvare
2016-07-09 4:34 ` [PATCH 2/7] i2c: cleanup i2c_register_adapter() by refactoring recovery init Wolfram Sang
` (6 subsequent siblings)
7 siblings, 1 reply; 11+ messages in thread
From: Wolfram Sang @ 2016-07-09 4:34 UTC (permalink / raw)
To: linux-i2c; +Cc: Wolfram Sang, Jean Delvare
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 1/7] i2c: free idr when sanity checks in i2c_register_adapter() fail
2016-07-09 4:34 ` [PATCH 1/7] i2c: free idr when sanity checks in i2c_register_adapter() fail Wolfram Sang
@ 2016-07-11 11:51 ` Jean Delvare
0 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2016-07-11 11:51 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linux-i2c
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/7] i2c: cleanup i2c_register_adapter() by refactoring recovery init
2016-07-09 4:34 [PATCH 0/7] i2c: improve error messages when registering an adapter Wolfram Sang
2016-07-09 4:34 ` [PATCH 1/7] i2c: free idr when sanity checks in i2c_register_adapter() fail Wolfram Sang
@ 2016-07-09 4:34 ` Wolfram Sang
2016-07-09 4:35 ` [PATCH 3/7] i2c: improve error messages in i2c_register_adapter() Wolfram Sang
` (5 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Wolfram Sang @ 2016-07-09 4:34 UTC (permalink / raw)
To: linux-i2c; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 3/7] i2c: improve error messages in i2c_register_adapter()
2016-07-09 4:34 [PATCH 0/7] i2c: improve error messages when registering an adapter Wolfram Sang
2016-07-09 4:34 ` [PATCH 1/7] i2c: free idr when sanity checks in i2c_register_adapter() fail Wolfram Sang
2016-07-09 4:34 ` [PATCH 2/7] i2c: cleanup i2c_register_adapter() by refactoring recovery init Wolfram Sang
@ 2016-07-09 4:35 ` Wolfram Sang
2016-07-09 4:35 ` [PATCH 4/7] i2c: add error message when obtaining idr fails Wolfram Sang
` (4 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Wolfram Sang @ 2016-07-09 4:35 UTC (permalink / raw)
To: linux-i2c; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 4/7] i2c: add error message when obtaining idr fails
2016-07-09 4:34 [PATCH 0/7] i2c: improve error messages when registering an adapter Wolfram Sang
` (2 preceding siblings ...)
2016-07-09 4:35 ` [PATCH 3/7] i2c: improve error messages in i2c_register_adapter() Wolfram Sang
@ 2016-07-09 4:35 ` Wolfram Sang
2016-07-09 4:35 ` [PATCH 5/7] i2c: print more info when of_i2c_notify fails Wolfram Sang
` (3 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Wolfram Sang @ 2016-07-09 4:35 UTC (permalink / raw)
To: linux-i2c; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 5/7] i2c: print more info when of_i2c_notify fails
2016-07-09 4:34 [PATCH 0/7] i2c: improve error messages when registering an adapter Wolfram Sang
` (3 preceding siblings ...)
2016-07-09 4:35 ` [PATCH 4/7] i2c: add error message when obtaining idr fails Wolfram Sang
@ 2016-07-09 4:35 ` Wolfram Sang
2016-07-09 4:35 ` [PATCH 6/7] i2c: print more info when acpi_i2c_space_handler() fails Wolfram Sang
` (2 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Wolfram Sang @ 2016-07-09 4:35 UTC (permalink / raw)
To: linux-i2c; +Cc: Wolfram Sang, Pantelis Antoniou
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 6/7] i2c: print more info when acpi_i2c_space_handler() fails
2016-07-09 4:34 [PATCH 0/7] i2c: improve error messages when registering an adapter Wolfram Sang
` (4 preceding siblings ...)
2016-07-09 4:35 ` [PATCH 5/7] i2c: print more info when of_i2c_notify fails Wolfram Sang
@ 2016-07-09 4:35 ` Wolfram Sang
2016-07-09 8:11 ` Mika Westerberg
2016-07-09 4:35 ` [PATCH 7/7] i2c: use pr_fmt in the core Wolfram Sang
2016-07-14 12:47 ` [PATCH 0/7] i2c: improve error messages when registering an adapter Wolfram Sang
7 siblings, 1 reply; 11+ messages in thread
From: Wolfram Sang @ 2016-07-09 4:35 UTC (permalink / raw)
To: linux-i2c; +Cc: Wolfram Sang, Mika Westerberg, Lan Tianyu
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 7/7] i2c: use pr_fmt in the core
2016-07-09 4:34 [PATCH 0/7] i2c: improve error messages when registering an adapter Wolfram Sang
` (5 preceding siblings ...)
2016-07-09 4:35 ` [PATCH 6/7] i2c: print more info when acpi_i2c_space_handler() fails Wolfram Sang
@ 2016-07-09 4:35 ` Wolfram Sang
2016-07-14 12:47 ` [PATCH 0/7] i2c: improve error messages when registering an adapter Wolfram Sang
7 siblings, 0 replies; 11+ messages in thread
From: Wolfram Sang @ 2016-07-09 4:35 UTC (permalink / raw)
To: linux-i2c; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 0/7] i2c: improve error messages when registering an adapter
2016-07-09 4:34 [PATCH 0/7] i2c: improve error messages when registering an adapter Wolfram Sang
` (6 preceding siblings ...)
2016-07-09 4:35 ` [PATCH 7/7] i2c: use pr_fmt in the core Wolfram Sang
@ 2016-07-14 12:47 ` Wolfram Sang
7 siblings, 0 replies; 11+ messages in thread
From: Wolfram Sang @ 2016-07-14 12:47 UTC (permalink / raw)
To: linux-i2c
[-- 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 --]
^ permalink raw reply [flat|nested] 11+ messages in thread