Re: [Intel-wired-lan] [PATCH] intel-wired-lan: igc: set TP bit in ethtool_link_ksettings.supported field

From: "Neftin, Sasha" <sasha.neftin@intel.com>
To: Prasad Koya <prasad@arista.com>,
	"Avivi, Amir" <amir.avivi@intel.com>,
	"Neftin, Sasha" <sasha.neftin@intel.com>
Cc: "Fuxbrumer, Devora" <devora.fuxbrumer@intel.com>,
	Salam Noureddine <noureddine@arista.com>,
	"Nguyen, Anthony L" <anthony.l.nguyen@intel.com>,
	gilligan@arista.com, intel-wired-lan@lists.osuosl.org, "Ruinskiy,
	Dima" <dima.ruinskiy@intel.com>,
	"Avivi, Amir" <amir.avivi@intel.com>
Subject: Re: [Intel-wired-lan] [PATCH] intel-wired-lan: igc: set TP bit in ethtool_link_ksettings.supported field
Date: Sun, 4 Jun 2023 10:19:54 +0300	[thread overview]
Message-ID: <61ead333-c9b1-1580-78e8-132337408e25@intel.com> (raw)
In-Reply-To: <CAKh1g548roQKgHu6aS+SAB2Ux+HkJHWgpfEFvvqqnHu56F1sDQ@mail.gmail.com>

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)
> 

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan