| Message ID | 20230601185353.17012-1-prasad@arista.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | intel-wired-lan: igc: set TP bit in ethtool_link_ksettings.supported field | expand |
Thanks for the quick review. Yes, we use ETHTOOL_GLINKSETTINGS ioctl to retrieve interface settings and expect to see one of TP or MII set in the 'supported' bitmask. I'll send out a new patch removing the if(). Would you accept the patch into your staging tree and later push it to the stable kernel branch? This is my first time sending to intel-wired-lan. Not sure how it works. Thank you. On Fri, Jun 2, 2023 at 12:34 AM Neftin, Sasha <sasha.neftin@intel.com> wrote: > On 6/1/2023 21:53, prasad@arista.com wrote: > > From: Prasad Koya <prasad@arista.com> > > > > if the physical media is twisted pair copper, set the TP bit in the > > 'supported' field > > > > Signed-off-by: Prasad Koya <prasad@arista.com> > > --- > > drivers/net/ethernet/intel/igc/igc_ethtool.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c > b/drivers/net/ethernet/intel/igc/igc_ethtool.c > > index 8cc077b712ad..7d197fa80d5d 100644 > > --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c > > +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c > > @@ -1707,6 +1707,8 @@ static int igc_ethtool_get_link_ksettings(struct > net_device *netdev, > > /* twisted pair */ > > cmd->base.port = PORT_TP; > > cmd->base.phy_address = hw->phy.addr; > > + if (hw->phy.media_type == igc_media_type_copper) > Thank you Prasad. i225/6 parts supported only copper media type. We can > drop the "if" condition. > > + ethtool_link_ksettings_add_link_mode(cmd, supported, TP); > Do you want to see: "Supported ports: [ TP ]"? That's right. > > > > /* advertising link modes */ > > if (hw->phy.autoneg_advertised & ADVERTISE_10_HALF) > >
Hi Sasha In our internal review, we found that igb and other vendors' drivers set the TP (or other media) bit in the 'advertising' field as well. So I made the and tested the change in 6.1.31 kernel. Will send that patch for review. Thank you. On Fri, Jun 2, 2023 at 12:49 AM Prasad Koya <prasad@arista.com> wrote: > Thanks for the quick review. > > Yes, we use ETHTOOL_GLINKSETTINGS ioctl to retrieve interface settings and > expect to see one of TP or MII set in the 'supported' bitmask. > > I'll send out a new patch removing the if(). Would you accept the patch > into your staging tree and later push it to the stable kernel branch? This > is my first time sending to intel-wired-lan. Not sure how it works. > > Thank you. > > On Fri, Jun 2, 2023 at 12:34 AM Neftin, Sasha <sasha.neftin@intel.com> > wrote: > >> On 6/1/2023 21:53, prasad@arista.com wrote: >> > From: Prasad Koya <prasad@arista.com> >> > >> > if the physical media is twisted pair copper, set the TP bit in the >> > 'supported' field >> > >> > Signed-off-by: Prasad Koya <prasad@arista.com> >> > --- >> > drivers/net/ethernet/intel/igc/igc_ethtool.c | 2 ++ >> > 1 file changed, 2 insertions(+) >> > >> > diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c >> b/drivers/net/ethernet/intel/igc/igc_ethtool.c >> > index 8cc077b712ad..7d197fa80d5d 100644 >> > --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c >> > +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c >> > @@ -1707,6 +1707,8 @@ static int igc_ethtool_get_link_ksettings(struct >> net_device *netdev, >> > /* twisted pair */ >> > cmd->base.port = PORT_TP; >> > cmd->base.phy_address = hw->phy.addr; >> > + if (hw->phy.media_type == igc_media_type_copper) >> Thank you Prasad. i225/6 parts supported only copper media type. We can >> drop the "if" condition. >> > + ethtool_link_ksettings_add_link_mode(cmd, supported, TP); >> Do you want to see: "Supported ports: [ TP ]"? That's right. >> > >> > /* advertising link modes */ >> > if (hw->phy.autoneg_advertised & ADVERTISE_10_HALF) >> >>
On 6/3/2023 09:05, Prasad Koya wrote: > Hi Sasha > > In our internal review, we found that igb and other vendors' drivers set > the TP (or other media) bit in the 'advertising' field as well. So I > made the and tested the change in 6.1.31 kernel. Will send that patch > for review. > > Thank you. > > On Fri, Jun 2, 2023 at 12:49 AM Prasad Koya <prasad@arista.com > <mailto:prasad@arista.com>> wrote: > > Thanks for the quick review. > > Yes, we use ETHTOOL_GLINKSETTINGS ioctl to retrieve interface > settings and expect to see one of TP or MII set in the 'supported' > bitmask. > > I'll send out a new patch removing the if(). Would you accept the > patch into your staging tree and later push it to the stable kernel > branch? This is my first time sending to intel-wired-lan. Not sure > how it works. 1. yes, definitely we will accept. 2. please, use the [iwl-net] prefix, for example: [iwl-net, v2] igc: set TP bit in ethtool_link_ksettings.supported field 3. you might add the fixes tag. Example: Fixes: 8c5ad0dae93c9 ("igc: Add ethtool support") Thanks, Sasha > > Thank you. > > On Fri, Jun 2, 2023 at 12:34 AM Neftin, Sasha > <sasha.neftin@intel.com <mailto:sasha.neftin@intel.com>> wrote: > > On 6/1/2023 21:53, prasad@arista.com <mailto:prasad@arista.com> > wrote: > > From: Prasad Koya <prasad@arista.com <mailto:prasad@arista.com>> > > > > if the physical media is twisted pair copper, set the TP bit > in the > > 'supported' field > > > > Signed-off-by: Prasad Koya <prasad@arista.com > <mailto:prasad@arista.com>> > > --- > > drivers/net/ethernet/intel/igc/igc_ethtool.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c > b/drivers/net/ethernet/intel/igc/igc_ethtool.c > > index 8cc077b712ad..7d197fa80d5d 100644 > > --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c > > +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c > > @@ -1707,6 +1707,8 @@ static int > igc_ethtool_get_link_ksettings(struct net_device *netdev, > > /* twisted pair */ > > cmd->base.port = PORT_TP; > > cmd->base.phy_address = hw->phy.addr; > > + if (hw->phy.media_type == igc_media_type_copper) > Thank you Prasad. i225/6 parts supported only copper media type. > We can > drop the "if" condition. > > + ethtool_link_ksettings_add_link_mode(cmd, > supported, TP); > Do you want to see: "Supported ports: [ TP ]"? That's right. > > > > /* advertising link modes */ > > if (hw->phy.autoneg_advertised & ADVERTISE_10_HALF) >
>>> 2. please, use the [iwl-net] prefix, for example: [iwl-net, v2] igc: set TP bit in ethtool_link_ksettings.supported field 3. you might add the fixes tag. Example: Fixes: 8c5ad0dae93c9 ("igc: Add ethtool support") <<< Sent the current version after adding (2) and (3) Thank you. On Sun, Jun 4, 2023 at 12:20 AM Neftin, Sasha <sasha.neftin@intel.com> wrote: > On 6/3/2023 09:05, Prasad Koya wrote: > > Hi Sasha > > > > In our internal review, we found that igb and other vendors' drivers set > > the TP (or other media) bit in the 'advertising' field as well. So I > > made the and tested the change in 6.1.31 kernel. Will send that patch > > for review. > > > > Thank you. > > > > On Fri, Jun 2, 2023 at 12:49 AM Prasad Koya <prasad@arista.com > > <mailto:prasad@arista.com>> wrote: > > > > Thanks for the quick review. > > > > Yes, we use ETHTOOL_GLINKSETTINGS ioctl to retrieve interface > > settings and expect to see one of TP or MII set in the 'supported' > > bitmask. > > > > I'll send out a new patch removing the if(). Would you accept the > > patch into your staging tree and later push it to the stable kernel > > branch? This is my first time sending to intel-wired-lan. Not sure > > how it works. > > 1. yes, definitely we will accept. > 2. please, use the [iwl-net] prefix, for example: > [iwl-net, v2] igc: set TP bit in ethtool_link_ksettings.supported field > 3. you might add the fixes tag. Example: > Fixes: 8c5ad0dae93c9 ("igc: Add ethtool support") > Thanks, > Sasha > > > > > Thank you. > > > > On Fri, Jun 2, 2023 at 12:34 AM Neftin, Sasha > > <sasha.neftin@intel.com <mailto:sasha.neftin@intel.com>> wrote: > > > > On 6/1/2023 21:53, prasad@arista.com <mailto:prasad@arista.com> > > wrote: > > > From: Prasad Koya <prasad@arista.com <mailto: > prasad@arista.com>> > > > > > > if the physical media is twisted pair copper, set the TP bit > > in the > > > 'supported' field > > > > > > Signed-off-by: Prasad Koya <prasad@arista.com > > <mailto:prasad@arista.com>> > > > --- > > > drivers/net/ethernet/intel/igc/igc_ethtool.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c > > b/drivers/net/ethernet/intel/igc/igc_ethtool.c > > > index 8cc077b712ad..7d197fa80d5d 100644 > > > --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c > > > +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c > > > @@ -1707,6 +1707,8 @@ static int > > igc_ethtool_get_link_ksettings(struct net_device *netdev, > > > /* twisted pair */ > > > cmd->base.port = PORT_TP; > > > cmd->base.phy_address = hw->phy.addr; > > > + if (hw->phy.media_type == igc_media_type_copper) > > Thank you Prasad. i225/6 parts supported only copper media type. > > We can > > drop the "if" condition. > > > + ethtool_link_ksettings_add_link_mode(cmd, > > supported, TP); > > Do you want to see: "Supported ports: [ TP ]"? That's right. > > > > > > /* advertising link modes */ > > > if (hw->phy.autoneg_advertised & ADVERTISE_10_HALF) > > > >
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c index 8cc077b712ad..7d197fa80d5d 100644 --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c @@ -1707,6 +1707,8 @@ static int igc_ethtool_get_link_ksettings(struct net_device *netdev, /* twisted pair */ cmd->base.port = PORT_TP; cmd->base.phy_address = hw->phy.addr; + if (hw->phy.media_type == igc_media_type_copper) + ethtool_link_ksettings_add_link_mode(cmd, supported, TP); /* advertising link modes */ if (hw->phy.autoneg_advertised & ADVERTISE_10_HALF)