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

On Wed, Feb 17, 2021 at 06:06:48PM +0100, Andrew Lunn wrote:

quoted

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