At Sat, 11 Dec 2010 17:51:26 +0100,
Tejun Heo wrote:
>
> flush_scheduled_work() is deprecated and scheduled to be removed.
>
> * cancel[_delayed]_work() + flush_scheduled_work() ->
> cancel[_delayed]_work_sync().
>
> * wm8350, wm8753 and soc-core use custom code to cancel a delayed
> work, execute it immediately if it was pending and wait for its
> completion. This is equivalent to flush_delayed_work_sync(). Use
> it instead.
>
> Signed-off-by: Tejun Heo <[email protected]>
> Cc: Takashi Iwai <[email protected]>
> Cc: Jaroslav Kysela <[email protected]>
> ---
> This is part of a series to remove flush_scheduled_work() usage to
> prepare for deprecation of flush_scheduled_work(). Patches in this
> series are self contained and mostly straight-forward.
>
> Please feel free to take it into the appropriate tree, or just ack it.
> In the latter case, I'll merge the patch through the workqueue tree
> during the next merge window.
The most of parts look OK to me, but I'm not sure about ASoC changes,
e.g. below one:
> diff --git a/sound/soc/codecs/wm8350.c b/sound/soc/codecs/wm8350.c
> index 7611add..b3e9fac 100644
> --- a/sound/soc/codecs/wm8350.c
> +++ b/sound/soc/codecs/wm8350.c
> @@ -1626,7 +1626,6 @@ static int wm8350_codec_remove(struct snd_soc_codec *codec)
> {
> struct wm8350_data *priv = snd_soc_codec_get_drvdata(codec);
> struct wm8350 *wm8350 = dev_get_platdata(codec->dev);
> - int ret;
>
> wm8350_clear_bits(wm8350, WM8350_JACK_DETECT,
> WM8350_JDL_ENA | WM8350_JDR_ENA);
> @@ -1641,15 +1640,9 @@ static int wm8350_codec_remove(struct snd_soc_codec *codec)
> priv->hpr.jack = NULL;
> priv->mic.jack = NULL;
>
> - /* cancel any work waiting to be queued. */
> - ret = cancel_delayed_work(&codec->delayed_work);
> -
> /* if there was any work waiting then we run it now and
> * wait for its completion */
> - if (ret) {
> - schedule_delayed_work(&codec->delayed_work, 0);
> - flush_scheduled_work();
> - }
> + flush_delayed_work_sync(&codec->delayed_work);
I vaguely remember Liam introduced this kind of code by some reason.
Liam, isn't it better for cancel_delayed_work_sync(), or should it be
like the above?
thanks,
Takashi
Hello, Takashi.
On 12/12/2010 09:56 AM, Takashi Iwai wrote:
>> diff --git a/sound/soc/codecs/wm8350.c b/sound/soc/codecs/wm8350.c
>> index 7611add..b3e9fac 100644
>> --- a/sound/soc/codecs/wm8350.c
>> +++ b/sound/soc/codecs/wm8350.c
>> @@ -1626,7 +1626,6 @@ static int wm8350_codec_remove(struct snd_soc_codec *codec)
>> {
>> struct wm8350_data *priv = snd_soc_codec_get_drvdata(codec);
>> struct wm8350 *wm8350 = dev_get_platdata(codec->dev);
>> - int ret;
>>
>> wm8350_clear_bits(wm8350, WM8350_JACK_DETECT,
>> WM8350_JDL_ENA | WM8350_JDR_ENA);
>> @@ -1641,15 +1640,9 @@ static int wm8350_codec_remove(struct snd_soc_codec *codec)
>> priv->hpr.jack = NULL;
>> priv->mic.jack = NULL;
>>
>> - /* cancel any work waiting to be queued. */
>> - ret = cancel_delayed_work(&codec->delayed_work);
>> -
>> /* if there was any work waiting then we run it now and
>> * wait for its completion */
>> - if (ret) {
>> - schedule_delayed_work(&codec->delayed_work, 0);
>> - flush_scheduled_work();
>> - }
>> + flush_delayed_work_sync(&codec->delayed_work);
>
> I vaguely remember Liam introduced this kind of code by some reason.
>
> Liam, isn't it better for cancel_delayed_work_sync(), or should it be
> like the above?
flush_delayed_work_sync() semantics has been recently changed such
that if a delayed work is pending it's queued immediately and then
completion is waited. IOW, the behavior remains unchanged with the
above change.
Thanks.
--
tejun
At Sun, 12 Dec 2010 10:15:55 +0100,
Tejun Heo wrote:
>
> Hello, Takashi.
>
> On 12/12/2010 09:56 AM, Takashi Iwai wrote:
> >> diff --git a/sound/soc/codecs/wm8350.c b/sound/soc/codecs/wm8350.c
> >> index 7611add..b3e9fac 100644
> >> --- a/sound/soc/codecs/wm8350.c
> >> +++ b/sound/soc/codecs/wm8350.c
> >> @@ -1626,7 +1626,6 @@ static int wm8350_codec_remove(struct snd_soc_codec *codec)
> >> {
> >> struct wm8350_data *priv = snd_soc_codec_get_drvdata(codec);
> >> struct wm8350 *wm8350 = dev_get_platdata(codec->dev);
> >> - int ret;
> >>
> >> wm8350_clear_bits(wm8350, WM8350_JACK_DETECT,
> >> WM8350_JDL_ENA | WM8350_JDR_ENA);
> >> @@ -1641,15 +1640,9 @@ static int wm8350_codec_remove(struct snd_soc_codec *codec)
> >> priv->hpr.jack = NULL;
> >> priv->mic.jack = NULL;
> >>
> >> - /* cancel any work waiting to be queued. */
> >> - ret = cancel_delayed_work(&codec->delayed_work);
> >> -
> >> /* if there was any work waiting then we run it now and
> >> * wait for its completion */
> >> - if (ret) {
> >> - schedule_delayed_work(&codec->delayed_work, 0);
> >> - flush_scheduled_work();
> >> - }
> >> + flush_delayed_work_sync(&codec->delayed_work);
> >
> > I vaguely remember Liam introduced this kind of code by some reason.
> >
> > Liam, isn't it better for cancel_delayed_work_sync(), or should it be
> > like the above?
>
> flush_delayed_work_sync() semantics has been recently changed such
> that if a delayed work is pending it's queued immediately and then
> completion is waited. IOW, the behavior remains unchanged with the
> above change.
Yes, I noticed it while I was reviewing your patch. So it's pretty
correct.
Meanwhile, I wondered whether it's the really wanted behavior for
that particular code path, thus the previous question to Liam.
thanks,
Takashi
On Sun, Dec 12, 2010 at 01:38:36PM +0100, Takashi Iwai wrote:
> Meanwhile, I wondered whether it's the really wanted behavior for
> that particular code path, thus the previous question to Liam.
Yes, it's desired behaviour. That's what the old code was trying to do.
I've not seen the original patch, could someone resend it to me please
(or send a pointer)?
On 12/12/2010 01:40 PM, Mark Brown wrote:
> On Sun, Dec 12, 2010 at 01:38:36PM +0100, Takashi Iwai wrote:
>
>> Meanwhile, I wondered whether it's the really wanted behavior for
>> that particular code path, thus the previous question to Liam.
>
> Yes, it's desired behaviour. That's what the old code was trying to do.
> I've not seen the original patch, could someone resend it to me please
> (or send a pointer)?
Weirdly, lkml seemed to have eaten the original postings. I can't
find them in any archive. The following is the original patch.
http://git.kernel.org/?p=linux/kernel/git/tj/wq.git;a=commitdiff;h=01f8126c2e14245cf0ba11a25fb9a291710066e1;hp=cd591981a8d06d6aaca6d7ef2412d12a7adf47ed
Thanks.
--
tejun
On Sun, Dec 12, 2010 at 05:02:25PM +0100, Tejun Heo wrote:
> Weirdly, lkml seemed to have eaten the original postings. I can't
> find them in any archive. The following is the original patch.
Possibly too big or something?
> http://git.kernel.org/?p=linux/kernel/git/tj/wq.git;a=commitdiff;h=01f8126c2e14245cf0ba11a25fb9a291710066e1;hp=cd591981a8d06d6aaca6d7ef2412d12a7adf47ed
Acked-by: Mark Brown <[email protected]>
For future reference please note that sound/soc is maintained as a
subtree of ALSA - if you could send patches to that separately to myself
and Liam as well as Takashi and Jaroslav that'd make life a little
easier.
Hello,
On 12/12/2010 07:47 PM, Mark Brown wrote:
> On Sun, Dec 12, 2010 at 05:02:25PM +0100, Tejun Heo wrote:
>
>> Weirdly, lkml seemed to have eaten the original postings. I can't
>> find them in any archive. The following is the original patch.
>
> Possibly too big or something?
>
>> http://git.kernel.org/?p=linux/kernel/git/tj/wq.git;a=commitdiff;h=01f8126c2e14245cf0ba11a25fb9a291710066e1;hp=cd591981a8d06d6aaca6d7ef2412d12a7adf47ed
>
> Acked-by: Mark Brown <[email protected]>
>
> For future reference please note that sound/soc is maintained as a
> subtree of ALSA - if you could send patches to that separately to myself
> and Liam as well as Takashi and Jaroslav that'd make life a little
> easier.
I can resplit and resend if necessary. Takashi, do you wanna take the
patch as-is or resplit? Or shall I route it through wq tree?
Thanks.
--
tejun
On Sun, Dec 12, 2010 at 07:50:02PM +0100, Tejun Heo wrote:
> > For future reference please note that sound/soc is maintained as a
> > subtree of ALSA - if you could send patches to that separately to myself
> > and Liam as well as Takashi and Jaroslav that'd make life a little
> > easier.
> I can resplit and resend if necessary. Takashi, do you wanna take the
> patch as-is or resplit? Or shall I route it through wq tree?
No need, just merge it along with the rest of the patch however that's
going. I was mentioning this for future reference rather than for the
present patch.
Liam, will the TWL6040 updates Magi posted the other day also be
affected? I seem to remember they added the same pattern as WM835x has.
At Sun, 12 Dec 2010 12:40:41 +0000,
Mark Brown wrote:
>
> On Sun, Dec 12, 2010 at 01:38:36PM +0100, Takashi Iwai wrote:
>
> > Meanwhile, I wondered whether it's the really wanted behavior for
> > that particular code path, thus the previous question to Liam.
>
> Yes, it's desired behaviour. That's what the old code was trying to do.
OK, now I merged to sound git tree.
Also it's merged back to topic/asoc branch with a conflict fix.
Please pull appropriately.
(I still don't remember why it had to be flush_work_sync() instead
of cancel_work_sync() in the remove callback path for ASoC, though...
Both aren't so much different nowadays and should work fine in such a
case, though :)
thanks,
Takashi
On Mon, 2010-12-13 at 09:34 +0100, Takashi Iwai wrote:
> At Sun, 12 Dec 2010 12:40:41 +0000,
> Mark Brown wrote:
> >
> > On Sun, Dec 12, 2010 at 01:38:36PM +0100, Takashi Iwai wrote:
> >
> > > Meanwhile, I wondered whether it's the really wanted behavior for
> > > that particular code path, thus the previous question to Liam.
> >
> > Yes, it's desired behaviour. That's what the old code was trying to do.
>
> OK, now I merged to sound git tree.
> Also it's merged back to topic/asoc branch with a conflict fix.
> Please pull appropriately.
>
> (I still don't remember why it had to be flush_work_sync() instead
> of cancel_work_sync() in the remove callback path for ASoC, though...
> Both aren't so much different nowadays and should work fine in such a
> case, though :)
Fwiw, I can't remember either why it had to be flush_work_sync() here.
The initial soc-core stuff was 5 years ago and the reason is well and
truly forgotten ;-)
Thanks
Liam
--
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk
On Sun, 2010-12-12 at 19:00 +0000, Mark Brown wrote:
> On Sun, Dec 12, 2010 at 07:50:02PM +0100, Tejun Heo wrote:
>
> > > For future reference please note that sound/soc is maintained as a
> > > subtree of ALSA - if you could send patches to that separately to myself
> > > and Liam as well as Takashi and Jaroslav that'd make life a little
> > > easier.
>
> > I can resplit and resend if necessary. Takashi, do you wanna take the
> > patch as-is or resplit? Or shall I route it through wq tree?
>
> No need, just merge it along with the rest of the patch however that's
> going. I was mentioning this for future reference rather than for the
> present patch.
>
> Liam, will the TWL6040 updates Magi posted the other day also be
> affected? I seem to remember they added the same pattern as WM835x has.
Not sure myself.
Magi, could you check your patch queue for any use of
flush_scheduled_work() here ?
Thanks
Liam
--
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk
On Mon, Dec 13, 2010 at 09:34:37AM +0100, Takashi Iwai wrote:
> OK, now I merged to sound git tree.
> Also it's merged back to topic/asoc branch with a conflict fix.
> Please pull appropriately.
Now merged up into my tree also.
Hi Liam,
> -----Original Message-----
> From: Liam Girdwood [mailto:[email protected]]
> Sent: Monday, December 13, 2010 7:32 AM
> To: Mark Brown; Olaya, Margarita
> Cc: Tejun Heo; Takashi Iwai; [email protected];
> Jaroslav Kysela
> Subject: Re: [PATCH 09/30] sound: don't use flush_scheduled_work()
>
> On Sun, 2010-12-12 at 19:00 +0000, Mark Brown wrote:
> > On Sun, Dec 12, 2010 at 07:50:02PM +0100, Tejun Heo wrote:
> >
> > > > For future reference please note that sound/soc is
> maintained as a
> > > > subtree of ALSA - if you could send patches to that
> separately to myself
> > > > and Liam as well as Takashi and Jaroslav that'd make
> life a little
> > > > easier.
> >
> > > I can resplit and resend if necessary. Takashi, do you
> wanna take the
> > > patch as-is or resplit? Or shall I route it through wq tree?
> >
> > No need, just merge it along with the rest of the patch
> however that's
> > going. I was mentioning this for future reference rather
> than for the
> > present patch.
> >
> > Liam, will the TWL6040 updates Magi posted the other day also be
> > affected? I seem to remember they added the same pattern
> as WM835x has.
>
> Not sure myself.
>
> Magi, could you check your patch queue for any use of
> flush_scheduled_work() here ?
flush_scheduled_work() is not being used in TWL6040 patches.
Regards,
Margarita
>
> Thanks
>
> Liam
> --
> Freelance Developer, SlimLogic Ltd
> ASoC and Voltage Regulator Maintainer.
> http://www.slimlogic.co.uk
>
> ????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?