[PATCH] phy: phy_ethtool_ksettings_set: Don't discard phy_start_aneg's return

[PATCH] phy: phy_ethtool_ksettings_set: Don't discard phy_start_aneg's return

[bage]

From: Bastian Germann <bage@linutronix.de>

Take the return of phy_start_aneg into account so that ethtool will handle
negotiation errors and not silently accept invalid input.

Fixes: 2d55173e71b0 ("phy: add generic function to support ksetting support")
Signed-off-by: Bastian Germann <bage@linutronix.de>
Reviewed-by: Benedikt Spranger <b.spranger@linutronix.de>
---
 drivers/net/phy/phy.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index a3bfb156c83d..f740b533abba 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -770,6 +770,8 @@ static int phy_poll_aneg_done(struct phy_device *phydev)
 int phy_ethtool_ksettings_set(struct phy_device *phydev,
 			      const struct ethtool_link_ksettings *cmd)
 {
+	int ret = 0;
+
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
 	u8 autoneg = cmd->base.autoneg;
 	u8 duplex = cmd->base.duplex;
@@ -815,10 +817,10 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
 	phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl;
 
 	/* Restart the PHY */
-	_phy_start_aneg(phydev);
+	ret = _phy_start_aneg(phydev);
 
 	mutex_unlock(&phydev->lock);
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL(phy_ethtool_ksettings_set);
 
-- 
2.30.2

Re: [PATCH] phy: phy_ethtool_ksettings_set: Don't discard phy_start_aneg's return

[Andrew Lunn]

On Fri, Nov 05, 2021 at 04:36:48PM +0100, bage@linutronix.de wrote:
> From: Bastian Germann <bage@linutronix.de>
> 
> Take the return of phy_start_aneg into account so that ethtool will handle
> negotiation errors and not silently accept invalid input.

Hi Bastian

What PHY driver are you using this with? phy_start_aneg() generally
does not return errors, except for -EIO/-TIMEDOUT because
communication with the PHY has failed. All parameter validation should
of already happened before the call to phy_start_aneg(). So i'm
wondering if the PHY driver is doing something wrong.

The change itself however does seems sensible. If the PHY has
disappeared, returning -EIO would be valid.

    Andrew

Re: [PATCH] phy: phy_ethtool_ksettings_set: Don't discard phy_start_aneg's return

[Heiner Kallweit]

On 05.11.2021 16:36, bage@linutronix.de wrote:
> From: Bastian Germann <bage@linutronix.de>
> 
> Take the return of phy_start_aneg into account so that ethtool will handle
> negotiation errors and not silently accept invalid input.
> 
> Fixes: 2d55173e71b0 ("phy: add generic function to support ksetting support")
> Signed-off-by: Bastian Germann <bage@linutronix.de>
> Reviewed-by: Benedikt Spranger <b.spranger@linutronix.de>

In addition to what Andrew said already:

- This patch won't apply on net due to a4db9055fdb9.
- Patch misses the "net" annotation.
- Prefix should be "net: phy:", not "phy:".

At least the formal aspects should have been covered by the internal review.

> ---
>  drivers/net/phy/phy.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index a3bfb156c83d..f740b533abba 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -770,6 +770,8 @@ static int phy_poll_aneg_done(struct phy_device *phydev)
>  int phy_ethtool_ksettings_set(struct phy_device *phydev,
>  			      const struct ethtool_link_ksettings *cmd)
>  {
> +	int ret = 0;

Why initializing ret?

> +
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
>  	u8 autoneg = cmd->base.autoneg;
>  	u8 duplex = cmd->base.duplex;
> @@ -815,10 +817,10 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
>  	phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl;
>  
>  	/* Restart the PHY */
> -	_phy_start_aneg(phydev);
> +	ret = _phy_start_aneg(phydev);
>  
>  	mutex_unlock(&phydev->lock);
> -	return 0;
> +	return ret;
>  }
>  EXPORT_SYMBOL(phy_ethtool_ksettings_set);
>  
> 

[PATCH net v2] net: phy: phy_ethtool_ksettings_set: Don't discard phy_start_aneg's return

[bage]

From: Bastian Germann <bage@linutronix.de>

Take the return of phy_start_aneg into account so that ethtool will handle
negotiation errors and not silently accept invalid input.

Fixes: 2d55173e71b0 ("phy: add generic function to support ksetting support")
Signed-off-by: Bastian Germann <bage@linutronix.de>
---
 drivers/net/phy/phy.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index beb2b66da132..59e024891f50 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -770,6 +770,8 @@ static int phy_poll_aneg_done(struct phy_device *phydev)
 int phy_ethtool_ksettings_set(struct phy_device *phydev,
 			      const struct ethtool_link_ksettings *cmd)
 {
+	int ret;
+
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
 	u8 autoneg = cmd->base.autoneg;
 	u8 duplex = cmd->base.duplex;
@@ -818,12 +820,13 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
 	if (phy_is_started(phydev)) {
 		phydev->state = PHY_UP;
 		phy_trigger_machine(phydev);
+		ret = 0;
 	} else {
-		_phy_start_aneg(phydev);
+		ret = _phy_start_aneg(phydev);
 	}
 
 	mutex_unlock(&phydev->lock);
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL(phy_ethtool_ksettings_set);
 
-- 
2.30.2

Re: [PATCH] phy: phy_ethtool_ksettings_set: Don't discard phy_start_aneg's return

[Bastian Germann]

Am 05.11.21 um 19:32 schrieb Andrew Lunn:
> What PHY driver are you using this with? phy_start_aneg() generally
> does not return errors, except for -EIO/-TIMEDOUT because
> communication with the PHY has failed. All parameter validation should
> of already happened before the call to phy_start_aneg(). So i'm
> wondering if the PHY driver is doing something wrong.

I am modifying broadcom phy and will make sure that I will end up nut having
error checks in the wrong place.

Re: [PATCH] phy: phy_ethtool_ksettings_set: Don't discard phy_start_aneg's return

[Bastian Germann]

Am 06.11.21 um 22:52 schrieb Heiner Kallweit:
> On 05.11.2021 16:36, bage@linutronix.de wrote:
>> From: Bastian Germann <bage@linutronix.de>
>>
>> Take the return of phy_start_aneg into account so that ethtool will handle
>> negotiation errors and not silently accept invalid input.
>>
>> Fixes: 2d55173e71b0 ("phy: add generic function to support ksetting support")
>> Signed-off-by: Bastian Germann <bage@linutronix.de>
>> Reviewed-by: Benedikt Spranger <b.spranger@linutronix.de>
> 
> In addition to what Andrew said already:
> 
> - This patch won't apply on net due to a4db9055fdb9.

Hi Heiner,

Thanks for your remarks. I would have gotten the first one right if the netdev
tree was documented in the MAINTAINERS file (T: ...) and not only in netdev-FAQ.

Maybe you want to address that.

Thanks,
Bastian

> - Patch misses the "net" annotation.
> - Prefix should be "net: phy:", not "phy:".
> 
> At least the formal aspects should have been covered by the internal review.
> 
>> ---
>>   drivers/net/phy/phy.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index a3bfb156c83d..f740b533abba 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -770,6 +770,8 @@ static int phy_poll_aneg_done(struct phy_device *phydev)
>>   int phy_ethtool_ksettings_set(struct phy_device *phydev,
>>   			      const struct ethtool_link_ksettings *cmd)
>>   {
>> +	int ret = 0;
> 
> Why initializing ret?
> 
>> +
>>   	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
>>   	u8 autoneg = cmd->base.autoneg;
>>   	u8 duplex = cmd->base.duplex;
>> @@ -815,10 +817,10 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
>>   	phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl;
>>   
>>   	/* Restart the PHY */
>> -	_phy_start_aneg(phydev);
>> +	ret = _phy_start_aneg(phydev);
>>   
>>   	mutex_unlock(&phydev->lock);
>> -	return 0;
>> +	return ret;
>>   }
>>   EXPORT_SYMBOL(phy_ethtool_ksettings_set);
>>   
>>
> 

Re: [PATCH net v2] net: phy: phy_ethtool_ksettings_set: Don't discard phy_start_aneg's return

[Russell King (Oracle)]

On Mon, Nov 08, 2021 at 03:18:34PM +0100, bage@linutronix.de wrote:
> From: Bastian Germann <bage@linutronix.de>
> 
> Take the return of phy_start_aneg into account so that ethtool will handle
> negotiation errors and not silently accept invalid input.

I don't think this description is accurate. If we get to call
phy_start_aneg() with invalid input, then something has already
gone wrong. As Andrew has already explained, an error from this
function means that something went wrong with PHY communication.
All validation should have happened prior to this function being
called.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net v2] net: phy: phy_ethtool_ksettings_set: Don't discard phy_start_aneg's return

[Benedikt Spranger]

On Mon, 8 Nov 2021 14:25:48 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Mon, Nov 08, 2021 at 03:18:34PM +0100, bage@linutronix.de wrote:
> > From: Bastian Germann <bage@linutronix.de>
> > 
> > Take the return of phy_start_aneg into account so that ethtool will
> > handle negotiation errors and not silently accept invalid input.
> 
> I don't think this description is accurate. If we get to call
> phy_start_aneg() with invalid input, then something has already
> gone wrong.
The MDI/MDIX/auto-MDIX settings are not checked before calling
phy_start_aneg(). If the PHY supports forcing MDI and auto-MDIX, but
not forcing MDIX _phy_start_aneg() returns a failure, which is silently
ignored.

> As Andrew has already explained, an error from this
> function means that something went wrong with PHY communication.
Or for MDI/MDIX setings, the PHY does not support a feature/setting.

> All validation should have happened prior to this function being
> called.
OK.
Just to be clear: The PHY driver should check the settings and return
an error before calling phy_ethtool_ksettings_set() ?

Thanks
    Bene

Re: [PATCH net v2] net: phy: phy_ethtool_ksettings_set: Don't discard phy_start_aneg's return

[Russell King (Oracle)]

On Mon, Nov 08, 2021 at 04:06:53PM +0100, Benedikt Spranger wrote:
> On Mon, 8 Nov 2021 14:25:48 +0000
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> > On Mon, Nov 08, 2021 at 03:18:34PM +0100, bage@linutronix.de wrote:
> > > From: Bastian Germann <bage@linutronix.de>
> > > 
> > > Take the return of phy_start_aneg into account so that ethtool will
> > > handle negotiation errors and not silently accept invalid input.
> > 
> > I don't think this description is accurate. If we get to call
> > phy_start_aneg() with invalid input, then something has already
> > gone wrong.
> The MDI/MDIX/auto-MDIX settings are not checked before calling
> phy_start_aneg(). If the PHY supports forcing MDI and auto-MDIX, but
> not forcing MDIX _phy_start_aneg() returns a failure, which is silently
> ignored.

That would be very bad if it were to happen.

ksettings_set() would be called. If phy_is_started() returns true, we
trigger the state machine after forcing PHY_UP state. The state machine
calls phy_start_aneg() and gets an error, calling phy_error(), and
the state machine is forced into PHY_HALTED mode with a kernel warning
and stack trace.

That is not nice behaviour for a user.

> Just to be clear: The PHY driver should check the settings and return
> an error before calling phy_ethtool_ksettings_set() ?

The PHY driver doesn't get an opportunity to validate the settings
prior to calling phy_ethtool_ksettings_set() - because the PHY driver
never calls this function. The MAC driver does. However, the MAC driver
doesn't know what the PHY will accept.

This clearly needs to be fixed in phylib.

As a result of your explanation, I now believe your patch to be
fundamentally incorrect given the current state, and it should not be
applied.

We should not accept a call to phy_ethtool_ksettings_set(), modify some
state (setting phydev->advertising/autoneg/master_slave_set/mdix_ctrl/
speed/duplex) and then return an error to the user indicating that the
call failed because we didn't like some parameter - e.g. the mdix_ctrl
value that we've writtent o phydev->mdix_ctrl.

Userspace validation should always happen _prior_ to accepting any
state from userland. If there's an issue with it, the call should fail
without modifying any state.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net v2] net: phy: phy_ethtool_ksettings_set: Don't discard phy_start_aneg's return

[Andrew Lunn]

On Mon, Nov 08, 2021 at 04:06:53PM +0100, Benedikt Spranger wrote:
> On Mon, 8 Nov 2021 14:25:48 +0000
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> > On Mon, Nov 08, 2021 at 03:18:34PM +0100, bage@linutronix.de wrote:
> > > From: Bastian Germann <bage@linutronix.de>
> > > 
> > > Take the return of phy_start_aneg into account so that ethtool will
> > > handle negotiation errors and not silently accept invalid input.
> > 
> > I don't think this description is accurate. If we get to call
> > phy_start_aneg() with invalid input, then something has already
> > gone wrong.
> The MDI/MDIX/auto-MDIX settings are not checked before calling
> phy_start_aneg(). If the PHY supports forcing MDI and auto-MDIX, but
> not forcing MDIX _phy_start_aneg() returns a failure, which is silently
> ignored.

Does the broadcom driver currently do this, or is this the new
functionality you are adding?

It actually seems odd that auto and MDI is supported, but not MDIX?  I
would suggest checking with Florian about that. Which particular
broadcom PHY is it?

   Andrew

Re: [PATCH net v2] net: phy: phy_ethtool_ksettings_set: Don't discard phy_start_aneg's return

[Bastian Germann]

Am 08.11.21 um 17:09 schrieb Andrew Lunn:
> On Mon, Nov 08, 2021 at 04:06:53PM +0100, Benedikt Spranger wrote:
>> On Mon, 8 Nov 2021 14:25:48 +0000
>> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
>>
>>> On Mon, Nov 08, 2021 at 03:18:34PM +0100, bage@linutronix.de wrote:
>>>> From: Bastian Germann <bage@linutronix.de>
>>>>
>>>> Take the return of phy_start_aneg into account so that ethtool will
>>>> handle negotiation errors and not silently accept invalid input.
>>>
>>> I don't think this description is accurate. If we get to call
>>> phy_start_aneg() with invalid input, then something has already
>>> gone wrong.
>> The MDI/MDIX/auto-MDIX settings are not checked before calling
>> phy_start_aneg(). If the PHY supports forcing MDI and auto-MDIX, but
>> not forcing MDIX _phy_start_aneg() returns a failure, which is silently
>> ignored.
> 
> Does the broadcom driver currently do this, or is this the new
> functionality you are adding?
> 
> It actually seems odd that auto and MDI is supported, but not MDIX?  I
> would suggest checking with Florian about that. Which particular
> broadcom PHY is it?

It is BCM53125. Currently, you can set "mdix auto|off|on" which does not take any effect.
The chip will do what is its default depending on copper autonegotiation.

I am adding support for setting "mdix auto|off". I want the thing to error on "mdix on".
Where would I add that check?

Re: [PATCH net v2] net: phy: phy_ethtool_ksettings_set: Don't discard phy_start_aneg's return

[Andrew Lunn]

> It is BCM53125. Currently, you can set "mdix auto|off|on" which does
> not take any effect.  The chip will do what is its default depending
> on copper autonegotiation.
> 
> I am adding support for setting "mdix auto|off". I want the thing to error on "mdix on".
> Where would I add that check?

/* MDI or MDI-X status/control - if MDI/MDI_X/AUTO is set then
 * the driver is required to renegotiate link
 */
#define ETH_TP_MDI_INVALID	0x00 /* status: unknown; control: unsupported */
#define ETH_TP_MDI		0x01 /* status: MDI;     control: force MDI */
#define ETH_TP_MDI_X		0x02 /* status: MDI-X;   control: force MDI-X */
#define ETH_TP_MDI_AUTO		0x03 /*                  control: auto-select */

So there are three valid settings. And you are saying you only want to
implement two of them? If the hardware can do all three, you should
implement all three.

	  Andrew

Re: [PATCH net v2] net: phy: phy_ethtool_ksettings_set: Don't discard phy_start_aneg's return

[Heiner Kallweit]

On 08.11.2021 18:57, Andrew Lunn wrote:
>> It is BCM53125. Currently, you can set "mdix auto|off|on" which does
>> not take any effect.  The chip will do what is its default depending
>> on copper autonegotiation.
>>
>> I am adding support for setting "mdix auto|off". I want the thing to error on "mdix on".
>> Where would I add that check?
> 
> /* MDI or MDI-X status/control - if MDI/MDI_X/AUTO is set then
>  * the driver is required to renegotiate link
>  */
> #define ETH_TP_MDI_INVALID	0x00 /* status: unknown; control: unsupported */
> #define ETH_TP_MDI		0x01 /* status: MDI;     control: force MDI */
> #define ETH_TP_MDI_X		0x02 /* status: MDI-X;   control: force MDI-X */
> #define ETH_TP_MDI_AUTO		0x03 /*                  control: auto-select */
> 
> So there are three valid settings. And you are saying you only want to
> implement two of them? If the hardware can do all three, you should
> implement all three.
> 

If we would like to support PHY's that don't support all MDI modes then
supposedly this would require to add ETHTOOL_LINK_MODE bits for the
MDI modes. Then we could use the generic mechanism to check the bits in
the "supported" bitmap.

> 	  Andrew
> 
> 

Re: [PATCH net v2] net: phy: phy_ethtool_ksettings_set: Don't discard phy_start_aneg's return

[Russell King (Oracle)]

On Mon, Nov 08, 2021 at 07:01:23PM +0100, Heiner Kallweit wrote:
> On 08.11.2021 18:57, Andrew Lunn wrote:
> >> It is BCM53125. Currently, you can set "mdix auto|off|on" which does
> >> not take any effect.  The chip will do what is its default depending
> >> on copper autonegotiation.
> >>
> >> I am adding support for setting "mdix auto|off". I want the thing to error on "mdix on".
> >> Where would I add that check?
> > 
> > /* MDI or MDI-X status/control - if MDI/MDI_X/AUTO is set then
> >  * the driver is required to renegotiate link
> >  */
> > #define ETH_TP_MDI_INVALID	0x00 /* status: unknown; control: unsupported */
> > #define ETH_TP_MDI		0x01 /* status: MDI;     control: force MDI */
> > #define ETH_TP_MDI_X		0x02 /* status: MDI-X;   control: force MDI-X */
> > #define ETH_TP_MDI_AUTO		0x03 /*                  control: auto-select */
> > 
> > So there are three valid settings. And you are saying you only want to
> > implement two of them? If the hardware can do all three, you should
> > implement all three.
> > 
> 
> If we would like to support PHY's that don't support all MDI modes then
> supposedly this would require to add ETHTOOL_LINK_MODE bits for the
> MDI modes. Then we could use the generic mechanism to check the bits in
> the "supported" bitmap.

We'll have to add more stuff to phylink to avoid MACs masking these
bits... ETHTOOL_LINK_MODE seems to be becoming a disorganised dumping
ground for random stuff. :(

Also, what would these bits in the advertising bitmap mean?

Finally, how do we handle the lack of these bits for existing PHYs
that already implement MDI modes?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net v2] net: phy: phy_ethtool_ksettings_set: Don't discard phy_start_aneg's return

[Benedikt Spranger]

On Mon, 8 Nov 2021 19:01:23 +0100
Heiner Kallweit <hkallweit1@gmail.com> wrote:

> If we would like to support PHY's that don't support all MDI modes
> then supposedly this would require to add ETHTOOL_LINK_MODE bits for
> the MDI modes. Then we could use the generic mechanism to check the
> bits in the "supported" bitmap.

The things are even worse:
The chip supports only auto-MDIX at Gigabit and force MDI and
auto-MDIX in 10/100 modes. No force MDIX at all.

A validation callback from phy_ethtool_ksettings_set() before
restarting the PHY seems reasonable for me. Something like:

	/* Verify the settings we care about. */
	if (autoneg != AUTONEG_ENABLE && autoneg != AUTONEG_DISABLE)
	        return -EINVAL;

        if (autoneg == AUTONEG_ENABLE && linkmode_empty(advertising))
	        return -EINVAL;

        if (autoneg == AUTONEG_DISABLE &&
	    ((speed != SPEED_1000 &&
	      speed != SPEED_100 &&
              speed != SPEED_10) ||
             (duplex != DUPLEX_HALF &&
              duplex != DUPLEX_FULL)))
                return -EINVAL;

	if (phydev->validate_cmd && phydev->validate_cmd(cmd))
		return -EINVAL;

Thanks
    Benedikt Spranger

Re: [PATCH net v2] net: phy: phy_ethtool_ksettings_set: Don't discard phy_start_aneg's return

[Heiner Kallweit]

On 08.11.2021 20:02, Benedikt Spranger wrote:
> On Mon, 8 Nov 2021 19:01:23 +0100
> Heiner Kallweit <hkallweit1@gmail.com> wrote:
> 
>> If we would like to support PHY's that don't support all MDI modes
>> then supposedly this would require to add ETHTOOL_LINK_MODE bits for
>> the MDI modes. Then we could use the generic mechanism to check the
>> bits in the "supported" bitmap.
> 
> The things are even worse:
> The chip supports only auto-MDIX at Gigabit and force MDI and

The Gigabit part seems to be normal. Gigabit supports neither
forced mode nor forced MDI settings.

> auto-MDIX in 10/100 modes. No force MDIX at all.
> 
> A validation callback from phy_ethtool_ksettings_set() before
> restarting the PHY seems reasonable for me. Something like:
> 
> 	/* Verify the settings we care about. */
> 	if (autoneg != AUTONEG_ENABLE && autoneg != AUTONEG_DISABLE)
> 	        return -EINVAL;
> 
>         if (autoneg == AUTONEG_ENABLE && linkmode_empty(advertising))
> 	        return -EINVAL;
> 
>         if (autoneg == AUTONEG_DISABLE &&
> 	    ((speed != SPEED_1000 &&
> 	      speed != SPEED_100 &&
>               speed != SPEED_10) ||
>              (duplex != DUPLEX_HALF &&
>               duplex != DUPLEX_FULL)))
>                 return -EINVAL;
> 
> 	if (phydev->validate_cmd && phydev->validate_cmd(cmd))
> 		return -EINVAL;
> 
> Thanks
>     Benedikt Spranger
> 

Re: [PATCH net v2] net: phy: phy_ethtool_ksettings_set: Don't discard phy_start_aneg's return

[Florian Fainelli]

On 11/8/21 11:02 AM, Benedikt Spranger wrote:
> On Mon, 8 Nov 2021 19:01:23 +0100
> Heiner Kallweit <hkallweit1@gmail.com> wrote:
> 
>> If we would like to support PHY's that don't support all MDI modes
>> then supposedly this would require to add ETHTOOL_LINK_MODE bits for
>> the MDI modes. Then we could use the generic mechanism to check the
>> bits in the "supported" bitmap.
> 
> The things are even worse:
> The chip supports only auto-MDIX at Gigabit and force MDI and
> auto-MDIX in 10/100 modes. No force MDIX at all.

Yes that appears to be correct from my reading of the 53125 datasheet.
Force MDI-X is optional anyway AFAICT
--
Florian

Re: [PATCH] phy: phy_ethtool_ksettings_set: Don't discard phy_start_aneg's return

[Heiner Kallweit]

On 08.11.2021 15:25, Bastian Germann wrote:
> Am 06.11.21 um 22:52 schrieb Heiner Kallweit:
>> On 05.11.2021 16:36, bage@linutronix.de wrote:
>>> From: Bastian Germann <bage@linutronix.de>
>>>
>>> Take the return of phy_start_aneg into account so that ethtool will handle
>>> negotiation errors and not silently accept invalid input.
>>>
>>> Fixes: 2d55173e71b0 ("phy: add generic function to support ksetting support")
>>> Signed-off-by: Bastian Germann <bage@linutronix.de>
>>> Reviewed-by: Benedikt Spranger <b.spranger@linutronix.de>
>>
>> In addition to what Andrew said already:
>>
>> - This patch won't apply on net due to a4db9055fdb9.
> 
> Hi Heiner,
> 
> Thanks for your remarks. I would have gotten the first one right if the netdev
> tree was documented in the MAINTAINERS file (T: ...) and not only in netdev-FAQ.
> 
> Maybe you want to address that.
> 

I'd interpret the MAINTAINERS structure in a way that if no T: entry is given
the one from the next upper directory should be used. And for drivers/net/
the T: entries are available.

It's a typical case that a subsystem has a bigger number of drivers which
use the repo of the subsystem, and just few drivers have an own repo,
working with pull requests.
IMO having to add the subsystem T: entries to every driver entry would be
not needed overhead.

Not sure however which userspace tools use the T: entries and whether they are
smart enough to consider entry hierarchies.

> Thanks,
> Bastian
> 
>> - Patch misses the "net" annotation.
>> - Prefix should be "net: phy:", not "phy:".
>>
>> At least the formal aspects should have been covered by the internal review.
>>
>>> ---
>>>   drivers/net/phy/phy.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>>> index a3bfb156c83d..f740b533abba 100644
>>> --- a/drivers/net/phy/phy.c
>>> +++ b/drivers/net/phy/phy.c
>>> @@ -770,6 +770,8 @@ static int phy_poll_aneg_done(struct phy_device *phydev)
>>>   int phy_ethtool_ksettings_set(struct phy_device *phydev,
>>>                     const struct ethtool_link_ksettings *cmd)
>>>   {
>>> +    int ret = 0;
>>
>> Why initializing ret?
>>
>>> +
>>>       __ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
>>>       u8 autoneg = cmd->base.autoneg;
>>>       u8 duplex = cmd->base.duplex;
>>> @@ -815,10 +817,10 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
>>>       phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl;
>>>         /* Restart the PHY */
>>> -    _phy_start_aneg(phydev);
>>> +    ret = _phy_start_aneg(phydev);
>>>         mutex_unlock(&phydev->lock);
>>> -    return 0;
>>> +    return ret;
>>>   }
>>>   EXPORT_SYMBOL(phy_ethtool_ksettings_set);
>>>  
>>