Re: [PATCH net] net: phy: Fix deadlocking in phy_error() invocation

[Serge Semin]

On Fri, Aug 18, 2023 at 03:07:49PM +0200, Andrew Lunn wrote:
> On Fri, Aug 18, 2023 at 03:54:45PM +0300, Serge Semin wrote:
> >  static void phy_process_error(struct phy_device *phydev)
> >  {
> > -	mutex_lock(&phydev->lock);
> > +	/* phydev->lock must be held for the state change to be safe */
> > +	if (!mutex_is_locked(&phydev->lock))
> > +		phydev_err(phydev, "PHY-device data unsafe context\n");
> > +
> >  	phydev->state = PHY_ERROR;
> > -	mutex_unlock(&phydev->lock);
> >  
> >  	phy_trigger_machine(phydev);
> >  }
> 
> Thanks for the patch Serge. It looks like a good implementation of
> what i suggested. But thinking about it further, if the error ever
> appears in somebodies kernel log, there is probably not enough
> information to actually fix it. There is no call path. So i think it
> should actually use WARN_ON_ONCE() so we get a stack trace.

A trace is already printed by means of WARN()/WARN_ON()
in the phy_process_error() method callers:
phy_error_precise()
and
phy_error()
Wouldn't it be too much to print it twice in a row?

We can redefine phy_error_precise() and phy_process_error() functions
to something like this:

static void phy_process_error(struct phy_device *phydev,
			      const void *func, int err)
{
	if (__ONCE_LITE_IF(!mutex_is_locked(&phydev->lock)))
		WARN(1, "PHY-device data unsafe context\n");
	else if (func)
		WARN(1, "%pS: returned: %d\n", func, err);
	else
		WARN_ON(1);

	phydev->state = PHY_ERROR;

	phy_trigger_machine(phydev);
}

static void phy_error_precise(struct phy_device *phydev,
			      const void *func, int err)
{
        mutex_lock(&phydev->lock);
        phy_process_error(phydev, func, err);
        mutex_unlock(&phydev->lock);
}

void phy_error(struct phy_device *phydev)
{
	phy_process_error(phydev, NULL, 0);
}
EXPORT_SYMBOL(phy_error);

Though in such implementation phy_error_precise() looks redundant. We
can freely move its body to the single user - phy_state_machine()
function.

Note a positive side effect of this implementation is that potentially
phy_error() can be converted to accepting a function pointer caused
the error (phy_read(), phy_write(), etc). Alternatively if the
conversion would look too bulky, phy_error_preciseI() could be just
EXPORT_SYMBOL()-ed with the PHY-device mutex locking being moved to
phy_state_machine().

> 
> Sorry for changing my mind.

No worries.

-Serge(y)

> 
>     Andrew
> 
> ---
> pw-bot: cr