[alsa-devel] [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 at alum.mit.edu>
> > > Signed-off-by: Jean Delvare <khali at linux-fr.org>
> > > Cc: Johannes Berg <johannes at 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.