Re: [PATCH net-next 4/5] net: phy: bcm7xxx: Add support for downshift/Wirespeed
On 11/23/2016 06:46 AM, Andrew Lunn wrote: 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. >> >> Just for my understanding (such that I will not make the same mistake >> again)... >> >> Why is it that phy functions such as get_wol needs to take the phy_lock and >> others like get_tunable does not. >> >> I do understand the arguments on why the lock should be held by the caller of >> get_tunable, but I do not understand why the same argument does not apply for >> get_wol. > > Hi Allan > > phy_ethtool_get_wol and friends probably should take the > phy_lock. This inconsistency is probably leading to locking > bugs. e.g. at803x_set_wol() does a read-modify-write, and does not > take the lock. > > There is no comment in the patch adding phy_ethtool_set_wol() to say > why the lock is not taken, and a quick look at the code does not > suggest a reason why it could not be taken/released by > phy_ethtool_set_wol(). Yes, this should happen. I don't see how we cannot have two user-space processes not racing with each other here for instance, see mv643xx_eth_get_wol and cpsw_get_wol. > > I think it would be a good idea to change this. > > phy_suspend()/phy_resume() might have good reasons to avoid the lock, > i've no idea how it is supposed to work. Is there a danger something > else is holding the lock and has already been suspended? I guess not, > otherwise there is little hope suspend would work at all. phy_suspend() and phy_resume() usually get called after phy_disconnect() or phy_stop() have been invoked, and even then this is during the Ethernet driver's suspend resume/resume path, so there is no room for concurrency to occur (user space is quiesced, and the PHY state machine is stopped/halted), but still, if we were to change the calling context it would be a good idea to acquire phydev->lock. -- Florian
Re: [PATCH net-next 4/5] net: phy: bcm7xxx: Add support for downshift/Wirespeed
> > > 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. > > Just for my understanding (such that I will not make the same mistake > again)... > > Why is it that phy functions such as get_wol needs to take the phy_lock and > others like get_tunable does not. > > I do understand the arguments on why the lock should be held by the caller of > get_tunable, but I do not understand why the same argument does not apply for > get_wol. Hi Allan phy_ethtool_get_wol and friends probably should take the phy_lock. This inconsistency is probably leading to locking bugs. e.g. at803x_set_wol() does a read-modify-write, and does not take the lock. There is no comment in the patch adding phy_ethtool_set_wol() to say why the lock is not taken, and a quick look at the code does not suggest a reason why it could not be taken/released by phy_ethtool_set_wol(). I think it would be a good idea to change this. phy_suspend()/phy_resume() might have good reasons to avoid the lock, i've no idea how it is supposed to work. Is there a danger something else is holding the lock and has already been suspended? I guess not, otherwise there is little hope suspend would work at all. Andrew
Re: [PATCH net-next 4/5] net: phy: bcm7xxx: Add support for downshift/Wirespeed
Hi,
On 22/11/16 12:07, Florian Fainelli wrote:
> 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.
Just for my understanding (such that I will not make the same mistake again)...
Why is it that phy functions such as get_wol needs to take the phy_lock and
others like get_tunable does not.
I do understand the arguments on why the lock should be held by the caller of
get_tunable, but I do not understand why the same argument does not apply for
get_wol.
/Allan
Re: [PATCH net-next 4/5] net: phy: bcm7xxx: Add support for downshift/Wirespeed
On 11/22/2016 12:57 PM, Andrew Lunn wrote: >>> 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. > > Hi Florian > > There are a couple of mutex locks/unlocks you will need to remove from > mscc.c when you centralize this mutex. Good point, thanks, let me review the mscc PHY driver and propose a more proper fix. -- Florian
Re: [PATCH net-next 4/5] net: phy: bcm7xxx: Add support for downshift/Wirespeed
> > 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. Hi Florian There are a couple of mutex locks/unlocks you will need to remove from mscc.c when you centralize this mutex. Andrew
Re: [PATCH net-next 4/5] net: phy: bcm7xxx: Add support for downshift/Wirespeed
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
Re: [PATCH net-next 4/5] net: phy: bcm7xxx: Add support for downshift/Wirespeed
> +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?
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()?
Andrew
[PATCH net-next 4/5] net: phy: bcm7xxx: Add support for downshift/Wirespeed
Add support for configuring the downshift/Wirespeed enable/disable
toggles and specify a link retry value ranging from 1 to 9. Since the
integrated BCM7xxx have issues when wirespeed is enabled and EEE is also
enabled, we do disable EEE if wirespeed is enabled.
Signed-off-by: Florian Fainelli
---
drivers/net/phy/bcm7xxx.c | 51 ++-
1 file changed, 50 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c
index b7789e879670..5b3be4c67be8 100644
--- a/drivers/net/phy/bcm7xxx.c
+++ b/drivers/net/phy/bcm7xxx.c
@@ -167,6 +167,7 @@ static int bcm7xxx_28nm_config_init(struct phy_device
*phydev)
{
u8 rev = PHY_BRCM_7XXX_REV(phydev->dev_flags);
u8 patch = PHY_BRCM_7XXX_PATCH(phydev->dev_flags);
+ u8 count;
int ret = 0;
pr_info_once("%s: %s PHY revision: 0x%02x, patch: %d\n",
@@ -199,7 +200,12 @@ static int bcm7xxx_28nm_config_init(struct phy_device
*phydev)
if (ret)
return ret;
- ret = bcm_phy_set_eee(phydev, true);
+ ret = bcm_phy_downshift_get(phydev, &count);
+ if (ret)
+ return ret;
+
+ /* Only enable EEE if Wirespeed/downshift is disabled */
+ ret = bcm_phy_set_eee(phydev, count == DOWNSHIFT_DEV_DISABLE);
if (ret)
return ret;
@@ -303,6 +309,47 @@ static int bcm7xxx_suspend(struct phy_device *phydev)
return 0;
}
+static int bcm7xxx_28nm_get_tunable(struct phy_device *phydev,
+ struct ethtool_tunable *tuna,
+ void *data)
+{
+ switch (tuna->id) {
+ case ETHTOOL_PHY_DOWNSHIFT:
+ return bcm_phy_downshift_get(phydev, (u8 *)data);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+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);
+}
+
#define BCM7XXX_28NM_GPHY(_oui, _name) \
{ \
.phy_id = (_oui), \
@@ -315,6 +362,8 @@ static int bcm7xxx_suspend(struct phy_device *phydev)
.config_aneg= genphy_config_aneg, \
.read_status= genphy_read_status, \
.resume = bcm7xxx_28nm_resume, \
+ .get_tunable= bcm7xxx_28nm_get_tunable, \
+ .set_tunable= bcm7xxx_28nm_set_tunable, \
}
#define BCM7XXX_40NM_EPHY(_oui, _name) \
--
2.9.3
