[PATCH] ks8851: Fix missing mutex_lock/unlock

[PATCH] ks8851: Fix missing mutex_lock/unlock

[mjr]

From: Matt Renzelmann <mjr@cs.wisc.edu>

All calls to ks8851_rdreg* and ks8851_wrreg* should be protected with
the driver's lock mutex.  A spurious interrupt may otherwise cause a
crash.  Thanks to Stephen Boyd, Flavio Leitner, and Ben Hutchings for
feedback.

Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
---

This modified version incorporates Ben Hutchings' bugfix by removing
the incorrect call to CIDER_REV_GET.

 drivers/net/ethernet/micrel/ks8851.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index c722aa6..e5dc075 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -1418,6 +1418,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 	struct net_device *ndev;
 	struct ks8851_net *ks;
 	int ret;
+	unsigned cider;
 
 	ndev = alloc_etherdev(sizeof(struct ks8851_net));
 	if (!ndev)
@@ -1484,8 +1485,8 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 	ks8851_soft_reset(ks, GRR_GSR);
 
 	/* simple check for a valid chip being connected to the bus */
-
-	if ((ks8851_rdreg16(ks, KS_CIDER) & ~CIDER_REV_MASK) != CIDER_ID) {
+	cider = ks8851_rdreg16(ks, KS_CIDER);
+	if ((cider & ~CIDER_REV_MASK) != CIDER_ID) {
 		dev_err(&spi->dev, "failed to read device ID\n");
 		ret = -ENODEV;
 		goto err_id;
@@ -1516,8 +1517,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 	}
 
 	netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
-		    CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
-		    ndev->dev_addr, ndev->irq,
+		    CIDER_REV_GET(cider), ndev->dev_addr, ndev->irq,
 		    ks->rc_ccr & CCR_EEPROM ? "has" : "no");
 
 	return 0;
-- 
1.7.5.4

Re: [PATCH] ks8851: Fix missing mutex_lock/unlock

[Flavio Leitner]

On Thu, 12 Apr 2012 18:15:30 -0500
mjr@cs.wisc.edu wrote:

> From: Matt Renzelmann <mjr@cs.wisc.edu>
> 
> All calls to ks8851_rdreg* and ks8851_wrreg* should be protected with
> the driver's lock mutex.  A spurious interrupt may otherwise cause a
> crash.  Thanks to Stephen Boyd, Flavio Leitner, and Ben Hutchings for
> feedback.
> 
> Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
> ---
> 
> This modified version incorporates Ben Hutchings' bugfix by removing
> the incorrect call to CIDER_REV_GET.
> 
>  drivers/net/ethernet/micrel/ks8851.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
> index c722aa6..e5dc075 100644
> --- a/drivers/net/ethernet/micrel/ks8851.c
> +++ b/drivers/net/ethernet/micrel/ks8851.c
> @@ -1418,6 +1418,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
>  	struct net_device *ndev;
>  	struct ks8851_net *ks;
>  	int ret;
> +	unsigned cider;


Basically what I have proposed, but I used 'unsigned int cider'.
Anyway, the driver uses just 'unsigned' everywhere.

Signed-off-by: Flavio Leitner <fbl@redhat.com>

Re: [PATCH] ks8851: Fix missing mutex_lock/unlock

[Stephen Boyd]

On 04/12/12 16:15, mjr@cs.wisc.edu wrote:
> From: Matt Renzelmann <mjr@cs.wisc.edu>
>
> All calls to ks8851_rdreg* and ks8851_wrreg* should be protected with
> the driver's lock mutex.  A spurious interrupt may otherwise cause a
> crash. 

This doesn't make any sense anymore because you don't do any mutex things.

>  Thanks to Stephen Boyd, Flavio Leitner, and Ben Hutchings for
> feedback.
>
> Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
> ---
>
> This modified version incorporates Ben Hutchings' bugfix by removing
> the incorrect call to CIDER_REV_GET.
>
>  drivers/net/ethernet/micrel/ks8851.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
> index c722aa6..e5dc075 100644
> --- a/drivers/net/ethernet/micrel/ks8851.c
> +++ b/drivers/net/ethernet/micrel/ks8851.c
> @@ -1418,6 +1418,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
>  	struct net_device *ndev;
>  	struct ks8851_net *ks;
>  	int ret;
> +	unsigned cider;
>  
>  	ndev = alloc_etherdev(sizeof(struct ks8851_net));
>  	if (!ndev)
> @@ -1484,8 +1485,8 @@ static int __devinit ks8851_probe(struct spi_device *spi)
>  	ks8851_soft_reset(ks, GRR_GSR);
>  
>  	/* simple check for a valid chip being connected to the bus */
> -
> -	if ((ks8851_rdreg16(ks, KS_CIDER) & ~CIDER_REV_MASK) != CIDER_ID) {
> +	cider = ks8851_rdreg16(ks, KS_CIDER);
> +	if ((cider & ~CIDER_REV_MASK) != CIDER_ID) {
>  		dev_err(&spi->dev, "failed to read device ID\n");
>  		ret = -ENODEV;
>  		goto err_id;
> @@ -1516,8 +1517,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
>  	}
>  
>  	netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
> -		    CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
> -		    ndev->dev_addr, ndev->irq,
> +		    CIDER_REV_GET(cider), ndev->dev_addr, ndev->irq,
>  		    ks->rc_ccr & CCR_EEPROM ? "has" : "no");
>  
>  	return 0;

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] ks8851: Fix missing mutex_lock/unlock

[David Miller]

From: mjr@cs.wisc.edu
Date: Fri, 13 Apr 2012 12:59:40 -0500

> From: Matt Renzelmann <mjr@cs.wisc.edu>
> 
> Move the ks8851_rdreg16 call above the call to request_irq and cache
> the result for subsequent repeated use.  A spurious interrupt may
> otherwise cause a crash.  Thanks to Stephen Boyd, Flavio Leitner, and
> Ben Hutchings for feedback.
> 
> Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>

Applied, thanks.

[PATCH] ks8851: Fix missing mutex_lock/unlock

[mjr]

From: Matt Renzelmann <mjr@cs.wisc.edu>

Move the ks8851_rdreg16 call above the call to request_irq and cache
the result for subsequent repeated use.  A spurious interrupt may
otherwise cause a crash.  Thanks to Stephen Boyd, Flavio Leitner, and
Ben Hutchings for feedback.

Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
---
 drivers/net/ethernet/micrel/ks8851.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index c722aa6..e5dc075 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -1418,6 +1418,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 	struct net_device *ndev;
 	struct ks8851_net *ks;
 	int ret;
+	unsigned cider;
 
 	ndev = alloc_etherdev(sizeof(struct ks8851_net));
 	if (!ndev)
@@ -1484,8 +1485,8 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 	ks8851_soft_reset(ks, GRR_GSR);
 
 	/* simple check for a valid chip being connected to the bus */
-
-	if ((ks8851_rdreg16(ks, KS_CIDER) & ~CIDER_REV_MASK) != CIDER_ID) {
+	cider = ks8851_rdreg16(ks, KS_CIDER);
+	if ((cider & ~CIDER_REV_MASK) != CIDER_ID) {
 		dev_err(&spi->dev, "failed to read device ID\n");
 		ret = -ENODEV;
 		goto err_id;
@@ -1516,8 +1517,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 	}
 
 	netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
-		    CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
-		    ndev->dev_addr, ndev->irq,
+		    CIDER_REV_GET(cider), ndev->dev_addr, ndev->irq,
 		    ks->rc_ccr & CCR_EEPROM ? "has" : "no");
 
 	return 0;
-- 
1.7.5.4

Re: [PATCH] ks8851: Fix missing mutex_lock/unlock

[Ben Hutchings]

On Thu, 2012-04-12 at 16:28 -0500, mjr@cs.wisc.edu wrote:
> From: Matt Renzelmann <mjr@cs.wisc.edu>
> 
> All calls to ks8851_rdreg* and ks8851_wrreg* should be protected with
> the driver's lock mutex.  A spurious interrupt may otherwise cause a
> crash.  Thanks to Stephen Boyd and Flavio Leitner for feedback.
> 
> Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
> ---
> 
> Here's the next revision.  This is basically Flavio Leitner's latest
> version without the mutex.  Please let me know if I'm missing
> anything and I can re-submit it.
> 
>  drivers/net/ethernet/micrel/ks8851.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
> index c722aa6..6528d66 100644
> --- a/drivers/net/ethernet/micrel/ks8851.c
> +++ b/drivers/net/ethernet/micrel/ks8851.c
> @@ -1418,6 +1418,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
>  	struct net_device *ndev;
>  	struct ks8851_net *ks;
>  	int ret;
> +	unsigned cider;
>  
>  	ndev = alloc_etherdev(sizeof(struct ks8851_net));
>  	if (!ndev)
> @@ -1484,8 +1485,8 @@ static int __devinit ks8851_probe(struct spi_device *spi)
>  	ks8851_soft_reset(ks, GRR_GSR);
>  
>  	/* simple check for a valid chip being connected to the bus */
> -
> -	if ((ks8851_rdreg16(ks, KS_CIDER) & ~CIDER_REV_MASK) != CIDER_ID) {
> +	cider = CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER));

So now 'cider' holds the revision bits.

> +	if ((cider & ~CIDER_REV_MASK) != CIDER_ID) {

But this is trying to check everything *but* the revision bits.

>  		dev_err(&spi->dev, "failed to read device ID\n");
>  		ret = -ENODEV;
>  		goto err_id;
> @@ -1516,8 +1517,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
>  	}
>  
>  	netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
> -		    CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
> -		    ndev->dev_addr, ndev->irq,
> +		    CIDER_REV_GET(cider), ndev->dev_addr, ndev->irq,

And this is trying to extract the revision bits, as if 'cider' holds the
full register value.

I think you need to remove the first CIDER_REV_GET().

Ben.

>  		    ks->rc_ccr & CCR_EEPROM ? "has" : "no");
>  
>  	return 0;

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH] ks8851: Fix missing mutex_lock/unlock

[mjr]

From: Matt Renzelmann <mjr@cs.wisc.edu>

All calls to ks8851_rdreg* and ks8851_wrreg* should be protected with
the driver's lock mutex.  A spurious interrupt may otherwise cause a
crash.  Thanks to Stephen Boyd and Flavio Leitner for feedback.

Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
---

Here's the next revision.  This is basically Flavio Leitner's latest
version without the mutex.  Please let me know if I'm missing
anything and I can re-submit it.

 drivers/net/ethernet/micrel/ks8851.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index c722aa6..6528d66 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -1418,6 +1418,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 	struct net_device *ndev;
 	struct ks8851_net *ks;
 	int ret;
+	unsigned cider;
 
 	ndev = alloc_etherdev(sizeof(struct ks8851_net));
 	if (!ndev)
@@ -1484,8 +1485,8 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 	ks8851_soft_reset(ks, GRR_GSR);
 
 	/* simple check for a valid chip being connected to the bus */
-
-	if ((ks8851_rdreg16(ks, KS_CIDER) & ~CIDER_REV_MASK) != CIDER_ID) {
+	cider = CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER));
+	if ((cider & ~CIDER_REV_MASK) != CIDER_ID) {
 		dev_err(&spi->dev, "failed to read device ID\n");
 		ret = -ENODEV;
 		goto err_id;
@@ -1516,8 +1517,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 	}
 
 	netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
-		    CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
-		    ndev->dev_addr, ndev->irq,
+		    CIDER_REV_GET(cider), ndev->dev_addr, ndev->irq,
 		    ks->rc_ccr & CCR_EEPROM ? "has" : "no");
 
 	return 0;
-- 
1.7.5.4

Re: [PATCH] ks8851: Fix missing mutex_lock/unlock

[Flavio Leitner]

On Thu, 12 Apr 2012 17:59:45 -0300
Flavio Leitner <fbl@redhat.com> wrote:

> On Thu, 12 Apr 2012 13:34:32 -0700
> Stephen Boyd <sboyd@codeaurora.org> wrote:
> 
> > On 04/12/12 13:06, mjr@cs.wisc.edu wrote:
> > > From: Matt Renzelmann <mjr@cs.wisc.edu>
> > >
> > > All calls to ks8851_rdreg* and ks8851_wrreg* should be protected with
> > > the driver's lock mutex.  A spurious interrupt may otherwise cause a
> > > crash.
> > >
> > > Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
> > > ---
> > >
> > > Thank you, Mr. Leitner, for providing feedback.  I agree with your
> > > changes and have updated the patch to reflect them.  I apologize for
> > > missing the driver name in the title -- I've updated the patch with
> > > that information as well.  Please let me know if there is anything
> > > else I should fix/change.
> > >
> > >  drivers/net/ethernet/micrel/ks8851.c |    8 ++++++--
> > >  1 files changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
> > > index c722aa6..20237dc 100644
> > > --- a/drivers/net/ethernet/micrel/ks8851.c
> > > +++ b/drivers/net/ethernet/micrel/ks8851.c
> > > @@ -1417,6 +1417,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
> > >  {
> > >  	struct net_device *ndev;
> > >  	struct ks8851_net *ks;
> > > +	int result;
> > >  	int ret;
> > >  
> > >  	ndev = alloc_etherdev(sizeof(struct ks8851_net));
> > > @@ -1515,9 +1516,12 @@ static int __devinit ks8851_probe(struct spi_device *spi)
> > >  		goto err_netdev;
> > >  	}
> > >  
> > > +	mutex_lock(&ks->lock);
> > > +	result = CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER));
> > > +	mutex_unlock(&ks->lock);
> > > +
> > >  	netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
> > > -		    CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
> > > -		    ndev->dev_addr, ndev->irq,
> > > +		    result, ndev->dev_addr, ndev->irq,
> > >  		    ks->rc_ccr & CCR_EEPROM ? "has" : "no");
> > >  
> > >  	return 0;
> > 
> > This register is already read in the probe function and the lock is not
> > held there so you seem to have missed a couple. I would guess it doesn't
> > really matter tha we don't grab the lock though because the device isn't
> > actively sending/receiving packets. How about this instead?
> 
> I believe that's because the IRQ isn't reserved yet, so there is no problem.

So, what about this instead:

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index c722aa6..7137f47 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -1417,6 +1417,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 {
 	struct net_device *ndev;
 	struct ks8851_net *ks;
+	unsigned int cider;
 	int ret;
 
 	ndev = alloc_etherdev(sizeof(struct ks8851_net));
@@ -1485,7 +1486,8 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 
 	/* simple check for a valid chip being connected to the bus */
 
-	if ((ks8851_rdreg16(ks, KS_CIDER) & ~CIDER_REV_MASK) != CIDER_ID) {
+	cider = ks8851_rdreg16(ks, KS_CIDER);
+	if ((cider & ~CIDER_REV_MASK) != CIDER_ID) {
 		dev_err(&spi->dev, "failed to read device ID\n");
 		ret = -ENODEV;
 		goto err_id;
@@ -1516,7 +1518,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 	}
 
 	netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
-		    CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
+		    CIDER_REV_GET(cider),
 		    ndev->dev_addr, ndev->irq,
 		    ks->rc_ccr & CCR_EEPROM ? "has" : "no");
 

fbl

Re: [PATCH] ks8851: Fix missing mutex_lock/unlock

[Flavio Leitner]

On Thu, 12 Apr 2012 13:34:32 -0700
Stephen Boyd <sboyd@codeaurora.org> wrote:

> On 04/12/12 13:06, mjr@cs.wisc.edu wrote:
> > From: Matt Renzelmann <mjr@cs.wisc.edu>
> >
> > All calls to ks8851_rdreg* and ks8851_wrreg* should be protected with
> > the driver's lock mutex.  A spurious interrupt may otherwise cause a
> > crash.
> >
> > Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
> > ---
> >
> > Thank you, Mr. Leitner, for providing feedback.  I agree with your
> > changes and have updated the patch to reflect them.  I apologize for
> > missing the driver name in the title -- I've updated the patch with
> > that information as well.  Please let me know if there is anything
> > else I should fix/change.
> >
> >  drivers/net/ethernet/micrel/ks8851.c |    8 ++++++--
> >  1 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
> > index c722aa6..20237dc 100644
> > --- a/drivers/net/ethernet/micrel/ks8851.c
> > +++ b/drivers/net/ethernet/micrel/ks8851.c
> > @@ -1417,6 +1417,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
> >  {
> >  	struct net_device *ndev;
> >  	struct ks8851_net *ks;
> > +	int result;
> >  	int ret;
> >  
> >  	ndev = alloc_etherdev(sizeof(struct ks8851_net));
> > @@ -1515,9 +1516,12 @@ static int __devinit ks8851_probe(struct spi_device *spi)
> >  		goto err_netdev;
> >  	}
> >  
> > +	mutex_lock(&ks->lock);
> > +	result = CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER));
> > +	mutex_unlock(&ks->lock);
> > +
> >  	netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
> > -		    CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
> > -		    ndev->dev_addr, ndev->irq,
> > +		    result, ndev->dev_addr, ndev->irq,
> >  		    ks->rc_ccr & CCR_EEPROM ? "has" : "no");
> >  
> >  	return 0;
> 
> This register is already read in the probe function and the lock is not
> held there so you seem to have missed a couple. I would guess it doesn't
> really matter tha we don't grab the lock though because the device isn't
> actively sending/receiving packets. How about this instead?

I believe that's because the IRQ isn't reserved yet, so there is no problem.


> diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
> index c722aa60..6f21fcd 100644
> --- a/drivers/net/ethernet/micrel/ks8851.c
> +++ b/drivers/net/ethernet/micrel/ks8851.c
> @@ -1418,6 +1418,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
>         struct net_device *ndev;
>         struct ks8851_net *ks;
>         int ret;
> +       unsigned cider;
>  
>         ndev = alloc_etherdev(sizeof(struct ks8851_net));
>         if (!ndev)
> @@ -1484,8 +1485,9 @@ static int __devinit ks8851_probe(struct spi_device *spi)
>         ks8851_soft_reset(ks, GRR_GSR);
>  
>         /* simple check for a valid chip being connected to the bus */
> -
> -       if ((ks8851_rdreg16(ks, KS_CIDER) & ~CIDER_REV_MASK) != CIDER_ID) {
> +       mutex_lock(&ks->lock);
> +       cider = ks8851_rdreg16(ks, KS_CIDER);
> +       if ((cider & ~CIDER_REV_MASK) != CIDER_ID) {
>                 dev_err(&spi->dev, "failed to read device ID\n");

If it fails here, the mutex is left locked.

>                 ret = -ENODEV;
>                 goto err_id;
> @@ -1493,6 +1495,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
>  
>         /* cache the contents of the CCR register for EEPROM, etc. */
>         ks->rc_ccr = ks8851_rdreg16(ks, KS_CCR);
> +       mutex_unlock(&ks->lock);
>  
>         if (ks->rc_ccr & CCR_EEPROM)
>                 ks->eeprom_size = 128;
> @@ -1516,8 +1519,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
>         }
>  
>         netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
> -                   CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
> -                   ndev->dev_addr, ndev->irq,
> +                   CIDER_REV_GET(cider), ndev->dev_addr, ndev->irq,
>                     ks->rc_ccr & CCR_EEPROM ? "has" : "no");
>  
>         return 0;

I don't know the hw specs, but assuming that revision doesn't change between
those reads or that reading it doesn't do anything else... the above looks
better, indeed.

fbl

RE: [PATCH] ks8851: Fix missing mutex_lock/unlock

[Matt Renzelmann]

> This register is already read in the probe function and the lock is not
> held there so you seem to have missed a couple. I would guess it doesn't
> really matter tha we don't grab the lock though because the device isn't
> actively sending/receiving packets. How about this instead?
> 

The reason I didn't go that way was the request_irq call is not made until near
the end of probe.  I believe only the ks8851_rdreg16 call in the netdev_info
statement actually needs the mutex protection.  That said, the approach you
posted looks reasonable as well and may be clearer.  I'm unsure as to which way
is best.

Thanks everyone for your help,
Matt

Re: [PATCH] ks8851: Fix missing mutex_lock/unlock

[Flavio Leitner]

On Thu, 12 Apr 2012 15:06:44 -0500
mjr@cs.wisc.edu wrote:

> From: Matt Renzelmann <mjr@cs.wisc.edu>
> 
> All calls to ks8851_rdreg* and ks8851_wrreg* should be protected with
> the driver's lock mutex.  A spurious interrupt may otherwise cause a
> crash.
> 
> Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
> ---
> 
> Thank you, Mr. Leitner, for providing feedback.  I agree with your
> changes and have updated the patch to reflect them.  I apologize for
> missing the driver name in the title -- I've updated the patch with
> that information as well.  Please let me know if there is anything
> else I should fix/change.

Signed-off-by: Flavio Leitner <fbl@redhat.com>

Re: [PATCH] ks8851: Fix missing mutex_lock/unlock

[Stephen Boyd]

On 04/12/12 13:06, mjr@cs.wisc.edu wrote:
> From: Matt Renzelmann <mjr@cs.wisc.edu>
>
> All calls to ks8851_rdreg* and ks8851_wrreg* should be protected with
> the driver's lock mutex.  A spurious interrupt may otherwise cause a
> crash.
>
> Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
> ---
>
> Thank you, Mr. Leitner, for providing feedback.  I agree with your
> changes and have updated the patch to reflect them.  I apologize for
> missing the driver name in the title -- I've updated the patch with
> that information as well.  Please let me know if there is anything
> else I should fix/change.
>
>  drivers/net/ethernet/micrel/ks8851.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
> index c722aa6..20237dc 100644
> --- a/drivers/net/ethernet/micrel/ks8851.c
> +++ b/drivers/net/ethernet/micrel/ks8851.c
> @@ -1417,6 +1417,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
>  {
>  	struct net_device *ndev;
>  	struct ks8851_net *ks;
> +	int result;
>  	int ret;
>  
>  	ndev = alloc_etherdev(sizeof(struct ks8851_net));
> @@ -1515,9 +1516,12 @@ static int __devinit ks8851_probe(struct spi_device *spi)
>  		goto err_netdev;
>  	}
>  
> +	mutex_lock(&ks->lock);
> +	result = CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER));
> +	mutex_unlock(&ks->lock);
> +
>  	netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
> -		    CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
> -		    ndev->dev_addr, ndev->irq,
> +		    result, ndev->dev_addr, ndev->irq,
>  		    ks->rc_ccr & CCR_EEPROM ? "has" : "no");
>  
>  	return 0;

This register is already read in the probe function and the lock is not
held there so you seem to have missed a couple. I would guess it doesn't
really matter tha we don't grab the lock though because the device isn't
actively sending/receiving packets. How about this instead?

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index c722aa60..6f21fcd 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -1418,6 +1418,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
        struct net_device *ndev;
        struct ks8851_net *ks;
        int ret;
+       unsigned cider;
 
        ndev = alloc_etherdev(sizeof(struct ks8851_net));
        if (!ndev)
@@ -1484,8 +1485,9 @@ static int __devinit ks8851_probe(struct spi_device *spi)
        ks8851_soft_reset(ks, GRR_GSR);
 
        /* simple check for a valid chip being connected to the bus */
-
-       if ((ks8851_rdreg16(ks, KS_CIDER) & ~CIDER_REV_MASK) != CIDER_ID) {
+       mutex_lock(&ks->lock);
+       cider = ks8851_rdreg16(ks, KS_CIDER);
+       if ((cider & ~CIDER_REV_MASK) != CIDER_ID) {
                dev_err(&spi->dev, "failed to read device ID\n");
                ret = -ENODEV;
                goto err_id;
@@ -1493,6 +1495,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 
        /* cache the contents of the CCR register for EEPROM, etc. */
        ks->rc_ccr = ks8851_rdreg16(ks, KS_CCR);
+       mutex_unlock(&ks->lock);
 
        if (ks->rc_ccr & CCR_EEPROM)
                ks->eeprom_size = 128;
@@ -1516,8 +1519,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
        }
 
        netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
-                   CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
-                   ndev->dev_addr, ndev->irq,
+                   CIDER_REV_GET(cider), ndev->dev_addr, ndev->irq,
                    ks->rc_ccr & CCR_EEPROM ? "has" : "no");
 
        return 0;


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] ks8851: Fix missing mutex_lock/unlock

[mjr]

From: Matt Renzelmann <mjr@cs.wisc.edu>

All calls to ks8851_rdreg* and ks8851_wrreg* should be protected with
the driver's lock mutex.  A spurious interrupt may otherwise cause a
crash.

Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
---

Thank you, Mr. Leitner, for providing feedback.  I agree with your
changes and have updated the patch to reflect them.  I apologize for
missing the driver name in the title -- I've updated the patch with
that information as well.  Please let me know if there is anything
else I should fix/change.

 drivers/net/ethernet/micrel/ks8851.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index c722aa6..20237dc 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -1417,6 +1417,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 {
 	struct net_device *ndev;
 	struct ks8851_net *ks;
+	int result;
 	int ret;
 
 	ndev = alloc_etherdev(sizeof(struct ks8851_net));
@@ -1515,9 +1516,12 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 		goto err_netdev;
 	}
 
+	mutex_lock(&ks->lock);
+	result = CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER));
+	mutex_unlock(&ks->lock);
+
 	netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
-		    CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
-		    ndev->dev_addr, ndev->irq,
+		    result, ndev->dev_addr, ndev->irq,
 		    ks->rc_ccr & CCR_EEPROM ? "has" : "no");
 
 	return 0;
-- 
1.7.5.4