[PATCH v5 net-next 0/2] Fixed warnings

[PATCH v5 net-next 0/2] Fixed warnings

[Divya Koppera]

Fixed warnings related to PTR_ERR and initialization.

Divya Koppera (2):
  net: phy: micrel: Fixed error related to uninitialized symbol ret
  net: phy: micrel: Fix warn: passing zero to PTR_ERR

 drivers/net/phy/micrel.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH v5 net-next 1/2] net: phy: micrel: Fixed error related to uninitialized symbol ret

[Divya Koppera]

Initialized return variable

Fixes Old smatch warnings:
drivers/net/phy/micrel.c:1750 ksz886x_cable_test_get_status() error:
uninitialized symbol 'ret'.

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Fixes: 21b688dabecb ("net: phy: micrel: Cable Diag feature for lan8814 phy")
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com>
---
v4 -> v5:
- No changes, added reviewed by tag.

v3 -> v4:
- Split the patch for different warnings.

v1 -> v3:
- No changes
---
 drivers/net/phy/micrel.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 26ce0c5defcd..1bcdb828db56 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -2088,7 +2088,8 @@ static int ksz886x_cable_test_get_status(struct phy_device *phydev,
 	const struct kszphy_type *type = phydev->drv->driver_data;
 	unsigned long pair_mask = type->pair_mask;
 	int retries = 20;
-	int pair, ret;
+	int ret = 0;
+	int pair;
 
 	*finished = false;
 
-- 
2.17.1

Re: [PATCH v5 net-next 1/2] net: phy: micrel: Fixed error related to uninitialized symbol ret

[Jiri Pirko]

Tue, Dec 06, 2022 at 08:35:10AM CET, Divya.Koppera@microchip.com wrote:
>Initialized return variable

Not only here, but in the patch/patchset subject, you should be
imperative to the codebase and tell it what to do.


>
>Fixes Old smatch warnings:
>drivers/net/phy/micrel.c:1750 ksz886x_cable_test_get_status() error:
>uninitialized symbol 'ret'.
>
>Reported-by: kernel test robot <lkp@intel.com>
>Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>Fixes: 21b688dabecb ("net: phy: micrel: Cable Diag feature for lan8814 phy")
>Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com>
>---
>v4 -> v5:
>- No changes, added reviewed by tag.
>
>v3 -> v4:
>- Split the patch for different warnings.
>
>v1 -> v3:
>- No changes
>---
> drivers/net/phy/micrel.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
>index 26ce0c5defcd..1bcdb828db56 100644
>--- a/drivers/net/phy/micrel.c
>+++ b/drivers/net/phy/micrel.c
>@@ -2088,7 +2088,8 @@ static int ksz886x_cable_test_get_status(struct phy_device *phydev,
> 	const struct kszphy_type *type = phydev->drv->driver_data;
> 	unsigned long pair_mask = type->pair_mask;
> 	int retries = 20;
>-	int pair, ret;
>+	int ret = 0;
>+	int pair;
> 
> 	*finished = false;
> 
>-- 
>2.17.1
>

[PATCH v5 net-next 2/2] net: phy: micrel: Fix warn: passing zero to PTR_ERR

[Divya Koppera]

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(-)

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;
-- 
2.17.1

Re: [PATCH v5 net-next 2/2] net: phy: micrel: Fix warn: passing zero to PTR_ERR

[Andrew Lunn]

> 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

RE: [PATCH v5 net-next 2/2] net: phy: micrel: Fix warn: passing zero to PTR_ERR

[Divya.Koppera]

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

Re: [PATCH v5 net-next 2/2] net: phy: micrel: Fix warn: passing zero to PTR_ERR

[Andrew Lunn]

> > > -     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

Re: [PATCH v5 net-next 2/2] net: phy: micrel: Fix warn: passing zero to PTR_ERR

[Richard Cochran]

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

RE: [PATCH v5 net-next 2/2] net: phy: micrel: Fix warn: passing zero to PTR_ERR

[Divya.Koppera]

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

Re: [PATCH v5 net-next 2/2] net: phy: micrel: Fix warn: passing zero to PTR_ERR

[Andrew Lunn]

> > 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

RE: [PATCH v5 net-next 2/2] net: phy: micrel: Fix warn: passing zero to PTR_ERR

[Divya.Koppera]

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