[PATCH net-next] net: phy: icplus: Call phy_restore_page() when phy_select_page() fails

[PATCH net-next] net: phy: icplus: Call phy_restore_page() when phy_select_page() fails

[Dan Carpenter]

Smatch warns that there is a locking issue in this function:

drivers/net/phy/icplus.c:273 ip101a_g_config_intr_pin()
warn: inconsistent returns '&phydev->mdio.bus->mdio_lock'.
  Locked on  : 242
  Unlocked on: 273

It turns out that the comments in phy_select_page() say we have to call
phy_restore_page() even if the call to phy_select_page() fails.

Fixes: f9bc51e6cce2 ("net: phy: icplus: fix paged register access")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/net/phy/icplus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index 4e15d4d02488..015b7b5aa776 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -239,7 +239,7 @@ static int ip101a_g_config_intr_pin(struct phy_device *phydev)
 
 	oldpage = phy_select_page(phydev, IP101G_DEFAULT_PAGE);
 	if (oldpage < 0)
-		return oldpage;
+		goto out;
 
 	/* configure the RXER/INTR_32 pin of the 32-pin IP101GR if needed: */
 	switch (priv->sel_intr32) {
-- 
2.30.0

Re: [PATCH net-next] net: phy: icplus: Call phy_restore_page() when phy_select_page() fails

[Michael Walle]

Am 2021-02-17 07:17, schrieb Dan Carpenter:
> Smatch warns that there is a locking issue in this function:
> 
> drivers/net/phy/icplus.c:273 ip101a_g_config_intr_pin()
> warn: inconsistent returns '&phydev->mdio.bus->mdio_lock'.
>   Locked on  : 242
>   Unlocked on: 273
> 
> It turns out that the comments in phy_select_page() say we have to call
> phy_restore_page() even if the call to phy_select_page() fails.
> 
> Fixes: f9bc51e6cce2 ("net: phy: icplus: fix paged register access")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Reviewed-by: Michael Walle <michael@walle.cc>

-michael

Re: [PATCH net-next] net: phy: icplus: Call phy_restore_page() when phy_select_page() fails

[Russell King - ARM Linux admin]

On Wed, Feb 17, 2021 at 09:17:59AM +0300, Dan Carpenter wrote:
> Smatch warns that there is a locking issue in this function:
> 
> drivers/net/phy/icplus.c:273 ip101a_g_config_intr_pin()
> warn: inconsistent returns '&phydev->mdio.bus->mdio_lock'.
>   Locked on  : 242
>   Unlocked on: 273
> 
> It turns out that the comments in phy_select_page() say we have to call
> phy_restore_page() even if the call to phy_select_page() fails.

It seems it's a total waste of time documenting functions...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next] net: phy: icplus: Call phy_restore_page() when phy_select_page() fails

[Michael Walle]

Am 2021-02-17 11:04, schrieb Russell King - ARM Linux admin:
> On Wed, Feb 17, 2021 at 09:17:59AM +0300, Dan Carpenter wrote:
>> Smatch warns that there is a locking issue in this function:
>> 
>> drivers/net/phy/icplus.c:273 ip101a_g_config_intr_pin()
>> warn: inconsistent returns '&phydev->mdio.bus->mdio_lock'.
>>   Locked on  : 242
>>   Unlocked on: 273
>> 
>> It turns out that the comments in phy_select_page() say we have to 
>> call
>> phy_restore_page() even if the call to phy_select_page() fails.
> 
> It seems it's a total waste of time documenting functions...

You once said

"""
Kernel development is fundamentally a difficult, frustrating and
depressing activity.
"""

But really this comment doesn't make it much better. Yes I've made
a mistake although I _read_ the function documentation. So shame on
me.

-michael

Re: [PATCH net-next] net: phy: icplus: Call phy_restore_page() when phy_select_page() fails

[Russell King - ARM Linux admin]

On Wed, Feb 17, 2021 at 11:12:11AM +0100, Michael Walle wrote:
> Am 2021-02-17 11:04, schrieb Russell King - ARM Linux admin:
> > On Wed, Feb 17, 2021 at 09:17:59AM +0300, Dan Carpenter wrote:
> > > Smatch warns that there is a locking issue in this function:
> > > 
> > > drivers/net/phy/icplus.c:273 ip101a_g_config_intr_pin()
> > > warn: inconsistent returns '&phydev->mdio.bus->mdio_lock'.
> > >   Locked on  : 242
> > >   Unlocked on: 273
> > > 
> > > It turns out that the comments in phy_select_page() say we have to
> > > call
> > > phy_restore_page() even if the call to phy_select_page() fails.
> > 
> > It seems it's a total waste of time documenting functions...
> 
> You once said
> 
> """
> Kernel development is fundamentally a difficult, frustrating and
> depressing activity.
> """
> 
> But really this comment doesn't make it much better. Yes I've made
> a mistake although I _read_ the function documentation. So shame on
> me.

It wasn't aimed at you - it was more pointing out that in the normal
process of kernel development, reading documentation is fairly low,
yet we spend time creating it. So, does writing documentation actually
help, or does it just slow down the development cycle? Does it have a
net positive value? Personally, I don't think it does.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next] net: phy: icplus: Call phy_restore_page() when phy_select_page() fails

[Dan Carpenter]

On Wed, Feb 17, 2021 at 09:17:59AM +0300, Dan Carpenter wrote:
> Smatch warns that there is a locking issue in this function:
> 
> drivers/net/phy/icplus.c:273 ip101a_g_config_intr_pin()
> warn: inconsistent returns '&phydev->mdio.bus->mdio_lock'.
>   Locked on  : 242
>   Unlocked on: 273
> 
> It turns out that the comments in phy_select_page() say we have to call
> phy_restore_page() even if the call to phy_select_page() fails.
> 
> Fixes: f9bc51e6cce2 ("net: phy: icplus: fix paged register access")

Don't apply this patch.  I have created a new Smatch warning for the
phy_select_page() behavior and it catches a couple similar bugs in the
same file.  I will send a v2 that fixes those as well.

regards,
dan carpenter

Re: [PATCH net-next] net: phy: icplus: Call phy_restore_page() when phy_select_page() fails

[Russell King - ARM Linux admin]

On Wed, Feb 17, 2021 at 05:28:38PM +0300, Dan Carpenter wrote:
> On Wed, Feb 17, 2021 at 09:17:59AM +0300, Dan Carpenter wrote:
> > Smatch warns that there is a locking issue in this function:
> > 
> > drivers/net/phy/icplus.c:273 ip101a_g_config_intr_pin()
> > warn: inconsistent returns '&phydev->mdio.bus->mdio_lock'.
> >   Locked on  : 242
> >   Unlocked on: 273
> > 
> > It turns out that the comments in phy_select_page() say we have to call
> > phy_restore_page() even if the call to phy_select_page() fails.
> > 
> > Fixes: f9bc51e6cce2 ("net: phy: icplus: fix paged register access")
> 
> Don't apply this patch.  I have created a new Smatch warning for the
> phy_select_page() behavior and it catches a couple similar bugs in the
> same file.  I will send a v2 that fixes those as well.

Yes, there are three instances of this in the file, all three need
fixing. Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next] net: phy: icplus: Call phy_restore_page() when phy_select_page() fails

[Russell King - ARM Linux admin]

On Wed, Feb 17, 2021 at 03:06:21PM +0000, Russell King - ARM Linux admin wrote:
> On Wed, Feb 17, 2021 at 05:28:38PM +0300, Dan Carpenter wrote:
> > On Wed, Feb 17, 2021 at 09:17:59AM +0300, Dan Carpenter wrote:
> > > Smatch warns that there is a locking issue in this function:
> > > 
> > > drivers/net/phy/icplus.c:273 ip101a_g_config_intr_pin()
> > > warn: inconsistent returns '&phydev->mdio.bus->mdio_lock'.
> > >   Locked on  : 242
> > >   Unlocked on: 273
> > > 
> > > It turns out that the comments in phy_select_page() say we have to call
> > > phy_restore_page() even if the call to phy_select_page() fails.
> > > 
> > > Fixes: f9bc51e6cce2 ("net: phy: icplus: fix paged register access")
> > 
> > Don't apply this patch.  I have created a new Smatch warning for the
> > phy_select_page() behavior and it catches a couple similar bugs in the
> > same file.  I will send a v2 that fixes those as well.
> 
> Yes, there are three instances of this in the file, all three need
> fixing. Thanks.

I'm wondering whether we need to add __acquires() and __releases()
annotations to some of these functions so that sparse can catch
these cases. Thoughts?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next] net: phy: icplus: Call phy_restore_page() when phy_select_page() fails

[Andrew Lunn]

> I'm wondering whether we need to add __acquires() and __releases()
> annotations to some of these functions so that sparse can catch
> these cases. Thoughts?

Hi Russell

The more tools we have for catching locking problems the better.
Jakubs patchwork bot should then catch them when a patch is submitted,
if the developer did not run sparse themselves.

       Andrew

Re: [PATCH net-next] net: phy: icplus: Call phy_restore_page() when phy_select_page() fails

[Dan Carpenter]

On Wed, Feb 17, 2021 at 06:06:48PM +0100, Andrew Lunn wrote:
> > I'm wondering whether we need to add __acquires() and __releases()
> > annotations to some of these functions so that sparse can catch
> > these cases. Thoughts?
> 
> Hi Russell
> 
> The more tools we have for catching locking problems the better.
> Jakubs patchwork bot should then catch them when a patch is submitted,
> if the developer did not run sparse themselves.

Here is how I wrote the check for Smatch.  The code in the kernel looks
like:

	oldpage = phy_select_page(phydev, 0x0007);

	...

	phy_restore_page(phydev, oldpage, 0);

So what I said is that if phy_select_page() returns an error code then
set "phydev" to &selected state.  Then if we call phy_restore_page()
set it to &undefined.  When we hit a return, check if we have any
"phydev" variables can possibly be in &selected state and print a
warning.

The code is below.

regards,
dan carpenter

#include "smatch.h"
#include "smatch_slist.h"

static int my_id;

STATE(selected);

static sval_t err_min = { .type = &int_ctype, .value = -4095 };
static sval_t err_max = { .type = &int_ctype, .value = -1 };

static void match_phy_select_page(struct expression *expr, const char *name, struct symbol *sym, void *data)
{
	set_state(my_id, name, sym, &selected);
}

static void match_phy_restore_page(struct expression *expr, const char *name, struct symbol *sym, void *data)
{
	set_state(my_id, name, sym, &undefined);
}

static void match_return(struct expression *expr)
{
	struct sm_state *sm;

	FOR_EACH_MY_SM(my_id, __get_cur_stree(), sm) {
		if (slist_has_state(sm->possible, &selected)) {
			sm_warning("phy_select_page() requires restore on error");
			return;
		}
	} END_FOR_EACH_SM(sm);
}

void check_phy_select_page_fail(int id)
{
	if (option_project != PROJ_KERNEL)
		return;

	my_id = id;

	return_implies_param_key("phy_select_page", err_min, err_max,
				 &match_phy_select_page, 0, "$", NULL);
	add_function_param_key_hook("phy_restore_page", &match_phy_restore_page,
				    0, "$", NULL);
	add_hook(&match_return, RETURN_HOOK);
}