From patchwork Thu Apr 12 19:23:19 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Flavio Leitner X-Patchwork-Id: 152165 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 8232CB708E for ; Fri, 13 Apr 2012 05:23:37 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756486Ab2DLTXf (ORCPT ); Thu, 12 Apr 2012 15:23:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:5685 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754139Ab2DLTXe (ORCPT ); Thu, 12 Apr 2012 15:23:34 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q3CJNQsw027393 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 12 Apr 2012 15:23:26 -0400 Received: from asterix.rh (ovpn-113-134.phx2.redhat.com [10.3.113.134]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q3CJNL2V008793; Thu, 12 Apr 2012 15:23:22 -0400 Date: Thu, 12 Apr 2012 16:23:19 -0300 From: Flavio Leitner To: mjr@cs.wisc.edu Cc: davem@davemloft.net, sboyd@codeaurora.org, ben@simtec.co.uk, netdev@vger.kernel.org Subject: Re: [PATCH] Fix missing mutex_lock/unlock Message-ID: <20120412162319.377ae3cf@asterix.rh> In-Reply-To: <1334244396-6978-1-git-send-email-mjr@cs.wisc.edu> References: <1334244396-6978-1-git-send-email-mjr@cs.wisc.edu> Mime-Version: 1.0 X-Scanned-By: MIMEDefang 2.67 on 10.5.11.12 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Thu, 12 Apr 2012 10:26:36 -0500 mjr@cs.wisc.edu wrote: > From: Matt Renzelmann > > 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 > --- > 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. fbl --- 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 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;