2002-10-03 08:48:21

by Mikael Pettersson

[permalink] [raw]
Subject: possible bug in 2.5.39 ISA PnP patch

On 2002-09-27 11:16:04, Jaroslav Kysela wrote:
> the read port (RDP) of ISA PnP cards must be set only when card is
>in the isolation phase. Bellow is a patch to follow the ISA PnP
>specification. Please, apply.
>...
>diff -Nru a/drivers/pnp/isapnp.c b/drivers/pnp/isapnp.c
>--- a/drivers/pnp/isapnp.c Fri Sep 27 13:13:51 2002
>+++ b/drivers/pnp/isapnp.c Fri Sep 27 13:13:51 2002
>@@ -1048,11 +1048,19 @@
> isapnp_wait();
> isapnp_key();
> isapnp_wake(csn);
>-#if 1 /* to avoid malfunction when the isapnptools package is used */
>- isapnp_set_rdp();
>- udelay(1000); /* delay 1000us */
>- write_address(0x01);
>- udelay(1000); /* delay 1000us */
>+#if 1
>+ /* to avoid malfunction when the isapnptools package is used */
>+ /* we must set RDP to our value again */
>+ /* it is possible to set RDP only in the isolation phase */
>+ /* Jens Thoms Toerring <[email protected]> */
>+ isapnp_write_byte(0x02, 0x04); /* clear CSN of card */
>+ mdelay(2); /* is this necessary? */
>+ isapnp_wake(csn); /* bring card into sleep state */
>+ isapnp_wake(0); /* bring card into isolation state */
>+ isapnp_set_rdp(); /* reset the RDP port */
>+ udelay(1000); /* delay 1000us */
>+ isapnp_write_byte(0x06, csn); /* reset CSN to previous value */
>+ udelay(250); /* is this necessary? */
> #endif
> if (logdev >= 0)
> isapnp_device(logdev);

Linus included this patch in 2.5.39. Unfortunately, it causes
my ISA PnP cards to malfunction:
- My 3c509(b?) combo TP/BNC Ethernet card stops working completely.
The kernel thinks the NIC is up, but the leds on its segment aren't
lit and no traffic can be generated.
Backing out this patch, or disabling ISAPNP, makes it work again.
- My ESS sound card is a multi-function device. With this patch, some
sub-devices get bogus resources listed in /proc/isapnp, as if the
data is all-bits-one. Backing out this patch solves the problem.

I have several other ISA PnP card at another location, but I won't be
able to test them until Saturday.

I can't comment on whether the new code in the patch correctly implements
the specification or not, but something's definitely wrong here. Do you
have any evidence of cards that malfunctioned with the old code?

Also, the comment "to avoid malfunction when the isapnptools package is used"
strikes me a bit strange. Why use isapnptools if the kernel has ISAPNP=y ?
Is that actually supposed to work?

/Mikael


2002-10-03 09:14:20

by Jaroslav Kysela

[permalink] [raw]
Subject: Re: possible bug in 2.5.39 ISA PnP patch

On Thu, 3 Oct 2002, Mikael Pettersson wrote:

> On 2002-09-27 11:16:04, Jaroslav Kysela wrote:
> > the read port (RDP) of ISA PnP cards must be set only when card is
> >in the isolation phase. Bellow is a patch to follow the ISA PnP
> >specification. Please, apply.
> >...
> >diff -Nru a/drivers/pnp/isapnp.c b/drivers/pnp/isapnp.c
> >--- a/drivers/pnp/isapnp.c Fri Sep 27 13:13:51 2002
> >+++ b/drivers/pnp/isapnp.c Fri Sep 27 13:13:51 2002
> >@@ -1048,11 +1048,19 @@
> > isapnp_wait();
> > isapnp_key();
> > isapnp_wake(csn);
> >-#if 1 /* to avoid malfunction when the isapnptools package is used */
> >- isapnp_set_rdp();
> >- udelay(1000); /* delay 1000us */
> >- write_address(0x01);
> >- udelay(1000); /* delay 1000us */
> >+#if 1
> >+ /* to avoid malfunction when the isapnptools package is used */
> >+ /* we must set RDP to our value again */
> >+ /* it is possible to set RDP only in the isolation phase */
> >+ /* Jens Thoms Toerring <[email protected]> */
> >+ isapnp_write_byte(0x02, 0x04); /* clear CSN of card */
> >+ mdelay(2); /* is this necessary? */
> >+ isapnp_wake(csn); /* bring card into sleep state */
> >+ isapnp_wake(0); /* bring card into isolation state */
> >+ isapnp_set_rdp(); /* reset the RDP port */
> >+ udelay(1000); /* delay 1000us */
> >+ isapnp_write_byte(0x06, csn); /* reset CSN to previous value */
> >+ udelay(250); /* is this necessary? */
> > #endif
> > if (logdev >= 0)
> > isapnp_device(logdev);
>
> Linus included this patch in 2.5.39. Unfortunately, it causes
> my ISA PnP cards to malfunction:
> - My 3c509(b?) combo TP/BNC Ethernet card stops working completely.
> The kernel thinks the NIC is up, but the leds on its segment aren't
> lit and no traffic can be generated.
> Backing out this patch, or disabling ISAPNP, makes it work again.
> - My ESS sound card is a multi-function device. With this patch, some
> sub-devices get bogus resources listed in /proc/isapnp, as if the
> data is all-bits-one. Backing out this patch solves the problem.
>
> I have several other ISA PnP card at another location, but I won't be
> able to test them until Saturday.
>
> I can't comment on whether the new code in the patch correctly implements
> the specification or not, but something's definitely wrong here. Do you
> have any evidence of cards that malfunctioned with the old code?
>
> Also, the comment "to avoid malfunction when the isapnptools package is used"
> strikes me a bit strange. Why use isapnptools if the kernel has ISAPNP=y ?
> Is that actually supposed to work?

Thanks for this report. Indeed, the code should be removed, if it causes
trouble. I recommend to comment the whole code trying to reset the RDP:

===== isapnp.c 1.14 vs edited =====
--- 1.14/drivers/pnp/isapnp.c Mon Sep 30 03:19:31 2002
+++ edited/isapnp.c Thu Oct 3 11:15:46 2002
@@ -1048,7 +1048,7 @@
isapnp_wait();
isapnp_key();
isapnp_wake(csn);
-#if 1
+#if 0
/* to avoid malfunction when the isapnptools package is used */
/* we must set RDP to our value again */
/* it is possible to set RDP only in the isolation phase */


Jaroslav

-----
Jaroslav Kysela <[email protected]>
Linux Kernel Sound Maintainer
ALSA Project http://www.alsa-project.org
SuSE Linux http://www.suse.com

2002-10-03 14:11:43

by Jens Thoms Toerring

[permalink] [raw]
Subject: Re: possible bug in 2.5.39 ISA PnP patch

On 2002-10-03 10:53:53, Mikael Pettersson <[email protected]> wrote:
> On 2002-09-27 11:16:04, Jaroslav Kysela wrote:
> > the read port (RDP) of ISA PnP cards must be set only when card is
> >in the isolation phase. Bellow is a patch to follow the ISA PnP
> >specification. Please, apply.

<patch snipped>

> Linus included this patch in 2.5.39. Unfortunately, it causes
> my ISA PnP cards to malfunction:
> - My 3c509(b?) combo TP/BNC Ethernet card stops working completely.
> The kernel thinks the NIC is up, but the leds on its segment aren't
> lit and no traffic can be generated.
> Backing out this patch, or disabling ISAPNP, makes it work again.
> - My ESS sound card is a multi-function device. With this patch, some
> sub-devices get bogus resources listed in /proc/isapnp, as if the
> data is all-bits-one. Backing out this patch solves the problem.
>
> I have several other ISA PnP card at another location, but I won't be
> able to test them until Saturday.
>
> I can't comment on whether the new code in the patch correctly implements
> the specification or not, but something's definitely wrong here. Do you
> have any evidence of cards that malfunctioned with the old code?

Yes, my HP82341D GPIB card does not work at all without the patch, that's
the reason I came up with it.

> Also, the comment "to avoid malfunction when the isapnptools package is
used"
> strikes me a bit strange. Why use isapnptools if the kernel has ISAPNP=y ?
> Is that actually supposed to work?

Of course, you are right, using isapnptools is definitely not a good
idea in this case. But too many people will do it anyway when there is
a problem with their cards, not realizing that it will lead to even
worse problems. Without the code after the "#if 1" running pnpdump may
keep not already configured cards from responding anymore and a reboot
is required to get the cards back into s sane state. And that's probably
the reason why the code was introduced in the first place...

Unfortunately, I only have one other ISAPnP card, so I couldn't test the
patch very thoroughly - it (unfortunately) worked with both cards. I have
an idea what is broken with the patch and I am trying to figure out a way
how to get it right. If I should come up with a solution would you be
prepared to help me by doing some tests with your cards?

To Jaroslav:

After rereading the standard I guess that the problem is the clearing of
the CSN - it may not clear the CSN of the card in Configuration state
only but, unfortunately, of *all* cards (except the ones in Wait state,
but at that moment all the others cards are in Sleep state since we
first had to send the isolation key and there's no way to get only a
subsets of the cards back into Wait state). This would explain the
problems Mikael is seeing.

Since we obviously can't do without clearing the CSNs of all cards
just to set the RDP of one card, we then also have to repeat the
whole isolation phase, at least that's the only way I can see at the
moment. Luckily, ISA PnP aren't hot-pluggable, so the sequence of
the cards during the isolation phase can't change and all cards will
end up with the same CSN they had before... Or do you think I got it
wrong or see any reasons why this won't work?

Best regards, Jens
--
_ _____ _____
| ||_ _||_ _| [email protected]
_ | | | | | |
| |_| | | | | | http://www.physik.fu-berlin.de/~toerring
\___/ens|_|homs|_|oerring