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

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

[dmitry pervushin]

Hey all,

The patch below is intended to fix two problems:
- trying to acquire spinlock twice on timeout condition
- create callback than can be used by platform code to initialize the
chip

Signed-off-by: dmitry pervushin <dpervushin@gmail.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 */

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

[Ben Dooks]

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.

> - 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 *' ?

This should have also been two seperate patches as there are two
different issues.
 
> Signed-off-by: dmitry pervushin <dpervushin@gmail.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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

  'a smiley only costs 4 bytes'

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

[dmitry pervushin]

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
> > - 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.

> This should have also been two seperate patches as there are two
> different issues.
>  
> > Signed-off-by: dmitry pervushin <dpervushin@gmail.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@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Ben Dooks]

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@gmail.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@vger.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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

  'a smiley only costs 4 bytes'