diff mbox series

[v3,net-next] net: phy: micrel: Fix warn: passing zero to PTR_ERR

Message ID 20221129101653.6921-1-Divya.Koppera@microchip.com
State New
Headers
Series [v3,net-next] net: phy: micrel: Fix warn: passing zero to PTR_ERR |

Commit Message

Divya Koppera Nov. 29, 2022, 10:16 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'

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

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")
Fixes: 21b688dabecb ("net: phy: micrel: Cable Diag feature for lan8814 phy")
Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com>
---
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 | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Comments

Horatiu Vultur Nov. 30, 2022, 2:50 p.m. UTC | #1 
The 11/29/2022 15:46, Divya Koppera wrote:

Hi Divya,

> 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'
> 
> Fixes Old smatch warnings:
> drivers/net/phy/micrel.c:1750 ksz886x_cable_test_get_status() error:
> uninitialized symbol 'ret'.

Shouldn't you split this patch in 2 different patches, as you fix 2
issues.
Also any reason why you target net-next and not net? Because I can
see the blamed patches on net branch.

> 
> 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")
> Fixes: 21b688dabecb ("net: phy: micrel: Cable Diag feature for lan8814 phy")
> Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com>
> ---
> 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 | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 26ce0c5defcd..3703e2fafbd4 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;
>  
> @@ -2970,12 +2971,13 @@ static int lan8814_config_intr(struct phy_device *phydev)
>  
>  static void lan8814_ptp_init(struct phy_device *phydev)
>  {
> +	struct lan8814_shared_priv *shared_priv = phydev->shared->priv;
>  	struct kszphy_priv *priv = phydev->priv;
>  	struct kszphy_ptp_priv *ptp_priv = &priv->ptp_priv;
>  	u32 temp;
>  
> -	if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) ||
> -	    !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING))
> +	/* Check if PHC support is missing at the configuration level */
> +	if (!shared_priv->ptp_clock)
>  		return;
>  
>  	lanphy_write_page_reg(phydev, 5, TSU_HARD_RESET, TSU_HARD_RESET_);
> @@ -3016,10 +3018,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);
>  
> @@ -3039,12 +3037,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
>
Divya Koppera Dec. 1, 2022, 7:08 a.m. UTC | #2 
Hi Horatiu,

> -----Original Message-----
> From: Horatiu Vultur <horatiu.vultur@microchip.com>
> Sent: Wednesday, November 30, 2022 8:21 PM
> To: Divya Koppera - I30481 <Divya.Koppera@microchip.com>
> Cc: 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; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; Madhuri Sripada - I34878
> <Madhuri.Sripada@microchip.com>
> Subject: Re: [PATCH v3 net-next] net: phy: micrel: Fix warn: passing zero to
> PTR_ERR
> 
> The 11/29/2022 15:46, Divya Koppera wrote:
> 
> Hi Divya,
> 
> > 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'
> >
> > Fixes Old smatch warnings:
> > drivers/net/phy/micrel.c:1750 ksz886x_cable_test_get_status() error:
> > uninitialized symbol 'ret'.
> 
> Shouldn't you split this patch in 2 different patches, as you fix 2 issues.

I got these warnings in single mail, so thought of fixing it in one patch. Also, one patch has single line change so did this way.
Yeah, splitting sense good, will do in next revision.

> Also any reason why you target net-next and not net? Because I can see the
> blamed patches on net branch.
> 

Initially I targeted for net-next and in second revision I moved to net as it is fix. But I got a comment as below. So again, targeted to net-next.

"
> v1 -> v2:
> - Handled NULL pointer case
> - Changed subject line with net-next to net

This is not a genuine bug fix, and so it should target next-next."


> >
> > 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")
> > Fixes: 21b688dabecb ("net: phy: micrel: Cable Diag feature for lan8814
> > phy")
> > Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com>
> > ---
> > 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 | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index
> > 26ce0c5defcd..3703e2fafbd4 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;
> >
> > @@ -2970,12 +2971,13 @@ static int lan8814_config_intr(struct
> > phy_device *phydev)
> >
> >  static void lan8814_ptp_init(struct phy_device *phydev)  {
> > +	struct lan8814_shared_priv *shared_priv = phydev->shared->priv;
> >  	struct kszphy_priv *priv = phydev->priv;
> >  	struct kszphy_ptp_priv *ptp_priv = &priv->ptp_priv;
> >  	u32 temp;
> >
> > -	if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) ||
> > -	    !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING))
> > +	/* Check if PHC support is missing at the configuration level */
> > +	if (!shared_priv->ptp_clock)
> >  		return;
> >
> >  	lanphy_write_page_reg(phydev, 5, TSU_HARD_RESET,
> TSU_HARD_RESET_);
> > @@ -3016,10 +3018,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);
> >
> > @@ -3039,12 +3037,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
> >
> 
> --
> /Horatiu
/Divya
Horatiu Vultur Dec. 1, 2022, 8:26 a.m. UTC | #3 
The 12/01/2022 07:08, Divya Koppera - I30481 wrote:
> Hi Horatiu,
> 
> > -----Original Message-----
> > From: Horatiu Vultur <horatiu.vultur@microchip.com>
> > Sent: Wednesday, November 30, 2022 8:21 PM
> > To: Divya Koppera - I30481 <Divya.Koppera@microchip.com>
> > Cc: 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; UNGLinuxDriver
> > <UNGLinuxDriver@microchip.com>; Madhuri Sripada - I34878
> > <Madhuri.Sripada@microchip.com>
> > Subject: Re: [PATCH v3 net-next] net: phy: micrel: Fix warn: passing zero to
> > PTR_ERR
> > 
> > The 11/29/2022 15:46, Divya Koppera wrote:
> > 
> > Hi Divya,
> > 
> > > 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'
> > >
> > > Fixes Old smatch warnings:
> > > drivers/net/phy/micrel.c:1750 ksz886x_cable_test_get_status() error:
> > > uninitialized symbol 'ret'.
> > 
> > Shouldn't you split this patch in 2 different patches, as you fix 2 issues.
> 
> I got these warnings in single mail, so thought of fixing it in one patch. Also, one patch has single line change so did this way.
> Yeah, splitting sense good, will do in next revision.
> 
> > Also any reason why you target net-next and not net? Because I can see the
> > blamed patches on net branch.
> > 
> 
> Initially I targeted for net-next and in second revision I moved to net as it is fix. But I got a comment as below. So again, targeted to net-next.
> 
> "
> > v1 -> v2:
> > - Handled NULL pointer case
> > - Changed subject line with net-next to net
> 
> This is not a genuine bug fix, and so it should target next-next."

That is fine by me.

...

> > >
> > >
> > >  static void lan8814_ptp_init(struct phy_device *phydev)  {
> > > +	struct lan8814_shared_priv *shared_priv = phydev->shared->priv;
> > >  	struct kszphy_priv *priv = phydev->priv;
> > >  	struct kszphy_ptp_priv *ptp_priv = &priv->ptp_priv;
> > >  	u32 temp;
> > >
> > > -	if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) ||
> > > -	    !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING))
> > > +	/* Check if PHC support is missing at the configuration level */
> > > +	if (!shared_priv->ptp_clock)
> > >  		return;

Sorry I forgot to mention this in the previous email.
Can you rename shared_priv to just shared. Because in all the other places
it is used shared and not shared_priv.
diff mbox series

Patch

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 26ce0c5defcd..3703e2fafbd4 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;
 
@@ -2970,12 +2971,13 @@  static int lan8814_config_intr(struct phy_device *phydev)
 
 static void lan8814_ptp_init(struct phy_device *phydev)
 {
+	struct lan8814_shared_priv *shared_priv = phydev->shared->priv;
 	struct kszphy_priv *priv = phydev->priv;
 	struct kszphy_ptp_priv *ptp_priv = &priv->ptp_priv;
 	u32 temp;
 
-	if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) ||
-	    !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING))
+	/* Check if PHC support is missing at the configuration level */
+	if (!shared_priv->ptp_clock)
 		return;
 
 	lanphy_write_page_reg(phydev, 5, TSU_HARD_RESET, TSU_HARD_RESET_);
@@ -3016,10 +3018,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);
 
@@ -3039,12 +3037,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;