Hello,
I found and fixed a little bug in the "Intel ICH (i8xx), SiS 7012,
NVidia nForce Audio or AMD 768/811x" driver (kernel 2.4.23-pre2) that
occured on my laptop with SiS 7012 onBoard sound and wanted to
contribute it to the official kernel sources.
In the maintainers file that came with the kernel I couldn't find the
maintainer of that particular driver so I'd appreciate if someone lead
me to the correct mailing list so I can post the bug and my patch to the
right place.
I wrote to [email protected] yesterday but got no answer yet.
Best regards,
Mehmet Ceyran
On Wednesday 03 September 2003 14:22, Mehmet Ceyran wrote:
Hi Mehmet,
> I found and fixed a little bug in the "Intel ICH (i8xx), SiS 7012,
> NVidia nForce Audio or AMD 768/811x" driver (kernel 2.4.23-pre2) that
> occured on my laptop with SiS 7012 onBoard sound and wanted to
> contribute it to the official kernel sources.
> In the maintainers file that came with the kernel I couldn't find the
> maintainer of that particular driver so I'd appreciate if someone lead
> me to the correct mailing list so I can post the bug and my patch to the
> right place.
It seems Alan is maintaining i8xx audio, but Alan is away for 1 year now.
Anyway, you could send him your patch CC'ing LKML.
ciao, Marc
Mehmet Ceyran wrote:
> Hello,
>
> I found and fixed a little bug in the "Intel ICH (i8xx), SiS 7012,
> NVidia nForce Audio or AMD 768/811x" driver (kernel 2.4.23-pre2) that
> occured on my laptop with SiS 7012 onBoard sound and wanted to
> contribute it to the official kernel sources.
>
> In the maintainers file that came with the kernel I couldn't find the
> maintainer of that particular driver so I'd appreciate if someone lead
> me to the correct mailing list so I can post the bug and my patch to the
> right place.
Please post the patch here, to linux-kernel, and CC Alan Cox
<[email protected]>
Jeff
Thanks to Jeff and Marc for your answers.
On my laptop (P4-2533, SiS 7012 onBoard sound) the i810 driver didn't
work in its shipped 2.4.23-pre2 and 2.4.22 (and probably below) form.
The kernel messages related to this were as follows:
---8<---
Intel 810 + AC97 Audio, version 0.24, 19:25:34 Aug 31 2003
PCI: Enabling device 00:02.7 (0000 -> 0001)
i810: SiS 7012 found at IO 0x1800 and 0x1c00, MEM 0x0000 and 0x0000, IRQ
10
i810_audio: Audio Controller supports 6 channels.
i810_audio: Defaulting to base 2 channel mode.
i810_audio: Resetting connection 0
ac97_codec: AC97 Audio codec, id: CRY52 (Cirrus Logic CS4299 rev D)
i810_audio: timed out waiting for codec 0 analog ready.
--->8---
After hesitating for a while (I code in C/C++ and many more languages
but I've never been to kernel and drivers, YET ;) ) I decided to look at
the sources and fix the bug if I could.
I found out that the driver gives the sound chip 10 chances to become
ready and my sound chip fails to do that in time. The following patch
gives the chip 100 tries instead of 10:
---8<---
--- i810_audio.c 2003-09-02 13:58:02.000000000 +0200
+++ i810_audio.c.new 2003-09-02 13:58:12.000000000 +0200
@@ -2727,7 +2727,7 @@
i810_ac97_get(codec, AC97_POWER_CONTROL) &
~0x7f00);
/* wait for analog ready */
- for (i=10; i && ((i810_ac97_get(codec, AC97_POWER_CONTROL) &
0xf) != 0xf); i--)
+ for (i=100; i && ((i810_ac97_get(codec, AC97_POWER_CONTROL) &
0xf) != 0xf); i--)
{
set_current_state(TASK_UNINTERRUPTIBLE);
schedule_timeout(HZ/20);
--->8---
Well, why not? The loop will only go through it's body 100 times if the
hardware is actually not available or corrupt and even in this case the
whole block won't take much time. It works for me and it should work for
all the other people using this driver too:
---8<---
i810: SiS 7012 found at IO 0x1800 and 0x1c00, MEM 0x0000 and 0x0000, IRQ
10
i810_audio: Audio Controller supports 6 channels.
i810_audio: Defaulting to base 2 channel mode.
i810_audio: Resetting connection 0
ac97_codec: AC97 Audio codec, id: CRY52 (Cirrus Logic CS4299 rev D)
i810_audio: AC'97 codec 0 supports AMAP, total channels = 2
--->8---
Umm, the maintainers list says I should try to include credit lines.
Well yeah, I'd like to have my name and email address in the changelog
if this gets through to the kernel ;). And I'm really interested in
contributing more than just this to the kernel.
In hope for feedback,
Mehmet Ceyran
On Thu, Sep 04, 2003 at 01:31:40AM +0200, Mehmet Ceyran wrote:
> I found out that the driver gives the sound chip 10 chances to become
> ready and my sound chip fails to do that in time. The following patch
> gives the chip 100 tries instead of 10:
>
> ---8<---
> --- i810_audio.c 2003-09-02 13:58:02.000000000 +0200
> +++ i810_audio.c.new 2003-09-02 13:58:12.000000000 +0200
> @@ -2727,7 +2727,7 @@
> i810_ac97_get(codec, AC97_POWER_CONTROL) &
> ~0x7f00);
>
> /* wait for analog ready */
> - for (i=10; i && ((i810_ac97_get(codec, AC97_POWER_CONTROL) &
> 0xf) != 0xf); i--)
> + for (i=100; i && ((i810_ac97_get(codec, AC97_POWER_CONTROL) &
> 0xf) != 0xf); i--)
> {
> set_current_state(TASK_UNINTERRUPTIBLE);
> schedule_timeout(HZ/20);
> --->8---
>
> Well, why not? The loop will only go through it's body 100 times if the
> hardware is actually not available or corrupt and even in this case the
> whole block won't take much time. It works for me and it should work for
> all the other people using this driver too:
Why busy wait especially when you can sleep 1ms each time and poll less?
Hi Mike,
>> Well, why not? The loop will only go through it's body 100 times if
>> the hardware is actually not available or corrupt and even in this
>> case the whole block won't take much time. It works for me and it
>> should work for all the other people using this driver too:
> Why busy wait especially when you can sleep 1ms each time and poll
less?
Well, I can tell you why I chose "busy wait" (kind of brainstorming):
- Why did the originator of the driver made the loop with 10 turns,
which is "busy wait" in your definition too?
- Imagine you have workers and you give them a job that's supposed to be
finished in six minutes. But these workers actually need seven minutes,
which you don't know. If that job is urgent, would you rather look if
it's done yet every 60 minutes or every six minutes? In the first case
(that's your model), you would get your positive result in one hour, in
the second case (my model), you'd get it in 12 minutes.
- Seeing the constant of HZ/20 in "schedule_timeout(HZ/20);" gives me
the feeling that this value is taken from some specification. Perhaps
it's from a i810 datasheet (I will look at that later because it's very
late now), but my fix is for SiS 7012 which could have slightly
different characteristics. In this case, by doing my "busy wait", you
don't modify the existing code which will initialize the chip in less
than or equal to 10 turns.
- When the driver is loaded at boot time I think it's the only piece of
code which is running at that moment, I don't think there are any other
threads working during driver init. If it's loaded as a module, the one
who loads it waits for its results anyway. It's almost impossible that
doing this "busy wait" will affect a running system with many
concurrently running processes at a high system load _noticeably_. It
will instead come to a result faster if the hardware is available (see
point 1).
Best Regards,
Mehmet
On Iau, 2003-09-04 at 02:43, Mike Fedyk wrote:
> > set_current_state(TASK_UNINTERRUPTIBLE);
> > schedule_timeout(HZ/20);
> Why busy wait especially when you can sleep 1ms each time and poll less?
I think you read it wrong - its sleeping...
On Iau, 2003-09-04 at 02:43, Mike Fedyk wrote:
> > set_current_state(TASK_UNINTERRUPTIBLE);
> > schedule_timeout(HZ/20);
> Why busy wait especially when you can sleep 1ms each time and poll less?
I think you read it wrong - its sleeping...
On Thu, Sep 04, 2003 at 12:01:43PM +0100, Alan Cox wrote:
> On Iau, 2003-09-04 at 02:43, Mike Fedyk wrote:
> > > set_current_state(TASK_UNINTERRUPTIBLE);
> > > schedule_timeout(HZ/20);
>
> > Why busy wait especially when you can sleep 1ms each time and poll less?
>
> I think you read it wrong - its sleeping...
Yep, you're right.