Kevin, > -----Original Message----- > From: Kevin Hilman [mailto:[email protected]] > Sent: Wednesday, February 11, 2009 12:20 AM > To: Narnakaje, Snehaprabha > Cc: [email protected] > Subject: Re: [PATCH] NET: dm9000 ethernet on dm355: fix NETDEV WATCHDOG > timeout issue > > Kevin Hilman <[email protected]> writes: > > > [email protected] writes: > > > >> From: Sneha Narnakaje <[email protected]> > >> > >> This patch fixes the NETDEV WATCHDOG timeout issue with dm9000 ethernet > >> driver on DM355, while using the NFS as root filesystem. > >> In the patch I have replaced the spin_lock/spin_unlock calls with > dm9000 > >> specific disable/enable interrupt calls, in the dm9000 hard_start_xmit > >> routine. Though it seems to be a generic issue discussed in the netdev > list, > >> this fix may not be a permanent solution, yet we can proceed with other > drivers > >> on DM355. > >> I have also enabled the dm9000 option in the DM355 default > configuration. > >> > [ ... ] > > >> --- a/drivers/net/dm9000.c > >> +++ b/drivers/net/dm9000.c > >> @@ -758,7 +758,8 @@ dm9000_start_xmit(struct sk_buff *skb, struct > net_device *dev) > >> if (db->tx_pkt_cnt > 1) > >> return 1; > >> > >> - spin_lock_irqsave(&db->lock, flags); > >> + /* Disable all interrupts */ > >> + iow(db, DM9000_IMR, IMR_PAR); > > > > I didn't look at all the locking in this driver, but for correctness, > > I'm guessing you probably need to do the dm9000-specific disable in > > addition to the spin_lock. That way you only add the extra interrupt > > disable, and don't change the locking. > > > > In otherwords, see patch below which goes on top of yours. It keeps > the locking intact. Together with yours, it basically replaces the > global IRQ disable with a dm9000 only IRQ disable.
I have tested your suggestion to keep the spin_lock/spin_unlock (and spin_lock_irqsave/spin_unlock_irqsave) mechanism along with my change to disable/re-enable dm9000 specific interrupts. Note that, disable/re-enable dm9000 specific interrupt sequence is required to get NFS working properly. > > The question still remains... what is different about the global IRQ > disable compared to the dm9000-only IRQ disable. After going through some testing, it looks like the issue is how we have the dm9000 interrupt configured on the DM355 EVM. On DM355 EVM, we have GPIO1 configured as dm9000 interrupt line. May be on other boards have a dedicated dm9000 interrupt line. If we have a dedicated interrupt (to which the disable/enable dm9000 interrupt is also tied), I think it is enough to use global IRQ disable/enable APIs. > > The other difference I can think of between this and is being replaced: > > If the xmit function is called with interrupts disabled, your patch > ensures that interrupts are (re)enabled when xmit finishes. If > interrupts are disabled when xmit is entered, it will exit with them > disabled as well due to the restore. To see if that is happening, you > might try to throw a 'WARN_ON(irqs_disabled());' in the xmit function > in the original version (before your patch.) There is a reason why we need to disable interrupts when we transmit dm9000 packets. Dm9000 hardware has an internal SRAM for the transmit buffers (two buffers), but the hardware is capable of sending one packet at a time. While dm9000 hardware is in the process of sending one packet, the driver can copy the skb data in the second SRAM transmit buffer. In the tx interrupt completion routine we ask dm9000 hardware to transmit the second packet. Thanks Sneha > > Kevin > > commit f0a524a6bd7900556a35dcc53f3a493576ba742c > Author: Kevin Hilman <[email protected]> > Date: Tue Feb 10 21:04:57 2009 -0800 > > NET: dm9000: re-add spinlock, but without IRQ save/restore > > diff --git a/drivers/net/dm9000.c b/drivers/net/dm9000.c > index 90e3fd6..20ded42 100644 > --- a/drivers/net/dm9000.c > +++ b/drivers/net/dm9000.c > @@ -750,7 +750,6 @@ static void dm9000_timeout(struct net_device *dev) > static int > dm9000_start_xmit(struct sk_buff *skb, struct net_device *dev) > { > - unsigned long flags; > board_info_t *db = netdev_priv(dev); > > dm9000_dbg(db, 3, "%s:\n", __func__); > @@ -761,6 +760,8 @@ dm9000_start_xmit(struct sk_buff *skb, struct > net_device *dev) > /* Disable all interrupts */ > iow(db, DM9000_IMR, IMR_PAR); > > + spin_lock(&db->lock); > + > /* Move data to DM9000 TX RAM */ > writeb(DM9000_MWCMD, db->io_addr); > > @@ -784,6 +785,8 @@ dm9000_start_xmit(struct sk_buff *skb, struct > net_device *dev) > netif_stop_queue(dev); > } > > + spin_unlock(&db->lock); > + > /* Re-enable interrupt */ > iow(db, DM9000_IMR, IMR_PAR | IMR_PTM | IMR_PRM); > _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
