From patchwork Wed Sep 30 15:15:49 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Takashi Iwai X-Patchwork-Id: 34606 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from bilbo.ozlabs.org (localhost [127.0.0.1]) by ozlabs.org (Postfix) with ESMTP id 81921B7CDD for ; Thu, 1 Oct 2009 01:16:02 +1000 (EST) Received: by ozlabs.org (Postfix) id 180CEB7BD0; Thu, 1 Oct 2009 01:15:55 +1000 (EST) Delivered-To: linuxppc-dev@ozlabs.org Received: from mx1.suse.de (cantor.suse.de [195.135.220.2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "mx1.suse.de", Issuer "CAcert Class 3 Root" (verified OK)) by ozlabs.org (Postfix) with ESMTPS id 62F36B7BC5 for ; Thu, 1 Oct 2009 01:15:54 +1000 (EST) Received: from relay1.suse.de (relay-ext.suse.de [195.135.221.8]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.suse.de (Postfix) with ESMTP id C9C6F8FEA2; Wed, 30 Sep 2009 17:15:49 +0200 (CEST) Date: Wed, 30 Sep 2009 17:15:49 +0200 Message-ID: From: Takashi Iwai To: Jean Delvare Subject: Re: [PATCH] sound: Don't assume i2c device probing always succeeds In-Reply-To: <20090930170006.76e145ca@hyperion.delvare> References: <20090930152542.3ef753ee@hyperion.delvare> <20090930170006.76e145ca@hyperion.delvare> User-Agent: Wanderlust/2.15.6 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.7 Emacs/23.1 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Cc: linuxppc-dev@ozlabs.org, Johannes Berg , Tim Shepard , alsa-devel@alsa-project.org, Jaroslav Kysela X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org Errors-To: linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org 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 > > > Signed-off-by: Jean Delvare > > > Cc: Johannes Berg > > > --- > > > 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.