[PATCH] sound: Don't assume i2c device probing always succeeds

[PATCH] sound: Don't assume i2c device probing always succeeds

[Jean Delvare]

If i2c device probing fails, then there is no driver to dereference
after calling i2c_new_device(). Stop assuming that probing will always
succeed, to avoid NULL pointer dereferences. We have an easier access
to the driver anyway.

Reported-by: Tim Shepard <shep@alum.mit.edu>
Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Johannes Berg <johannes@sipsolutions.net>
---
The code is similar to the one in therm_adt746x, for which Tim reported
a real-world oops, so it should be fixed ASAP.

 sound/aoa/codecs/onyx.c |    4 +++-
 sound/aoa/codecs/tas.c  |    4 +++-
 sound/ppc/keywest.c     |    4 +++-
 3 files changed, 9 insertions(+), 3 deletions(-)

--- linux-2.6.32-rc1.orig/sound/aoa/codecs/onyx.c	2009-09-30 15:13:12.000000000 +0200
+++ linux-2.6.32-rc1/sound/aoa/codecs/onyx.c	2009-09-30 15:13:58.000000000 +0200
@@ -996,6 +996,8 @@ static void onyx_exit_codec(struct aoa_c
 	onyx->codec.soundbus_dev->detach_codec(onyx->codec.soundbus_dev, onyx);
 }
 
+static struct i2c_driver onyx_driver;
+
 static int onyx_create(struct i2c_adapter *adapter,
 		       struct device_node *node,
 		       int addr)
@@ -1027,7 +1029,7 @@ static int onyx_create(struct i2c_adapte
 	 * Let i2c-core delete that device on driver removal.
 	 * This is safe because i2c-core holds the core_lock mutex for us.
 	 */
-	list_add_tail(&client->detected, &client->driver->clients);
+	list_add_tail(&client->detected, &onyx_driver.clients);
 	return 0;
 }
 
--- linux-2.6.32-rc1.orig/sound/aoa/codecs/tas.c	2009-09-30 15:13:12.000000000 +0200
+++ linux-2.6.32-rc1/sound/aoa/codecs/tas.c	2009-09-30 15:13:58.000000000 +0200
@@ -882,6 +882,8 @@ static void tas_exit_codec(struct aoa_co
 }
 
 
+static struct i2c_driver tas_driver;
+
 static int tas_create(struct i2c_adapter *adapter,
 		       struct device_node *node,
 		       int addr)
@@ -902,7 +904,7 @@ static int tas_create(struct i2c_adapter
 	 * Let i2c-core delete that device on driver removal.
 	 * This is safe because i2c-core holds the core_lock mutex for us.
 	 */
-	list_add_tail(&client->detected, &client->driver->clients);
+	list_add_tail(&client->detected, &tas_driver.clients);
 	return 0;
 }
 
--- linux-2.6.32-rc1.orig/sound/ppc/keywest.c	2009-09-30 15:13:12.000000000 +0200
+++ linux-2.6.32-rc1/sound/ppc/keywest.c	2009-09-30 15:13:58.000000000 +0200
@@ -40,6 +40,8 @@ static int keywest_probe(struct i2c_clie
 	return 0;
 }
 
+struct i2c_driver keywest_driver;
+
 /*
  * This is kind of a hack, best would be to turn powermac to fixed i2c
  * bus numbers and declare the sound device as part of platform
@@ -65,7 +67,7 @@ static int keywest_attach_adapter(struct
 	 * This is safe because i2c-core holds the core_lock mutex for us.
 	 */
 	list_add_tail(&keywest_ctx->client->detected,
-		      &keywest_ctx->client->driver->clients);
+		      &keywest_driver.clients);
 	return 0;
 }
 


-- 
Jean Delvare

Re: [PATCH] sound: Don't assume i2c device probing always succeeds

[Takashi Iwai]

At Wed, 30 Sep 2009 15:25:42 +0200,
Jean Delvare wrote:
> 
> If i2c device probing fails, then there is no driver to dereference
> after calling i2c_new_device(). Stop assuming that probing will always
> succeed, to avoid NULL pointer dereferences. We have an easier access
> to the driver anyway.
> 
> Reported-by: Tim Shepard <shep@alum.mit.edu>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: Johannes Berg <johannes@sipsolutions.net>
> ---
> The code is similar to the one in therm_adt746x, for which Tim reported
> a real-world oops, so it should be fixed ASAP.

Jean, thanks for the patch.

I'm just wondering whether the additional NULL check of client->driver
would be enough?  If yes, sound/aoa/onyx.c has it, at least, and we
can add the similar checks to the rest, too.


Takashi

> 
>  sound/aoa/codecs/onyx.c |    4 +++-
>  sound/aoa/codecs/tas.c  |    4 +++-
>  sound/ppc/keywest.c     |    4 +++-
>  3 files changed, 9 insertions(+), 3 deletions(-)
> 
> --- linux-2.6.32-rc1.orig/sound/aoa/codecs/onyx.c	2009-09-30 15:13:12.000000000 +0200
> +++ linux-2.6.32-rc1/sound/aoa/codecs/onyx.c	2009-09-30 15:13:58.000000000 +0200
> @@ -996,6 +996,8 @@ static void onyx_exit_codec(struct aoa_c
>  	onyx->codec.soundbus_dev->detach_codec(onyx->codec.soundbus_dev, onyx);
>  }
>  
> +static struct i2c_driver onyx_driver;
> +
>  static int onyx_create(struct i2c_adapter *adapter,
>  		       struct device_node *node,
>  		       int addr)
> @@ -1027,7 +1029,7 @@ static int onyx_create(struct i2c_adapte
>  	 * Let i2c-core delete that device on driver removal.
>  	 * This is safe because i2c-core holds the core_lock mutex for us.
>  	 */
> -	list_add_tail(&client->detected, &client->driver->clients);
> +	list_add_tail(&client->detected, &onyx_driver.clients);
>  	return 0;
>  }
>  
> --- linux-2.6.32-rc1.orig/sound/aoa/codecs/tas.c	2009-09-30 15:13:12.000000000 +0200
> +++ linux-2.6.32-rc1/sound/aoa/codecs/tas.c	2009-09-30 15:13:58.000000000 +0200
> @@ -882,6 +882,8 @@ static void tas_exit_codec(struct aoa_co
>  }
>  
>  
> +static struct i2c_driver tas_driver;
> +
>  static int tas_create(struct i2c_adapter *adapter,
>  		       struct device_node *node,
>  		       int addr)
> @@ -902,7 +904,7 @@ static int tas_create(struct i2c_adapter
>  	 * Let i2c-core delete that device on driver removal.
>  	 * This is safe because i2c-core holds the core_lock mutex for us.
>  	 */
> -	list_add_tail(&client->detected, &client->driver->clients);
> +	list_add_tail(&client->detected, &tas_driver.clients);
>  	return 0;
>  }
>  
> --- linux-2.6.32-rc1.orig/sound/ppc/keywest.c	2009-09-30 15:13:12.000000000 +0200
> +++ linux-2.6.32-rc1/sound/ppc/keywest.c	2009-09-30 15:13:58.000000000 +0200
> @@ -40,6 +40,8 @@ static int keywest_probe(struct i2c_clie
>  	return 0;
>  }
>  
> +struct i2c_driver keywest_driver;
> +
>  /*
>   * This is kind of a hack, best would be to turn powermac to fixed i2c
>   * bus numbers and declare the sound device as part of platform
> @@ -65,7 +67,7 @@ static int keywest_attach_adapter(struct
>  	 * This is safe because i2c-core holds the core_lock mutex for us.
>  	 */
>  	list_add_tail(&keywest_ctx->client->detected,
> -		      &keywest_ctx->client->driver->clients);
> +		      &keywest_driver.clients);
>  	return 0;
>  }
>  
> 
> 
> -- 
> Jean Delvare
> 

Re: [PATCH] sound: Don't assume i2c device probing always succeeds

[Jean Delvare]

Hi Takashi,

Thanks for the swift reply.

On Wed, 30 Sep 2009 16:13:49 +0200, Takashi Iwai wrote:
> At Wed, 30 Sep 2009 15:25:42 +0200,
> Jean Delvare wrote:
> > 
> > If i2c device probing fails, then there is no driver to dereference
> > after calling i2c_new_device(). Stop assuming that probing will always
> > succeed, to avoid NULL pointer dereferences. We have an easier access
> > to the driver anyway.
> > 
> > Reported-by: Tim Shepard <shep@alum.mit.edu>
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > Cc: Johannes Berg <johannes@sipsolutions.net>
> > ---
> > The code is similar to the one in therm_adt746x, for which Tim reported
> > a real-world oops, so it should be fixed ASAP.
> 
> Jean, thanks for the patch.
> 
> I'm just wondering whether the additional NULL check of client->driver
> would be enough?  If yes, sound/aoa/onyx.c has it, at least, and we
> can add the similar checks to the rest, too.

The NULL check of client->driver, if followed by a call to
i2c_unregister_device(), would indeed be enough. But unlike the onyx
driver which we know we sometimes load erroneously, the other drivers
should never fail. I am reluctant to add code to handle error cases
which are not supposed to happen, which is why I prefer my proposed
fix: it does not add code.

That being said, I will be happy with any solution that fixes the
problem, so if you prefer client->driver NULL checks, I can send a
patch doing this.

Thanks,
-- 
Jean Delvare

Re: [PATCH] sound: Don't assume i2c device probing always succeeds

[Johannes Berg]

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

On Wed, 2009-09-30 at 17:00 +0200, Jean Delvare wrote:

> The NULL check of client->driver, if followed by a call to
> i2c_unregister_device(), would indeed be enough. But unlike the onyx
> driver which we know we sometimes load erroneously, the other drivers
> should never fail.

All of these drivers can be loaded manually and then fail though, or am
I misunderstanding something?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] sound: Don't assume i2c device probing always succeeds

[Jean Delvare]

On Wed, 30 Sep 2009 17:05:11 +0200, Johannes Berg wrote:
> On Wed, 2009-09-30 at 17:00 +0200, Jean Delvare wrote:
> 
> > The NULL check of client->driver, if followed by a call to
> > i2c_unregister_device(), would indeed be enough. But unlike the onyx
> > driver which we know we sometimes load erroneously, the other drivers
> > should never fail.
> 
> All of these drivers can be loaded manually and then fail though, or am
> I misunderstanding something?

I don't think so. At least tas and keywest have checks before they
attempt to instantiate an i2c client, which I think are reliable. The
onyx case is different because apparently some machines have it but are
difficult to detect:

	/* if that didn't work, try desperate mode for older
	 * machines that have stuff missing from the device tree */

And then we have to attempt to instantiate i2c devices at a not
completely known address, and that may fail. I think this is the reason
why onyx has this extra client->driver NULL check and the other two
drivers do not.

I would really love if someone with good knowledge of the device tree
on mac would convert all these hacks to proper i2c device declarations.
All the infrastructure is available already, but I don't know enough
about open firmware and mac the device tree to do it myself.

-- 
Jean Delvare

Re: [PATCH] sound: Don't assume i2c device probing always succeeds

[Takashi Iwai]

At Wed, 30 Sep 2009 17:00:06 +0200,
Jean Delvare wrote:
> 
> Hi Takashi,
> 
> Thanks for the swift reply.
> 
> On Wed, 30 Sep 2009 16:13:49 +0200, Takashi Iwai wrote:
> > At Wed, 30 Sep 2009 15:25:42 +0200,
> > Jean Delvare wrote:
> > > 
> > > If i2c device probing fails, then there is no driver to dereference
> > > after calling i2c_new_device(). Stop assuming that probing will always
> > > succeed, to avoid NULL pointer dereferences. We have an easier access
> > > to the driver anyway.
> > > 
> > > Reported-by: Tim Shepard <shep@alum.mit.edu>
> > > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > > Cc: Johannes Berg <johannes@sipsolutions.net>
> > > ---
> > > The code is similar to the one in therm_adt746x, for which Tim reported
> > > a real-world oops, so it should be fixed ASAP.
> > 
> > Jean, thanks for the patch.
> > 
> > I'm just wondering whether the additional NULL check of client->driver
> > would be enough?  If yes, sound/aoa/onyx.c has it, at least, and we
> > can add the similar checks to the rest, too.
> 
> The NULL check of client->driver, if followed by a call to
> i2c_unregister_device(), would indeed be enough. But unlike the onyx
> driver which we know we sometimes load erroneously, the other drivers
> should never fail. I am reluctant to add code to handle error cases
> which are not supposed to happen, which is why I prefer my proposed
> fix: it does not add code.
> 
> That being said, I will be happy with any solution that fixes the
> problem, so if you prefer client->driver NULL checks, I can send a
> patch doing this.

Yes, indeed I prefer NULL check because the user can know the error
at the right place.  I share your concern about the code addition,
though :)

I already made a patch below, but it's totally untested.
It'd be helpful if someone can do review and build-test it.


thanks,

Takashi

---
diff --git a/sound/aoa/codecs/tas.c b/sound/aoa/codecs/tas.c
index f0ebc97..0f810c8 100644
--- a/sound/aoa/codecs/tas.c
+++ b/sound/aoa/codecs/tas.c
@@ -897,6 +897,10 @@ static int tas_create(struct i2c_adapter *adapter,
 	client = i2c_new_device(adapter, &info);
 	if (!client)
 		return -ENODEV;
+	if (!client->driver) {
+		i2c_unregister_device(client);
+		return -ENODEV;
+	}
 
 	/*
 	 * Let i2c-core delete that device on driver removal.
diff --git a/sound/ppc/keywest.c b/sound/ppc/keywest.c
index 835fa19..473c5a6 100644
--- a/sound/ppc/keywest.c
+++ b/sound/ppc/keywest.c
@@ -59,6 +59,13 @@ static int keywest_attach_adapter(struct i2c_adapter *adapter)
 	strlcpy(info.type, "keywest", I2C_NAME_SIZE);
 	info.addr = keywest_ctx->addr;
 	keywest_ctx->client = i2c_new_device(adapter, &info);
+	if (!keywest_ctx->client)
+		return -ENODEV;
+	if (!keywest_ctx->client->driver) {
+		i2c_unregister_device(keywest_ctx->client);
+		keywest_ctx->client = NULL;
+		return -ENODEV;
+	}
 	
 	/*
 	 * Let i2c-core delete that device on driver removal.

Re: [PATCH] sound: Don't assume i2c device probing always succeeds

[Jean Delvare]

On Wed, 30 Sep 2009 17:15:49 +0200, Takashi Iwai wrote:
> Yes, indeed I prefer NULL check because the user can know the error
> at the right place.  I share your concern about the code addition,
> though :)
> 
> I already made a patch below, but it's totally untested.
> It'd be helpful if someone can do review and build-test it.
> 
> 
> thanks,
> 
> Takashi
> 
> ---
> diff --git a/sound/aoa/codecs/tas.c b/sound/aoa/codecs/tas.c
> index f0ebc97..0f810c8 100644
> --- a/sound/aoa/codecs/tas.c
> +++ b/sound/aoa/codecs/tas.c
> @@ -897,6 +897,10 @@ static int tas_create(struct i2c_adapter *adapter,
>  	client = i2c_new_device(adapter, &info);
>  	if (!client)
>  		return -ENODEV;
> +	if (!client->driver) {
> +		i2c_unregister_device(client);
> +		return -ENODEV;
> +	}
>  
>  	/*
>  	 * Let i2c-core delete that device on driver removal.
> diff --git a/sound/ppc/keywest.c b/sound/ppc/keywest.c
> index 835fa19..473c5a6 100644
> --- a/sound/ppc/keywest.c
> +++ b/sound/ppc/keywest.c
> @@ -59,6 +59,13 @@ static int keywest_attach_adapter(struct i2c_adapter *adapter)
>  	strlcpy(info.type, "keywest", I2C_NAME_SIZE);
>  	info.addr = keywest_ctx->addr;
>  	keywest_ctx->client = i2c_new_device(adapter, &info);
> +	if (!keywest_ctx->client)
> +		return -ENODEV;
> +	if (!keywest_ctx->client->driver) {
> +		i2c_unregister_device(keywest_ctx->client);
> +		keywest_ctx->client = NULL;
> +		return -ENODEV;
> +	}
>  	
>  	/*
>  	 * Let i2c-core delete that device on driver removal.

This looks good to me. Please add the following comment before the
client->driver check in both drivers:

	/*
	 * We know the driver is already loaded, so the device should be
	 * already bound. If not it means binding failed, and then there
	 * is no point in keeping the device instantiated.
	 */

Otherwise it's a little difficult to understand why the check is there.

-- 
Jean Delvare

Re: [PATCH] sound: Don't assume i2c device probing always succeeds

[Takashi Iwai]

At Wed, 30 Sep 2009 18:55:05 +0200,
Jean Delvare wrote:
> 
> On Wed, 30 Sep 2009 17:15:49 +0200, Takashi Iwai wrote:
> > Yes, indeed I prefer NULL check because the user can know the error
> > at the right place.  I share your concern about the code addition,
> > though :)
> > 
> > I already made a patch below, but it's totally untested.
> > It'd be helpful if someone can do review and build-test it.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > ---
> > diff --git a/sound/aoa/codecs/tas.c b/sound/aoa/codecs/tas.c
> > index f0ebc97..0f810c8 100644
> > --- a/sound/aoa/codecs/tas.c
> > +++ b/sound/aoa/codecs/tas.c
> > @@ -897,6 +897,10 @@ static int tas_create(struct i2c_adapter *adapter,
> >  	client = i2c_new_device(adapter, &info);
> >  	if (!client)
> >  		return -ENODEV;
> > +	if (!client->driver) {
> > +		i2c_unregister_device(client);
> > +		return -ENODEV;
> > +	}
> >  
> >  	/*
> >  	 * Let i2c-core delete that device on driver removal.
> > diff --git a/sound/ppc/keywest.c b/sound/ppc/keywest.c
> > index 835fa19..473c5a6 100644
> > --- a/sound/ppc/keywest.c
> > +++ b/sound/ppc/keywest.c
> > @@ -59,6 +59,13 @@ static int keywest_attach_adapter(struct i2c_adapter *adapter)
> >  	strlcpy(info.type, "keywest", I2C_NAME_SIZE);
> >  	info.addr = keywest_ctx->addr;
> >  	keywest_ctx->client = i2c_new_device(adapter, &info);
> > +	if (!keywest_ctx->client)
> > +		return -ENODEV;
> > +	if (!keywest_ctx->client->driver) {
> > +		i2c_unregister_device(keywest_ctx->client);
> > +		keywest_ctx->client = NULL;
> > +		return -ENODEV;
> > +	}
> >  	
> >  	/*
> >  	 * Let i2c-core delete that device on driver removal.
> 
> This looks good to me. Please add the following comment before the
> client->driver check in both drivers:
> 
> 	/*
> 	 * We know the driver is already loaded, so the device should be
> 	 * already bound. If not it means binding failed, and then there
> 	 * is no point in keeping the device instantiated.
> 	 */
> 
> Otherwise it's a little difficult to understand why the check is there.

Fair enough.  I applied the patch with the comment now.
Thanks!


Takashi

Re: [PATCH] sound: Don't assume i2c device probing always succeeds

[Jean Delvare]

Hi Takashi,

On Thu, 01 Oct 2009 08:52:59 +0200, Takashi Iwai wrote:
> At Wed, 30 Sep 2009 18:55:05 +0200,
> Jean Delvare wrote:
> > 
> > On Wed, 30 Sep 2009 17:15:49 +0200, Takashi Iwai wrote:
> > > Yes, indeed I prefer NULL check because the user can know the error
> > > at the right place.  I share your concern about the code addition,
> > > though :)
> > > 
> > > I already made a patch below, but it's totally untested.
> > > It'd be helpful if someone can do review and build-test it.
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > > 
> > > ---
> > > diff --git a/sound/aoa/codecs/tas.c b/sound/aoa/codecs/tas.c
> > > index f0ebc97..0f810c8 100644
> > > --- a/sound/aoa/codecs/tas.c
> > > +++ b/sound/aoa/codecs/tas.c
> > > @@ -897,6 +897,10 @@ static int tas_create(struct i2c_adapter *adapter,
> > >  	client = i2c_new_device(adapter, &info);
> > >  	if (!client)
> > >  		return -ENODEV;
> > > +	if (!client->driver) {
> > > +		i2c_unregister_device(client);
> > > +		return -ENODEV;
> > > +	}
> > >  
> > >  	/*
> > >  	 * Let i2c-core delete that device on driver removal.
> > > diff --git a/sound/ppc/keywest.c b/sound/ppc/keywest.c
> > > index 835fa19..473c5a6 100644
> > > --- a/sound/ppc/keywest.c
> > > +++ b/sound/ppc/keywest.c
> > > @@ -59,6 +59,13 @@ static int keywest_attach_adapter(struct i2c_adapter *adapter)
> > >  	strlcpy(info.type, "keywest", I2C_NAME_SIZE);
> > >  	info.addr = keywest_ctx->addr;
> > >  	keywest_ctx->client = i2c_new_device(adapter, &info);
> > > +	if (!keywest_ctx->client)
> > > +		return -ENODEV;
> > > +	if (!keywest_ctx->client->driver) {
> > > +		i2c_unregister_device(keywest_ctx->client);
> > > +		keywest_ctx->client = NULL;
> > > +		return -ENODEV;
> > > +	}
> > >  	
> > >  	/*
> > >  	 * Let i2c-core delete that device on driver removal.
> > 
> > This looks good to me. Please add the following comment before the
> > client->driver check in both drivers:
> > 
> > 	/*
> > 	 * We know the driver is already loaded, so the device should be
> > 	 * already bound. If not it means binding failed, and then there
> > 	 * is no point in keeping the device instantiated.
> > 	 */
> > 
> > Otherwise it's a little difficult to understand why the check is there.
> 
> Fair enough.  I applied the patch with the comment now.
> Thanks!

I see this is upstream now. While the keywest fix was essentially
theoretical, the tas one addresses a crash which really could happen,
so I think it would be worth sending to stable for 2.6.31. What do you
think? Will you take care, or do you want me to?

Thanks,
-- 
Jean Delvare

Re: [PATCH] sound: Don't assume i2c device probing always succeeds

[Takashi Iwai]

At Sun, 4 Oct 2009 11:35:21 +0200,
Jean Delvare wrote:
> 
> Hi Takashi,
> 
> On Thu, 01 Oct 2009 08:52:59 +0200, Takashi Iwai wrote:
> > At Wed, 30 Sep 2009 18:55:05 +0200,
> > Jean Delvare wrote:
> > > 
> > > On Wed, 30 Sep 2009 17:15:49 +0200, Takashi Iwai wrote:
> > > > Yes, indeed I prefer NULL check because the user can know the error
> > > > at the right place.  I share your concern about the code addition,
> > > > though :)
> > > > 
> > > > I already made a patch below, but it's totally untested.
> > > > It'd be helpful if someone can do review and build-test it.
> > > > 
> > > > 
> > > > thanks,
> > > > 
> > > > Takashi
> > > > 
> > > > ---
> > > > diff --git a/sound/aoa/codecs/tas.c b/sound/aoa/codecs/tas.c
> > > > index f0ebc97..0f810c8 100644
> > > > --- a/sound/aoa/codecs/tas.c
> > > > +++ b/sound/aoa/codecs/tas.c
> > > > @@ -897,6 +897,10 @@ static int tas_create(struct i2c_adapter *adapter,
> > > >  	client = i2c_new_device(adapter, &info);
> > > >  	if (!client)
> > > >  		return -ENODEV;
> > > > +	if (!client->driver) {
> > > > +		i2c_unregister_device(client);
> > > > +		return -ENODEV;
> > > > +	}
> > > >  
> > > >  	/*
> > > >  	 * Let i2c-core delete that device on driver removal.
> > > > diff --git a/sound/ppc/keywest.c b/sound/ppc/keywest.c
> > > > index 835fa19..473c5a6 100644
> > > > --- a/sound/ppc/keywest.c
> > > > +++ b/sound/ppc/keywest.c
> > > > @@ -59,6 +59,13 @@ static int keywest_attach_adapter(struct i2c_adapter *adapter)
> > > >  	strlcpy(info.type, "keywest", I2C_NAME_SIZE);
> > > >  	info.addr = keywest_ctx->addr;
> > > >  	keywest_ctx->client = i2c_new_device(adapter, &info);
> > > > +	if (!keywest_ctx->client)
> > > > +		return -ENODEV;
> > > > +	if (!keywest_ctx->client->driver) {
> > > > +		i2c_unregister_device(keywest_ctx->client);
> > > > +		keywest_ctx->client = NULL;
> > > > +		return -ENODEV;
> > > > +	}
> > > >  	
> > > >  	/*
> > > >  	 * Let i2c-core delete that device on driver removal.
> > > 
> > > This looks good to me. Please add the following comment before the
> > > client->driver check in both drivers:
> > > 
> > > 	/*
> > > 	 * We know the driver is already loaded, so the device should be
> > > 	 * already bound. If not it means binding failed, and then there
> > > 	 * is no point in keeping the device instantiated.
> > > 	 */
> > > 
> > > Otherwise it's a little difficult to understand why the check is there.
> > 
> > Fair enough.  I applied the patch with the comment now.
> > Thanks!
> 
> I see this is upstream now. While the keywest fix was essentially
> theoretical, the tas one addresses a crash which really could happen,
> so I think it would be worth sending to stable for 2.6.31. What do you
> think? Will you take care, or do you want me to?

Agreed, it's safer to send the patch to stable tree.
I'm going to submit it.


thanks,

Takashi