On 11/22/2016 12:02 PM, Andrew Lunn wrote:
>> +static int bcm7xxx_28nm_set_tunable(struct phy_device *phydev,
>> + struct ethtool_tunable *tuna,
>> + const void *data)
>> +{
>> + u8 count = *(u8 *)data;
>> + int ret;
>> +
>> + switch (tuna->id) {
>> + case ETHTOOL_PHY_DOWNSHIFT:
>> + ret = bcm_phy_downshift_set(phydev, count);
>> + break;
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + if (ret)
>> + return ret;
>> +
>> + /* Disable EEE advertisment since this prevents the PHY
>> + * from successfully linking up, trigger auto-negotiation restart
>> + * to let the MAC decide what to do.
>> + */
>> + ret = bcm_phy_set_eee(phydev, count == DOWNSHIFT_DEV_DISABLE);
>> + if (ret)
>> + return ret;
>> +
>> + return genphy_restart_aneg(phydev);
>> +}
>
> Hi Florian
>
> Is the locking O.K. here? The core code does not take the phy lock.
> But i think your shadow register accesses at least need to be
> protected by the lock?
There should be some kind of protection, but I was expecting it to be
done at the caller level, so that when {get,set}_tunable run, they are
serialized with respect to each other, clearly, by looking at the code,
this is not the case.
>
> Maybe we should think about this locking a bit. It is normal for the
> lock to be held when using ops in the phy driver structure. The
> exception is suspend/resume. Maybe we should also take the lock before
> calling the phydev->drv->get_tunable() and phydev->drv->set_tunable()?
Yes, that certainly seems like a good approach to me, let me cook a
patch doing that.
--
Florian
