| Message ID | 0c7c97b0-bfe1-4143-a562-2019f86912fc@exht1.ad.emulex.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 06/18/2012 05:44 AM, Parav Pandit wrote: > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index 297370a..1ebfa6e 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -1153,6 +1153,10 @@ struct ethtool_ops { > #define SUPPORTED_10000baseR_FEC (1<< 20) > #define SUPPORTED_20000baseMLD2_Full (1<< 21) > #define SUPPORTED_20000baseKR2_Full (1<< 22) > +#define SUPPORTED_40000baseKR4_Full (1<< 23) > +#define SUPPORTED_40000baseCR4_Full (1<< 24) > +#define SUPPORTED_40000baseSR4_Full (1<< 25) > +#define SUPPORTED_40000baseLR4_Full (1<< 26) > > /* Indicates what features are advertised by the interface. */ > #define ADVERTISED_10baseT_Half (1<< 0) > @@ -1178,6 +1182,10 @@ struct ethtool_ops { > #define ADVERTISED_10000baseR_FEC (1<< 20) > #define ADVERTISED_20000baseMLD2_Full (1<< 21) > #define ADVERTISED_20000baseKR2_Full (1<< 22) > +#define ADVERTISED_40000baseKR4_Full (1<< 23) > +#define ADVERTISED_40000baseCR4_Full (1<< 24) > +#define ADVERTISED_40000baseSR4_Full (1<< 25) > +#define ADVERTISED_40000baseLR4_Full (1<< 26) Any idea how many defines will be wanted for 100 Gbit Ethernet? Supported and advertising in ethtool_cmd are __u32s... rick jones -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-06-18 at 09:27 -0700, Rick Jones wrote: > On 06/18/2012 05:44 AM, Parav Pandit wrote: > > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > > index 297370a..1ebfa6e 100644 > > --- a/include/linux/ethtool.h > > +++ b/include/linux/ethtool.h > > @@ -1153,6 +1153,10 @@ struct ethtool_ops { > > #define SUPPORTED_10000baseR_FEC (1<< 20) > > #define SUPPORTED_20000baseMLD2_Full (1<< 21) > > #define SUPPORTED_20000baseKR2_Full (1<< 22) > > +#define SUPPORTED_40000baseKR4_Full (1<< 23) > > +#define SUPPORTED_40000baseCR4_Full (1<< 24) > > +#define SUPPORTED_40000baseSR4_Full (1<< 25) > > +#define SUPPORTED_40000baseLR4_Full (1<< 26) > > > > /* Indicates what features are advertised by the interface. */ > > #define ADVERTISED_10baseT_Half (1<< 0) > > @@ -1178,6 +1182,10 @@ struct ethtool_ops { > > #define ADVERTISED_10000baseR_FEC (1<< 20) > > #define ADVERTISED_20000baseMLD2_Full (1<< 21) > > #define ADVERTISED_20000baseKR2_Full (1<< 22) > > +#define ADVERTISED_40000baseKR4_Full (1<< 23) > > +#define ADVERTISED_40000baseCR4_Full (1<< 24) > > +#define ADVERTISED_40000baseSR4_Full (1<< 25) > > +#define ADVERTISED_40000baseLR4_Full (1<< 26) > > Any idea how many defines will be wanted for 100 Gbit Ethernet? > Supported and advertising in ethtool_cmd are __u32s... There are 9 bytes reserved in struct ethtool_cmd, so we can potentially add extend each of supported, advertising and lp_advertising by 24 bits. But it might be better to define a new, cleaner struct ethtool_cmd. Ben.
On Mon, 2012-06-18 at 18:14 +0530, Parav Pandit wrote: > 1. link speed of 40GbE and #4 KR4, CR4, SR4, LR4 modes defined. > 2. removed code replication for tov calculation for 1G, 10G and > made is common for 1G, 10G, 40G. [...] > @@ -1185,12 +1193,13 @@ struct ethtool_ops { > * it was forced up into this mode or autonegotiated. > */ > > -/* The forced speed, 10Mb, 100Mb, gigabit, 2.5Gb, 10GbE. */ > +/* The forced speed, 10Mb, 100Mb, gigabit, 2.5Gb, 10GbE, 40GbE. */ > #define SPEED_10 10 > #define SPEED_100 100 > #define SPEED_1000 1000 > #define SPEED_2500 2500 > #define SPEED_10000 10000 > +#define SPEED_40000 40000 > #define SPEED_UNKNOWN -1 I don't think there's any need to name all possible link speeds, and it just encourages the bad practice of ethtool API users checking for specific values. You may notice there is no SPEED_20000. > /* Duplex, half or full. */ > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index 8a10d5b..dd0e503 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -542,13 +542,11 @@ static int prb_calc_retire_blk_tmo(struct packet_sock *po, > rtnl_unlock(); > if (!err) { > switch (ecmd.speed) { > - case SPEED_10000: > - msec = 1; > - div = 10000/1000; > - break; > case SPEED_1000: > + case SPEED_10000: > + case SPEED_40000: > msec = 1; > - div = 1000/1000; > + div = ecmd.speed / 1000; > break; This function should be fixed properly. Firstly, it must use ethtool_cmd_speed() rather than directly accessing ecmd.speed. Secondly, it should allow any speed value rather than checking for specific values. Then there will be no need to make further changes for 100G or any other new speed. Ben. > /* > * If the link speed is so slow you don't really
> -----Original Message----- > From: Rick Jones [mailto:rick.jones2@hp.com] > Sent: Monday, June 18, 2012 9:58 PM > To: Pandit, Parav > Cc: netdev@vger.kernel.org; bhutchings@solarflare.com > Subject: Re: [PATCH] net: added support for 40GbE link. > > On 06/18/2012 05:44 AM, Parav Pandit wrote: > > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index > > 297370a..1ebfa6e 100644 > > --- a/include/linux/ethtool.h > > +++ b/include/linux/ethtool.h > > @@ -1153,6 +1153,10 @@ struct ethtool_ops { > > #define SUPPORTED_10000baseR_FEC (1<< 20) > > #define SUPPORTED_20000baseMLD2_Full (1<< 21) > > #define SUPPORTED_20000baseKR2_Full (1<< 22) > > +#define SUPPORTED_40000baseKR4_Full (1<< 23) > > +#define SUPPORTED_40000baseCR4_Full (1<< 24) > > +#define SUPPORTED_40000baseSR4_Full (1<< 25) > > +#define SUPPORTED_40000baseLR4_Full (1<< 26) > > > > /* Indicates what features are advertised by the interface. */ > > #define ADVERTISED_10baseT_Half (1<< 0) > > @@ -1178,6 +1182,10 @@ struct ethtool_ops { > > #define ADVERTISED_10000baseR_FEC (1<< 20) > > #define ADVERTISED_20000baseMLD2_Full (1<< 21) > > #define ADVERTISED_20000baseKR2_Full (1<< 22) > > +#define ADVERTISED_40000baseKR4_Full (1<< 23) > > +#define ADVERTISED_40000baseCR4_Full (1<< 24) > > +#define ADVERTISED_40000baseSR4_Full (1<< 25) > > +#define ADVERTISED_40000baseLR4_Full (1<< 26) > > Any idea how many defines will be wanted for 100 Gbit Ethernet? > Supported and advertising in ethtool_cmd are __u32s... > 100G supports CR10, ER4, LR4, Base-R and SR10 interfaces. So 5 bits. We have space from 27 to 31 bits for 100G as well. > rick jones -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Ben Hutchings <bhutchings@solarflare.com> Date: Mon, 18 Jun 2012 18:09:36 +0100 > On Mon, 2012-06-18 at 18:14 +0530, Parav Pandit wrote: ... >> -/* The forced speed, 10Mb, 100Mb, gigabit, 2.5Gb, 10GbE. */ >> +/* The forced speed, 10Mb, 100Mb, gigabit, 2.5Gb, 10GbE, 40GbE. */ > > I don't think there's any need to name all possible link speeds, and it > just encourages the bad practice of ethtool API users checking for > specific values. You may notice there is no SPEED_20000. Agreed. >> @@ -542,13 +542,11 @@ static int prb_calc_retire_blk_tmo(struct packet_sock *po, ... > This function should be fixed properly. Firstly, it must use > ethtool_cmd_speed() rather than directly accessing ecmd.speed. > Secondly, it should allow any speed value rather than checking for > specific values. Then there will be no need to make further changes for > 100G or any other new speed. Agreed. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: David Miller [mailto:davem@davemloft.net] > Sent: Tuesday, June 19, 2012 12:59 PM > To: bhutchings@solarflare.com > Cc: Pandit, Parav; netdev@vger.kernel.org > Subject: Re: [PATCH] net: added support for 40GbE link. > > From: Ben Hutchings <bhutchings@solarflare.com> > Date: Mon, 18 Jun 2012 18:09:36 +0100 > > > On Mon, 2012-06-18 at 18:14 +0530, Parav Pandit wrote: > ... > >> -/* The forced speed, 10Mb, 100Mb, gigabit, 2.5Gb, 10GbE. */ > >> +/* The forced speed, 10Mb, 100Mb, gigabit, 2.5Gb, 10GbE, 40GbE. */ > > > > I don't think there's any need to name all possible link speeds, and > > it just encourages the bad practice of ethtool API users checking for > > specific values. You may notice there is no SPEED_20000. > > Agreed. Should eventually all net driver should remove using SPEED_xxxxxx and start using hard coded value of 10, 100, 1000, 20000? > > >> @@ -542,13 +542,11 @@ static int prb_calc_retire_blk_tmo(struct > >> packet_sock *po, > ... > > This function should be fixed properly. Firstly, it must use > > ethtool_cmd_speed() rather than directly accessing ecmd.speed. > > Secondly, it should allow any speed value rather than checking for > > specific values. Then there will be no need to make further changes > > for 100G or any other new speed. > > Agreed. That means ethtool_cmd_speed() should not be called in this function? If I understand correctly, it should return the value of 8ms (for 10Mb,s 100Mbps, 2.5 Gbps, 20Gbps) as today and remaining it should return calculated value? Or Function needs a fix for all these speeds (10Mbps, 100Mbs, 20Gbps too)? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: <Parav.Pandit@Emulex.Com> Date: Tue, 19 Jun 2012 07:33:12 +0000 > Should eventually all net driver should remove using SPEED_xxxxxx and start using hard coded value of 10, 100, 1000, 20000? No, the ones that exist can stay, just no new ones. > That means ethtool_cmd_speed() should not be called in this function? Ben said that it must be called, what are you talking about? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: David Miller [mailto:davem@davemloft.net] > Sent: Tuesday, June 19, 2012 1:05 PM > To: Pandit, Parav > Cc: bhutchings@solarflare.com; netdev@vger.kernel.org > Subject: Re: [PATCH] net: added support for 40GbE link. > > From: <Parav.Pandit@Emulex.Com> > Date: Tue, 19 Jun 2012 07:33:12 +0000 > > > Should eventually all net driver should remove using SPEED_xxxxxx and > start using hard coded value of 10, 100, 1000, 20000? > > No, the ones that exist can stay, just no new ones. > So driver which supports 40Gpbs, 100Gbps should hardcode to 40000, 100000 respectively? > > That means ethtool_cmd_speed() should not be called in this function? > > Ben said that it must be called, what are you talking about? Sorry, I wanted to ask - Do you need switch case for speed like below new code or its should be speed independent code? switch (ethtool_cmd_speed()) { case SPEED_100: case SPEED_10: return DEFAULT_PRB_RETIRE_TOV; default: msec = 1; div = ethtool_cmd_speed() / 1000; break; /* } -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2012-06-19 at 07:42 +0000, Parav.Pandit@Emulex.Com wrote: > > -----Original Message----- > > From: David Miller [mailto:davem@davemloft.net] > > Sent: Tuesday, June 19, 2012 1:05 PM > > To: Pandit, Parav > > Cc: bhutchings@solarflare.com; netdev@vger.kernel.org > > Subject: Re: [PATCH] net: added support for 40GbE link. > > > > From: <Parav.Pandit@Emulex.Com> > > Date: Tue, 19 Jun 2012 07:33:12 +0000 > > > > > Should eventually all net driver should remove using SPEED_xxxxxx and > > start using hard coded value of 10, 100, 1000, 20000? > > > > No, the ones that exist can stay, just no new ones. > > > So driver which supports 40Gpbs, 100Gbps should hardcode to 40000, 100000 respectively? Right. > > > That means ethtool_cmd_speed() should not be called in this function? > > > > Ben said that it must be called, what are you talking about? > > Sorry, I wanted to ask - Do you need switch case for speed like below new code or its should be speed independent code? > switch (ethtool_cmd_speed()) { > case SPEED_100: > case SPEED_10: > return DEFAULT_PRB_RETIRE_TOV; > default: > msec = 1; > div = ethtool_cmd_speed() / 1000; > break; > /* > } I was thinking of something like: u64 speed = ethtool_cmd_speed(&ecmd); if (speed < 1000 || speed == SPEED_UNKNOWN) return DEFAULT_PRB_RETIRE_TOV; msec = 1; div = speed / 1000; Ben.
o.k. I am sending PATCH v1 with suggested fixes in short while for ethtool and kernel both. Parav > -----Original Message----- > From: Ben Hutchings [mailto:bhutchings@solarflare.com] > Sent: Tuesday, June 19, 2012 7:42 PM > To: Pandit, Parav > Cc: davem@davemloft.net; netdev@vger.kernel.org > Subject: RE: [PATCH] net: added support for 40GbE link. > > On Tue, 2012-06-19 at 07:42 +0000, Parav.Pandit@Emulex.Com wrote: > > > -----Original Message----- > > > From: David Miller [mailto:davem@davemloft.net] > > > Sent: Tuesday, June 19, 2012 1:05 PM > > > To: Pandit, Parav > > > Cc: bhutchings@solarflare.com; netdev@vger.kernel.org > > > Subject: Re: [PATCH] net: added support for 40GbE link. > > > > > > From: <Parav.Pandit@Emulex.Com> > > > Date: Tue, 19 Jun 2012 07:33:12 +0000 > > > > > > > Should eventually all net driver should remove using SPEED_xxxxxx > > > > and > > > start using hard coded value of 10, 100, 1000, 20000? > > > > > > No, the ones that exist can stay, just no new ones. > > > > > So driver which supports 40Gpbs, 100Gbps should hardcode to 40000, > 100000 respectively? > > Right. > > > > > That means ethtool_cmd_speed() should not be called in this function? > > > > > > Ben said that it must be called, what are you talking about? > > > > Sorry, I wanted to ask - Do you need switch case for speed like below new > code or its should be speed independent code? > > switch (ethtool_cmd_speed()) { > > case SPEED_100: > > case SPEED_10: > > return DEFAULT_PRB_RETIRE_TOV; > > default: > > msec = 1; > > div = ethtool_cmd_speed() / 1000; > > break; > > /* > > } > > I was thinking of something like: > > u64 speed = ethtool_cmd_speed(&ecmd); > if (speed < 1000 || speed == SPEED_UNKNOWN) > return DEFAULT_PRB_RETIRE_TOV; > msec = 1; > div = speed / 1000; > > Ben. > > -- > Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; > that's the marketing department's job. > They asked us to note that Solarflare product names are trademarked.
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 297370a..1ebfa6e 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -1153,6 +1153,10 @@ struct ethtool_ops { #define SUPPORTED_10000baseR_FEC (1 << 20) #define SUPPORTED_20000baseMLD2_Full (1 << 21) #define SUPPORTED_20000baseKR2_Full (1 << 22) +#define SUPPORTED_40000baseKR4_Full (1 << 23) +#define SUPPORTED_40000baseCR4_Full (1 << 24) +#define SUPPORTED_40000baseSR4_Full (1 << 25) +#define SUPPORTED_40000baseLR4_Full (1 << 26) /* Indicates what features are advertised by the interface. */ #define ADVERTISED_10baseT_Half (1 << 0) @@ -1178,6 +1182,10 @@ struct ethtool_ops { #define ADVERTISED_10000baseR_FEC (1 << 20) #define ADVERTISED_20000baseMLD2_Full (1 << 21) #define ADVERTISED_20000baseKR2_Full (1 << 22) +#define ADVERTISED_40000baseKR4_Full (1 << 23) +#define ADVERTISED_40000baseCR4_Full (1 << 24) +#define ADVERTISED_40000baseSR4_Full (1 << 25) +#define ADVERTISED_40000baseLR4_Full (1 << 26) /* The following are all involved in forcing a particular link * mode for the device for setting things. When getting the @@ -1185,12 +1193,13 @@ struct ethtool_ops { * it was forced up into this mode or autonegotiated. */ -/* The forced speed, 10Mb, 100Mb, gigabit, 2.5Gb, 10GbE. */ +/* The forced speed, 10Mb, 100Mb, gigabit, 2.5Gb, 10GbE, 40GbE. */ #define SPEED_10 10 #define SPEED_100 100 #define SPEED_1000 1000 #define SPEED_2500 2500 #define SPEED_10000 10000 +#define SPEED_40000 40000 #define SPEED_UNKNOWN -1 /* Duplex, half or full. */ diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 8a10d5b..dd0e503 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -542,13 +542,11 @@ static int prb_calc_retire_blk_tmo(struct packet_sock *po, rtnl_unlock(); if (!err) { switch (ecmd.speed) { - case SPEED_10000: - msec = 1; - div = 10000/1000; - break; case SPEED_1000: + case SPEED_10000: + case SPEED_40000: msec = 1; - div = 1000/1000; + div = ecmd.speed / 1000; break; /* * If the link speed is so slow you don't really
1. link speed of 40GbE and #4 KR4, CR4, SR4, LR4 modes defined. 2. removed code replication for tov calculation for 1G, 10G and made is common for 1G, 10G, 40G. Port cost calculation changes for bridging for 40G will be done once have more clarify from 802.1d spec in coming days. Signed-off-by: Parav Pandit <parav.pandit@emulex.com> --- include/linux/ethtool.h | 11 ++++++++++- net/packet/af_packet.c | 8 +++----- 2 files changed, 13 insertions(+), 6 deletions(-)