[alsa-devel] ESI Juli@ crash with external clock switch - patch

[Takashi Iwai]

At Thu, 15 Jan 2015 22:15:46 +0100,
Pavel Hofman wrote:
> 
> Dne 12.1.2015 v 16:43 Takashi Iwai napsal(a):
> > At Mon, 12 Jan 2015 09:34:52 +0100,
> > Pavel Hofman wrote:
> >>
> >>
> >> I wish I could help but unfortunately my practical knowledge of kernel
> >> workqueues is close to zero :-( Of course I will test the patches and
> >> will extend them for quartet with testing too.
> >
> > How about the patch below?  This is a quick fix for 3.19 (and
> > stable).  More better fixes will follow later once after it's
> > confirmed to work.
> 
> Hi,
> 
> Thanks a lot, the patch seems to run fine. Only ak4114_init_regs has to 
> be called every samplerate change, otherwise the receiver does not 
> re-run the samplerate detection and its register with detected 
> samplerate does not update its value.

OK, I'm going to send a fix series including the relevant correction.
Give it a try later.

> 
> The following patch seems to run ok:
> 
> diff --git a/include/sound/ak4114.h b/include/sound/ak4114.h
> index 52f02a6..796834b 100644
> --- a/include/sound/ak4114.h
> +++ b/include/sound/ak4114.h
> @@ -169,6 +169,7 @@ struct ak4114 {
>          ak4114_read_t * read;
>          void * private_data;
>          unsigned int init: 1;
> +       bool in_workq;
>          spinlock_t lock;
>          unsigned char regmap[6];
>          unsigned char txcsb[5];
> diff --git a/sound/i2c/other/ak4114.c b/sound/i2c/other/ak4114.c
> index c7f5633..6d643b7 100644
> --- a/sound/i2c/other/ak4114.c
> +++ b/sound/i2c/other/ak4114.c
> @@ -152,10 +152,12 @@ static void ak4114_init_regs(struct ak4114 *chip)
> 
>   void snd_ak4114_reinit(struct ak4114 *chip)
>   {
> +       ak4114_init_regs(chip);
> +       if (chip->in_workq)
> +               return;
>          chip->init = 1;
>          mb();
> -       flush_delayed_work(&chip->work);
> -       ak4114_init_regs(chip);
> +       cancel_delayed_work_sync(&chip->work);
>          /* bring up statistics / event queing */
>          chip->init = 0;
>          if (chip->kctls[0])
> @@ -612,10 +614,12 @@ static void ak4114_stats(struct work_struct *work)
>   {
>          struct ak4114 *chip = container_of(work, struct ak4114, work.work);
> 
> -       if (!chip->init)
> +       chip->in_workq = true;
> +       if (!chip->init) {
>                  snd_ak4114_check_rate_and_errors(chip, chip->check_flags);
> -
> -       schedule_delayed_work(&chip->work, HZ / 10);
> +               schedule_delayed_work(&chip->work, HZ / 10);
> +       }
> +       chip->in_workq = false;
>   }
> 
>   EXPORT_SYMBOL(snd_ak4114_create);
> 
> 
> >
> >>> The HZ/10 isn't that bad, but the problem is that it's unconditionally
> >>> running even if user doesn't need/want.
> >>
> >> It is useful only for the external clock mode. In fact the detection of
> >> incoming SPDIF rate is not reliable for internal clock in Juli (while it
> >> works just fine in Quartet, its FPGA pins configure the SPDIF receiver
> >> differently). IMO the thread could be running only when clock is
> >> switched to external.
> >
> > Yeah, we can do some smart task change in addition to manual on/off.
> > Maybe it's good to have an enum control for that.
> 
> I am not sure users would want/need to disable a feature which detects 
> incoming samplerate. IMO if the work thread is running only in the 
> external clock mode, nothing more is needed.

Hm, but you can still see the other attributes of SPDIF input frames,
right?  Or all these useless when the clock is set to internal?
If so, it'd be easy to add the dynamic turn on/off per the clock
mode.


Takashi