Subject: Re: [PATCH] dm9000: fix spinlock issue and introduce platform_init callback

From: Ben Dooks <[email-masked]>
Date: Tue, 20 Nov 2007 15:07:19 +0000

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 <[email-masked]>
> > > 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 [email-masked]
> > > 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 [email-masked]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ben ([email-masked], http://www.fluff.org/)

  'a smiley only costs 4 bytes'