sound: Don't assume i2c device probing always succeeds

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(-)

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
>

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,

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

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.