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

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

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);
}