* [PATCH] ks8851: Fix missing mutex_lock/unlock
@ 2012-04-12 20:06 mjr
2012-04-12 20:34 ` Stephen Boyd
2012-04-12 20:40 ` Flavio Leitner
0 siblings, 2 replies; 13+ messages in thread
From: mjr @ 2012-04-12 20:06 UTC (permalink / raw)
To: fbl; +Cc: davem, sboyd, ben, netdev, Matt Renzelmann
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] ks8851: Fix missing mutex_lock/unlock
2012-04-12 20:06 [PATCH] ks8851: Fix missing mutex_lock/unlock mjr
@ 2012-04-12 20:34 ` Stephen Boyd
2012-04-12 20:56 ` Matt Renzelmann
2012-04-12 20:59 ` Flavio Leitner
2012-04-12 20:40 ` Flavio Leitner
1 sibling, 2 replies; 13+ messages in thread
From: Stephen Boyd @ 2012-04-12 20:34 UTC (permalink / raw)
To: mjr; +Cc: fbl, davem, ben, netdev
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.
^ permalink raw reply related [flat|nested] 13+ messages in thread* RE: [PATCH] ks8851: Fix missing mutex_lock/unlock
2012-04-12 20:34 ` Stephen Boyd
@ 2012-04-12 20:56 ` Matt Renzelmann
2012-04-12 20:59 ` Flavio Leitner
1 sibling, 0 replies; 13+ messages in thread
From: Matt Renzelmann @ 2012-04-12 20:56 UTC (permalink / raw)
To: 'Stephen Boyd', mjr; +Cc: fbl, davem, ben, netdev
> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ks8851: Fix missing mutex_lock/unlock
2012-04-12 20:34 ` Stephen Boyd
2012-04-12 20:56 ` Matt Renzelmann
@ 2012-04-12 20:59 ` Flavio Leitner
2012-04-12 21:19 ` Flavio Leitner
1 sibling, 1 reply; 13+ messages in thread
From: Flavio Leitner @ 2012-04-12 20:59 UTC (permalink / raw)
To: Stephen Boyd; +Cc: mjr, davem, ben, netdev
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] ks8851: Fix missing mutex_lock/unlock
2012-04-12 20:59 ` Flavio Leitner
@ 2012-04-12 21:19 ` Flavio Leitner
0 siblings, 0 replies; 13+ messages in thread
From: Flavio Leitner @ 2012-04-12 21:19 UTC (permalink / raw)
To: Flavio Leitner; +Cc: Stephen Boyd, mjr, davem, ben, netdev
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] ks8851: Fix missing mutex_lock/unlock
2012-04-12 20:06 [PATCH] ks8851: Fix missing mutex_lock/unlock mjr
2012-04-12 20:34 ` Stephen Boyd
@ 2012-04-12 20:40 ` Flavio Leitner
1 sibling, 0 replies; 13+ messages in thread
From: Flavio Leitner @ 2012-04-12 20:40 UTC (permalink / raw)
To: mjr; +Cc: davem, sboyd, ben, netdev
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>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] ks8851: Fix missing mutex_lock/unlock
@ 2012-04-12 21:28 mjr
2012-04-12 22:03 ` Ben Hutchings
0 siblings, 1 reply; 13+ messages in thread
From: mjr @ 2012-04-12 21:28 UTC (permalink / raw)
To: fbl; +Cc: davem, sboyd, ben, netdev, Matt Renzelmann
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] ks8851: Fix missing mutex_lock/unlock
2012-04-12 21:28 mjr
@ 2012-04-12 22:03 ` Ben Hutchings
0 siblings, 0 replies; 13+ messages in thread
From: Ben Hutchings @ 2012-04-12 22:03 UTC (permalink / raw)
To: mjr; +Cc: fbl, davem, sboyd, ben, netdev
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] ks8851: Fix missing mutex_lock/unlock
@ 2012-04-12 23:15 mjr
2012-04-13 0:22 ` Flavio Leitner
2012-04-13 17:46 ` Stephen Boyd
0 siblings, 2 replies; 13+ messages in thread
From: mjr @ 2012-04-12 23:15 UTC (permalink / raw)
To: fbl; +Cc: davem, sboyd, ben, bhutchings, netdev, Matt Renzelmann
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] ks8851: Fix missing mutex_lock/unlock
2012-04-12 23:15 mjr
@ 2012-04-13 0:22 ` Flavio Leitner
2012-04-13 17:46 ` Stephen Boyd
1 sibling, 0 replies; 13+ messages in thread
From: Flavio Leitner @ 2012-04-13 0:22 UTC (permalink / raw)
To: mjr; +Cc: davem, sboyd, ben, bhutchings, netdev
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>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ks8851: Fix missing mutex_lock/unlock
2012-04-12 23:15 mjr
2012-04-13 0:22 ` Flavio Leitner
@ 2012-04-13 17:46 ` Stephen Boyd
1 sibling, 0 replies; 13+ messages in thread
From: Stephen Boyd @ 2012-04-13 17:46 UTC (permalink / raw)
To: mjr; +Cc: fbl, davem, Ben Dooks, bhutchings, netdev
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] ks8851: Fix missing mutex_lock/unlock
@ 2012-04-13 17:59 mjr
2012-04-13 18:05 ` David Miller
0 siblings, 1 reply; 13+ messages in thread
From: mjr @ 2012-04-13 17:59 UTC (permalink / raw)
To: fbl; +Cc: davem, sboyd, ben, bhutchings, netdev, Matt Renzelmann
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] ks8851: Fix missing mutex_lock/unlock
2012-04-13 17:59 mjr
@ 2012-04-13 18:05 ` David Miller
0 siblings, 0 replies; 13+ messages in thread
From: David Miller @ 2012-04-13 18:05 UTC (permalink / raw)
To: mjr; +Cc: fbl, sboyd, ben, bhutchings, netdev
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2012-04-13 18:07 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-12 20:06 [PATCH] ks8851: Fix missing mutex_lock/unlock mjr
2012-04-12 20:34 ` Stephen Boyd
2012-04-12 20:56 ` Matt Renzelmann
2012-04-12 20:59 ` Flavio Leitner
2012-04-12 21:19 ` Flavio Leitner
2012-04-12 20:40 ` Flavio Leitner
2012-04-12 21:28 mjr
2012-04-12 22:03 ` Ben Hutchings
2012-04-12 23:15 mjr
2012-04-13 0:22 ` Flavio Leitner
2012-04-13 17:46 ` Stephen Boyd
2012-04-13 17:59 mjr
2012-04-13 18:05 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.