Re: sis190

[Francois Romieu <romieu@fr.zoreil.com>]

Pascal CHAPPERON <pascal.chapperon@wanadoo.fr> :
[...]
> 1) sis190 freezes the box when kernel PREEMPT is enabled :
> 
> I made many tries, but i could not solve it;
> - it does not occur while receiving huge files.
> - it does not occur when only a few packets are
>   transmitted (remote connection, ls, find)
> - it occurs only while transmiting huge files AND
>   trying to do someting else (open a new term,...)
> - I could transfer a huge file (700MB) several times
>   as i was at the console (and i could switch to another
> console to perform find, ls,... during the transfer).

Are you saying that PREEMPT and X are both needed to freeze the box ?

If so, could you try preempt + console + rsync (+ dd from disk) ?

> I managed the system so the sis190 had its own IRQ, but it
> made no difference.
> 
> As i suspected nvidia driver, i switched to nv driver : no result.

Please keep binary modules out of the loop until the things are stable.
I do not want to try and diagnose a bug in a system wherein such an amount
of unknown code was loaded (even if it was unloaded later).

> It seems to me that a task inside the sis190_tx_interrupt() is 
> not protected against preemption (and it is probably the same
> on a SMP not prempted).
> 
> I tried to play with spinlocks, but with no result :
> @@ -621,6 +621,7 @@ static irqreturn_t sis190_interrupt(int
>         void __iomem *ioaddr = tp->mmio_addr;
>         int handled = 0;
>         int boguscnt;
> +       unsigned long flags;
> 
>         for (boguscnt = max_interrupt_work; boguscnt > 0; boguscnt--) {
>                 u32 status = SIS_R32(IntrStatus);
> @@ -651,9 +652,9 @@ static irqreturn_t sis190_interrupt(int
>                         sis190_rx_interrupt(dev, tp, ioaddr);
> 
>                 if (status & TxQ0Int) {
> -                       spin_lock(&tp->lock);
> +                       spin_lock_irqsave(&tp->lock, flags);
>                         sis190_tx_interrupt(dev, tp, ioaddr);
> -                       spin_unlock(&tp->lock);
> +                       spin_unlock_irqrestore(&tp->lock, flags);

Afaik the irq handler is already protected against reentrancy (see
kernel/irq.c::__do_IRQ) and softirq (see arch/xxx/kernel/irq.c::irq_exit).
So this change should not make a difference.

[...]
> @@ -581,6 +581,7 @@ static void sis190_tx_interrupt(struct n
>                                 struct sis190_private *tp, void __iomem *ioaddr)
>  {
>         unsigned int tx_left, dirty_tx = tp->dirty_tx;
> +       unsigned long flags;
> 
>         for (tx_left = tp->cur_tx - dirty_tx; tx_left > 0; tx_left--) {
>                 unsigned int entry = dirty_tx % NUM_TX_DESC;
> @@ -604,10 +605,12 @@ static void sis190_tx_interrupt(struct n
>                 dirty_tx++;
>         }
> 
> +       spin_lock_irqsave(&tp->lock, flags);
>         if (tp->dirty_tx != dirty_tx) {
>                 tp->dirty_tx = dirty_tx;
>                 netif_wake_queue(dev);
>         }
> +       spin_unlock_irqrestore(&tp->lock, flags);
>  }

The irqsave/restore should not be needed for the same reason as above.

> In fact, i don't know where are the critical sections...

In the Tx path the critical section is related to netif_{start/stop}_queue.

> 2) sis190 freezes the box when the link partner is
> a r8169 forced in 10 full autoneg off (preempted or not
> preempted kernel) :

The r8169 driver will not necessarily do what you would expect when you
autoneg it off and it faces an unstable driver. I'll send an update for it.

Any TX timeout message on the console ? Freeze == sysrq has no effect
and keyboard leds do not blink any more ?

There is an updated version at 
http://www.zoreil.com/~romieu/sis190/20050630-2.6.13-rc1-sis190-test.patch

It would be nice to know how it behaves wrt preempt (no need to experiment
with the media management), especially if you can describe the freeze more
specifically.

--
Ueimor