[RFC PATCH 1/1] ethtool: adding support for multiple slave port configuration

[RFC PATCH 1/1] ethtool: adding support for multiple slave port configuration

[Mugunthan V N]

Some Ethernet Swtich controllers like CPSW in AM335x, TI814x, DRA7x and
AM43xx SoCs, Network Coprocessor in AM5K2E0x, Realtek Switch controllers
etc has to capability of conneting multiple networks using L2 switching
and has multiple phys. With the existing code, ethtool can communicate
only to one phy.

To enable user to communicate multiple phy connected to single Ethernet
Switch controller, intoducing a optional new parameter in Ethtool interface
to pass which slave to set/get the phy configuration.

Signed-off-by: Mugunthan V N <mugunthanvnm-l0cyMroinI0@public.gmane.org>
---
 include/uapi/linux/ethtool.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 96ade34..3011427 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -60,6 +60,9 @@
  *	and other link features that the link partner advertised
  *	through autonegotiation; 0 if unknown or not applicable.
  *	Read-only.
+ * @slave_port: Specify which slave port to be used to set/get
+ *	parmeters, for example which slave port phy to be used for
+ *	set/get phy capabilities
  *
  * The link speed in Mbps is split between @speed and @speed_hi.  Use
  * the ethtool_cmd_speed() and ethtool_cmd_speed_set() functions to
@@ -107,7 +110,8 @@ struct ethtool_cmd {
 	__u8	eth_tp_mdix;
 	__u8	eth_tp_mdix_ctrl;
 	__u32	lp_advertising;
-	__u32	reserved[2];
+	__u32	slave_port;
+	__u32	reserved;
 };
 
 static inline void ethtool_cmd_speed_set(struct ethtool_cmd *ep,
-- 
2.0.2.673.g9ab0882

Re: [RFC PATCH 1/1] ethtool: adding support for multiple slave port configuration

[Ben Hutchings]

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

On Fri, 2014-07-25 at 17:58 +0530, Mugunthan V N wrote:
> Some Ethernet Swtich controllers like CPSW in AM335x, TI814x, DRA7x and
> AM43xx SoCs, Network Coprocessor in AM5K2E0x, Realtek Switch controllers
> etc has to capability of conneting multiple networks using L2 switching
> and has multiple phys. With the existing code, ethtool can communicate
> only to one phy.
> 
> To enable user to communicate multiple phy connected to single Ethernet
> Switch controller, intoducing a optional new parameter in Ethtool interface
> to pass which slave to set/get the phy configuration.

There was some discussion about configuration APIs for hardware/firmware
bridges earlier this year and I thought there was a consensus for
assigning a network device to each port.  This would remove the need to
identify ports within a device.  But I may have misremembered.

> Signed-off-by: Mugunthan V N <mugunthanvnm-l0cyMroinI0@public.gmane.org>
> ---
>  include/uapi/linux/ethtool.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 96ade34..3011427 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -60,6 +60,9 @@
>   *	and other link features that the link partner advertised
>   *	through autonegotiation; 0 if unknown or not applicable.
>   *	Read-only.
> + * @slave_port: Specify which slave port to be used to set/get
> + *	parmeters, for example which slave port phy to be used for
> + *	set/get phy capabilities

The difficulty with assigning the reserved fields in struct ethtool_cmd
is that nothing has ever checked that they are set to 0.  So if we were
to assign this field and support it in ethtool, someone might run it on
an older kernel version and all configuration changes will be made to
port 0 rather than the one they specified.  I don't think it would be
acceptable to tell users that 'oh, the port number option silently fails
on older kernel versions'.

So at the very least you would also need to add some way for userland to
find out whether the driver will check the value of this field.

Ben.

>   * The link speed in Mbps is split between @speed and @speed_hi.  Use
>   * the ethtool_cmd_speed() and ethtool_cmd_speed_set() functions to
> @@ -107,7 +110,8 @@ struct ethtool_cmd {
>  	__u8	eth_tp_mdix;
>  	__u8	eth_tp_mdix_ctrl;
>  	__u32	lp_advertising;
> -	__u32	reserved[2];
> +	__u32	slave_port;
> +	__u32	reserved;
>  };
>  
>  static inline void ethtool_cmd_speed_set(struct ethtool_cmd *ep,

-- 
Ben Hutchings
The generation of random numbers is too important to be left to chance.
                                                            - Robert Coveyou

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

Re: [RFC PATCH 1/1] ethtool: adding support for multiple slave port configuration

[John Fastabend]

On 07/26/2014 07:47 PM, Ben Hutchings wrote:
> On Fri, 2014-07-25 at 17:58 +0530, Mugunthan V N wrote:
>> Some Ethernet Swtich controllers like CPSW in AM335x, TI814x, DRA7x and
>> AM43xx SoCs, Network Coprocessor in AM5K2E0x, Realtek Switch controllers
>> etc has to capability of conneting multiple networks using L2 switching
>> and has multiple phys. With the existing code, ethtool can communicate
>> only to one phy.
>>
>> To enable user to communicate multiple phy connected to single Ethernet
>> Switch controller, intoducing a optional new parameter in Ethtool interface
>> to pass which slave to set/get the phy configuration.
>
> There was some discussion about configuration APIs for hardware/firmware
> bridges earlier this year and I thought there was a consensus for
> assigning a network device to each port.  This would remove the need to
> identify ports within a device.  But I may have misremembered.
>

I like the approach of creating a network device for each port over
having to use ethtool to program/discover them. I am currently looking
at writing management applications for this and IMO it is much easier
to discover and listen for events on network devices vs polling ethtool
and iterating through slave indexs. Also you miss a lot of functionality
that may be useful MTU for example that is not available configured via
ethtool.

One of the sticking points in earlier discussions was how to handle
devices that have limited support for slave devices. When we create a
netdev we expect the stack can bind to it and TX/RX packets which as
I understand is not always possible? (I missed why we couldn't recv the
packets over a switch port though with some skb->dev manipulation). In
this case a feature flag could be used to resolve the feature
dependencies.

.John


-- 
John Fastabend         Intel Corporation

Re: [RFC PATCH 1/1] ethtool: adding support for multiple slave port configuration

[Mugunthan V N]

On Sunday 27 July 2014 09:39 PM, John Fastabend wrote:
> On 07/26/2014 07:47 PM, Ben Hutchings wrote:
>> On Fri, 2014-07-25 at 17:58 +0530, Mugunthan V N wrote:
>>> Some Ethernet Swtich controllers like CPSW in AM335x, TI814x, DRA7x and
>>> AM43xx SoCs, Network Coprocessor in AM5K2E0x, Realtek Switch
>>> controllers
>>> etc has to capability of conneting multiple networks using L2 switching
>>> and has multiple phys. With the existing code, ethtool can communicate
>>> only to one phy.
>>>
>>> To enable user to communicate multiple phy connected to single Ethernet
>>> Switch controller, intoducing a optional new parameter in Ethtool
>>> interface
>>> to pass which slave to set/get the phy configuration.
>>
>> There was some discussion about configuration APIs for hardware/firmware
>> bridges earlier this year and I thought there was a consensus for
>> assigning a network device to each port.  This would remove the need to
>> identify ports within a device.  But I may have misremembered.
>>
>
> I like the approach of creating a network device for each port over
> having to use ethtool to program/discover them. I am currently looking
> at writing management applications for this and IMO it is much easier
> to discover and listen for events on network devices vs polling ethtool
> and iterating through slave indexs. Also you miss a lot of functionality
> that may be useful MTU for example that is not available configured via
> ethtool.
>
> One of the sticking points in earlier discussions was how to handle
> devices that have limited support for slave devices. When we create a
> netdev we expect the stack can bind to it and TX/RX packets which as
> I understand is not always possible? (I missed why we couldn't recv the
> packets over a switch port though with some skb->dev manipulation). In
> this case a feature flag could be used to resolve the feature
> dependencies.
>
>

John

I am also interested in participating in the above management api
development.

Regards
Mugunthan V N

Re: [RFC PATCH 1/1] ethtool: adding support for multiple slave port configuration

[Andy Gospodarek]

On Sun, Jul 27, 2014 at 03:47:01AM +0100, Ben Hutchings wrote:
> On Fri, 2014-07-25 at 17:58 +0530, Mugunthan V N wrote:
> > Some Ethernet Swtich controllers like CPSW in AM335x, TI814x, DRA7x and
> > AM43xx SoCs, Network Coprocessor in AM5K2E0x, Realtek Switch controllers
> > etc has to capability of conneting multiple networks using L2 switching
> > and has multiple phys. With the existing code, ethtool can communicate
> > only to one phy.
> > 
> > To enable user to communicate multiple phy connected to single Ethernet
> > Switch controller, intoducing a optional new parameter in Ethtool interface
> > to pass which slave to set/get the phy configuration.
> 
> There was some discussion about configuration APIs for hardware/firmware
> bridges earlier this year and I thought there was a consensus for
> assigning a network device to each port.  This would remove the need to
> identify ports within a device.  But I may have misremembered.

Using a netdev per port was more or less the concensus for how to handle
the management of the individual ports connected to a hardware/firmware
switch.  Various suggestions were made to add a new struct (or not add
one) to at least link the netdevs used for each port to capture and
describe the hardware/firmware switch.

If individual netdevs are created (even if their functionality is
extremely limited) you could then use those devices with the base switch
driver that exists now, correct?  It seems like this must be an option
as this patch is simply a way to export this data/configuration to
userspace via ethtool.

> 
> > Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
> > ---
> >  include/uapi/linux/ethtool.h | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> > index 96ade34..3011427 100644
> > --- a/include/uapi/linux/ethtool.h
> > +++ b/include/uapi/linux/ethtool.h
> > @@ -60,6 +60,9 @@
> >   *	and other link features that the link partner advertised
> >   *	through autonegotiation; 0 if unknown or not applicable.
> >   *	Read-only.
> > + * @slave_port: Specify which slave port to be used to set/get
> > + *	parmeters, for example which slave port phy to be used for
> > + *	set/get phy capabilities
> 
> The difficulty with assigning the reserved fields in struct ethtool_cmd
> is that nothing has ever checked that they are set to 0.  So if we were
> to assign this field and support it in ethtool, someone might run it on
> an older kernel version and all configuration changes will be made to
> port 0 rather than the one they specified.  I don't think it would be
> acceptable to tell users that 'oh, the port number option silently fails
> on older kernel versions'.
> 
> So at the very least you would also need to add some way for userland to
> find out whether the driver will check the value of this field.
> 
> Ben.
> 
> >   * The link speed in Mbps is split between @speed and @speed_hi.  Use
> >   * the ethtool_cmd_speed() and ethtool_cmd_speed_set() functions to
> > @@ -107,7 +110,8 @@ struct ethtool_cmd {
> >  	__u8	eth_tp_mdix;
> >  	__u8	eth_tp_mdix_ctrl;
> >  	__u32	lp_advertising;
> > -	__u32	reserved[2];
> > +	__u32	slave_port;
> > +	__u32	reserved;
> >  };
> >  
> >  static inline void ethtool_cmd_speed_set(struct ethtool_cmd *ep,
> 
> -- 
> Ben Hutchings
> The generation of random numbers is too important to be left to chance.
>                                                             - Robert Coveyou