From: Don Fry <brazilnut@us•ibm.com>
To: Francois Romieu <romieu@fr•zoreil.com>
Cc: tsbogend@alpha•franken.de, jgarzik@pobox•com, netdev@vger•kernel.org
Subject: Re: [PATCH 8/9] pcnet32: NAPI implementation.
Date: Thu, 29 Jun 2006 16:41:04 -0700 [thread overview]
Message-ID: <20060629234104.GA22583@us.ibm.com> (raw)
In-Reply-To: <20060629222438.GB8122@electric-eye.fr.zoreil.com>
On Fri, Jun 30, 2006 at 12:24:38AM +0200, Francois Romieu wrote:
> Nit below.
>
> Don Fry <brazilnut@us•ibm.com> :
> [...]
> > --- linux-2.6.17-git13/drivers/net/purge.pcnet32.c Thu Jun 29 13:28:24 2006
> > +++ linux-2.6.17-git13/drivers/net/pcnet32.c Thu Jun 29 13:28:31 2006
> [...]
> > +#ifdef CONFIG_PCNET32_NAPI
> > +static int pcnet32_poll(struct net_device *dev, int *budget)
> > +{
> [...]
> > + netif_rx_complete(dev);
> > +
> > + spin_lock_irqsave(&lp->lock, flags);
>
> No need to save/restore (it's true in pcnet32_{get_regs/suspend} too).
This lock is taken by the interrupt handler and my reading of
spinlocks.txt says I do need to use spin_lock_irqsave unless I
misunderstand. The only spin_lock() is in the interrupt handler itself,
all others are spin_lock_irqsave.
>
> > +
> > + /* clear interrupt masks */
> > + val = lp->a.read_csr(ioaddr, CSR3);
> > + val &= 0x00ff;
> > + lp->a.write_csr(ioaddr, CSR3, val);
> > +
> > + /* Set interrupt enable. */
> > + lp->a.write_csr(ioaddr, CSR0, CSR0_INTEN);
>
> Insert mmiowb():
Is this required in addition to the two outw() in write_csr? There are
rmb() and wmb() in the places that need them, but no mmiowb() anywhere.
What are the factors for when mmiowb needs to be inserted?
>
> > +
> > + spin_unlock_irqrestore(&lp->lock, flags);
>
> [...]
> > @@ -2413,183 +2608,41 @@ pcnet32_interrupt(int irq, void *dev_id,
> > dev->name, csr0);
> > /* unlike for the lance, there is no restart needed */
> > }
> > -
> > - if (must_restart) {
> > +#ifdef CONFIG_PCNET32_NAPI
> > + if (netif_rx_schedule_prep(dev)) {
> > + u16 val;
> > + /* set interrupt masks */
> > + val = lp->a.read_csr(ioaddr, CSR3);
> > + val |= 0x5f00;
> > + lp->a.write_csr(ioaddr, CSR3, val);
>
> Insert mmiowb();
>
> --
> Ueimor
--
Don Fry
brazilnut@us•ibm.com
next prev parent reply other threads:[~2006-06-29 23:40 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-06-29 20:55 [PATCH 8/9] pcnet32: NAPI implementation Don Fry
2006-06-29 22:24 ` Francois Romieu
2006-06-29 23:41 ` Don Fry [this message]
2006-06-30 7:15 ` Francois Romieu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20060629234104.GA22583@us.ibm.com \
--to=brazilnut@us$(echo .)ibm.com \
--cc=jgarzik@pobox$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=romieu@fr$(echo .)zoreil.com \
--cc=tsbogend@alpha$(echo .)franken.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox