[PATCH] ASoC: Fix cs4270 error path

[PATCH] ASoC: Fix cs4270 error path

[Jean Delvare]

The error path in cs4270_probe/cs4270_remove is pretty broken:
* If cs4270_probe fails, codec is leaked.
* If snd_soc_register_card fails, cs4270_i2c_driver stays registered.
* If I2C support is enabled but no I2C device is found, i2c_del_driver
  is never called (neither in cs4270_probe nor in cs4270_remove.)

Fix the first 2 problems by implementing a clean error path in
cs4270_probe and jumping to its labels as needed. Fix the 3rd problem
by removing the condition to call i2c_del_driver in cs4270_remove.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
Timur, can you please review and test this patch? I do not have the
hardware so I couldn't test it myself.

 sound/soc/codecs/cs4270.c |   25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

--- linux-2.6.27-rc5.orig/sound/soc/codecs/cs4270.c	2008-08-31 13:59:17.000000000 +0200
+++ linux-2.6.27-rc5/sound/soc/codecs/cs4270.c	2008-08-31 14:23:34.000000000 +0200
@@ -727,7 +727,7 @@ static int cs4270_probe(struct platform_
 	ret = snd_soc_new_pcms(socdev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1);
 	if (ret < 0) {
 		printk(KERN_ERR "cs4270: failed to create PCMs\n");
-		return ret;
+		goto error_free_codec;
 	}
 
 #ifdef USE_I2C
@@ -736,8 +736,7 @@ static int cs4270_probe(struct platform_
 	ret = i2c_add_driver(&cs4270_i2c_driver);
 	if (ret) {
 		printk(KERN_ERR "cs4270: failed to attach driver");
-		snd_soc_free_pcms(socdev);
-		return ret;
+		goto error_free_pcms;
 	}
 
 	/* Did we find a CS4270 on the I2C bus? */
@@ -759,10 +758,23 @@ static int cs4270_probe(struct platform_
 	ret = snd_soc_register_card(socdev);
 	if (ret < 0) {
 		printk(KERN_ERR "cs4270: failed to register card\n");
-		snd_soc_free_pcms(socdev);
-		return ret;
+		goto error_del_driver;
 	}
 
+	return 0;
+
+error_del_driver:
+#ifdef USE_I2C
+	i2c_del_driver(&cs4270_i2c_driver);
+
+error_free_pcms:
+#endif
+	snd_soc_free_pcms(socdev);
+
+error_free_codec:
+	kfree(socdev->codec);
+	socdev->codec = NULL;
+
 	return ret;
 }
 
@@ -773,8 +785,7 @@ static int cs4270_remove(struct platform
 	snd_soc_free_pcms(socdev);
 
 #ifdef USE_I2C
-	if (socdev->codec->control_data)
-		i2c_del_driver(&cs4270_i2c_driver);
+	i2c_del_driver(&cs4270_i2c_driver);
 #endif
 
 	kfree(socdev->codec);


-- 
Jean Delvare

Re: [PATCH] ASoC: Fix cs4270 error path

[Jean Delvare]

Timur,

Any news about this fix? This should go in kernel 2.6.28.

On Sun, 31 Aug 2008 14:42:14 +0200, Jean Delvare wrote:
> The error path in cs4270_probe/cs4270_remove is pretty broken:
> * If cs4270_probe fails, codec is leaked.
> * If snd_soc_register_card fails, cs4270_i2c_driver stays registered.
> * If I2C support is enabled but no I2C device is found, i2c_del_driver
>   is never called (neither in cs4270_probe nor in cs4270_remove.)
> 
> Fix the first 2 problems by implementing a clean error path in
> cs4270_probe and jumping to its labels as needed. Fix the 3rd problem
> by removing the condition to call i2c_del_driver in cs4270_remove.
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> ---
> Timur, can you please review and test this patch? I do not have the
> hardware so I couldn't test it myself.
> 
>  sound/soc/codecs/cs4270.c |   25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> --- linux-2.6.27-rc5.orig/sound/soc/codecs/cs4270.c	2008-08-31 13:59:17.000000000 +0200
> +++ linux-2.6.27-rc5/sound/soc/codecs/cs4270.c	2008-08-31 14:23:34.000000000 +0200
> @@ -727,7 +727,7 @@ static int cs4270_probe(struct platform_
>  	ret = snd_soc_new_pcms(socdev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1);
>  	if (ret < 0) {
>  		printk(KERN_ERR "cs4270: failed to create PCMs\n");
> -		return ret;
> +		goto error_free_codec;
>  	}
>  
>  #ifdef USE_I2C
> @@ -736,8 +736,7 @@ static int cs4270_probe(struct platform_
>  	ret = i2c_add_driver(&cs4270_i2c_driver);
>  	if (ret) {
>  		printk(KERN_ERR "cs4270: failed to attach driver");
> -		snd_soc_free_pcms(socdev);
> -		return ret;
> +		goto error_free_pcms;
>  	}
>  
>  	/* Did we find a CS4270 on the I2C bus? */
> @@ -759,10 +758,23 @@ static int cs4270_probe(struct platform_
>  	ret = snd_soc_register_card(socdev);
>  	if (ret < 0) {
>  		printk(KERN_ERR "cs4270: failed to register card\n");
> -		snd_soc_free_pcms(socdev);
> -		return ret;
> +		goto error_del_driver;
>  	}
>  
> +	return 0;
> +
> +error_del_driver:
> +#ifdef USE_I2C
> +	i2c_del_driver(&cs4270_i2c_driver);
> +
> +error_free_pcms:
> +#endif
> +	snd_soc_free_pcms(socdev);
> +
> +error_free_codec:
> +	kfree(socdev->codec);
> +	socdev->codec = NULL;
> +
>  	return ret;
>  }
>  
> @@ -773,8 +785,7 @@ static int cs4270_remove(struct platform
>  	snd_soc_free_pcms(socdev);
>  
>  #ifdef USE_I2C
> -	if (socdev->codec->control_data)
> -		i2c_del_driver(&cs4270_i2c_driver);
> +	i2c_del_driver(&cs4270_i2c_driver);
>  #endif
>  
>  	kfree(socdev->codec);
> 
> 


-- 
Jean Delvare

Re: [PATCH] ASoC: Fix cs4270 error path

[Timur Tabi]

Sorry I didn't get to this earlier.  I just fell off my radar.

On Sun, Aug 31, 2008 at 7:42 AM, Jean Delvare <khali@linux-fr.org> wrote:
> The error path in cs4270_probe/cs4270_remove is pretty broken:
> * If cs4270_probe fails, codec is leaked.
> * If snd_soc_register_card fails, cs4270_i2c_driver stays registered.

So far, so good.

> * If I2C support is enabled but no I2C device is found, i2c_del_driver
>  is never called (neither in cs4270_probe nor in cs4270_remove.)

Hmm... The only time that can happen is if the device tree is wrong or
the hardware is broken.  This means that cs4270_i2c_probe() will
return an error.  What does i2c_add_driver() return in that case?  If
it still returns 0, then control_data will be NULL, but with your
patch, i2c_del_driver() will still be called.

Also, I think there's a bug in cs4270_i2c_probe(), where it will call
i2c_detach_driver() if it fails.  It shouldn't do that.  This is also
gated on codec->control_data, so it's a related bug.

Lastly, you may need to rebase the patch, since the code's changed.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ASoC: Fix cs4270 error path

[Jean Delvare]

Hi Timur,

On Mon, 22 Sep 2008 15:35:17 -0500, Timur Tabi wrote:
> Sorry I didn't get to this earlier.  I just fell off my radar.
> 
> On Sun, Aug 31, 2008 at 7:42 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > The error path in cs4270_probe/cs4270_remove is pretty broken:
> > * If cs4270_probe fails, codec is leaked.
> > * If snd_soc_register_card fails, cs4270_i2c_driver stays registered.
> 
> So far, so good.
> 
> > * If I2C support is enabled but no I2C device is found, i2c_del_driver
> >  is never called (neither in cs4270_probe nor in cs4270_remove.)
> 
> Hmm... The only time that can happen is if the device tree is wrong or
> the hardware is broken.

The whole point of error paths is to handle cases that were not
supposed to happen.

> (...)  This means that cs4270_i2c_probe() will
> return an error.  What does i2c_add_driver() return in that case?

As per the device driver model, the success or failure of device probes
has zero influence on drivers. So, yes, i2c_add_driver() still succeeds
and returns 0.

> (...)  If
> it still returns 0, then control_data will be NULL, but with your
> patch, i2c_del_driver() will still be called.

Of course, it will be. It has to. This is exactly the bug I am fixing!
If i2c_add_driver() succeeds then i2c_del_driver() must be called in the
cleanup path. It's really that easy. Whether an I2C device was found or
not, must not influence that.

> 
> Also, I think there's a bug in cs4270_i2c_probe(), where it will call
> i2c_detach_driver() if it fails.  It shouldn't do that.  This is also
> gated on codec->control_data, so it's a related bug.

You are right, this is a bug. Apparently you forgot the error path when
converting the driver to a new-style i2c driver. A few lines below,
there's also:
	kfree(i2c_client);
which should be removed.

I disagree that this is related with my initial patch though. It's a
different error path, and it's also broken, but that's hardly enough to
make both bugs "related". So I'll send a separate patch.

> 
> Lastly, you may need to rebase the patch, since the code's changed.

My patch still applies fine (with offset, but that's OK), so I fail to
see why I would need to rebase it.

-- 
Jean Delvare

Re: [PATCH] ASoC: Fix cs4270 error path

[Timur Tabi]

On Sun, Aug 31, 2008 at 7:42 AM, Jean Delvare <khali@linux-fr.org> wrote:
> The error path in cs4270_probe/cs4270_remove is pretty broken:
> * If cs4270_probe fails, codec is leaked.
> * If snd_soc_register_card fails, cs4270_i2c_driver stays registered.
> * If I2C support is enabled but no I2C device is found, i2c_del_driver
>  is never called (neither in cs4270_probe nor in cs4270_remove.)
>
> Fix the first 2 problems by implementing a clean error path in
> cs4270_probe and jumping to its labels as needed. Fix the 3rd problem
> by removing the condition to call i2c_del_driver in cs4270_remove.
>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>

Acked-By: Timur Tabi <timur@freescale.com>

Takashi, this patch needs to go into 2.6.27 as well.  Sorry about
that.  I don't know how I missed so many problems with my code.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ASoC: Fix cs4270 error path

[Takashi Iwai]

At Mon, 29 Sep 2008 08:42:15 -0500,
Timur Tabi wrote:
> 
> On Sun, Aug 31, 2008 at 7:42 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > The error path in cs4270_probe/cs4270_remove is pretty broken:
> > * If cs4270_probe fails, codec is leaked.
> > * If snd_soc_register_card fails, cs4270_i2c_driver stays registered.
> > * If I2C support is enabled but no I2C device is found, i2c_del_driver
> >  is never called (neither in cs4270_probe nor in cs4270_remove.)
> >
> > Fix the first 2 problems by implementing a clean error path in
> > cs4270_probe and jumping to its labels as needed. Fix the 3rd problem
> > by removing the condition to call i2c_del_driver in cs4270_remove.
> >
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> 
> Acked-By: Timur Tabi <timur@freescale.com>
> 
> Takashi, this patch needs to go into 2.6.27 as well.  Sorry about
> that.  I don't know how I missed so many problems with my code.

Don't worry, I already put it to the queue, too.


Takashi

Re: [PATCH] ASoC: Fix cs4270 error path

[Jean Delvare]

Hi Takashi,

On Mon, 29 Sep 2008 15:48:03 +0200, Takashi Iwai wrote:
> At Mon, 29 Sep 2008 08:42:15 -0500,
> Timur Tabi wrote:
> > 
> > On Sun, Aug 31, 2008 at 7:42 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > > The error path in cs4270_probe/cs4270_remove is pretty broken:
> > > * If cs4270_probe fails, codec is leaked.
> > > * If snd_soc_register_card fails, cs4270_i2c_driver stays registered.
> > > * If I2C support is enabled but no I2C device is found, i2c_del_driver
> > >  is never called (neither in cs4270_probe nor in cs4270_remove.)
> > >
> > > Fix the first 2 problems by implementing a clean error path in
> > > cs4270_probe and jumping to its labels as needed. Fix the 3rd problem
> > > by removing the condition to call i2c_del_driver in cs4270_remove.
> > >
> > > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > 
> > Acked-By: Timur Tabi <timur@freescale.com>
> > 
> > Takashi, this patch needs to go into 2.6.27 as well.  Sorry about
> > that.  I don't know how I missed so many problems with my code.
> 
> Don't worry, I already put it to the queue, too.

I fear there's some confusion there. There are two different patches
fixing error paths in cs4270. One fixing a fallout from the new-style
i2c driver conversion (in cs4270_i2c_probe), under name "ASoC: Fix
another cs4270 error path". This one you pushed to Linus last night.

But there's another one, named "ASoC: Fix cs4270 error path",
originally posted by myself on August 31st, fixing the error path of
cs4270_probe. This is the one Timur was just acking, but I do _not_ see
it in your queue, so I suspect you missed it. I can resend it if it
helps.

Thanks,
-- 
Jean Delvare

Re: [PATCH] ASoC: Fix cs4270 error path

[Takashi Iwai]

At Tue, 30 Sep 2008 10:31:37 +0200,
Jean Delvare wrote:
> 
> Hi Takashi,
> 
> On Mon, 29 Sep 2008 15:48:03 +0200, Takashi Iwai wrote:
> > At Mon, 29 Sep 2008 08:42:15 -0500,
> > Timur Tabi wrote:
> > > 
> > > On Sun, Aug 31, 2008 at 7:42 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > > > The error path in cs4270_probe/cs4270_remove is pretty broken:
> > > > * If cs4270_probe fails, codec is leaked.
> > > > * If snd_soc_register_card fails, cs4270_i2c_driver stays registered.
> > > > * If I2C support is enabled but no I2C device is found, i2c_del_driver
> > > >  is never called (neither in cs4270_probe nor in cs4270_remove.)
> > > >
> > > > Fix the first 2 problems by implementing a clean error path in
> > > > cs4270_probe and jumping to its labels as needed. Fix the 3rd problem
> > > > by removing the condition to call i2c_del_driver in cs4270_remove.
> > > >
> > > > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > > 
> > > Acked-By: Timur Tabi <timur@freescale.com>
> > > 
> > > Takashi, this patch needs to go into 2.6.27 as well.  Sorry about
> > > that.  I don't know how I missed so many problems with my code.
> > 
> > Don't worry, I already put it to the queue, too.
> 
> I fear there's some confusion there. There are two different patches
> fixing error paths in cs4270. One fixing a fallout from the new-style
> i2c driver conversion (in cs4270_i2c_probe), under name "ASoC: Fix
> another cs4270 error path". This one you pushed to Linus last night.
> 
> But there's another one, named "ASoC: Fix cs4270 error path",
> originally posted by myself on August 31st, fixing the error path of
> cs4270_probe. This is the one Timur was just acking, but I do _not_ see
> it in your queue, so I suspect you missed it. I can resend it if it
> helps.

Oh, OK, then I must have missed that.  Could you repost?
And, this *must* go to 2.6.27, or not?

Frankly, this series of cs4270 patches have been hard to handle
because it was always unclear what the patch is for.
The description "It's for 2.6.x" is too ambiguous because it doesn't
always mean the purpose but also can mean the based version of the
patch.  So, a more clear sign would be really helpful for me at the
next time...


thanks,

Takashi

Re: [PATCH] ASoC: Fix cs4270 error path

[Jean Delvare]

Hi Takashi,

On Tue, 30 Sep 2008 10:53:20 +0200, Takashi Iwai wrote:
> At Tue, 30 Sep 2008 10:31:37 +0200,
> Jean Delvare wrote:
> > I fear there's some confusion there. There are two different patches
> > fixing error paths in cs4270. One fixing a fallout from the new-style
> > i2c driver conversion (in cs4270_i2c_probe), under name "ASoC: Fix
> > another cs4270 error path". This one you pushed to Linus last night.
> > 
> > But there's another one, named "ASoC: Fix cs4270 error path",
> > originally posted by myself on August 31st, fixing the error path of
> > cs4270_probe. This is the one Timur was just acking, but I do _not_ see
> > it in your queue, so I suspect you missed it. I can resend it if it
> > helps.
> 
> Oh, OK, then I must have missed that.  Could you repost?

Will do in a minute.

> And, this *must* go to 2.6.27, or not?

It could go in 2.6.27, certainly, but I wouldn't say it *must* go
there. The patch is "only" fixing an error path, which by definition
isn't supposed to be needed unless something unexpected happens, in
which case the driver probably won't work anyway. So it doesn't deserve
delaying 2.6.27, and sending a pull request to Linus for just that
patch would probably be overkill. But if you get the opportunity to
send such a pull request for another problem, then it makes sense to
include this cs4270 fix as well.

Anyway it's really up to you and Timur. I just happened to notice the
bug and posted a fix, but I'm not even using the driver myself.

> Frankly, this series of cs4270 patches have been hard to handle
> because it was always unclear what the patch is for.
> The description "It's for 2.6.x" is too ambiguous because it doesn't
> always mean the purpose but also can mean the based version of the
> patch.  So, a more clear sign would be really helpful for me at the
> next time...

OK, sorry about that. I'll try to make things clearer next time.

-- 
Jean Delvare

[PATCH] ASoC: Fix cs4270 error path

[Jean Delvare]

The error path in cs4270_probe/cs4270_remove is pretty broken:
* If cs4270_probe fails, codec is leaked.
* If snd_soc_register_card fails, cs4270_i2c_driver stays registered.
* If I2C support is enabled but no I2C device is found, i2c_del_driver
  is never called (neither in cs4270_probe nor in cs4270_remove.

Fix all 3 problems by implementing a clean error path in cs4270_probe
and jumping to its labels as needed.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Acked-by: Timur Tabi <timur@freescale.com>
---
 sound/soc/codecs/cs4270.c |   25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

--- linux-2.6.27-rc8.orig/sound/soc/codecs/cs4270.c	2008-09-30 11:11:22.000000000 +0200
+++ linux-2.6.27-rc8/sound/soc/codecs/cs4270.c	2008-09-30 11:33:03.000000000 +0200
@@ -681,7 +681,7 @@ static int cs4270_probe(struct platform_
 	ret = snd_soc_new_pcms(socdev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1);
 	if (ret < 0) {
 		printk(KERN_ERR "cs4270: failed to create PCMs\n");
-		return ret;
+		goto error_free_codec;
 	}
 
 #ifdef USE_I2C
@@ -690,8 +690,7 @@ static int cs4270_probe(struct platform_
 	ret = i2c_add_driver(&cs4270_i2c_driver);
 	if (ret) {
 		printk(KERN_ERR "cs4270: failed to attach driver");
-		snd_soc_free_pcms(socdev);
-		return ret;
+		goto error_free_pcms;
 	}
 
 	/* Did we find a CS4270 on the I2C bus? */
@@ -713,10 +712,23 @@ static int cs4270_probe(struct platform_
 	ret = snd_soc_register_card(socdev);
 	if (ret < 0) {
 		printk(KERN_ERR "cs4270: failed to register card\n");
-		snd_soc_free_pcms(socdev);
-		return ret;
+		goto error_del_driver;
 	}
 
+	return 0;
+
+error_del_driver:
+#ifdef USE_I2C
+	i2c_del_driver(&cs4270_i2c_driver);
+
+error_free_pcms:
+#endif
+	snd_soc_free_pcms(socdev);
+
+error_free_codec:
+	kfree(socdev->codec);
+	socdev->codec = NULL;
+
 	return ret;
 }
 
@@ -727,8 +739,7 @@ static int cs4270_remove(struct platform
 	snd_soc_free_pcms(socdev);
 
 #ifdef USE_I2C
-	if (socdev->codec->control_data)
-		i2c_del_driver(&cs4270_i2c_driver);
+	i2c_del_driver(&cs4270_i2c_driver);
 #endif
 
 	kfree(socdev->codec);


-- 
Jean Delvare

Re: [PATCH] ASoC: Fix cs4270 error path

[Takashi Iwai]

At Tue, 30 Sep 2008 11:38:55 +0200,
Jean Delvare wrote:
> 
> Hi Takashi,
> 
> On Tue, 30 Sep 2008 10:53:20 +0200, Takashi Iwai wrote:
> > At Tue, 30 Sep 2008 10:31:37 +0200,
> > Jean Delvare wrote:
> > > I fear there's some confusion there. There are two different patches
> > > fixing error paths in cs4270. One fixing a fallout from the new-style
> > > i2c driver conversion (in cs4270_i2c_probe), under name "ASoC: Fix
> > > another cs4270 error path". This one you pushed to Linus last night.
> > > 
> > > But there's another one, named "ASoC: Fix cs4270 error path",
> > > originally posted by myself on August 31st, fixing the error path of
> > > cs4270_probe. This is the one Timur was just acking, but I do _not_ see
> > > it in your queue, so I suspect you missed it. I can resend it if it
> > > helps.
> > 
> > Oh, OK, then I must have missed that.  Could you repost?
> 
> Will do in a minute.
> 
> > And, this *must* go to 2.6.27, or not?
> 
> It could go in 2.6.27, certainly, but I wouldn't say it *must* go
> there. The patch is "only" fixing an error path, which by definition
> isn't supposed to be needed unless something unexpected happens, in
> which case the driver probably won't work anyway. So it doesn't deserve
> delaying 2.6.27, and sending a pull request to Linus for just that
> patch would probably be overkill. But if you get the opportunity to
> send such a pull request for another problem, then it makes sense to
> include this cs4270 fix as well.

There is one quirk fix for a Dell laptop, so I'm going to put it in
the next pull request before 2.6.27 final.


thanks,

Takashi

Re: [PATCH] ASoC: Fix cs4270 error path

[Timur Tabi]

Takashi Iwai wrote:

> Oh, OK, then I must have missed that.  Could you repost?
> And, this *must* go to 2.6.27, or not?

The only patch that needs to go into 2.6.27 is the one titled "alsa: make the
CS4270 driver a new-style I2C driver" from me.  This one is missing from Linus'
tree.

I notice that "ALSA: ASoC: Fix another cs4270 error path" is in Linus' tree, but
nothing else is.

> Frankly, this series of cs4270 patches have been hard to handle
> because it was always unclear what the patch is for.
> The description "It's for 2.6.x" is too ambiguous because it doesn't
> always mean the purpose but also can mean the based version of the
> patch.  So, a more clear sign would be really helpful for me at the
> next time...

I can do that, but I'm not sure how I can be any clearer.  "This is for 2.6.x",
to me at least, means exactly that - that this patch should be applied to the
branch for 2.6.27, which is either a release candidate (i.e. 2.6.27-rcX) or a
bug fix (i.e. 2.6.27.x), depending on what's next.

If you want me to use different wording, just tell me what I should say.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ASoC: Fix cs4270 error path

[Takashi Iwai]

At Tue, 30 Sep 2008 09:25:50 -0500,
Timur Tabi wrote:
> 
> Takashi Iwai wrote:
> 
> > Oh, OK, then I must have missed that.  Could you repost?
> > And, this *must* go to 2.6.27, or not?
> 
> The only patch that needs to go into 2.6.27 is the one titled "alsa: make the
> CS4270 driver a new-style I2C driver" from me.

So, do you mean that this patch (ASoC: Fix cs4270 error path) doesn't
have to go into 2.6.27?  Hell, there are still things unclear to
me...

>  This one is missing from Linus'
> tree.

It's already in 2.6.27-rc8:
ec2cd95f340fb07b905839ee219b3846ecf58396 ALSA: make the CS4270 driver a new-style I2C driver
    
> I notice that "ALSA: ASoC: Fix another cs4270 error path" is in Linus' tree, but
> nothing else is.
> 
> > Frankly, this series of cs4270 patches have been hard to handle
> > because it was always unclear what the patch is for.
> > The description "It's for 2.6.x" is too ambiguous because it doesn't
> > always mean the purpose but also can mean the based version of the
> > patch.  So, a more clear sign would be really helpful for me at the
> > next time...
> 
> I can do that, but I'm not sure how I can be any clearer.  "This is for 2.6.x",
> to me at least, means exactly that - that this patch should be applied to the
> branch for 2.6.27, which is either a release candidate (i.e. 2.6.27-rcX) or a
> bug fix (i.e. 2.6.27.x), depending on what's next.
> 
> If you want me to use different wording, just tell me what I should say.

Just suggest more clearly that your patch is to be merged as soon as
possible.  For example,  "apply this to next 2.6.27-rc8 pull request"
or "merge this to the upstream immediately", or so.

In short: "A is for B" is too passive and ambiguous.  Rather say
simply "Do X".


thanks,

Takashi

Re: [PATCH] ASoC: Fix cs4270 error path

[Timur Tabi]

Takashi Iwai wrote:

> So, do you mean that this patch (ASoC: Fix cs4270 error path) doesn't
> have to go into 2.6.27?  Hell, there are still things unclear to
> me...

I think "ASoC: Fix cs4270 error path" should go in, but Jean is correct - it
just fixes an error condition that will never happen in real life.  It's your
call if you want to push it up.

>>  This one is missing from Linus'
>> tree.
> 
> It's already in 2.6.27-rc8:
> ec2cd95f340fb07b905839ee219b3846ecf58396 ALSA: make the CS4270 driver a new-style I2C driver

Ah, I misread the search page.  I see it now.

> In short: "A is for B" is too passive and ambiguous.  Rather say
> simply "Do X".

Got it.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ASoC: Fix cs4270 error path

[Takashi Iwai]

At Tue, 30 Sep 2008 09:56:15 -0500,
Timur Tabi wrote:
> 
> Takashi Iwai wrote:
> 
> > So, do you mean that this patch (ASoC: Fix cs4270 error path) doesn't
> > have to go into 2.6.27?  Hell, there are still things unclear to
> > me...
> 
> I think "ASoC: Fix cs4270 error path" should go in, but Jean is correct - it
> just fixes an error condition that will never happen in real life.  It's your
> call if you want to push it up.

OK, thanks for checking.  Then this is now in my queue for the next
pull request with another patch, and will be likely into 2.6.27.


Takashi