Hello!
On 05/24/2018 05:11 PM, Vladimir Zapolskiy wrote:
>>> The change fixes a sleep in atomic context issue, which can be
>>> always triggered by running 'ethtool -r' command, because
>>> phy_start_aneg() protects phydev fields by a mutex.
You don't say that *not* grabbing the spinlock is safe...
>>> Another note is that the change implicitly replaces phy_start_aneg()
>>> with a newer phy_restart_aneg().
Hm, perphaps this could be a material for a separate patch?
>>>
>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@xxxxxxxxxx>
>>> ---
>>> drivers/net/ethernet/renesas/ravb_main.c | 17 +----------------
>>> 1 file changed, 1 insertion(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 68f122140966..4a043eb0e2aa 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -1150,21 +1150,6 @@ static int ravb_set_link_ksettings(struct net_device *ndev,
>>> return error;
>>> }
>>>
>>> -static int ravb_nway_reset(struct net_device *ndev)
>>> -{
>>> - struct ravb_private *priv = netdev_priv(ndev);
>>> - int error = -ENODEV;
>>> - unsigned long flags;
>>> -
>>> - if (ndev->phydev) {
>>> - spin_lock_irqsave(&priv->lock, flags);
>>> - error = phy_start_aneg(ndev->phydev);
>>> - spin_unlock_irqrestore(&priv->lock, flags);
>>> - }
>>
>> Eck! phylib assumes thread context and takes a mutex while calling
>> into the PHY driver.
>>
>> It would be good to add some sort of fixes: tag. Maybe for the commit
>> that added the generic nway_reset? That would at least cover some
>> stable kernels.
>>
>> Reviewed-by: Andrew Lunn <andrew@xxxxxxx>
>>
>
> Hi Andrew, thank you for review.
>
> generally it makes sense to add Fixes tag, but as I said in
> the commit message the problem is present before reused phy_ethtool_*()
> functions were added to the kernel, so some kind of juggling with
> the proper kernel version would be required in assumption that
> the fixes are backported as an unmodified changes.
The -stable fixes can vary from version to version, IIUC. You could be
asked to backport your patch if Greg KH (or somebody else from the -stable
kernel maintainers) gets in trouble backporting your patch.
> Hopefully Sergei as the driver maintainer can verify the fixes on
I'm *not* a maintainer, just a humble reviewer! :-)
> older kernels and suggest the right kernel versions for backporting.
This would be asking too much from me, I'm afraid...
Still, Dave, could you please give me a couple of days to spend on
this series?
> --
> With best wishes,
> Vladimir
MBR, Sergei
