[alsa-devel] Fwd: [PATCH] Dreamcast AICA sound driver G2 bus handling

[Takashi Iwai]

At Sun, 16 Sep 2007 12:12:08 +0100,
Adrian McMenamin wrote:
> 
> Resending this as I typed the wrong address for alsa-devel last night.
> A general clean up as well as some changes to interrupt handling.
> 
> 
> 
> This patch handles instability on the Dreamcast G2 bus while PIO or
> DMA is underway on the System -> AICA channel.
> 
> Without the suspension of interrupts when PIO or DMA is underway the
> G2 bus is prone to timeouts leading to seemingly random crashes. This
> is particularly visible in cases such as maple bus (see
> http://lkml.org/lkml/2007/9/15/181) hotplugging but the patch is good
> for all conditions.

Hm, it isn't optimal but if it's safe...

BTW, what is the exat matter?
Is it race or do you have to always disable IRQ during DMA?
In the former case, spin_lock_irqsave() is basically the way to go.


> Signed-off by: Adrian McMenamin <adrian at mcmen.demon.co.uk>
> 
>  MODULE_AUTHOR("Adrian McMenamin <adrian at mcmen.demon.co.uk>");
> @@ -106,11 +108,14 @@ static void spu_write_wait(void)
>  static void spu_memset(u32 toi, u32 what, int length)
>  {
>         int i;
> +       unsigned long flags;
>         snd_assert(length % 4 == 0, return);
>         for (i = 0; i < length; i++) {
>                 if (!(i % 8))
>                         spu_write_wait();
> +               local_irq_save(flags);
>                 writel(what, toi + SPU_MEMORY_BASE);
> +               local_irq_restore(flags);
>                 toi++;
>         }
>  }

It would be more readable to define a function to call writel in
irq-disabled state.


> @@ -537,15 +565,18 @@ static struct snd_kcontrol_new
> snd_aica_pcmvolume_control __devinitdata = {
>  static int load_aica_firmware(void)
>  {
>         int err;
> +       unsigned long flags;
>         const struct firmware *fw_entry;
>         spu_reset();
>         err = request_firmware(&fw_entry, "aica_firmware.bin", &pd->dev);
> -       if (unlikely(err))
> +       if (err)
>                 return err;
>         /* write firware into memory */

Could you split out the removal of unlikely() as another patch?
Then it'll be more logical to follow the changes.


> +       local_irq_save(flags);
>         spu_disable();
>         spu_memload(0, fw_entry->data, fw_entry->size);
>         spu_enable();
> +       local_irq_restore(flags);

Is the irq-disablement really needed, too?



thanks,

Takashi