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