Message ID | 20221206073511.4772-3-Divya.Koppera@microchip.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:f944:0:0:0:0:0 with SMTP id q4csp2683124wrr; Mon, 5 Dec 2022 23:46:22 -0800 (PST) X-Google-Smtp-Source: AA0mqf5xYIDYY7GfIIci3aTo3WA+PSvsL0fRxK6YFji60/fcTIzCGJ29MyZemfs8iwbLG1MJOm2v X-Received: by 2002:a17:902:bc86:b0:187:282c:9b95 with SMTP id bb6-20020a170902bc8600b00187282c9b95mr69785313plb.41.1670312782469; Mon, 05 Dec 2022 23:46:22 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1670312782; cv=none; d=google.com; s=arc-20160816; b=PXWBP4YJXqhRIcUK6eSy4R0eK70ciDVzp0QSvpIdkQcdy1RUrnmGgBbyre5JzLtldP MX6QcRpkSQZldNIcg+c1pB6QcuFeeeY+JsbJf7yOoHhSdYvDN+xHBRZY/93nxYo/8ujO 07oGfFUxTADb/5uD5Dd5Jo7wtYpW+9gyIhdLjD5S2AIMaUV922fiV/P1MosXkY1jf5oN ooTaqgK4p3laa3TDI0sDeedMC0U67iwL9kcUZbMPKHmTL7OHEHcVI4Xzuh2RWy6mjgyF l4NktgedT9F5/EzGAr/GC57y80M9I+jGwGfUsWZru+ecRNvcoKmtQjjCkhy3RX1OcXok /CXg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:mime-version:references:in-reply-to:message-id :date:subject:cc:to:from:dkim-signature; bh=Q4dMkcABuoFl1WHhQi9d9dBt7jZZ2okUKugN5eUgbX4=; b=wqiGYABWNTq8plZmJyOwiTELIGC8MLN+cBDC59DbrIOMMW/piN1YW47WvAlrLFCHQf s+KYGqe6kT6U4lRZETte6AOcr4pHqLEEq+5eEhQ49SM0CnPqdwF/oqW+GESupXeve2GS +0EJzU8TsFPfzJ2vvsG5QJX6bYJ0WzUTTGPFwRK786NV6pyoMPFwxUdvInk4Yxdu2J18 7h8u/JuvGr0S0br6CuSbSBBymun+qgh3iF/aOi3/yo/jKkxQm4wqAulOK8LsjK4Ve51n gjHWxYd19uYWZbg4Yquw2jqerLATPAy2/IoKo46ZKSBe/iwBPXBEdMvLaYgi0lP1Mwwp M2zw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@microchip.com header.s=mchp header.b=OBVKaOaa; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=microchip.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id pi15-20020a17090b1e4f00b0020d4dc7fa97si17726119pjb.110.2022.12.05.23.46.09; Mon, 05 Dec 2022 23:46:22 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@microchip.com header.s=mchp header.b=OBVKaOaa; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=microchip.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231771AbiLFHfb (ORCPT <rfc822;jaysivo@gmail.com> + 99 others); Tue, 6 Dec 2022 02:35:31 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56628 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231259AbiLFHf1 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 6 Dec 2022 02:35:27 -0500 Received: from esa.microchip.iphmx.com (esa.microchip.iphmx.com [68.232.153.233]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 48D5513F4D; Mon, 5 Dec 2022 23:35:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=microchip.com; i=@microchip.com; q=dns/txt; s=mchp; t=1670312126; x=1701848126; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version; bh=CJ9T+KjJEZq//Ndg3Z1vr1PdPPLfD+21dYJxtCGFGZs=; b=OBVKaOaayl6gxTiP9DYV4LuhvkxDt74vPPzbezXC5kH5xWsjEVa5KjqG 1w1f0DBQIf+8WZqpZP2BVmy8GTtWSRnhb/b+6josYz4mlxNOiCnMuGmnP /059aUfv5xE1RTIzd6Msj0xdZaXVAl/QTXssoETd1IRv9CaOmkyVbluyV dFwZsQ5IgvV0iyq1CJTMIlFY+Z5ksJN93ligFM3oVZ1n4fNXtx6j/bS8O 9VxtnBJwG1bFYoQbQYtQm3WR+2ya4k26jAkKdSaGSxWf4sYMgWoQV/hr2 ABKV/L0dTHogiceFSHywTEoXxFWanrNY9b08vaGDHpEEOYhd3t6lhBZVM A==; X-IronPort-AV: E=Sophos;i="5.96,220,1665471600"; d="scan'208";a="191865242" Received: from unknown (HELO email.microchip.com) ([170.129.1.10]) by esa3.microchip.iphmx.com with ESMTP/TLS/AES256-SHA256; 06 Dec 2022 00:35:25 -0700 Received: from chn-vm-ex04.mchp-main.com (10.10.85.152) by chn-vm-ex02.mchp-main.com (10.10.85.144) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.12; Tue, 6 Dec 2022 00:35:25 -0700 Received: from training-HP-280-G1-MT-PC.microchip.com (10.10.115.15) by chn-vm-ex04.mchp-main.com (10.10.85.152) with Microsoft SMTP Server id 15.1.2507.12 via Frontend Transport; Tue, 6 Dec 2022 00:35:21 -0700 From: Divya Koppera <Divya.Koppera@microchip.com> To: <andrew@lunn.ch>, <hkallweit1@gmail.com>, <linux@armlinux.org.uk>, <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>, <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <richardcochran@gmail.com> CC: <UNGLinuxDriver@microchip.com> Subject: [PATCH v5 net-next 2/2] net: phy: micrel: Fix warn: passing zero to PTR_ERR Date: Tue, 6 Dec 2022 13:05:11 +0530 Message-ID: <20221206073511.4772-3-Divya.Koppera@microchip.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20221206073511.4772-1-Divya.Koppera@microchip.com> References: <20221206073511.4772-1-Divya.Koppera@microchip.com> MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1751449896202232228?= X-GMAIL-MSGID: =?utf-8?q?1751449896202232228?= |
Series |
Fixed warnings
|
|
Commit Message
Divya Koppera
Dec. 6, 2022, 7:35 a.m. UTC
Handle the NULL pointer case
Fixes New smatch warnings:
drivers/net/phy/micrel.c:2613 lan8814_ptp_probe_once() warn: passing zero to 'PTR_ERR'
vim +/PTR_ERR +2613 drivers/net/phy/micrel.c
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Fixes: ece19502834d ("net: phy: micrel: 1588 support for LAN8814 phy")
Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com>
---
v4 -> v5:
- Removed run time check and added compile time check for PHC
v3 -> v4:
- Split the patch for different warnings
- Renamed variable from shared_priv to shared.
v2 -> v3:
- Changed subject line from net to net-next
- Removed config check for ptp and clock configuration
instead added null check for ptp_clock
- Fixed one more warning related to initialisaton.
v1 -> v2:
- Handled NULL pointer case
- Changed subject line with net-next to net
---
drivers/net/phy/micrel.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
Comments
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > index 1bcdb828db56..650ef53fcf20 100644 > --- a/drivers/net/phy/micrel.c > +++ b/drivers/net/phy/micrel.c > @@ -3017,10 +3017,6 @@ static int lan8814_ptp_probe_once(struct phy_device *phydev) > { > struct lan8814_shared_priv *shared = phydev->shared->priv; > > - if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) || > - !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING)) > - return 0; > - Why are you removing this ? Andrew
Hi Andrew, > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Tuesday, December 6, 2022 6:38 PM > To: Divya Koppera - I30481 <Divya.Koppera@microchip.com> > Cc: hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > richardcochran@gmail.com; UNGLinuxDriver > <UNGLinuxDriver@microchip.com> > Subject: Re: [PATCH v5 net-next 2/2] net: phy: micrel: Fix warn: passing zero > to PTR_ERR > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index > > 1bcdb828db56..650ef53fcf20 100644 > > --- a/drivers/net/phy/micrel.c > > +++ b/drivers/net/phy/micrel.c > > @@ -3017,10 +3017,6 @@ static int lan8814_ptp_probe_once(struct > > phy_device *phydev) { > > struct lan8814_shared_priv *shared = phydev->shared->priv; > > > > - if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) || > > - !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING)) > > - return 0; > > - > > Why are you removing this ? > I got review comment from Richard in v2 as below, making it as consistent by checking ptp_clock. So removed it in next revision. " > static int lan8814_ptp_probe_once(struct phy_device *phydev) > { > struct lan8814_shared_priv *shared = phydev->shared->priv; > > if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) || > !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING)) > return 0; It is weird to use macros here, but not before calling ptp_clock_register. Make it consistent by checking shared->ptp_clock instead. That is also better form." > Andrew
> > > - if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) || > > > - !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING)) > > > - return 0; > > > - > > > > Why are you removing this ? > > > > I got review comment from Richard in v2 as below, making it as consistent by checking ptp_clock. So removed it in next revision. > > " > static int lan8814_ptp_probe_once(struct phy_device *phydev) > > { > > struct lan8814_shared_priv *shared = phydev->shared->priv; > > > > if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) || > > !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING)) > > return 0; > > It is weird to use macros here, but not before calling ptp_clock_register. > Make it consistent by checking shared->ptp_clock instead. > That is also better form." O.K. If Richard said this fine. Just out of interest, could you disassemble lan8814_ptp_probe_once() when CONFIG_PTP_1588_CLOCK is disabled, with and without this check? My guess is, when PTP is disabled, the mutex still gets initialised, all the member of shared->ptp_clock_info are set. The optimise cannot remove it. With the macro check, the function is empty. So you end up with a slightly bigger text size. Andrew
On Tue, Dec 06, 2022 at 03:08:59PM +0100, Andrew Lunn wrote: > > > > - if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) || > > > > - !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING)) > > > > - return 0; > > > > - > > > > > > Why are you removing this ? > > > > > > > I got review comment from Richard in v2 as below, making it as consistent by checking ptp_clock. So removed it in next revision. > > > > " > static int lan8814_ptp_probe_once(struct phy_device *phydev) > > > { > > > struct lan8814_shared_priv *shared = phydev->shared->priv; > > > > > > if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) || > > > !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING)) > > > return 0; > > > > It is weird to use macros here, but not before calling ptp_clock_register. > > Make it consistent by checking shared->ptp_clock instead. > > That is also better form." > > O.K. If Richard said this fine. I just want to avoid drivers will #ifdef|IS_ENABLED all over the place. Thanks, Richard
Hi Andrew, > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Tuesday, December 6, 2022 7:39 PM > To: Divya Koppera - I30481 <Divya.Koppera@microchip.com> > Cc: hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > richardcochran@gmail.com; UNGLinuxDriver > <UNGLinuxDriver@microchip.com> > Subject: Re: [PATCH v5 net-next 2/2] net: phy: micrel: Fix warn: passing zero > to PTR_ERR > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > > > > - if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) || > > > > - !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING)) > > > > - return 0; > > > > - > > > > > > Why are you removing this ? > > > > > > > I got review comment from Richard in v2 as below, making it as consistent > by checking ptp_clock. So removed it in next revision. > > > > " > static int lan8814_ptp_probe_once(struct phy_device *phydev) > > > { > > > struct lan8814_shared_priv *shared = phydev->shared->priv; > > > > > > if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) || > > > !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING)) > > > return 0; > > > > It is weird to use macros here, but not before calling ptp_clock_register. > > Make it consistent by checking shared->ptp_clock instead. > > That is also better form." > > O.K. If Richard said this fine. > > Just out of interest, could you disassemble lan8814_ptp_probe_once() when > CONFIG_PTP_1588_CLOCK is disabled, with and without this check? > If I understand correctly, With (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) check, initialization of mutex and members of shared->ptp_clock_info need to be done in first function. Without above check ptp_clock_register should be done in second function. Correct me if I'm wrong. In this case, if first function is bypassed because of clock disable config, no need to go to second function. Could you please check below solution, if that works fine? > My guess is, when PTP is disabled, the mutex still gets initialised, all the > member of shared->ptp_clock_info are set. The optimise cannot remove it. > With the macro check, the function is empty. So you end up with a slightly > bigger text size. > If that is the case, I'll keep the CONFIG_PTP_1588_CLOCK disabled check before calling lan8814_ptp_probe_once. Next thing is if CONFIG_PTP_1588_CLOCK disabled check pass then ptp_clock_register will never return null because we are bypassing hardware clock check before calling function itself. So, I can remove ptp_clock null check too. Let me know if this works, I'll do changes in next revision. > Andrew
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > > content is safe > > > > > > > - if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) || > > > > > - !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING)) > > > > > - return 0; > > > > > - > > > > > > > > Why are you removing this ? > > > > > > > > > > I got review comment from Richard in v2 as below, making it as consistent > > by checking ptp_clock. So removed it in next revision. > > > > > > " > static int lan8814_ptp_probe_once(struct phy_device *phydev) > > > > { > > > > struct lan8814_shared_priv *shared = phydev->shared->priv; > > > > > > > > if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) || > > > > !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING)) > > > > return 0; > > > > > > It is weird to use macros here, but not before calling ptp_clock_register. > > > Make it consistent by checking shared->ptp_clock instead. > > > That is also better form." > > > > O.K. If Richard said this fine. Since Richard wants this removed, i would just remove it. The object code saving is probably not much. Andrew
Hi Andrew, > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Monday, December 12, 2022 3:42 PM > To: Divya Koppera - I30481 <Divya.Koppera@microchip.com> > Cc: hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > richardcochran@gmail.com; UNGLinuxDriver > <UNGLinuxDriver@microchip.com> > Subject: Re: [PATCH v5 net-next 2/2] net: phy: micrel: Fix warn: passing zero > to PTR_ERR > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > > know the content is safe > > > > > > > > > - if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) || > > > > > > - !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING)) > > > > > > - return 0; > > > > > > - > > > > > > > > > > Why are you removing this ? > > > > > > > > > > > > > I got review comment from Richard in v2 as below, making it as > > > > consistent > > > by checking ptp_clock. So removed it in next revision. > > > > > > > > " > static int lan8814_ptp_probe_once(struct phy_device *phydev) > > > > > { > > > > > struct lan8814_shared_priv *shared = > > > > > phydev->shared->priv; > > > > > > > > > > if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) || > > > > > !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING)) > > > > > return 0; > > > > > > > > It is weird to use macros here, but not before calling ptp_clock_register. > > > > Make it consistent by checking shared->ptp_clock instead. > > > > That is also better form." > > > > > > O.K. If Richard said this fine. > > Since Richard wants this removed, i would just remove it. The object code > saving is probably not much. Okay, then I'll resend the patch. > > Andrew
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index 1bcdb828db56..650ef53fcf20 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -3017,10 +3017,6 @@ static int lan8814_ptp_probe_once(struct phy_device *phydev) { struct lan8814_shared_priv *shared = phydev->shared->priv; - if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) || - !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING)) - return 0; - /* Initialise shared lock for clock*/ mutex_init(&shared->shared_lock); @@ -3040,12 +3036,16 @@ static int lan8814_ptp_probe_once(struct phy_device *phydev) shared->ptp_clock = ptp_clock_register(&shared->ptp_clock_info, &phydev->mdio.dev); - if (IS_ERR_OR_NULL(shared->ptp_clock)) { + if (IS_ERR(shared->ptp_clock)) { phydev_err(phydev, "ptp_clock_register failed %lu\n", PTR_ERR(shared->ptp_clock)); return -EINVAL; } + /* Check if PHC support is missing at the configuration level */ + if (!shared->ptp_clock) + return 0; + phydev_dbg(phydev, "successfully registered ptp clock\n"); shared->phydev = phydev;