* Re: [PATCH] Fix missing mutex_lock/unlock
2012-04-12 15:26 [PATCH] Fix missing mutex_lock/unlock mjr
@ 2012-04-12 19:23 ` Flavio Leitner
0 siblings, 0 replies; 2+ messages in thread
From: Flavio Leitner @ 2012-04-12 19:23 UTC (permalink / raw)
To: mjr; +Cc: davem, sboyd, ben, netdev
On Thu, 12 Apr 2012 10:26:36 -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>
> ---
> Hello,
>
> I'm new to the kernel development process so I hope I've not screwed
> this up with this extra text. We found a potential issue using a new
> driver testing tool called SymDrive. It looks legitimate to me, so
> I'm reporting it. We hope to make this tool available in the future.
> Please let me know if I should modify the patch or re-send without
> this commentary. Thanks in advance for your patience.
>
It is recommended to put the driver name in the subject,
for instance:
[PATCH] ks8851: Fix missing mutex_lock/unlock
See the driver log for more examples.
> drivers/net/ethernet/micrel/ks8851.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
> index c722aa6..fa2001a 100644
> --- a/drivers/net/ethernet/micrel/ks8851.c
> +++ b/drivers/net/ethernet/micrel/ks8851.c
> @@ -1515,11 +1515,15 @@ static int __devinit ks8851_probe(struct spi_device *spi)
> goto err_netdev;
> }
>
> + mutex_lock(&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,
> ks->rc_ccr & CCR_EEPROM ? "has" : "no");
>
> + mutex_unlock(&ks->lock);
> +
> return 0;
It's weird to look a mutex being hold to call netdev_info().
Perhaps change it to look like below as the netdev_info()
does a lot more things and holding the lock during that
seems to be a waste.
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;
fbl
^ permalink raw reply related [flat|nested] 2+ messages in thread