Currently i2c_del_adapter() returns 0 on success and potentially an error code
on failure. Unfortunately this doesn't mix too well with the Linux device driver
model. An i2c_adapter is usually registered in a drivers probe callback and
removed in the drivers remove callback. The Linux device driver model assumes
that a device may disappear at any time, e.g. because it is on a hot-plug-able
bus and the device is physically removed or because it is unbound from it's
driver. This means that a driver's remove callback is not allowed to fail. This
has lead to code fragments like the following:
rc = i2c_del_adapter(&board->i2c_adap);
BUG_ON(rc);
Currently there are two code paths which can cause to i2c_del_adapter() to
return an error code:
1) If the adapter that is passed to i2c_del_adapter() is not registered
i2c_del_adapter() returns -EINVAL
2) If an I2C client, which is registered as a child to the adapter, implements
the detach_adapter callback and detach_adapter returns an error the removal of
the adapter is aborted and i2c_del_adapter() returns the error returned by the
clients detach_adapter callback.
The first probably never happens unless there is a serious bug in the driver
calling i2c_del_adapter(), but I think even then, for the sake of sanitizing the
API we can argue that the purpose of i2c_del_adapter() is to remove the adapter
from the system. If the adapter currently isn't registered this becomes a no-op
and we can return success, without having to do anything. The second also never
happens, since the detach_adapter callback has been deprecated for quite some
time now and there are currently no drivers left which implement it.
So realisticly i2c_del_adapter() already always returns 0 and all code checking
for errors is more or less dead code. And in fact the majority of the users of
i2c_del_adapter() already ignore the return value, but there are still some
drivers (both newer and older) which assume that i2c_del_adapter() might fail.
This series aims at making it explicit that i2c_del_adapter() can not fail by
making its return type void.
The series will start by making i2c_del_adapter() always return success. For
this it will update the case, where a non-registered adapter is passed in, to
return 0 instead of -EINVAL and remove all code related to the unused
detach_adapter callback. Afterward the series will update all users of
i2c_del_adapter() to ignore the return value (since it is always 0 now). And
finally the series will change the return type of i2c_del_adapter() to void.
The same is true for i2c_del_mux_adapter() which is mostly just a wrapper
around i2c_del_adapter(), so it is also updated in this series.
- Lars
Lars-Peter Clausen (6):
i2c: Remove detach_adapter
i2c: i2c_del_adapter: Don't treat removing a non-registered adapter
as error
i2c: Ignore return value of i2c_del_adapter()
i2c: Make return type of i2c_del_adapter() void
i2c: Ignore the return value of i2c_del_mux_adapter()
i2c: Make the return type of i2c_del_mux_adapter() void
drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c | 3 +--
drivers/i2c/busses/i2c-amd756-s4882.c | 6 +----
drivers/i2c/busses/i2c-at91.c | 5 ++--
drivers/i2c/busses/i2c-cbus-gpio.c | 4 ++-
drivers/i2c/busses/i2c-intel-mid.c | 3 +--
drivers/i2c/busses/i2c-mv64xxx.c | 5 ++--
drivers/i2c/busses/i2c-mxs.c | 5 +---
drivers/i2c/busses/i2c-nforce2-s4985.c | 6 +----
drivers/i2c/busses/i2c-powermac.c | 10 ++-----
drivers/i2c/busses/i2c-puv3.c | 10 ++-----
drivers/i2c/busses/i2c-viperboard.c | 5 ++--
drivers/i2c/i2c-core.c | 39 +++++++++-------------------
drivers/i2c/i2c-mux.c | 9 ++-----
drivers/i2c/muxes/i2c-mux-pca954x.c | 6 ++---
drivers/media/pci/bt8xx/bttv-input.c | 6 ++---
drivers/media/pci/mantis/mantis_i2c.c | 4 ++-
drivers/net/ethernet/sfc/falcon.c | 6 ++---
drivers/staging/media/go7007/go7007-driver.c | 7 ++---
include/linux/i2c-mux.h | 2 +-
include/linux/i2c.h | 9 +++----
20 files changed, 48 insertions(+), 102 deletions(-)
--
1.8.0
i2c_del_mux_adapter() always returns 0. So all checks testing whether it will be
non zero will always evaluate to false and the conditional code is dead code.
This patch updates all callers of i2c_del_mux_adapter() to ignore its return
value and assume that it will always succeed (which it will). A subsequent
patch will make the return type of i2c_del_mux_adapter() void.
Cc: Guenter Roeck <[email protected]>
Signed-off-by: Lars-Peter Clausen <[email protected]>
---
drivers/i2c/muxes/i2c-mux-pca954x.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 8e43872..a531d80 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -262,13 +262,11 @@ static int pca954x_remove(struct i2c_client *client)
{
struct pca954x *data = i2c_get_clientdata(client);
const struct chip_desc *chip = &chips[data->type];
- int i, err;
+ int i;
for (i = 0; i < chip->nchans; ++i)
if (data->virt_adaps[i]) {
- err = i2c_del_mux_adapter(data->virt_adaps[i]);
- if (err)
- return err;
+ i2c_del_mux_adapter(data->virt_adaps[i]);
data->virt_adaps[i] = NULL;
}
--
1.8.0
i2c_del_mux_adapter always returns 0 and none of it current users check its
return value anyway. It is also an essential requirement of the Linux device
driver model, that functions which may be called from a device's remove callback
to free resources provided by the device, are not allowed to fail. This is the
case for i2c_del_mux_adapter(), so make its return type void to make the
fact that it won't fail explicit.
Signed-off-by: Lars-Peter Clausen <[email protected]>
---
drivers/i2c/i2c-mux.c | 4 +---
include/linux/i2c-mux.h | 2 +-
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 361b78d..7409ebb 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -191,14 +191,12 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
}
EXPORT_SYMBOL_GPL(i2c_add_mux_adapter);
-int i2c_del_mux_adapter(struct i2c_adapter *adap)
+void i2c_del_mux_adapter(struct i2c_adapter *adap)
{
struct i2c_mux_priv *priv = adap->algo_data;
i2c_del_adapter(adap);
kfree(priv);
-
- return 0;
}
EXPORT_SYMBOL_GPL(i2c_del_mux_adapter);
diff --git a/include/linux/i2c-mux.h b/include/linux/i2c-mux.h
index 40cb05a..b5f9a00 100644
--- a/include/linux/i2c-mux.h
+++ b/include/linux/i2c-mux.h
@@ -42,7 +42,7 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
int (*deselect) (struct i2c_adapter *,
void *mux_dev, u32 chan_id));
-int i2c_del_mux_adapter(struct i2c_adapter *adap);
+void i2c_del_mux_adapter(struct i2c_adapter *adap);
#endif /* __KERNEL__ */
--
1.8.0
Currently i2c_del_adapter() returns -EINVAL when it gets an adapter which is not
registered. But none of the users of i2c_del_adapter() depend on this behavior,
so for the sake of being able to sanitize the return type of i2c_del_adapter
argue, that the purpose of i2c_del_adapter() is to remove an I2C adapter from
the system. If the adapter is not registered in the first place this becomes a
no-op. So we can return success without having to do anything.
Signed-off-by: Lars-Peter Clausen <[email protected]>
---
drivers/i2c/i2c-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index a853cb3..7727d33 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1072,7 +1072,7 @@ int i2c_del_adapter(struct i2c_adapter *adap)
if (found != adap) {
pr_debug("i2c-core: attempting to delete unregistered "
"adapter [%s]\n", adap->name);
- return -EINVAL;
+ return 0;
}
/* Tell drivers about this removal */
--
1.8.0
i2c_del_adapter() is usually called from a drivers remove callback. The Linux
device driver model does not allow the remove callback to fail and all resources
allocated in the probe callback need to be freed, as well as all resources which
have been provided to the rest of the kernel(for example a I2C adapter) need to
be revoked. So any function revoking such resources isn't allowed to fail
either. i2c_del_adapter() adheres to this requirement and will never fail. But
i2c_del_adapter()'s return type is int, which may cause driver authors to think
that it can fail. This led to code constructs like:
ret = i2c_del_adapter(...);
BUG_ON(ret);
Since i2c_del_adapter() always returns 0 the BUG_ON is never hit and essentially
becomes dead code, which means it can be removed. Making the return type of
i2c_del_adapter() void makes it explicit that the function will never fail and
should prevent constructs like the above from re-appearing in the kernel code.
All callers of i2c_del_adapter() have already been updated in a previous patch
to ignore the return value, so the conversion of the return type from int to
void can be done without causing any build failures.
Signed-off-by: Lars-Peter Clausen <[email protected]>
---
drivers/i2c/i2c-core.c | 6 ++----
include/linux/i2c.h | 2 +-
2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 7727d33..838d030 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1060,7 +1060,7 @@ static int __process_removed_adapter(struct device_driver *d, void *data)
* This unregisters an I2C adapter which was previously registered
* by @i2c_add_adapter or @i2c_add_numbered_adapter.
*/
-int i2c_del_adapter(struct i2c_adapter *adap)
+void i2c_del_adapter(struct i2c_adapter *adap)
{
struct i2c_adapter *found;
struct i2c_client *client, *next;
@@ -1072,7 +1072,7 @@ int i2c_del_adapter(struct i2c_adapter *adap)
if (found != adap) {
pr_debug("i2c-core: attempting to delete unregistered "
"adapter [%s]\n", adap->name);
- return 0;
+ return;
}
/* Tell drivers about this removal */
@@ -1124,8 +1124,6 @@ int i2c_del_adapter(struct i2c_adapter *adap)
/* Clear the device structure in case this adapter is ever going to be
added again */
memset(&adap->dev, 0, sizeof(adap->dev));
-
- return 0;
}
EXPORT_SYMBOL(i2c_del_adapter);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 8ffab3f..dec1702 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -447,7 +447,7 @@ void i2c_unlock_adapter(struct i2c_adapter *);
*/
#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
extern int i2c_add_adapter(struct i2c_adapter *);
-extern int i2c_del_adapter(struct i2c_adapter *);
+extern void i2c_del_adapter(struct i2c_adapter *);
extern int i2c_add_numbered_adapter(struct i2c_adapter *);
extern int i2c_register_driver(struct module *, struct i2c_driver *);
--
1.8.0
The detach_adapter callback has been deprecated for quite some time and has no
user left. Keeping it alive blocks other cleanups, so remove it.
Signed-off-by: Lars-Peter Clausen <[email protected]>
---
drivers/i2c/i2c-core.c | 33 ++++++++++-----------------------
include/linux/i2c.h | 7 ++-----
2 files changed, 12 insertions(+), 28 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index f7dfe87..a853cb3 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -47,7 +47,7 @@
/* core_lock protects i2c_adapter_idr, and guarantees
that device detection, deletion of detected devices, and attach_adapter
- and detach_adapter calls are serialized */
+ calls are serialized */
static DEFINE_MUTEX(core_lock);
static DEFINE_IDR(i2c_adapter_idr);
@@ -1013,11 +1013,10 @@ int i2c_add_numbered_adapter(struct i2c_adapter *adap)
}
EXPORT_SYMBOL_GPL(i2c_add_numbered_adapter);
-static int i2c_do_del_adapter(struct i2c_driver *driver,
+static void i2c_do_del_adapter(struct i2c_driver *driver,
struct i2c_adapter *adapter)
{
struct i2c_client *client, *_n;
- int res;
/* Remove the devices we created ourselves as the result of hardware
* probing (using a driver's detect method) */
@@ -1029,16 +1028,6 @@ static int i2c_do_del_adapter(struct i2c_driver *driver,
i2c_unregister_device(client);
}
}
-
- if (!driver->detach_adapter)
- return 0;
- dev_warn(&adapter->dev, "%s: detach_adapter method is deprecated\n",
- driver->driver.name);
- res = driver->detach_adapter(adapter);
- if (res)
- dev_err(&adapter->dev, "detach_adapter failed (%d) "
- "for driver [%s]\n", res, driver->driver.name);
- return res;
}
static int __unregister_client(struct device *dev, void *dummy)
@@ -1059,7 +1048,8 @@ static int __unregister_dummy(struct device *dev, void *dummy)
static int __process_removed_adapter(struct device_driver *d, void *data)
{
- return i2c_do_del_adapter(to_i2c_driver(d), data);
+ i2c_do_del_adapter(to_i2c_driver(d), data);
+ return 0;
}
/**
@@ -1072,7 +1062,6 @@ static int __process_removed_adapter(struct device_driver *d, void *data)
*/
int i2c_del_adapter(struct i2c_adapter *adap)
{
- int res = 0;
struct i2c_adapter *found;
struct i2c_client *client, *next;
@@ -1088,11 +1077,9 @@ int i2c_del_adapter(struct i2c_adapter *adap)
/* Tell drivers about this removal */
mutex_lock(&core_lock);
- res = bus_for_each_drv(&i2c_bus_type, NULL, adap,
+ bus_for_each_drv(&i2c_bus_type, NULL, adap,
__process_removed_adapter);
mutex_unlock(&core_lock);
- if (res)
- return res;
/* Remove devices instantiated from sysfs */
mutex_lock_nested(&adap->userspace_clients_lock,
@@ -1111,8 +1098,8 @@ int i2c_del_adapter(struct i2c_adapter *adap)
* we can't remove the dummy devices during the first pass: they
* could have been instantiated by real devices wishing to clean
* them up properly, so we give them a chance to do that first. */
- res = device_for_each_child(&adap->dev, NULL, __unregister_client);
- res = device_for_each_child(&adap->dev, NULL, __unregister_dummy);
+ device_for_each_child(&adap->dev, NULL, __unregister_client);
+ device_for_each_child(&adap->dev, NULL, __unregister_dummy);
#ifdef CONFIG_I2C_COMPAT
class_compat_remove_link(i2c_adapter_compat_class, &adap->dev,
@@ -1208,9 +1195,9 @@ EXPORT_SYMBOL(i2c_register_driver);
static int __process_removed_driver(struct device *dev, void *data)
{
- if (dev->type != &i2c_adapter_type)
- return 0;
- return i2c_do_del_adapter(data, to_i2c_adapter(dev));
+ if (dev->type == &i2c_adapter_type)
+ i2c_do_del_adapter(data, to_i2c_adapter(dev));
+ return 0;
}
/**
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index d0c4db7..8ffab3f 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -125,7 +125,6 @@ extern s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client,
* struct i2c_driver - represent an I2C device driver
* @class: What kind of i2c device we instantiate (for detect)
* @attach_adapter: Callback for bus addition (deprecated)
- * @detach_adapter: Callback for bus removal (deprecated)
* @probe: Callback for device binding
* @remove: Callback for device unbinding
* @shutdown: Callback for device shutdown
@@ -162,12 +161,10 @@ extern s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client,
struct i2c_driver {
unsigned int class;
- /* Notifies the driver that a new bus has appeared or is about to be
- * removed. You should avoid using this, it will be removed in a
- * near future.
+ /* Notifies the driver that a new bus has appeared. You should avoid
+ * using this, it will be removed in a near future.
*/
int (*attach_adapter)(struct i2c_adapter *) __deprecated;
- int (*detach_adapter)(struct i2c_adapter *) __deprecated;
/* Standard driver model interfaces */
int (*probe)(struct i2c_client *, const struct i2c_device_id *);
--
1.8.0
i2c_del_adapter() always returns 0. So all checks testing whether it will be
non zero will always evaluate to false and the conditional code is dead code.
This patch updates all callers of i2c_del_mux_adapter() to ignore the return
value and assume that it will always succeed (which it will). In a subsequent
patch the return type of i2c_del_adapter() will be made void.
Cc: Jean Delvare <[email protected]>
Cc: Guan Xuetao <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Ben Hutchings <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Alan Cox <[email protected]>
Cc: Nicolas Ferre <[email protected]>
Cc: Aaro Koskinen <[email protected]>
Cc: Marek Vasut <[email protected]> (commit_signer:11/20=55%)
Cc: Shawn Guo <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Lars Poeschel <[email protected]>
Signed-off-by: Lars-Peter Clausen <[email protected]>
---
drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c | 3 +--
drivers/i2c/busses/i2c-amd756-s4882.c | 6 +-----
drivers/i2c/busses/i2c-at91.c | 5 ++---
drivers/i2c/busses/i2c-cbus-gpio.c | 4 +++-
drivers/i2c/busses/i2c-intel-mid.c | 3 +--
drivers/i2c/busses/i2c-mv64xxx.c | 5 ++---
drivers/i2c/busses/i2c-mxs.c | 5 +----
drivers/i2c/busses/i2c-nforce2-s4985.c | 6 +-----
drivers/i2c/busses/i2c-powermac.c | 10 ++--------
drivers/i2c/busses/i2c-puv3.c | 10 ++--------
drivers/i2c/busses/i2c-viperboard.c | 5 ++---
drivers/i2c/i2c-mux.c | 5 +----
drivers/media/pci/bt8xx/bttv-input.c | 6 +++---
drivers/media/pci/mantis/mantis_i2c.c | 4 +++-
drivers/net/ethernet/sfc/falcon.c | 6 ++----
drivers/staging/media/go7007/go7007-driver.c | 7 ++-----
16 files changed, 29 insertions(+), 61 deletions(-)
diff --git a/drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c b/drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c
index 88627e3..1eb86c7 100644
--- a/drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c
+++ b/drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c
@@ -319,8 +319,7 @@ void oaktrail_hdmi_i2c_exit(struct pci_dev *dev)
struct hdmi_i2c_dev *i2c_dev;
hdmi_dev = pci_get_drvdata(dev);
- if (i2c_del_adapter(&oaktrail_hdmi_i2c_adapter))
- DRM_DEBUG_DRIVER("Failed to delete hdmi-i2c adapter\n");
+ i2c_del_adapter(&oaktrail_hdmi_i2c_adapter);
i2c_dev = hdmi_dev->i2c_dev;
kfree(i2c_dev);
diff --git a/drivers/i2c/busses/i2c-amd756-s4882.c b/drivers/i2c/busses/i2c-amd756-s4882.c
index 378fcb5..07f01ac 100644
--- a/drivers/i2c/busses/i2c-amd756-s4882.c
+++ b/drivers/i2c/busses/i2c-amd756-s4882.c
@@ -169,11 +169,7 @@ static int __init amd756_s4882_init(void)
}
/* Unregister physical bus */
- error = i2c_del_adapter(&amd756_smbus);
- if (error) {
- dev_err(&amd756_smbus.dev, "Physical bus removal failed\n");
- goto ERROR0;
- }
+ i2c_del_adapter(&amd756_smbus);
printk(KERN_INFO "Enabling SMBus multiplexing for Tyan S4882\n");
/* Define the 5 virtual adapters and algorithms structures */
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 75195e3..6604587 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -785,12 +785,11 @@ static int at91_twi_probe(struct platform_device *pdev)
static int at91_twi_remove(struct platform_device *pdev)
{
struct at91_twi_dev *dev = platform_get_drvdata(pdev);
- int rc;
- rc = i2c_del_adapter(&dev->adapter);
+ i2c_del_adapter(&dev->adapter);
clk_disable_unprepare(dev->clk);
- return rc;
+ return 0;
}
#ifdef CONFIG_PM
diff --git a/drivers/i2c/busses/i2c-cbus-gpio.c b/drivers/i2c/busses/i2c-cbus-gpio.c
index 98386d6..1be13ac 100644
--- a/drivers/i2c/busses/i2c-cbus-gpio.c
+++ b/drivers/i2c/busses/i2c-cbus-gpio.c
@@ -206,7 +206,9 @@ static int cbus_i2c_remove(struct platform_device *pdev)
{
struct i2c_adapter *adapter = platform_get_drvdata(pdev);
- return i2c_del_adapter(adapter);
+ i2c_del_adapter(adapter);
+
+ return 0;
}
static int cbus_i2c_probe(struct platform_device *pdev)
diff --git a/drivers/i2c/busses/i2c-intel-mid.c b/drivers/i2c/busses/i2c-intel-mid.c
index 323fa01..0fb6597 100644
--- a/drivers/i2c/busses/i2c-intel-mid.c
+++ b/drivers/i2c/busses/i2c-intel-mid.c
@@ -1082,8 +1082,7 @@ static void intel_mid_i2c_remove(struct pci_dev *dev)
{
struct intel_mid_i2c_private *mrst = pci_get_drvdata(dev);
intel_mid_i2c_disable(&mrst->adap);
- if (i2c_del_adapter(&mrst->adap))
- dev_err(&dev->dev, "Failed to delete i2c adapter");
+ i2c_del_adapter(&mrst->adap);
free_irq(dev->irq, mrst);
iounmap(mrst->base);
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 8b20ef8..3bbd65d 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -701,9 +701,8 @@ static int
mv64xxx_i2c_remove(struct platform_device *dev)
{
struct mv64xxx_i2c_data *drv_data = platform_get_drvdata(dev);
- int rc;
- rc = i2c_del_adapter(&drv_data->adapter);
+ i2c_del_adapter(&drv_data->adapter);
free_irq(drv_data->irq, drv_data);
mv64xxx_i2c_unmap_regs(drv_data);
#if defined(CONFIG_HAVE_CLK)
@@ -715,7 +714,7 @@ mv64xxx_i2c_remove(struct platform_device *dev)
#endif
kfree(drv_data);
- return rc;
+ return 0;
}
static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 120f246..804eb6b 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -686,11 +686,8 @@ static int mxs_i2c_probe(struct platform_device *pdev)
static int mxs_i2c_remove(struct platform_device *pdev)
{
struct mxs_i2c_dev *i2c = platform_get_drvdata(pdev);
- int ret;
- ret = i2c_del_adapter(&i2c->adapter);
- if (ret)
- return -EBUSY;
+ i2c_del_adapter(&i2c->adapter);
if (i2c->dmach)
dma_release_channel(i2c->dmach);
diff --git a/drivers/i2c/busses/i2c-nforce2-s4985.c b/drivers/i2c/busses/i2c-nforce2-s4985.c
index 29015eb..2ca268d 100644
--- a/drivers/i2c/busses/i2c-nforce2-s4985.c
+++ b/drivers/i2c/busses/i2c-nforce2-s4985.c
@@ -164,11 +164,7 @@ static int __init nforce2_s4985_init(void)
}
/* Unregister physical bus */
- error = i2c_del_adapter(nforce2_smbus);
- if (error) {
- dev_err(&nforce2_smbus->dev, "Physical bus removal failed\n");
- goto ERROR0;
- }
+ i2c_del_adapter(nforce2_smbus);
printk(KERN_INFO "Enabling SMBus multiplexing for Tyan S4985\n");
/* Define the 5 virtual adapters and algorithms structures */
diff --git a/drivers/i2c/busses/i2c-powermac.c b/drivers/i2c/busses/i2c-powermac.c
index da54e67..8dc90da 100644
--- a/drivers/i2c/busses/i2c-powermac.c
+++ b/drivers/i2c/busses/i2c-powermac.c
@@ -213,14 +213,8 @@ static const struct i2c_algorithm i2c_powermac_algorithm = {
static int i2c_powermac_remove(struct platform_device *dev)
{
struct i2c_adapter *adapter = platform_get_drvdata(dev);
- int rc;
-
- rc = i2c_del_adapter(adapter);
- /* We aren't that prepared to deal with this... */
- if (rc)
- printk(KERN_WARNING
- "i2c-powermac.c: Failed to remove bus %s !\n",
- adapter->name);
+
+ i2c_del_adapter(adapter);
memset(adapter, 0, sizeof(*adapter));
return 0;
diff --git a/drivers/i2c/busses/i2c-puv3.c b/drivers/i2c/busses/i2c-puv3.c
index 261d7db..4fd47bc 100644
--- a/drivers/i2c/busses/i2c-puv3.c
+++ b/drivers/i2c/busses/i2c-puv3.c
@@ -234,21 +234,15 @@ static int puv3_i2c_remove(struct platform_device *pdev)
{
struct i2c_adapter *adapter = platform_get_drvdata(pdev);
struct resource *mem;
- int rc;
- rc = i2c_del_adapter(adapter);
- if (rc) {
- dev_err(&pdev->dev, "Adapter '%s' delete fail\n",
- adapter->name);
- return rc;
- }
+ i2c_del_adapter(adapter);
put_device(&pdev->dev);
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
release_mem_region(mem->start, resource_size(mem));
- return rc;
+ return 0;
}
#ifdef CONFIG_PM
diff --git a/drivers/i2c/busses/i2c-viperboard.c b/drivers/i2c/busses/i2c-viperboard.c
index f45c32c..c68450c 100644
--- a/drivers/i2c/busses/i2c-viperboard.c
+++ b/drivers/i2c/busses/i2c-viperboard.c
@@ -421,11 +421,10 @@ error:
static int vprbrd_i2c_remove(struct platform_device *pdev)
{
struct vprbrd_i2c *vb_i2c = platform_get_drvdata(pdev);
- int ret;
- ret = i2c_del_adapter(&vb_i2c->i2c);
+ i2c_del_adapter(&vb_i2c->i2c);
- return ret;
+ return 0;
}
static struct platform_driver vprbrd_i2c_driver = {
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index d94e0ce..361b78d 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -194,11 +194,8 @@ EXPORT_SYMBOL_GPL(i2c_add_mux_adapter);
int i2c_del_mux_adapter(struct i2c_adapter *adap)
{
struct i2c_mux_priv *priv = adap->algo_data;
- int ret;
- ret = i2c_del_adapter(adap);
- if (ret < 0)
- return ret;
+ i2c_del_adapter(adap);
kfree(priv);
return 0;
diff --git a/drivers/media/pci/bt8xx/bttv-input.c b/drivers/media/pci/bt8xx/bttv-input.c
index 04207a7..f42d26d 100644
--- a/drivers/media/pci/bt8xx/bttv-input.c
+++ b/drivers/media/pci/bt8xx/bttv-input.c
@@ -413,10 +413,10 @@ void init_bttv_i2c_ir(struct bttv *btv)
int fini_bttv_i2c(struct bttv *btv)
{
- if (0 != btv->i2c_rc)
- return 0;
+ if (btv->i2c_rc == 0)
+ i2c_del_adapter(&btv->c.i2c_adap);
- return i2c_del_adapter(&btv->c.i2c_adap);
+ return 0;
}
int bttv_input_init(struct bttv *btv)
diff --git a/drivers/media/pci/mantis/mantis_i2c.c b/drivers/media/pci/mantis/mantis_i2c.c
index 937fb9d..895ddba 100644
--- a/drivers/media/pci/mantis/mantis_i2c.c
+++ b/drivers/media/pci/mantis/mantis_i2c.c
@@ -261,6 +261,8 @@ int mantis_i2c_exit(struct mantis_pci *mantis)
mmwrite((intmask & ~MANTIS_INT_I2CDONE), MANTIS_INT_MASK);
dprintk(MANTIS_DEBUG, 1, "Removing I2C adapter");
- return i2c_del_adapter(&mantis->adapter);
+ i2c_del_adapter(&mantis->adapter);
+
+ return 0;
}
EXPORT_SYMBOL_GPL(mantis_i2c_exit);
diff --git a/drivers/net/ethernet/sfc/falcon.c b/drivers/net/ethernet/sfc/falcon.c
index 49bcd19..defed0e 100644
--- a/drivers/net/ethernet/sfc/falcon.c
+++ b/drivers/net/ethernet/sfc/falcon.c
@@ -1528,7 +1528,7 @@ static int falcon_probe_nic(struct efx_nic *efx)
return 0;
fail6:
- BUG_ON(i2c_del_adapter(&board->i2c_adap));
+ i2c_del_adapter(&board->i2c_adap);
memset(&board->i2c_adap, 0, sizeof(board->i2c_adap));
fail5:
efx_nic_free_buffer(efx, &efx->irq_status);
@@ -1665,13 +1665,11 @@ static void falcon_remove_nic(struct efx_nic *efx)
{
struct falcon_nic_data *nic_data = efx->nic_data;
struct falcon_board *board = falcon_board(efx);
- int rc;
board->type->fini(efx);
/* Remove I2C adapter and clear it in preparation for a retry */
- rc = i2c_del_adapter(&board->i2c_adap);
- BUG_ON(rc);
+ i2c_del_adapter(&board->i2c_adap);
memset(&board->i2c_adap, 0, sizeof(board->i2c_adap));
efx_nic_free_buffer(efx, &efx->irq_status);
diff --git a/drivers/staging/media/go7007/go7007-driver.c b/drivers/staging/media/go7007/go7007-driver.c
index 6695091..6f83c52 100644
--- a/drivers/staging/media/go7007/go7007-driver.c
+++ b/drivers/staging/media/go7007/go7007-driver.c
@@ -647,11 +647,8 @@ EXPORT_SYMBOL(go7007_alloc);
void go7007_remove(struct go7007 *go)
{
if (go->i2c_adapter_online) {
- if (i2c_del_adapter(&go->i2c_adapter) == 0)
- go->i2c_adapter_online = 0;
- else
- v4l2_err(&go->v4l2_dev,
- "error removing I2C adapter!\n");
+ i2c_del_adapter(&go->i2c_adapter);
+ go->i2c_adapter_online = 0;
}
if (go->audio_enabled)
--
1.8.0
On Sat, 2013-03-09 at 19:16 +0100, Lars-Peter Clausen wrote:
> i2c_del_adapter() always returns 0. So all checks testing whether it will be
> non zero will always evaluate to false and the conditional code is dead code.
> This patch updates all callers of i2c_del_mux_adapter() to ignore the return
> value and assume that it will always succeed (which it will). In a subsequent
> patch the return type of i2c_del_adapter() will be made void.
>
> Cc: Jean Delvare <[email protected]>
> Cc: Guan Xuetao <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Ben Hutchings <[email protected]>
Acked-by: Ben Hutchings <[email protected]>
(for the sfc changes)
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Alan Cox <[email protected]>
> Cc: Nicolas Ferre <[email protected]>
> Cc: Aaro Koskinen <[email protected]>
> Cc: Marek Vasut <[email protected]> (commit_signer:11/20=55%)
You should probably remove that get_maintainer.pl annotation!
Ben.
> Cc: Shawn Guo <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Lars Poeschel <[email protected]>
> Signed-off-by: Lars-Peter Clausen <[email protected]>
[...]
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
On Sat, Mar 09, 2013 at 06:16:48PM -0000, Lars-Peter Clausen wrote:
> i2c_del_mux_adapter() always returns 0. So all checks testing whether it will be
> non zero will always evaluate to false and the conditional code is dead code.
> This patch updates all callers of i2c_del_mux_adapter() to ignore its return
> value and assume that it will always succeed (which it will). A subsequent
> patch will make the return type of i2c_del_mux_adapter() void.
>
> Cc: Guenter Roeck <[email protected]>
Acked-by: Guenter Roeck <[email protected]>
> Signed-off-by: Lars-Peter Clausen <[email protected]>
>
> ---
> drivers/i2c/muxes/i2c-mux-pca954x.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 8e43872..a531d80 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -262,13 +262,11 @@ static int pca954x_remove(struct i2c_client *client)
> {
> struct pca954x *data = i2c_get_clientdata(client);
> const struct chip_desc *chip = &chips[data->type];
> - int i, err;
> + int i;
>
> for (i = 0; i < chip->nchans; ++i)
> if (data->virt_adaps[i]) {
> - err = i2c_del_mux_adapter(data->virt_adaps[i]);
> - if (err)
> - return err;
> + i2c_del_mux_adapter(data->virt_adaps[i]);
> data->virt_adaps[i] = NULL;
> }
>
On Sat, Mar 09, 2013 at 07:16:43PM +0100, Lars-Peter Clausen wrote:
> Currently i2c_del_adapter() returns 0 on success and potentially an error code
> on failure. Unfortunately this doesn't mix too well with the Linux device driver
> model. An i2c_adapter is usually registered in a drivers probe callback and
> removed in the drivers remove callback. The Linux device driver model assumes
> that a device may disappear at any time, e.g. because it is on a hot-plug-able
> bus and the device is physically removed or because it is unbound from it's
> driver. This means that a driver's remove callback is not allowed to fail. This
> has lead to code fragments like the following:
>
> rc = i2c_del_adapter(&board->i2c_adap);
> BUG_ON(rc);
>
> Currently there are two code paths which can cause to i2c_del_adapter() to
> return an error code:
> 1) If the adapter that is passed to i2c_del_adapter() is not registered
> i2c_del_adapter() returns -EINVAL
> 2) If an I2C client, which is registered as a child to the adapter, implements
> the detach_adapter callback and detach_adapter returns an error the removal of
> the adapter is aborted and i2c_del_adapter() returns the error returned by the
> clients detach_adapter callback.
>
> The first probably never happens unless there is a serious bug in the driver
> calling i2c_del_adapter(), but I think even then, for the sake of sanitizing the
> API we can argue that the purpose of i2c_del_adapter() is to remove the adapter
> from the system. If the adapter currently isn't registered this becomes a no-op
> and we can return success, without having to do anything. The second also never
> happens, since the detach_adapter callback has been deprecated for quite some
> time now and there are currently no drivers left which implement it.
>
> So realisticly i2c_del_adapter() already always returns 0 and all code checking
> for errors is more or less dead code. And in fact the majority of the users of
> i2c_del_adapter() already ignore the return value, but there are still some
> drivers (both newer and older) which assume that i2c_del_adapter() might fail.
> This series aims at making it explicit that i2c_del_adapter() can not fail by
> making its return type void.
>
> The series will start by making i2c_del_adapter() always return success. For
> this it will update the case, where a non-registered adapter is passed in, to
> return 0 instead of -EINVAL and remove all code related to the unused
> detach_adapter callback. Afterward the series will update all users of
> i2c_del_adapter() to ignore the return value (since it is always 0 now). And
> finally the series will change the return type of i2c_del_adapter() to void.
>
> The same is true for i2c_del_mux_adapter() which is mostly just a wrapper
> around i2c_del_adapter(), so it is also updated in this series.
Looks great from a high level view. Will now dive into more detailed
review, but really looking good. Thanks for doing this.
On Sat, Mar 09, 2013 at 07:16:46PM +0100, Lars-Peter Clausen wrote:
> i2c_del_adapter() always returns 0. So all checks testing whether it will be
> non zero will always evaluate to false and the conditional code is dead code.
> This patch updates all callers of i2c_del_mux_adapter() to ignore the return
> value and assume that it will always succeed (which it will). In a subsequent
> patch the return type of i2c_del_adapter() will be made void.
>
> Cc: Jean Delvare <[email protected]>
> Cc: Guan Xuetao <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Ben Hutchings <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Alan Cox <[email protected]>
> Cc: Nicolas Ferre <[email protected]>
> Cc: Aaro Koskinen <[email protected]>
> Cc: Marek Vasut <[email protected]> (commit_signer:11/20=55%)
> Cc: Shawn Guo <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Lars Poeschel <[email protected]>
> Signed-off-by: Lars-Peter Clausen <[email protected]>
Ping for more acks outside the i2c realm.
> ---
> drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c | 3 +--
> drivers/i2c/busses/i2c-amd756-s4882.c | 6 +-----
> drivers/i2c/busses/i2c-at91.c | 5 ++---
> drivers/i2c/busses/i2c-cbus-gpio.c | 4 +++-
> drivers/i2c/busses/i2c-intel-mid.c | 3 +--
> drivers/i2c/busses/i2c-mv64xxx.c | 5 ++---
> drivers/i2c/busses/i2c-mxs.c | 5 +----
> drivers/i2c/busses/i2c-nforce2-s4985.c | 6 +-----
> drivers/i2c/busses/i2c-powermac.c | 10 ++--------
> drivers/i2c/busses/i2c-puv3.c | 10 ++--------
> drivers/i2c/busses/i2c-viperboard.c | 5 ++---
> drivers/i2c/i2c-mux.c | 5 +----
> drivers/media/pci/bt8xx/bttv-input.c | 6 +++---
> drivers/media/pci/mantis/mantis_i2c.c | 4 +++-
> drivers/net/ethernet/sfc/falcon.c | 6 ++----
> drivers/staging/media/go7007/go7007-driver.c | 7 ++-----
> 16 files changed, 29 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c b/drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c
> index 88627e3..1eb86c7 100644
> --- a/drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c
> +++ b/drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c
> @@ -319,8 +319,7 @@ void oaktrail_hdmi_i2c_exit(struct pci_dev *dev)
> struct hdmi_i2c_dev *i2c_dev;
>
> hdmi_dev = pci_get_drvdata(dev);
> - if (i2c_del_adapter(&oaktrail_hdmi_i2c_adapter))
> - DRM_DEBUG_DRIVER("Failed to delete hdmi-i2c adapter\n");
> + i2c_del_adapter(&oaktrail_hdmi_i2c_adapter);
>
> i2c_dev = hdmi_dev->i2c_dev;
> kfree(i2c_dev);
> diff --git a/drivers/i2c/busses/i2c-amd756-s4882.c b/drivers/i2c/busses/i2c-amd756-s4882.c
> index 378fcb5..07f01ac 100644
> --- a/drivers/i2c/busses/i2c-amd756-s4882.c
> +++ b/drivers/i2c/busses/i2c-amd756-s4882.c
> @@ -169,11 +169,7 @@ static int __init amd756_s4882_init(void)
> }
>
> /* Unregister physical bus */
> - error = i2c_del_adapter(&amd756_smbus);
> - if (error) {
> - dev_err(&amd756_smbus.dev, "Physical bus removal failed\n");
> - goto ERROR0;
> - }
> + i2c_del_adapter(&amd756_smbus);
>
> printk(KERN_INFO "Enabling SMBus multiplexing for Tyan S4882\n");
> /* Define the 5 virtual adapters and algorithms structures */
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index 75195e3..6604587 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -785,12 +785,11 @@ static int at91_twi_probe(struct platform_device *pdev)
> static int at91_twi_remove(struct platform_device *pdev)
> {
> struct at91_twi_dev *dev = platform_get_drvdata(pdev);
> - int rc;
>
> - rc = i2c_del_adapter(&dev->adapter);
> + i2c_del_adapter(&dev->adapter);
> clk_disable_unprepare(dev->clk);
>
> - return rc;
> + return 0;
> }
>
> #ifdef CONFIG_PM
> diff --git a/drivers/i2c/busses/i2c-cbus-gpio.c b/drivers/i2c/busses/i2c-cbus-gpio.c
> index 98386d6..1be13ac 100644
> --- a/drivers/i2c/busses/i2c-cbus-gpio.c
> +++ b/drivers/i2c/busses/i2c-cbus-gpio.c
> @@ -206,7 +206,9 @@ static int cbus_i2c_remove(struct platform_device *pdev)
> {
> struct i2c_adapter *adapter = platform_get_drvdata(pdev);
>
> - return i2c_del_adapter(adapter);
> + i2c_del_adapter(adapter);
> +
> + return 0;
> }
>
> static int cbus_i2c_probe(struct platform_device *pdev)
> diff --git a/drivers/i2c/busses/i2c-intel-mid.c b/drivers/i2c/busses/i2c-intel-mid.c
> index 323fa01..0fb6597 100644
> --- a/drivers/i2c/busses/i2c-intel-mid.c
> +++ b/drivers/i2c/busses/i2c-intel-mid.c
> @@ -1082,8 +1082,7 @@ static void intel_mid_i2c_remove(struct pci_dev *dev)
> {
> struct intel_mid_i2c_private *mrst = pci_get_drvdata(dev);
> intel_mid_i2c_disable(&mrst->adap);
> - if (i2c_del_adapter(&mrst->adap))
> - dev_err(&dev->dev, "Failed to delete i2c adapter");
> + i2c_del_adapter(&mrst->adap);
>
> free_irq(dev->irq, mrst);
> iounmap(mrst->base);
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index 8b20ef8..3bbd65d 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -701,9 +701,8 @@ static int
> mv64xxx_i2c_remove(struct platform_device *dev)
> {
> struct mv64xxx_i2c_data *drv_data = platform_get_drvdata(dev);
> - int rc;
>
> - rc = i2c_del_adapter(&drv_data->adapter);
> + i2c_del_adapter(&drv_data->adapter);
> free_irq(drv_data->irq, drv_data);
> mv64xxx_i2c_unmap_regs(drv_data);
> #if defined(CONFIG_HAVE_CLK)
> @@ -715,7 +714,7 @@ mv64xxx_i2c_remove(struct platform_device *dev)
> #endif
> kfree(drv_data);
>
> - return rc;
> + return 0;
> }
>
> static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
> diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
> index 120f246..804eb6b 100644
> --- a/drivers/i2c/busses/i2c-mxs.c
> +++ b/drivers/i2c/busses/i2c-mxs.c
> @@ -686,11 +686,8 @@ static int mxs_i2c_probe(struct platform_device *pdev)
> static int mxs_i2c_remove(struct platform_device *pdev)
> {
> struct mxs_i2c_dev *i2c = platform_get_drvdata(pdev);
> - int ret;
>
> - ret = i2c_del_adapter(&i2c->adapter);
> - if (ret)
> - return -EBUSY;
> + i2c_del_adapter(&i2c->adapter);
>
> if (i2c->dmach)
> dma_release_channel(i2c->dmach);
> diff --git a/drivers/i2c/busses/i2c-nforce2-s4985.c b/drivers/i2c/busses/i2c-nforce2-s4985.c
> index 29015eb..2ca268d 100644
> --- a/drivers/i2c/busses/i2c-nforce2-s4985.c
> +++ b/drivers/i2c/busses/i2c-nforce2-s4985.c
> @@ -164,11 +164,7 @@ static int __init nforce2_s4985_init(void)
> }
>
> /* Unregister physical bus */
> - error = i2c_del_adapter(nforce2_smbus);
> - if (error) {
> - dev_err(&nforce2_smbus->dev, "Physical bus removal failed\n");
> - goto ERROR0;
> - }
> + i2c_del_adapter(nforce2_smbus);
>
> printk(KERN_INFO "Enabling SMBus multiplexing for Tyan S4985\n");
> /* Define the 5 virtual adapters and algorithms structures */
> diff --git a/drivers/i2c/busses/i2c-powermac.c b/drivers/i2c/busses/i2c-powermac.c
> index da54e67..8dc90da 100644
> --- a/drivers/i2c/busses/i2c-powermac.c
> +++ b/drivers/i2c/busses/i2c-powermac.c
> @@ -213,14 +213,8 @@ static const struct i2c_algorithm i2c_powermac_algorithm = {
> static int i2c_powermac_remove(struct platform_device *dev)
> {
> struct i2c_adapter *adapter = platform_get_drvdata(dev);
> - int rc;
> -
> - rc = i2c_del_adapter(adapter);
> - /* We aren't that prepared to deal with this... */
> - if (rc)
> - printk(KERN_WARNING
> - "i2c-powermac.c: Failed to remove bus %s !\n",
> - adapter->name);
> +
> + i2c_del_adapter(adapter);
> memset(adapter, 0, sizeof(*adapter));
>
> return 0;
> diff --git a/drivers/i2c/busses/i2c-puv3.c b/drivers/i2c/busses/i2c-puv3.c
> index 261d7db..4fd47bc 100644
> --- a/drivers/i2c/busses/i2c-puv3.c
> +++ b/drivers/i2c/busses/i2c-puv3.c
> @@ -234,21 +234,15 @@ static int puv3_i2c_remove(struct platform_device *pdev)
> {
> struct i2c_adapter *adapter = platform_get_drvdata(pdev);
> struct resource *mem;
> - int rc;
>
> - rc = i2c_del_adapter(adapter);
> - if (rc) {
> - dev_err(&pdev->dev, "Adapter '%s' delete fail\n",
> - adapter->name);
> - return rc;
> - }
> + i2c_del_adapter(adapter);
>
> put_device(&pdev->dev);
>
> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> release_mem_region(mem->start, resource_size(mem));
>
> - return rc;
> + return 0;
> }
>
> #ifdef CONFIG_PM
> diff --git a/drivers/i2c/busses/i2c-viperboard.c b/drivers/i2c/busses/i2c-viperboard.c
> index f45c32c..c68450c 100644
> --- a/drivers/i2c/busses/i2c-viperboard.c
> +++ b/drivers/i2c/busses/i2c-viperboard.c
> @@ -421,11 +421,10 @@ error:
> static int vprbrd_i2c_remove(struct platform_device *pdev)
> {
> struct vprbrd_i2c *vb_i2c = platform_get_drvdata(pdev);
> - int ret;
>
> - ret = i2c_del_adapter(&vb_i2c->i2c);
> + i2c_del_adapter(&vb_i2c->i2c);
>
> - return ret;
> + return 0;
> }
>
> static struct platform_driver vprbrd_i2c_driver = {
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index d94e0ce..361b78d 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -194,11 +194,8 @@ EXPORT_SYMBOL_GPL(i2c_add_mux_adapter);
> int i2c_del_mux_adapter(struct i2c_adapter *adap)
> {
> struct i2c_mux_priv *priv = adap->algo_data;
> - int ret;
>
> - ret = i2c_del_adapter(adap);
> - if (ret < 0)
> - return ret;
> + i2c_del_adapter(adap);
> kfree(priv);
>
> return 0;
> diff --git a/drivers/media/pci/bt8xx/bttv-input.c b/drivers/media/pci/bt8xx/bttv-input.c
> index 04207a7..f42d26d 100644
> --- a/drivers/media/pci/bt8xx/bttv-input.c
> +++ b/drivers/media/pci/bt8xx/bttv-input.c
> @@ -413,10 +413,10 @@ void init_bttv_i2c_ir(struct bttv *btv)
>
> int fini_bttv_i2c(struct bttv *btv)
> {
> - if (0 != btv->i2c_rc)
> - return 0;
> + if (btv->i2c_rc == 0)
> + i2c_del_adapter(&btv->c.i2c_adap);
>
> - return i2c_del_adapter(&btv->c.i2c_adap);
> + return 0;
> }
>
> int bttv_input_init(struct bttv *btv)
> diff --git a/drivers/media/pci/mantis/mantis_i2c.c b/drivers/media/pci/mantis/mantis_i2c.c
> index 937fb9d..895ddba 100644
> --- a/drivers/media/pci/mantis/mantis_i2c.c
> +++ b/drivers/media/pci/mantis/mantis_i2c.c
> @@ -261,6 +261,8 @@ int mantis_i2c_exit(struct mantis_pci *mantis)
> mmwrite((intmask & ~MANTIS_INT_I2CDONE), MANTIS_INT_MASK);
>
> dprintk(MANTIS_DEBUG, 1, "Removing I2C adapter");
> - return i2c_del_adapter(&mantis->adapter);
> + i2c_del_adapter(&mantis->adapter);
> +
> + return 0;
> }
> EXPORT_SYMBOL_GPL(mantis_i2c_exit);
> diff --git a/drivers/net/ethernet/sfc/falcon.c b/drivers/net/ethernet/sfc/falcon.c
> index 49bcd19..defed0e 100644
> --- a/drivers/net/ethernet/sfc/falcon.c
> +++ b/drivers/net/ethernet/sfc/falcon.c
> @@ -1528,7 +1528,7 @@ static int falcon_probe_nic(struct efx_nic *efx)
> return 0;
>
> fail6:
> - BUG_ON(i2c_del_adapter(&board->i2c_adap));
> + i2c_del_adapter(&board->i2c_adap);
> memset(&board->i2c_adap, 0, sizeof(board->i2c_adap));
> fail5:
> efx_nic_free_buffer(efx, &efx->irq_status);
> @@ -1665,13 +1665,11 @@ static void falcon_remove_nic(struct efx_nic *efx)
> {
> struct falcon_nic_data *nic_data = efx->nic_data;
> struct falcon_board *board = falcon_board(efx);
> - int rc;
>
> board->type->fini(efx);
>
> /* Remove I2C adapter and clear it in preparation for a retry */
> - rc = i2c_del_adapter(&board->i2c_adap);
> - BUG_ON(rc);
> + i2c_del_adapter(&board->i2c_adap);
> memset(&board->i2c_adap, 0, sizeof(board->i2c_adap));
>
> efx_nic_free_buffer(efx, &efx->irq_status);
> diff --git a/drivers/staging/media/go7007/go7007-driver.c b/drivers/staging/media/go7007/go7007-driver.c
> index 6695091..6f83c52 100644
> --- a/drivers/staging/media/go7007/go7007-driver.c
> +++ b/drivers/staging/media/go7007/go7007-driver.c
> @@ -647,11 +647,8 @@ EXPORT_SYMBOL(go7007_alloc);
> void go7007_remove(struct go7007 *go)
> {
> if (go->i2c_adapter_online) {
> - if (i2c_del_adapter(&go->i2c_adapter) == 0)
> - go->i2c_adapter_online = 0;
> - else
> - v4l2_err(&go->v4l2_dev,
> - "error removing I2C adapter!\n");
> + i2c_del_adapter(&go->i2c_adapter);
> + go->i2c_adapter_online = 0;
> }
>
> if (go->audio_enabled)
> --
> 1.8.0
>
Hi Lars-Peter,
On Sat, 9 Mar 2013 19:16:46 +0100, Lars-Peter Clausen wrote:
> i2c_del_adapter() always returns 0.
I beg you pardon? i2c_del_adapter() starts with:
int i2c_del_adapter(struct i2c_adapter *adap)
{
int res = 0;
struct i2c_adapter *found;
struct i2c_client *client, *next;
/* First make sure that this adapter was ever added */
mutex_lock(&core_lock);
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);
return -EINVAL;
}
/* Tell drivers about this removal */
mutex_lock(&core_lock);
res = bus_for_each_drv(&i2c_bus_type, NULL, adap,
__process_removed_adapter);
mutex_unlock(&core_lock);
if (res)
return res;
(...)
So, no, it doesn't "always return 0".
Thus nack from me.
--
Jean Delvare
Hi Jean,
On 03/30/2013 08:55 AM, Jean Delvare wrote:
> Hi Lars-Peter,
>
> On Sat, 9 Mar 2013 19:16:46 +0100, Lars-Peter Clausen wrote:
>> i2c_del_adapter() always returns 0.
>
> I beg you pardon? i2c_del_adapter() starts with:
>
> int i2c_del_adapter(struct i2c_adapter *adap)
> {
> int res = 0;
> struct i2c_adapter *found;
> struct i2c_client *client, *next;
>
> /* First make sure that this adapter was ever added */
> mutex_lock(&core_lock);
> 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);
> return -EINVAL;
> }
>
> /* Tell drivers about this removal */
> mutex_lock(&core_lock);
> res = bus_for_each_drv(&i2c_bus_type, NULL, adap,
> __process_removed_adapter);
> mutex_unlock(&core_lock);
> if (res)
> return res;
> (...)
>
> So, no, it doesn't "always return 0".
>
Patch 1 and 2 in this series remove those two instances:
https://lkml.org/lkml/2013/3/9/72
https://lkml.org/lkml/2013/3/9/86
For an explanation why this should be done please take a look at the cover
letter to this patch series https://lkml.org/lkml/2013/3/9/73
- Lars
On Sat, 30 Mar 2013 09:11:54 +0100, Lars-Peter Clausen wrote:
> Hi Jean,
>
> On 03/30/2013 08:55 AM, Jean Delvare wrote:
> > Hi Lars-Peter,
> >
> > On Sat, 9 Mar 2013 19:16:46 +0100, Lars-Peter Clausen wrote:
> >> i2c_del_adapter() always returns 0.
> >
> > I beg you pardon? i2c_del_adapter() starts with:
> >
> > int i2c_del_adapter(struct i2c_adapter *adap)
> > {
> > int res = 0;
> > struct i2c_adapter *found;
> > struct i2c_client *client, *next;
> >
> > /* First make sure that this adapter was ever added */
> > mutex_lock(&core_lock);
> > 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);
> > return -EINVAL;
> > }
> >
> > /* Tell drivers about this removal */
> > mutex_lock(&core_lock);
> > res = bus_for_each_drv(&i2c_bus_type, NULL, adap,
> > __process_removed_adapter);
> > mutex_unlock(&core_lock);
> > if (res)
> > return res;
> > (...)
> >
> > So, no, it doesn't "always return 0".
> >
>
> Patch 1 and 2 in this series remove those two instances:
> https://lkml.org/lkml/2013/3/9/72
> https://lkml.org/lkml/2013/3/9/86
>
> For an explanation why this should be done please take a look at the cover
> letter to this patch series https://lkml.org/lkml/2013/3/9/73
Well well... Either you think this must be done and isn't questionable,
and you shouldn't bother Cc'ing a dozen driver maintainers and waiting
for them to ack. Or you think this is something for discussion and you
should consistently Cc all interested parties on the whole patch
series. If you send me one random patch in the middle of a series and
expect me to go fish for the missing parts so that I can make sense of
it all and make sane and useful comments, well this isn't going to
happen, sorry.
Now with the above pointers, I can actually make useful comments, and I
will.
--
Jean Delvare
Hi Lars,
On Sat, 9 Mar 2013 19:16:44 +0100, Lars-Peter Clausen wrote:
> The detach_adapter callback has been deprecated for quite some time and has no
> user left. Keeping it alive blocks other cleanups, so remove it.
I'm all for it. Originally I intended to remove both attach_adapter and
detach_adapter at the same time, unfortunately there are still users of
attach_adapter around, despite it being deprecated for 2 or 3 years
now. So the patch removing them is still sitting on my disk. Getting
rid of at least detach_adapter now is a good idea.
One minor comment:
> @@ -1088,11 +1077,9 @@ int i2c_del_adapter(struct i2c_adapter *adap)
>
> /* Tell drivers about this removal */
> mutex_lock(&core_lock);
> - res = bus_for_each_drv(&i2c_bus_type, NULL, adap,
> + bus_for_each_drv(&i2c_bus_type, NULL, adap,
> __process_removed_adapter);
This would fit on a single line now.
Reviewed-by: Jean Delvare <[email protected]>
--
Jean Delvare
On Sat, 9 Mar 2013 19:16:45 +0100, Lars-Peter Clausen wrote:
> Currently i2c_del_adapter() returns -EINVAL when it gets an adapter which is not
> registered. But none of the users of i2c_del_adapter() depend on this behavior,
At least two used to depend on it actually, and this is even why this
code was added in the first place. See:
http://lists.lm-sensors.org/pipermail/lm-sensors/2004-November/009350.html
The check was added for the i2c-amd756-s4882 and i2c-nforce2-s4985
old-style SMBus multiplexing drivers.
Meanwhile a safer check was added:
commit 399d6b26539d83dd734746dc2292d53fbc5807b2
Author: Jean Delvare <[email protected]>
Date: Sun Aug 10 22:56:15 2008 +0200
i2c: Fix oops on bus multiplexer driver loading
The two I2C bus multiplexer drivers (i2c-amd756-s4882 and
i2c-nforce2-s4985) make use of the bus they want to multiplex before
checking if it is really present. Swap the instructions to test for
presence first. This fixes a oops reported by Ingo Molnar.
So these drivers indeed no longer depend on i2c_del_adapter() to detect
whether their parent I2C adapter has been added or not.
These two drivers should be removed and replaced with code using the
standard I2C multiplexing infrastructure anyway, but it's not clear
when anyone will find the time to actually do that.
> so for the sake of being able to sanitize the return type of i2c_del_adapter
> argue, that the purpose of i2c_del_adapter() is to remove an I2C adapter from
> the system. If the adapter is not registered in the first place this becomes a
> no-op. So we can return success without having to do anything.
>
> Signed-off-by: Lars-Peter Clausen <[email protected]>
> ---
> drivers/i2c/i2c-core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index a853cb3..7727d33 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -1072,7 +1072,7 @@ int i2c_del_adapter(struct i2c_adapter *adap)
> if (found != adap) {
> pr_debug("i2c-core: attempting to delete unregistered "
> "adapter [%s]\n", adap->name);
> - return -EINVAL;
> + return 0;
> }
>
> /* Tell drivers about this removal */
Reviewed-by: Jean Delvare <[email protected]>
A more radical approach would be to say that you are simply not
supposed to try and remove an adapter that was never added, so the
whole check is pointless and should be removed. After all,
i2c_unregister_device() and i2c_del_driver() do not have such a check.
I don't know how cautious other subsystems are but I suspect most don't
have such a check either.
--
Jean Delvare
Hi Lars,
On Sat, 9 Mar 2013 19:16:43 +0100, Lars-Peter Clausen wrote:
> Currently i2c_del_adapter() returns 0 on success and potentially an error code
> on failure. Unfortunately this doesn't mix too well with the Linux device driver
> model. (...)
I see:
struct device_driver {
(...)
int (*probe) (struct device *dev);
int (*remove) (struct device *dev);
So the driver core does allow remove functions to return an error. Are
you going to fix all subsystems as you are doing for i2c now, and then
change device_driver.remove to return void? If not, I don't see the
point of changing it in i2c.
--
Jean Delvare
On 03/30/2013 09:13 PM, Jean Delvare wrote:
> Hi Lars,
>
> On Sat, 9 Mar 2013 19:16:43 +0100, Lars-Peter Clausen wrote:
>> Currently i2c_del_adapter() returns 0 on success and potentially an error code
>> on failure. Unfortunately this doesn't mix too well with the Linux device driver
>> model. (...)
>
> I see:
>
> struct device_driver {
> (...)
> int (*probe) (struct device *dev);
> int (*remove) (struct device *dev);
>
> So the driver core does allow remove functions to return an error.
Well, the return type is int, but the return value is never checked. So you
can return an error value, but the device driver core is going to care and
the device is still removed. So any code which does return an error in it's
probe function in the assumption that this means the device will still be
present is broken and leaves the system in an undefined state. So if that
happens the kernel will probably crash sooner or later, or if you are lucky
you only created a few resources leaks.
And no we can't change the core to handle errors from a drivers remove
callback. It's a basic inherent property of the Linux device driver model
that any device can be removed at any time.
> Are you going to fix all subsystems as you are doing for i2c now, and then
> change device_driver.remove to return void? If not, I don't see the
> point of changing it in i2c.
>
As I said it's a bug if a driver returns an error in its remove function.
And the fact that the return type of the remove callback is int is pretty
much misleading in this regard, so the long term goal is to make the return
type void. But that's a long way to go until we get there, fixing the return
type of i2c_del_adapter() is kind of the low hanging fruit.
- Lars
On Sat, 30 Mar 2013 21:43:29 +0100, Lars-Peter Clausen wrote:
> On 03/30/2013 09:13 PM, Jean Delvare wrote:
> > I see:
> >
> > struct device_driver {
> > (...)
> > int (*probe) (struct device *dev);
> > int (*remove) (struct device *dev);
> >
> > So the driver core does allow remove functions to return an error.
>
> Well, the return type is int, but the return value is never checked. So you
> can return an error value, but the device driver core is going to care and
> the device is still removed. So any code which does return an error in it's
> probe function in the assumption that this means the device will still be
> present is broken and leaves the system in an undefined state. So if that
> happens the kernel will probably crash sooner or later, or if you are lucky
> you only created a few resources leaks.
>
> And no we can't change the core to handle errors from a drivers remove
> callback. It's a basic inherent property of the Linux device driver model
> that any device can be removed at any time.
>
> > Are you going to fix all subsystems as you are doing for i2c now, and then
> > change device_driver.remove to return void? If not, I don't see the
> > point of changing it in i2c.
>
> As I said it's a bug if a driver returns an error in its remove function.
> And the fact that the return type of the remove callback is int is pretty
> much misleading in this regard, so the long term goal is to make the return
> type void. But that's a long way to go until we get there, fixing the return
> type of i2c_del_adapter() is kind of the low hanging fruit.
OK, makes sense, thanks for the clarification.
--
Jean Delvare
On Sat, 9 Mar 2013 19:16:46 +0100, Lars-Peter Clausen wrote:
> i2c_del_adapter() always returns 0. So all checks testing whether it will be
> non zero will always evaluate to false and the conditional code is dead code.
> This patch updates all callers of i2c_del_mux_adapter() to ignore the return
> value and assume that it will always succeed (which it will). In a subsequent
> patch the return type of i2c_del_adapter() will be made void.
>
> Cc: Jean Delvare <[email protected]>
> Cc: Guan Xuetao <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Ben Hutchings <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Alan Cox <[email protected]>
> Cc: Nicolas Ferre <[email protected]>
> Cc: Aaro Koskinen <[email protected]>
> Cc: Marek Vasut <[email protected]> (commit_signer:11/20=55%)
> Cc: Shawn Guo <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Lars Poeschel <[email protected]>
> Signed-off-by: Lars-Peter Clausen <[email protected]>
> ---
> drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c | 3 +--
> drivers/i2c/busses/i2c-amd756-s4882.c | 6 +-----
> drivers/i2c/busses/i2c-at91.c | 5 ++---
> drivers/i2c/busses/i2c-cbus-gpio.c | 4 +++-
> drivers/i2c/busses/i2c-intel-mid.c | 3 +--
> drivers/i2c/busses/i2c-mv64xxx.c | 5 ++---
> drivers/i2c/busses/i2c-mxs.c | 5 +----
> drivers/i2c/busses/i2c-nforce2-s4985.c | 6 +-----
> drivers/i2c/busses/i2c-powermac.c | 10 ++--------
> drivers/i2c/busses/i2c-puv3.c | 10 ++--------
> drivers/i2c/busses/i2c-viperboard.c | 5 ++---
> drivers/i2c/i2c-mux.c | 5 +----
> drivers/media/pci/bt8xx/bttv-input.c | 6 +++---
> drivers/media/pci/mantis/mantis_i2c.c | 4 +++-
> drivers/net/ethernet/sfc/falcon.c | 6 ++----
> drivers/staging/media/go7007/go7007-driver.c | 7 ++-----
> 16 files changed, 29 insertions(+), 61 deletions(-)
> (...)
For i2c-amd756-s4882, i2c-nforce2-s4985 and i2c-mux:
Reviewed-by: Jean Delvare <[email protected]>
--
Jean Delvare
On Sat, 9 Mar 2013 19:16:47 +0100, Lars-Peter Clausen wrote:
> i2c_del_adapter() is usually called from a drivers remove callback. The Linux
> device driver model does not allow the remove callback to fail and all resources
> allocated in the probe callback need to be freed, as well as all resources which
> have been provided to the rest of the kernel(for example a I2C adapter) need to
> be revoked. So any function revoking such resources isn't allowed to fail
> either. i2c_del_adapter() adheres to this requirement and will never fail. But
> i2c_del_adapter()'s return type is int, which may cause driver authors to think
> that it can fail. This led to code constructs like:
>
> ret = i2c_del_adapter(...);
> BUG_ON(ret);
>
> Since i2c_del_adapter() always returns 0 the BUG_ON is never hit and essentially
> becomes dead code, which means it can be removed. Making the return type of
> i2c_del_adapter() void makes it explicit that the function will never fail and
> should prevent constructs like the above from re-appearing in the kernel code.
>
> All callers of i2c_del_adapter() have already been updated in a previous patch
> to ignore the return value, so the conversion of the return type from int to
> void can be done without causing any build failures.
>
> Signed-off-by: Lars-Peter Clausen <[email protected]>
> ---
> drivers/i2c/i2c-core.c | 6 ++----
> include/linux/i2c.h | 2 +-
> 2 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 7727d33..838d030 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -1060,7 +1060,7 @@ static int __process_removed_adapter(struct device_driver *d, void *data)
> * This unregisters an I2C adapter which was previously registered
> * by @i2c_add_adapter or @i2c_add_numbered_adapter.
> */
> -int i2c_del_adapter(struct i2c_adapter *adap)
> +void i2c_del_adapter(struct i2c_adapter *adap)
> {
> struct i2c_adapter *found;
> struct i2c_client *client, *next;
> @@ -1072,7 +1072,7 @@ int i2c_del_adapter(struct i2c_adapter *adap)
> if (found != adap) {
> pr_debug("i2c-core: attempting to delete unregistered "
> "adapter [%s]\n", adap->name);
> - return 0;
> + return;
> }
>
> /* Tell drivers about this removal */
> @@ -1124,8 +1124,6 @@ int i2c_del_adapter(struct i2c_adapter *adap)
> /* Clear the device structure in case this adapter is ever going to be
> added again */
> memset(&adap->dev, 0, sizeof(adap->dev));
> -
> - return 0;
> }
> EXPORT_SYMBOL(i2c_del_adapter);
>
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 8ffab3f..dec1702 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -447,7 +447,7 @@ void i2c_unlock_adapter(struct i2c_adapter *);
> */
> #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
> extern int i2c_add_adapter(struct i2c_adapter *);
> -extern int i2c_del_adapter(struct i2c_adapter *);
> +extern void i2c_del_adapter(struct i2c_adapter *);
> extern int i2c_add_numbered_adapter(struct i2c_adapter *);
>
> extern int i2c_register_driver(struct module *, struct i2c_driver *);
Reviewed-by: Jean Delvare <[email protected]>
--
Jean Delvare
On Sat, 9 Mar 2013 19:16:48 +0100, Lars-Peter Clausen wrote:
> i2c_del_mux_adapter() always returns 0. So all checks testing whether it will be
> non zero will always evaluate to false and the conditional code is dead code.
> This patch updates all callers of i2c_del_mux_adapter() to ignore its return
> value and assume that it will always succeed (which it will). A subsequent
> patch will make the return type of i2c_del_mux_adapter() void.
>
> Cc: Guenter Roeck <[email protected]>
> Signed-off-by: Lars-Peter Clausen <[email protected]>
> ---
> drivers/i2c/muxes/i2c-mux-pca954x.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 8e43872..a531d80 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -262,13 +262,11 @@ static int pca954x_remove(struct i2c_client *client)
> {
> struct pca954x *data = i2c_get_clientdata(client);
> const struct chip_desc *chip = &chips[data->type];
> - int i, err;
> + int i;
>
> for (i = 0; i < chip->nchans; ++i)
> if (data->virt_adaps[i]) {
> - err = i2c_del_mux_adapter(data->virt_adaps[i]);
> - if (err)
> - return err;
> + i2c_del_mux_adapter(data->virt_adaps[i]);
> data->virt_adaps[i] = NULL;
> }
>
Reviewed-by: Jean Delvare <[email protected]>
--
Jean Delvare
On Sat, 9 Mar 2013 19:16:49 +0100, Lars-Peter Clausen wrote:
> i2c_del_mux_adapter always returns 0 and none of it current users check its
> return value anyway. It is also an essential requirement of the Linux device
> driver model, that functions which may be called from a device's remove callback
> to free resources provided by the device, are not allowed to fail. This is the
> case for i2c_del_mux_adapter(), so make its return type void to make the
> fact that it won't fail explicit.
>
> Signed-off-by: Lars-Peter Clausen <[email protected]>
> ---
> drivers/i2c/i2c-mux.c | 4 +---
> include/linux/i2c-mux.h | 2 +-
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index 361b78d..7409ebb 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -191,14 +191,12 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
> }
> EXPORT_SYMBOL_GPL(i2c_add_mux_adapter);
>
> -int i2c_del_mux_adapter(struct i2c_adapter *adap)
> +void i2c_del_mux_adapter(struct i2c_adapter *adap)
> {
> struct i2c_mux_priv *priv = adap->algo_data;
>
> i2c_del_adapter(adap);
> kfree(priv);
> -
> - return 0;
> }
> EXPORT_SYMBOL_GPL(i2c_del_mux_adapter);
>
> diff --git a/include/linux/i2c-mux.h b/include/linux/i2c-mux.h
> index 40cb05a..b5f9a00 100644
> --- a/include/linux/i2c-mux.h
> +++ b/include/linux/i2c-mux.h
> @@ -42,7 +42,7 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
> int (*deselect) (struct i2c_adapter *,
> void *mux_dev, u32 chan_id));
>
> -int i2c_del_mux_adapter(struct i2c_adapter *adap);
> +void i2c_del_mux_adapter(struct i2c_adapter *adap);
>
> #endif /* __KERNEL__ */
>
Reviewed-by: Jean Delvare <[email protected]>
--
Jean Delvare
On Sat, 9 Mar 2013 19:16:43 +0100, Lars-Peter Clausen wrote:
> Lars-Peter Clausen (6):
> i2c: Remove detach_adapter
> i2c: i2c_del_adapter: Don't treat removing a non-registered adapter
> as error
> i2c: Ignore return value of i2c_del_adapter()
> i2c: Make return type of i2c_del_adapter() void
> i2c: Ignore the return value of i2c_del_mux_adapter()
> i2c: Make the return type of i2c_del_mux_adapter() void
FWIW I tested the whole series successfully on my Asus Z8NA-D6 board.
--
Jean Delvare
On Sat, Mar 09, 2013 at 07:16:46PM +0100, Lars-Peter Clausen wrote:
> i2c_del_adapter() always returns 0. So all checks testing whether it will be
> non zero will always evaluate to false and the conditional code is dead code.
> This patch updates all callers of i2c_del_mux_adapter() to ignore the return
> value and assume that it will always succeed (which it will). In a subsequent
> patch the return type of i2c_del_adapter() will be made void.
>
...
> Cc: Shawn Guo <[email protected]>
...
> drivers/i2c/busses/i2c-mxs.c | 5 +----
Acked-by: Shawn Guo <[email protected]>
On Sat, Mar 09, 2013 at 07:16:43PM +0100, Lars-Peter Clausen wrote:
> Currently i2c_del_adapter() returns 0 on success and potentially an error code
> on failure. Unfortunately this doesn't mix too well with the Linux device driver
> model. An i2c_adapter is usually registered in a drivers probe callback and
> removed in the drivers remove callback. The Linux device driver model assumes
> that a device may disappear at any time, e.g. because it is on a hot-plug-able
> bus and the device is physically removed or because it is unbound from it's
> driver. This means that a driver's remove callback is not allowed to fail. This
> has lead to code fragments like the following:
>
> rc = i2c_del_adapter(&board->i2c_adap);
> BUG_ON(rc);
>
> Currently there are two code paths which can cause to i2c_del_adapter() to
> return an error code:
> 1) If the adapter that is passed to i2c_del_adapter() is not registered
> i2c_del_adapter() returns -EINVAL
> 2) If an I2C client, which is registered as a child to the adapter, implements
> the detach_adapter callback and detach_adapter returns an error the removal of
> the adapter is aborted and i2c_del_adapter() returns the error returned by the
> clients detach_adapter callback.
>
> The first probably never happens unless there is a serious bug in the driver
> calling i2c_del_adapter(), but I think even then, for the sake of sanitizing the
> API we can argue that the purpose of i2c_del_adapter() is to remove the adapter
> from the system. If the adapter currently isn't registered this becomes a no-op
> and we can return success, without having to do anything. The second also never
> happens, since the detach_adapter callback has been deprecated for quite some
> time now and there are currently no drivers left which implement it.
>
> So realisticly i2c_del_adapter() already always returns 0 and all code checking
> for errors is more or less dead code. And in fact the majority of the users of
> i2c_del_adapter() already ignore the return value, but there are still some
> drivers (both newer and older) which assume that i2c_del_adapter() might fail.
> This series aims at making it explicit that i2c_del_adapter() can not fail by
> making its return type void.
>
> The series will start by making i2c_del_adapter() always return success. For
> this it will update the case, where a non-registered adapter is passed in, to
> return 0 instead of -EINVAL and remove all code related to the unused
> detach_adapter callback. Afterward the series will update all users of
> i2c_del_adapter() to ignore the return value (since it is always 0 now). And
> finally the series will change the return type of i2c_del_adapter() to void.
>
> The same is true for i2c_del_mux_adapter() which is mostly just a wrapper
> around i2c_del_adapter(), so it is also updated in this series.
Thanks for doing this and thanks to Jean for the review. I pretty much
came to the same questions and conclusions.
Applied to for-next, thanks!