netdev - Re: [PATCH] natsemi: netpoll fixes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 12 Mar 2007 19:11:20 +0000
From:	Mark Brown <broonie@...ena.org.uk>
To:	Sergei Shtylyov <sshtylyov@...mvista.com>
Cc:	Mark Huth <mhuth@...sta.com>, jgarzik@...ox.com,
	netdev@...r.kernel.org
Subject: Re: [PATCH] natsemi: netpoll fixes

On Mon, Mar 12, 2007 at 04:05:48PM +0300, Sergei Shtylyov wrote:
> Mark Brown wrote:

> >hands_off is stronger than that - it's used for sync with some of the
> >other code paths like suspend/resume and means "don't touch the chip".
> >I've added a new driver local flag instead.

>    I'm not sure it was worth it -- we already had __LINK_STATE_RX_SCHED...

It would be nice but as well as being a general layering violation it
could cause problems when trying to quiesce the hardware since
netif_poll_disable() sets this without actually scheduling the poll.

> >>  Yeah, it seems currently unjustified.  However IntrEnable would have 
> >>  been an ultimate criterion on taking or ignoring an interrupt 
> >>  otherwise...

>>>I guess the tradeoff depends on the probability
>>>of getting the isr called when NAPI is active for the device.

>    I mean I didn't understand why there's tradeoff and how it depends on 
>    the probability...

Reading device registers means going over the PCI bus, which is
expensive.  Shadowing the interrupt mask state in the driver makes
the driver more complicated but means that we avoid synchronous PCI
accesses, reducing the number of cycles the CPU spends stalled doing
those.

The performance benefit from the extra code complexity depends on how
often we end up doing the extra read.  Since one of the things NAPI does
is provide some interrupt mitigation the extra work is most likely to be
noticable if some other device is generating interrupts that trigger the
natsemi interrupt handler.

> >Moving netdev_rx() would fix that one but there's some others too -
> >there's one in the timer routine if the chip crashes.  In the case you

>    Erm, sorry, I'm not seeing it -- could you "point with finger" please? 
>    :-)

In netdev_timer() when the device is using PORT_TP if the DspCfg read
back from the chip differs from the one we think we programmed into it
then the driver thinks the PHY fell over.  It then goes through an init
sequence, including init_registers() which will reset IntrEnable among
other things.

> >describe above the consequences shouldn't be too bad since it tends to
> >only occur at high volume so further traffic will tend to occur and
> >cause things to recover - all the testing of that patch was done with
> >the bug present and no ill effects.

>    Oversized packets occur only at high volume? Is it some errata?

It's an errata - AN 1287 which you can get from the National web site.
It's not actually that chip that's getting oversided packets, what
happens is that the state machine which reads data off the wire gets
confused and eventually locks up.  Before locking up it will usually
report one or more oversided packets so this is a useful hint that we
should reset the recieve state machine in order to recover from this.

>    (If I only knew somebody else was working on that issue, it could have 
> saved my cycles, sigh... but well, I should have said  that I was going to 
> recast the patch. :-)

Yeah, me too.  I'll submit this one once I've finished testing, then
audit other IntrStatus accesses.

> >-		       readl(ioaddr + IntrMask));
> >+	spin_lock_irqsave(&np->intr_lock, flags);

>    Yeah, I've suspected that we need to grab np->lock here... but does that 
> separate spinlock actually protect us from anything?

...

> >+	/* We need to ensure that the ISR doesn't run between telling
> >+	 * NAPI we're done and enabling the interrupt. */
> 
>    Why? :-O

This is to ensure that the interrupt handler can't run between us
marking that the poll has complete and reenabling the interrupt from the
chip.  That would mean that the chip would end up with interrupts from
the chip enabled while the poll is scheduled.

-- 
"You grabbed my hand and we fell into it, like a daydream - or a fever."

Download attachment "signature.asc" of type "application/pgp-signature" (308 bytes)