[PATCH 0/1] Fix an issue with 'ethtool' configuring PHYs

[PATCH 0/1] Fix an issue with 'ethtool' configuring PHYs

[Arun Parameswaran]

Hi,

This patch fixes an issue while using the 'ethtool' to query/set PHY's
configurations. The 'ethtool' interface in the Kernel seem to always
query settings for PHY0, irrespective of the PHY specified, while
issuing the 'ethtool' command.

For example when issueing 'ethtool -s eth0 phyad 1 speed 100' command
to set the speed of PHY1's speed, the current software would first
retrieve PHY0 settings, then apply the request settings (in this case
'speed') on top. It then uses this (PHY0's config with new speed)
settings to update the PHY1, which now ends up with partial
configurations of PHY0 and the new speed.

This patch series is based on Linux v4.1-rc4 and is available in:
https://github.com/Broadcom/cygnus-linux/tree/net-core-ethtool-fix-v1

Arun Parameswaran (1):
  SWMAPYVR-4339 net: core: 'ethtool' issue with querying phy settings

 net/core/ethtool.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

-- 
1.7.9.5

[PATCH 1/1] net: core: 'ethtool' issue with querying phy settings

[Arun Parameswaran]

When trying to configure the settings for PHY1, using commands
like 'ethtool -s eth0 phyad 1 speed 100', the 'ethtool' seems to
modify other settings apart from the speed of the PHY1, in the
above case.

The ethtool seems to query the settings for PHY0, and use this
as the base to apply the new settings to the PHY1. This is
causing the other settings of the PHY 1 to be wrongly
configured.

The issue is caused by the '_ethtool_get_settings()' API, which
gets called because of the 'ETHTOOL_GSET' command, is clearing
the 'cmd' pointer (of type 'struct ethtool_cmd') by calling
memset. This clears all the parameters (if any) passed for the
'ETHTOOL_GSET' cmd. So the driver's callback is always invoked
with 'cmd->phy_address' as '0'.

The '_ethtool_get_settings()' is called from other files in the
'net/core'. So the fix is applied to the 'ethtool_get_settings()'
which is only called in the context of the 'ethtool'.

Signed-off-by: Arun Parameswaran <aparames@broadcom.com>
Reviewed-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
 net/core/ethtool.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 1d00b89..1347e11 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -359,7 +359,15 @@ static int ethtool_get_settings(struct net_device *dev, void __user *useraddr)
 	int err;
 	struct ethtool_cmd cmd;
 
-	err = __ethtool_get_settings(dev, &cmd);
+	if (!dev->ethtool_ops->get_settings)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
+		return -EFAULT;
+
+	cmd.cmd = ETHTOOL_GSET;
+
+	err = dev->ethtool_ops->get_settings(dev, &cmd);
 	if (err < 0)
 		return err;
 
-- 
1.7.9.5

Re: [PATCH 1/1] net: core: 'ethtool' issue with querying phy settings

[David Miller]

From: Arun Parameswaran <aparames@broadcom.com>
Date: Wed, 20 May 2015 14:35:30 -0700

> When trying to configure the settings for PHY1, using commands
> like 'ethtool -s eth0 phyad 1 speed 100', the 'ethtool' seems to
> modify other settings apart from the speed of the PHY1, in the
> above case.
> 
> The ethtool seems to query the settings for PHY0, and use this
> as the base to apply the new settings to the PHY1. This is
> causing the other settings of the PHY 1 to be wrongly
> configured.
> 
> The issue is caused by the '_ethtool_get_settings()' API, which
> gets called because of the 'ETHTOOL_GSET' command, is clearing
> the 'cmd' pointer (of type 'struct ethtool_cmd') by calling
> memset. This clears all the parameters (if any) passed for the
> 'ETHTOOL_GSET' cmd. So the driver's callback is always invoked
> with 'cmd->phy_address' as '0'.
> 
> The '_ethtool_get_settings()' is called from other files in the
> 'net/core'. So the fix is applied to the 'ethtool_get_settings()'
> which is only called in the context of the 'ethtool'.
> 
> Signed-off-by: Arun Parameswaran <aparames@broadcom.com>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>

Applied and queued up for -stable, thanks.

Re: [PATCH 1/1] net: core: 'ethtool' issue with querying phy settings

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1695 bytes --]

On Fri, 2015-05-22 at 16:15 -0400, David Miller wrote:
> From: Arun Parameswaran <aparames@broadcom.com>
> Date: Wed, 20 May 2015 14:35:30 -0700
> 
> > When trying to configure the settings for PHY1, using commands
> > like 'ethtool -s eth0 phyad 1 speed 100', the 'ethtool' seems to
> > modify other settings apart from the speed of the PHY1, in the
> > above case.
> > 
> > The ethtool seems to query the settings for PHY0, and use this
> > as the base to apply the new settings to the PHY1. This is
> > causing the other settings of the PHY 1 to be wrongly
> > configured.
> > 
> > The issue is caused by the '_ethtool_get_settings()' API, which
> > gets called because of the 'ETHTOOL_GSET' command, is clearing
> > the 'cmd' pointer (of type 'struct ethtool_cmd') by calling
> > memset. This clears all the parameters (if any) passed for the
> > 'ETHTOOL_GSET' cmd. So the driver's callback is always invoked
> > with 'cmd->phy_address' as '0'.
> > 
> > The '_ethtool_get_settings()' is called from other files in the
> > 'net/core'. So the fix is applied to the 'ethtool_get_settings()'
> > which is only called in the context of the 'ethtool'.
> > 
> > Signed-off-by: Arun Parameswaran <aparames@broadcom.com>
> > Reviewed-by: Ray Jui <rjui@broadcom.com>
> > Reviewed-by: Scott Branden <sbranden@broadcom.com>
> 
> Applied and queued up for -stable, thanks.

Please revert this.  This is an incompatible API change, not a bug fix.
The established semantics are that 'phyad' is filled in by the driver;
it is not a parameter to the ETHTOOL_GSET command.

Ben.

-- 
Ben Hutchings
Reality is just a crutch for people who can't handle science fiction.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 1/1] net: core: 'ethtool' issue with querying phy settings

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Sun, 31 May 2015 20:54:06 +0100

> On Fri, 2015-05-22 at 16:15 -0400, David Miller wrote:
>> From: Arun Parameswaran <aparames@broadcom.com>
>> Date: Wed, 20 May 2015 14:35:30 -0700
>> 
>> > When trying to configure the settings for PHY1, using commands
>> > like 'ethtool -s eth0 phyad 1 speed 100', the 'ethtool' seems to
>> > modify other settings apart from the speed of the PHY1, in the
>> > above case.
>> > 
>> > The ethtool seems to query the settings for PHY0, and use this
>> > as the base to apply the new settings to the PHY1. This is
>> > causing the other settings of the PHY 1 to be wrongly
>> > configured.
>> > 
>> > The issue is caused by the '_ethtool_get_settings()' API, which
>> > gets called because of the 'ETHTOOL_GSET' command, is clearing
>> > the 'cmd' pointer (of type 'struct ethtool_cmd') by calling
>> > memset. This clears all the parameters (if any) passed for the
>> > 'ETHTOOL_GSET' cmd. So the driver's callback is always invoked
>> > with 'cmd->phy_address' as '0'.
>> > 
>> > The '_ethtool_get_settings()' is called from other files in the
>> > 'net/core'. So the fix is applied to the 'ethtool_get_settings()'
>> > which is only called in the context of the 'ethtool'.
>> > 
>> > Signed-off-by: Arun Parameswaran <aparames@broadcom.com>
>> > Reviewed-by: Ray Jui <rjui@broadcom.com>
>> > Reviewed-by: Scott Branden <sbranden@broadcom.com>
>> 
>> Applied and queued up for -stable, thanks.
> 
> Please revert this.  This is an incompatible API change, not a bug fix.
> The established semantics are that 'phyad' is filled in by the driver;
> it is not a parameter to the ETHTOOL_GSET command.

But then how in the world can the user specify specific PHY ADs for
a device that will respond to more than one?

Re: [PATCH 1/1] net: core: 'ethtool' issue with querying phy settings

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2661 bytes --]

On Sun, 2015-05-31 at 17:19 -0700, David Miller wrote:
> From: Ben Hutchings <ben@decadent.org.uk>
> Date: Sun, 31 May 2015 20:54:06 +0100
> 
> > On Fri, 2015-05-22 at 16:15 -0400, David Miller wrote:
> >> From: Arun Parameswaran <aparames@broadcom.com>
> >> Date: Wed, 20 May 2015 14:35:30 -0700
> >> 
> >> > When trying to configure the settings for PHY1, using commands
> >> > like 'ethtool -s eth0 phyad 1 speed 100', the 'ethtool' seems to
> >> > modify other settings apart from the speed of the PHY1, in the
> >> > above case.
> >> > 
> >> > The ethtool seems to query the settings for PHY0, and use this
> >> > as the base to apply the new settings to the PHY1. This is
> >> > causing the other settings of the PHY 1 to be wrongly
> >> > configured.
> >> > 
> >> > The issue is caused by the '_ethtool_get_settings()' API, which
> >> > gets called because of the 'ETHTOOL_GSET' command, is clearing
> >> > the 'cmd' pointer (of type 'struct ethtool_cmd') by calling
> >> > memset. This clears all the parameters (if any) passed for the
> >> > 'ETHTOOL_GSET' cmd. So the driver's callback is always invoked
> >> > with 'cmd->phy_address' as '0'.
> >> > 
> >> > The '_ethtool_get_settings()' is called from other files in the
> >> > 'net/core'. So the fix is applied to the 'ethtool_get_settings()'
> >> > which is only called in the context of the 'ethtool'.
> >> > 
> >> > Signed-off-by: Arun Parameswaran <aparames@broadcom.com>
> >> > Reviewed-by: Ray Jui <rjui@broadcom.com>
> >> > Reviewed-by: Scott Branden <sbranden@broadcom.com>
> >> 
> >> Applied and queued up for -stable, thanks.
> > 
> > Please revert this.  This is an incompatible API change, not a bug fix.
> > The established semantics are that 'phyad' is filled in by the driver;
> > it is not a parameter to the ETHTOOL_GSET command.
> 
> But then how in the world can the user specify specific PHY ADs for
> a device that will respond to more than one?

ETHTOOL_SSET sets the current PHY address and ETHTOOL_GSET gets it.

If multiple PHYs need to be configured for a single link then the driver
should configure them all at the same time rather than making it the
administrator's problem.

What we can't support with the current API are:
- multiple physical links behind a single net device (different
  configuration possible for each link)
- multiple PHYs are needed for a single link, and the driver can't
  automatically decide which to use (multiple addresses to set)

Ben.

-- 
Ben Hutchings
Power corrupts.  Absolute power is kind of neat.
                           - John Lehman, Secretary of the US Navy 1981-1987

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 1/1] net: core: 'ethtool' issue with querying phy settings

[Arun Parameswaran]

On 15-06-01 11:05 AM, Ben Hutchings wrote:
> On Sun, 2015-05-31 at 17:19 -0700, David Miller wrote:
>> From: Ben Hutchings <ben@decadent.org.uk>
>> Date: Sun, 31 May 2015 20:54:06 +0100
>>
>>> On Fri, 2015-05-22 at 16:15 -0400, David Miller wrote:
>>>> From: Arun Parameswaran <aparames@broadcom.com>
>>>> Date: Wed, 20 May 2015 14:35:30 -0700
>>>>
>>>>> When trying to configure the settings for PHY1, using commands
>>>>> like 'ethtool -s eth0 phyad 1 speed 100', the 'ethtool' seems to
>>>>> modify other settings apart from the speed of the PHY1, in the
>>>>> above case.
>>>>>
>>>>> The ethtool seems to query the settings for PHY0, and use this
>>>>> as the base to apply the new settings to the PHY1. This is
>>>>> causing the other settings of the PHY 1 to be wrongly
>>>>> configured.
>>>>>
>>>>> The issue is caused by the '_ethtool_get_settings()' API, which
>>>>> gets called because of the 'ETHTOOL_GSET' command, is clearing
>>>>> the 'cmd' pointer (of type 'struct ethtool_cmd') by calling
>>>>> memset. This clears all the parameters (if any) passed for the
>>>>> 'ETHTOOL_GSET' cmd. So the driver's callback is always invoked
>>>>> with 'cmd->phy_address' as '0'.
>>>>>
>>>>> The '_ethtool_get_settings()' is called from other files in the
>>>>> 'net/core'. So the fix is applied to the 'ethtool_get_settings()'
>>>>> which is only called in the context of the 'ethtool'.
>>>>>
>>>>> Signed-off-by: Arun Parameswaran <aparames@broadcom.com>
>>>>> Reviewed-by: Ray Jui <rjui@broadcom.com>
>>>>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
>>>>
>>>> Applied and queued up for -stable, thanks.
>>>
>>> Please revert this.  This is an incompatible API change, not a bug fix.
>>> The established semantics are that 'phyad' is filled in by the driver;
>>> it is not a parameter to the ETHTOOL_GSET command.
>>
>> But then how in the world can the user specify specific PHY ADs for
>> a device that will respond to more than one?
> 
> ETHTOOL_SSET sets the current PHY address and ETHTOOL_GSET gets it.
> 
> If multiple PHYs need to be configured for a single link then the driver
> should configure them all at the same time rather than making it the
> administrator's problem.
> 
> What we can't support with the current API are:
> - multiple physical links behind a single net device (different
>   configuration possible for each link)
> - multiple PHYs are needed for a single link, and the driver can't
>   automatically decide which to use (multiple addresses to set)
> 
> Ben.
> 
The patch doesn't affect the current functionality except that the
programs/'ethtool' using the interface needs to ensure the command
structure integrity.

Kernel should transport the data to and from the driver and let the
programs/ethtool handle the data/command. It is better if the Kernel
doesn't manipulate the command/data being requested/passed, in this
case it is the hard-coding of PHY id (by the memset). This adds
flexibility to the interface.

This patch along with the change suggested in the 'ethtool' app
(separate patch set sent for the app), provides the flexibility to be
able to query/set a specific PHY irrespective of the driver design.

In our SoC, we have one MAC connected to multiple PHYs with an internal
switch in between.
We have only one net device as we can have only one MAC address and this
better reflects the hardware state.

It would be nice for the 'ethtool' to be flexible to support querying
specific PHY irrespective of the net implementation, but that is being
discussed in the other thread.


Thanks
Arun

Re: [PATCH 1/1] net: core: 'ethtool' issue with querying phy settings

[David Miller]

From: Arun Parameswaran <aparames@broadcom.com>
Date: Mon, 1 Jun 2015 14:41:43 -0700

> It would be nice for the 'ethtool' to be flexible to support querying
> specific PHY irrespective of the net implementation, but that is being
> discussed in the other thread.

Please stop arguing about this, it isn't valid.

Your device is a switch, and therefore needs to be represented properly
with the proper number of net_device objects.

Even more importantly, the ethtool API is established and you cannot
change these semantics without potentially breaking lots of applications
and libraries out there.

Your change is reverted, and I will absolutely not entertain any
attempt to again change the semantics of this ethtool operation.

Thanks.

Re: [PATCH 1/1] net: core: 'ethtool' issue with querying phy settings

[Arun Parameswaran]

On 15-06-01 02:46 PM, David Miller wrote:
> From: Arun Parameswaran <aparames@broadcom.com>
> Date: Mon, 1 Jun 2015 14:41:43 -0700
> 
>> It would be nice for the 'ethtool' to be flexible to support querying
>> specific PHY irrespective of the net implementation, but that is being
>> discussed in the other thread.
> 
> Please stop arguing about this, it isn't valid.
> 
> Your device is a switch, and therefore needs to be represented properly
> with the proper number of net_device objects.
> 
> Even more importantly, the ethtool API is established and you cannot
> change these semantics without potentially breaking lots of applications
> and libraries out there.
> 
> Your change is reverted, and I will absolutely not entertain any
> attempt to again change the semantics of this ethtool operation.
> 
> Thanks.
> 
I apologize if the patch broke any conventions, it was not my intend. I
understand the implications on other programs that use the interface.

Just so that I don’t make this mistake in the future and to understand this
better, does this mean that the 'phyad' parameter specified in the
'ethtool' command line is ignored ?

Thanks
Arun

Re: [PATCH 1/1] net: core: 'ethtool' issue with querying phy settings

[David Miller]

From: Arun Parameswaran <aparames@broadcom.com>
Date: Mon, 1 Jun 2015 16:53:19 -0700

> Just so that I don’t make this mistake in the future and to
> understand this better, does this mean that the 'phyad' parameter
> specified in the 'ethtool' command line is ignored ?

It cannot serve any valid purpose for a GET, so yes.

Re: [PATCH 1/1] net: core: 'ethtool' issue with querying phy settings

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1798 bytes --]

On Mon, 2015-06-01 at 16:53 -0700, Arun Parameswaran wrote:
> On 15-06-01 02:46 PM, David Miller wrote:
> > From: Arun Parameswaran <aparames@broadcom.com>
> > Date: Mon, 1 Jun 2015 14:41:43 -0700
> > 
> >> It would be nice for the 'ethtool' to be flexible to support querying
> >> specific PHY irrespective of the net implementation, but that is being
> >> discussed in the other thread.
> > 
> > Please stop arguing about this, it isn't valid.
> > 
> > Your device is a switch, and therefore needs to be represented properly
> > with the proper number of net_device objects.
> > 
> > Even more importantly, the ethtool API is established and you cannot
> > change these semantics without potentially breaking lots of applications
> > and libraries out there.
> > 
> > Your change is reverted, and I will absolutely not entertain any
> > attempt to again change the semantics of this ethtool operation.
> > 
> > Thanks.
> > 
> I apologize if the patch broke any conventions, it was not my intend. I
> understand the implications on other programs that use the interface.
> 
> Just so that I don’t make this mistake in the future and to understand this
> better, does this mean that the 'phyad' parameter specified in the
> 'ethtool' command line is ignored ?

There are some very old Ethernet cards where the driver can't
automatically detect which PHY address to use, or where the MAC can be
attached to one of several different PHYs.  This is what the 'phyad'
parameter is used for.  I think the only remaining driver that supports
changing the PHY address like this is natsemi.

Ben.

-- 
Ben Hutchings
The obvious mathematical breakthrough [to break modern encryption] would be
development of an easy way to factor large prime numbers. - Bill Gates

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]