| Message ID | 20221206073511.4772-3-Divya.Koppera@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fixed warnings | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 10 of 10 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 27 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
> 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;
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(-)