Re: [PATCH 0/6] Make return type of i2c_del_adapter() (and i2c_del_mux_adapter()) void
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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] Make return type of i2c_del_adapter() (and i2c_del_mux_adapter()) void
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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] i2c: i2c_del_adapter: Don't treat removing a non-registered adapter as error
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
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
> ---
> 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
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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] i2c: Remove detach_adapter
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 -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] i2c: Ignore return value of i2c_del_adapter()
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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] i2c: Ignore return value of i2c_del_adapter()
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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] i2c: Ignore return value of i2c_del_adapter()
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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
