linux-kernel - Re: [PATCH 2.6.18-rc6 1/2] dllink driver: porting v1.19 to linux 2.6.18-rc6

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]

Message-Id: <1157886602.2926.36.camel@localhost.localdomain>
Date:	Sun, 10 Sep 2006 14:10:02 +0300
From:	Hayim Shaul <hayim@...rtent.com>
To:	Arjan van de Ven <arjan@...radead.org>
Cc:	Jeff Garzik <jeff@...zik.org>, netdev@...r.kernel.org,
	edward_peng@...nk.com.tw, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2.6.18-rc6 1/2] dllink driver: porting v1.19 to linux
	2.6.18-rc6

I'm not really an expert, and I didn't understand all your remarks
but I can tell you this:

The driver supplied with 2.6.15 looks like dlink's driver version 1.17.
I had a dlink NIC that got stuck once in a while running that driver.

dlink's version 1.19 is written for 2.4 kernels, so all I did was
convert it to 2.6 kernels.

The new version still got stuck once in a while.
Maybe because the bugs you pointed out.

I don't have the dlink NIC anymore so I don't see how I can help here.
Maybe Edward can answer to that.

Sorry.


On Thu, 2006-09-07 at 11:19 +0200, Arjan van de Ven wrote:
> > @@ -335,8 +374,9 @@
> >  #endif
> >  	/* Read eeprom */
> >  	for (i = 0; i < 128; i++) {
> > -		((u16 *) sromdata)[i] = le16_to_cpu (read_eeprom (ioaddr, i));
> > +		((u16 *) sromdata)[i] = cpu_to_le16 (read_eeprom (ioaddr, i));
> >  	}
> > +	psrom->crc = le32_to_cpu(psrom->crc);
> 
> this looks wrong, the data comes from the hw as le, so le*_to_cpu()
> sounds the right direction
> 
> > @@ -401,7 +441,7 @@
> >  	int i;
> >  	u16 macctrl;
> >  	
> > -	i = request_irq (dev->irq, &rio_interrupt, IRQF_SHARED, dev->name, dev);
> > +	i = request_irq (dev->irq, &rio_interrupt, SA_SHIRQ, dev->name, dev);
> >  	if (i)
> >  		return i;
> 
> this is backing out a fix/conversion to the new API. Bad.
> 
> 
> >  	
> > @@ -434,9 +474,12 @@
> >  	writeb (0x30, ioaddr + RxDMABurstThresh);
> >  	writeb (0x30, ioaddr + RxDMAUrgentThresh);
> >  	writel (0x0007ffff, ioaddr + RmonStatMask);
> > +
> >  	/* clear statistics */
> >  	clear_stats (dev);
> >  
> > +	atomic_set(&np->tx_desc_lock, 0);
> 
> I'm quite scared by this naming; it suggests home-brew locking
> 
> >  	dev->trans_start = jiffies;
> > +	tasklet_enable(&np->tx_tasklet);
> > +	writew(DEFAULT_INTR, ioaddr + IntEnable);
> > +	return;
> 
> this looks like a PCI posting bug
> 
> 
> > -rio_free_tx (struct net_device *dev, int irq) 
> > +rio_free_tx (struct net_device *dev) 
> >  {
> >  	struct netdev_private *np = netdev_priv(dev);
> >  	int entry = np->old_tx % TX_RING_SIZE;
> > -	int tx_use = 0;
> >  	unsigned long flag = 0;
> > +	int irq = in_interrupt();
> 
> eeeeep
> 
> > +
> > +	if (atomic_read(&np->tx_desc_lock))
> > +		return;
> > +	atomic_inc(&np->tx_desc_lock);
> 
> and yes.. it is broken self made locking....
> there is a nice race between the _read and the _inc here.
> 
> 
> >  	
> >  	if (irq)
> >  		spin_lock(&np->tx_lock);
> >  	else
> >  		spin_lock_irqsave(&np->tx_lock, flag);
> 
> double eeeep
> 
> this is wrong to do with in_interrupt() as gating factor!
> Always doing the irqsave() is fine btw
> 
> 
> 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/