netdev - Re: [PATCH] dm9000: fix spinlock issue and introduce platform

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] [day] [month] [year] [list]

Message-ID: <20071120150719.GD3244@fluff.org.uk>
Date:	Tue, 20 Nov 2007 15:07:19 +0000
From:	Ben Dooks <ben@...ff.org>
To:	dmitry pervushin <dpervushin@...il.com>
Cc:	Ben Dooks <ben@...ff.org>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] dm9000: fix spinlock issue and introduce platform_init
	callback

On Tue, Nov 20, 2007 at 05:59:49PM +0300, dmitry pervushin wrote:
> 
> On ???, 2007-11-20 at 14:51 +0000, Ben Dooks wrote:
> > On Tue, Nov 20, 2007 at 05:43:42PM +0300, dmitry pervushin wrote:
> > > Hey all,
> > > 
> > > The patch below is intended to fix two problems:
> > > - trying to acquire spinlock twice on timeout condition
> > 
> > I'll have a look into this, although I think we may be better of
> > not dropping the spinlock and simply moving it out of
> > dm9000_init_dm9000() and ensure all callers of this function
> > hold the lock already.

> actually, dm9000_hash_table holds this spin lock. Well, I'll try to rework this

I think that dropping and re-aquiring the lock is creating the
possibility that something else could happen in this sequence
of events.

> > > - create callback than can be used by platform code to initialize the
> > > chip
> > 
> > Hi, what purpose is the callback for? and if it takes a platform
> > device why is the prototype for 'void *' ?
> to initialize the chip. My target board has to configure GPIO/CPLD
> registers properly to activate dm9000. I do not want to move this
> initialization to common code -- the GPIO lines might be shared and I'd
> like to configure them only if it is needed.

Aha, could the comment in the header file be expanded to say
something like "Callback for machine or board dependant initialisation,
such as enabling the routing of control signals to the DM9000" to signify
what is meant to be happening here.
 
> > This should have also been two seperate patches as there are two
> > different issues.

Just as an further explanation, the spinlock fix is ideal to push
now, whereas the callback patch is probably 2.6.25 material.

> > > Signed-off-by: dmitry pervushin <dpervushin@...il.com>
> > > Index: linux/drivers/net/dm9000.c
> > > ===================================================================
> > > --- linux.orig/drivers/net/dm9000.c
> > > +++ linux/drivers/net/dm9000.c
> > > @@ -335,8 +335,11 @@ static void dm9000_timeout(struct net_de
> > >  
> > >  	netif_stop_queue(dev);
> > >  	dm9000_reset(db);
> > > +	spin_unlock_irqrestore(&db->lock,flags);
> > > +
> > >  	dm9000_init_dm9000(dev);
> > >  	/* We can accept TX packets again */
> > > +	spin_lock_irqsave(&db->lock,flags);
> > >  	dev->trans_start = jiffies;
> > >  	netif_wake_queue(dev);
> > >  
> > > @@ -526,6 +529,8 @@ dm9000_probe(struct platform_device *pde
> > >  			db->dumpblk = pdata->dumpblk;
> > >  	}
> > >  
> > > + 	if (pdata && pdata->platform_init)
> > > + 			pdata->platform_init(pdev);
> > >  	dm9000_reset(db);
> > >  
> > >  	/* try two times, DM9000 sometimes gets the first read wrong */
> > > @@ -1155,10 +1160,13 @@ dm9000_drv_resume(struct platform_device
> > >  {
> > >  	struct net_device *ndev = platform_get_drvdata(dev);
> > >  	board_info_t *db = (board_info_t *) ndev->priv;
> > > + 	struct dm9000_plat_data *pdata = dev->dev.platform_data;
> > >  
> > >  	if (ndev) {
> > >  
> > >  		if (netif_running(ndev)) {
> > > + 			if (pdata && pdata->platform_init)
> > > +         	                pdata->platform_init(dev);
> > >  			dm9000_reset(db);
> > >  			dm9000_init_dm9000(ndev);
> > >  
> > > Index: linux/include/linux/dm9000.h
> > > ===================================================================
> > > --- linux.orig/include/linux/dm9000.h
> > > +++ linux/include/linux/dm9000.h
> > > @@ -30,6 +30,9 @@ struct dm9000_plat_data {
> > >  	void	(*inblk)(void __iomem *reg, void *data, int len);
> > >  	void	(*outblk)(void __iomem *reg, void *data, int len);
> > >  	void	(*dumpblk)(void __iomem *reg, int len);
> > > +
> > > +	/* platform init, if any */
> > > +	void 	(*platform_init)(void *);
> > >  };
> > >  
> > >  #endif /* __DM9000_PLATFORM_DATA */
> > > 
> > > 
> > > -
> > > To unsubscribe from this list: send the line "unsubscribe netdev" in
> > > the body of a message to majordomo@...r.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ben (ben@...ff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html