[PATCH 0/2] Fix couple of issues with 'ethtool' get/set API's

[PATCH 0/2] Fix couple of issues with 'ethtool' get/set API's

[Arun Parameswaran]

Hi,
The patch fixes 2 issues with 'ethtool' getting/setting parametres in
the do_gset() do_sset() API's.

I have pushed a patch to the Kernel to fix an issue in the handling of
the 'ethtool' commands which got accepted.
This Kernel patch was based on Linux v4.1-rc4 and is available in:
https://github.com/Broadcom/cygnus-linux/tree/net-core-ethtool-fix-v1

The Kernel was always clearing the command from the 'ethtool' resulting
in all operations to deal with PHY0. This prevents querying/setting
PHY 1's settings.

The first patch is required due to the fact that Kernel will not be
clearing the command data structure. Without this the query will fail
as the command will get to the Kernel with arbitary phyad info.

The second patch is to fix the issue in which the 'ethtool' gets the
settings of the wrong phyad before applying the new settings.

These changes will still work with older Kernel's (as the Kernel
clears the command data structure).

Thanks

Arun Parameswaran (2):
  ethtool: Clear the command data structure before sending requests
  ethtool: Fix an issue with handling 'phyad' while updating settings

 ethtool.c |    9 +++++++++
 1 file changed, 9 insertions(+)

-- 
1.7.9.5

[PATCH 1/2] ethtool: Clear the command data structure before sending requests

[Arun Parameswaran]

The 'ethtool' is not clearing the command data structure in the
do_gset() & do_sset() API's while sending commands to get/set
parametres. This used to work since the Kernel clears the
data structure (which was a bug and is being fixed).

This patch adds a 'memset' to the do_gset() & do_sset() to clear
the command data structure. This will prevent the 'ethtool' from
passing wrong parametres, for example incorrect 'phyad'.

Signed-off-by: Arun Parameswaran <aparames@broadcom.com>
Reviewed-by: JD (Jiandong) Zheng <jdzheng@broadcom.com>
---
 ethtool.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/ethtool.c b/ethtool.c
index 01b13a6..668c189 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -2275,6 +2275,7 @@ static int do_gset(struct cmd_context *ctx)
 
 	fprintf(stdout, "Settings for %s:\n", ctx->devname);
 
+	memset(&ecmd, 0, sizeof(struct ethtool_cmd));
 	ecmd.cmd = ETHTOOL_GSET;
 	err = send_ioctl(ctx, &ecmd);
 	if (err == 0) {
@@ -2520,6 +2521,8 @@ static int do_sset(struct cmd_context *ctx)
 	if (gset_changed) {
 		struct ethtool_cmd ecmd;
 
+		memset(&ecmd, 0, sizeof(struct ethtool_cmd));
+
 		ecmd.cmd = ETHTOOL_GSET;
 		err = send_ioctl(ctx, &ecmd);
 		if (err < 0) {
-- 
1.7.9.5

[PATCH 2/2] ethtool: Fix an issue with handling 'phyad' while updating settings

[Arun Parameswaran]

When using the 'ethtool' to set PHY1's settings, the 'ethtool'
seems to do the 'get' settings with incorrect PHY ID and then
apply the new settings on it.

For example when using the command, to change PHY1's speed,
'ethtool -s eth0 phyad 1 speed 100', the 'ethtool' will end up
querying PHY0's settings and then apply the new settings specified
in the command on top of the PHY0's settings. So the PHY1 will
end up with worng settings (partial settings of PHY0 for example).

This patch fixes the issue by specifying the 'phyad' or 'port'
requested in the command to query for the settings, before applying
the new ones.

Signed-off-by: Arun Parameswaran <aparames@broadcom.com>
Reviewed-by: JD (Jiandong) Zheng <jdzheng@broadcom.com>
---
 ethtool.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/ethtool.c b/ethtool.c
index 668c189..6c57297 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -2523,6 +2523,12 @@ static int do_sset(struct cmd_context *ctx)
 
 		memset(&ecmd, 0, sizeof(struct ethtool_cmd));
 
+		if (phyad_wanted != -1)
+			ecmd.phy_address = phyad_wanted;
+
+		if (port_wanted != -1)
+			ecmd.port = port_wanted;
+
 		ecmd.cmd = ETHTOOL_GSET;
 		err = send_ioctl(ctx, &ecmd);
 		if (err < 0) {
-- 
1.7.9.5

Re: [PATCH 0/2] Fix couple of issues with 'ethtool' get/set API's

[Ben Hutchings]

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

On Fri, 2015-05-22 at 15:43 -0700, Arun Parameswaran wrote:
> Hi,
> The patch fixes 2 issues with 'ethtool' getting/setting parametres in
> the do_gset() do_sset() API's.
> 
> I have pushed a patch to the Kernel to fix an issue in the handling of
> the 'ethtool' commands which got accepted.
> This Kernel patch was based on Linux v4.1-rc4 and is available in:
> https://github.com/Broadcom/cygnus-linux/tree/net-core-ethtool-fix-v1
> 
> The Kernel was always clearing the command from the 'ethtool' resulting
> in all operations to deal with PHY0. This prevents querying/setting
> PHY 1's settings.
[...]

Each net device can be associated with a single PHY at a time, and the
ETHTOOL_GSET implementation should fill in the PHY address in the
ethtool_cmd::phy_address field.  Where there are multiple PHYs that can
be connected to the net device's MAC, an ETHTOOL_SSET operation can be
used to change that PHY address.

The ethtool API is not meant for controlling other PHYs that aren't
connected to the MAC; if you want to do that then create more net
devices for them or use the MDIO ioctls.

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 0/2] Fix couple of issues with 'ethtool' get/set API's

[Arun Parameswaran]

On 15-05-31 12:59 PM, Ben Hutchings wrote:
> On Fri, 2015-05-22 at 15:43 -0700, Arun Parameswaran wrote:
>> Hi,
>> The patch fixes 2 issues with 'ethtool' getting/setting parametres in
>> the do_gset() do_sset() API's.
>>
>> I have pushed a patch to the Kernel to fix an issue in the handling of
>> the 'ethtool' commands which got accepted.
>> This Kernel patch was based on Linux v4.1-rc4 and is available in:
>> https://github.com/Broadcom/cygnus-linux/tree/net-core-ethtool-fix-v1
>>
>> The Kernel was always clearing the command from the 'ethtool' resulting
>> in all operations to deal with PHY0. This prevents querying/setting
>> PHY 1's settings.
> [...]
> 
> Each net device can be associated with a single PHY at a time, and the
> ETHTOOL_GSET implementation should fill in the PHY address in the
> ethtool_cmd::phy_address field.  Where there are multiple PHYs that can
> be connected to the net device's MAC, an ETHTOOL_SSET operation can be
> used to change that PHY address.
> 
The above can be done by the driver when there is one PHY per MAC. In our
case we have multiple PHYs controlled by the same MAC. I should have
clarified this earlier, I apologize.

When we specify the 'phyad', in the command line, we were expecting the
'ethtool' to fetch/set data for that 'phyad'. This is the intend of the
patch.

With the patch (in 'ethtool' and Kernel), if 'phyad' is not specified, it
will still function as you described above, it will be up to the driver to
return the proper 'phyad' and related settings.

> The ethtool API is not meant for controlling other PHYs that aren't
> connected to the MAC; if you want to do that then create more net
> devices for them or use the MDIO ioctls.
> 
In the SoC, there are multiple PHYs (in our case there are 2) controlled
by the same MAC. We are trying to use 'ethtool' to control both the PHYs
connected to the same MAC.

> 
> Ben.
> 

Thanks
Arun

Re: [PATCH 0/2] Fix couple of issues with 'ethtool' get/set API's

[Ben Hutchings]

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

On Mon, 2015-06-01 at 10:14 -0700, Arun Parameswaran wrote:
> On 15-05-31 12:59 PM, Ben Hutchings wrote:
> > On Fri, 2015-05-22 at 15:43 -0700, Arun Parameswaran wrote:
> >> Hi,
> >> The patch fixes 2 issues with 'ethtool' getting/setting parametres in
> >> the do_gset() do_sset() API's.
> >>
> >> I have pushed a patch to the Kernel to fix an issue in the handling of
> >> the 'ethtool' commands which got accepted.
> >> This Kernel patch was based on Linux v4.1-rc4 and is available in:
> >> https://github.com/Broadcom/cygnus-linux/tree/net-core-ethtool-fix-v1
> >>
> >> The Kernel was always clearing the command from the 'ethtool' resulting
> >> in all operations to deal with PHY0. This prevents querying/setting
> >> PHY 1's settings.
> > [...]
> > 
> > Each net device can be associated with a single PHY at a time, and the
> > ETHTOOL_GSET implementation should fill in the PHY address in the
> > ethtool_cmd::phy_address field.  Where there are multiple PHYs that can
> > be connected to the net device's MAC, an ETHTOOL_SSET operation can be
> > used to change that PHY address.
> > 
> The above can be done by the driver when there is one PHY per MAC. In our
> case we have multiple PHYs controlled by the same MAC. I should have
> clarified this earlier, I apologize.

I understand that you can have multiple PHYs on the same MDIO bus, but
not how the MAC can use them at the same time.  Is this hardware level
bonding?  Or are multiple PHYs needed for a single link?

> When we specify the 'phyad', in the command line, we were expecting the
> 'ethtool' to fetch/set data for that 'phyad'. This is the intend of the
> patch.
> 
> With the patch (in 'ethtool' and Kernel), if 'phyad' is not specified, it
> will still function as you described above, it will be up to the driver to
> return the proper 'phyad' and related settings.
[...]

But without the patch in ethtool and other programs calling this API
(it's not just the ethtool command!), you get random junk as the
phy_address.  How will you tell whether it's valid or not?

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 0/2] Fix couple of issues with 'ethtool' get/set API's

[Arun Parameswaran]

On 15-06-01 11:07 AM, Ben Hutchings wrote:
> On Mon, 2015-06-01 at 10:14 -0700, Arun Parameswaran wrote:
>> On 15-05-31 12:59 PM, Ben Hutchings wrote:
>>> On Fri, 2015-05-22 at 15:43 -0700, Arun Parameswaran wrote:
>>>> Hi,
>>>> The patch fixes 2 issues with 'ethtool' getting/setting parametres in
>>>> the do_gset() do_sset() API's.
>>>>
>>>> I have pushed a patch to the Kernel to fix an issue in the handling of
>>>> the 'ethtool' commands which got accepted.
>>>> This Kernel patch was based on Linux v4.1-rc4 and is available in:
>>>> https://github.com/Broadcom/cygnus-linux/tree/net-core-ethtool-fix-v1
>>>>
>>>> The Kernel was always clearing the command from the 'ethtool' resulting
>>>> in all operations to deal with PHY0. This prevents querying/setting
>>>> PHY 1's settings.
>>> [...]
>>>
>>> Each net device can be associated with a single PHY at a time, and the
>>> ETHTOOL_GSET implementation should fill in the PHY address in the
>>> ethtool_cmd::phy_address field.  Where there are multiple PHYs that can
>>> be connected to the net device's MAC, an ETHTOOL_SSET operation can be
>>> used to change that PHY address.
>>>
>> The above can be done by the driver when there is one PHY per MAC. In our
>> case we have multiple PHYs controlled by the same MAC. I should have
>> clarified this earlier, I apologize.
> 
> I understand that you can have multiple PHYs on the same MDIO bus, but
> not how the MAC can use them at the same time.  Is this hardware level
> bonding?  Or are multiple PHYs needed for a single link?
> 
We have an internal switch which manages the traffic to the PHY's (ports).
There is 1 PHY per external port.
The MAC is connected to the internal port of the switch.

>> When we specify the 'phyad', in the command line, we were expecting the
>> 'ethtool' to fetch/set data for that 'phyad'. This is the intend of the
>> patch.
>>
>> With the patch (in 'ethtool' and Kernel), if 'phyad' is not specified, it
>> will still function as you described above, it will be up to the driver to
>> return the proper 'phyad' and related settings.
> [...]
> 
> But without the patch in ethtool and other programs calling this API
> (it's not just the ethtool command!), you get random junk as the
> phy_address.  How will you tell whether it's valid or not?
Yes, if other programs make use of the 'ethtool' commands, they will
have to clear the data structure. I think that the clearing of the command
data structure should be the responsibility of the programs calling it.
I understand that the programs didn't have to do it as Kernel was clearing
the command structure for them.

But this prevents the 'ethtool' from being used to get/set data of
specific PHY's.

> 
> Ben.
> 

Thanks
Arun

Re: [PATCH 0/2] Fix couple of issues with 'ethtool' get/set API's

[Ben Hutchings]

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

On Mon, 2015-06-01 at 12:12 -0700, Arun Parameswaran wrote:
> On 15-06-01 11:07 AM, Ben Hutchings wrote:
> > On Mon, 2015-06-01 at 10:14 -0700, Arun Parameswaran wrote:
> >> On 15-05-31 12:59 PM, Ben Hutchings wrote:
> >>> On Fri, 2015-05-22 at 15:43 -0700, Arun Parameswaran wrote:
> >>>> Hi,
> >>>> The patch fixes 2 issues with 'ethtool' getting/setting parametres in
> >>>> the do_gset() do_sset() API's.
> >>>>
> >>>> I have pushed a patch to the Kernel to fix an issue in the handling of
> >>>> the 'ethtool' commands which got accepted.
> >>>> This Kernel patch was based on Linux v4.1-rc4 and is available in:
> >>>> https://github.com/Broadcom/cygnus-linux/tree/net-core-ethtool-fix-v1
> >>>>
> >>>> The Kernel was always clearing the command from the 'ethtool' resulting
> >>>> in all operations to deal with PHY0. This prevents querying/setting
> >>>> PHY 1's settings.
> >>> [...]
> >>>
> >>> Each net device can be associated with a single PHY at a time, and the
> >>> ETHTOOL_GSET implementation should fill in the PHY address in the
> >>> ethtool_cmd::phy_address field.  Where there are multiple PHYs that can
> >>> be connected to the net device's MAC, an ETHTOOL_SSET operation can be
> >>> used to change that PHY address.
> >>>
> >> The above can be done by the driver when there is one PHY per MAC. In our
> >> case we have multiple PHYs controlled by the same MAC. I should have
> >> clarified this earlier, I apologize.
> > 
> > I understand that you can have multiple PHYs on the same MDIO bus, but
> > not how the MAC can use them at the same time.  Is this hardware level
> > bonding?  Or are multiple PHYs needed for a single link?
> > 
> We have an internal switch which manages the traffic to the PHY's (ports).
> There is 1 PHY per external port.
> The MAC is connected to the internal port of the switch.

Then you should create net devices for those external ports as well as
the internal port.

If I understand the switchdev API rightly, the external port devices
should implement the ethtool {get,set}_settings operations and the
ndo_switch_parent_id_get operation.  The existing net device should
expose only the internal link to the switch (which presumably isn't
configurable at all).

[...]
> But this prevents the 'ethtool' from being used to get/set data of
> specific PHY's.

That is fine because it is meant to manage the net device's own link (in
this case, the internal port), not other switch ports.

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 0/2] Fix couple of issues with 'ethtool' get/set API's

[Arun Parameswaran]

On 15-06-01 12:29 PM, Ben Hutchings wrote:
> On Mon, 2015-06-01 at 12:12 -0700, Arun Parameswaran wrote:
>> On 15-06-01 11:07 AM, Ben Hutchings wrote:
>>> On Mon, 2015-06-01 at 10:14 -0700, Arun Parameswaran wrote:
>>>> On 15-05-31 12:59 PM, Ben Hutchings wrote:
>>>>> On Fri, 2015-05-22 at 15:43 -0700, Arun Parameswaran wrote:
>>>>>> Hi,
>>>>>> The patch fixes 2 issues with 'ethtool' getting/setting parametres in
>>>>>> the do_gset() do_sset() API's.
>>>>>>
>>>>>> I have pushed a patch to the Kernel to fix an issue in the handling of
>>>>>> the 'ethtool' commands which got accepted.
>>>>>> This Kernel patch was based on Linux v4.1-rc4 and is available in:
>>>>>> https://github.com/Broadcom/cygnus-linux/tree/net-core-ethtool-fix-v1
>>>>>>
>>>>>> The Kernel was always clearing the command from the 'ethtool' resulting
>>>>>> in all operations to deal with PHY0. This prevents querying/setting
>>>>>> PHY 1's settings.
>>>>> [...]
>>>>>
>>>>> Each net device can be associated with a single PHY at a time, and the
>>>>> ETHTOOL_GSET implementation should fill in the PHY address in the
>>>>> ethtool_cmd::phy_address field.  Where there are multiple PHYs that can
>>>>> be connected to the net device's MAC, an ETHTOOL_SSET operation can be
>>>>> used to change that PHY address.
>>>>>
>>>> The above can be done by the driver when there is one PHY per MAC. In our
>>>> case we have multiple PHYs controlled by the same MAC. I should have
>>>> clarified this earlier, I apologize.
>>>
>>> I understand that you can have multiple PHYs on the same MDIO bus, but
>>> not how the MAC can use them at the same time.  Is this hardware level
>>> bonding?  Or are multiple PHYs needed for a single link?
>>>
>> We have an internal switch which manages the traffic to the PHY's (ports).
>> There is 1 PHY per external port.
>> The MAC is connected to the internal port of the switch.
> 
> Then you should create net devices for those external ports as well as
> the internal port.
We can only have one MAC address as we have only one MAC. So we prefer to
have a single net device which reflects the hardware better.

> 
> If I understand the switchdev API rightly, the external port devices
> should implement the ethtool {get,set}_settings operations and the
> ndo_switch_parent_id_get operation.  The existing net device should
> expose only the internal link to the switch (which presumably isn't
> configurable at all).
I agree, there can be an implementation like the above.
The switch can be handled in multiple ways, in our case we use one net
device as we are limited by the MAC.

Since the 'ethtool' provides the 'phyad' parameter to specifically
address a particular PHY, I think the 'ethtool' should allow for this,
irrespective of the net implementation. Wouldn't this make the 'ethtool'
flexible and detached from the network driver implementation.

> 
> [...]
>> But this prevents the 'ethtool' from being used to get/set data of
>> specific PHY's.
> 
> That is fine because it is meant to manage the net device's own link (in
> this case, the internal port), not other switch ports.
> 
> Ben.
> 

Re: [PATCH 0/2] Fix couple of issues with 'ethtool' get/set API's

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Mon, 01 Jun 2015 20:29:23 +0100

> On Mon, 2015-06-01 at 12:12 -0700, Arun Parameswaran wrote:
>> But this prevents the 'ethtool' from being used to get/set data of
>> specific PHY's.
> 
> That is fine because it is meant to manage the net device's own link (in
> this case, the internal port), not other switch ports.

Ok I'm convinced, I will revert the change in question.

Thanks Ben.