Received-SPF: pass (google.com: domain of linux-kernel+bounces-170189-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240503101824.32717-1-en-wei.wu@canonical.com> <7f533913-fba9-4a29-86a5-d3b32ac44632@intel.com>
In-Reply-To: <7f533913-fba9-4a29-86a5-d3b32ac44632@intel.com>
From: En-Wei WU <en-wei.wu@canonical.com>
Date: Tue, 7 May 2024 00:46:56 +0800
Message-ID: <CAMqyJG1Fyt1pZJqEjQN_kqXwfJ+HnqvW1PnAOEEpzoS9f37KBg@mail.gmail.com>
Subject: Re: [Intel-wired-lan] [PATCH v2 1/2] e1000e: let the sleep codes run
 every time
To: Sasha Neftin <sasha.neftin@intel.com>
Cc: netdev@vger.kernel.org, rickywu0421@gmail.com, 
	linux-kernel@vger.kernel.org, edumazet@google.com, 
	intel-wired-lan@lists.osuosl.org, kuba@kernel.org, anthony.l.nguyen@intel.com, 
	pabeni@redhat.com, davem@davemloft.net, 
	"Brandeburg, Jesse" <jesse.brandeburg@intel.com>, "Ruinskiy, Dima" <dima.ruinskiy@intel.com>, 
	"Lifshits, Vitaly" <vitaly.lifshits@intel.com>
Content-Type: text/plain; charset="UTF-8"

Thank you for your time.

Originally, sleep codes would only be executed if the first read fails
or the link status that is read is down. Some circumstances like the
[v2,2/2] "e1000e: fix link fluctuations problem" would need a delay
before first reading/accessing the PHY IEEE register, so that it won't
read the instability of the link status bit in the PHY status
register.

I've realized that this approach isn't good enough since the purpose
is only to fix the problem in another patch and it also changes the
behavior.

Here is the modification of the patch [v2,2/2] "e1000e: fix link
fluctuations problem":
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -1428,7 +1428,17 @@  static s32
e1000_check_for_copper_link_ich8lan(struct e1000_hw *hw)
- ret_val = e1000e_phy_has_link_generic(hw, 1, 0, &link);
/* comments */
+ ret_val = e1000e_phy_has_link_generic(hw, COPPER_LINK_UP_LIMIT,
100000, &link);

Do you think we can just add a msleep/usleep_range in front of the
e1000e_phy_has_link_generic() instead of modifying the sleep codes in
e1000e_phy_has_link_generic()?

Thanks.

On Mon, 6 May 2024 at 23:53, Sasha Neftin <sasha.neftin@intel.com> wrote:
>
> On 03/05/2024 13:18, Ricky Wu wrote:
> > Originally, the sleep codes being moved forward only
> > ran if we met some conditions (e.g. BMSR_LSTATUS bit
> > not set in phy_status). Moving these sleep codes forward
> > makes the usec_interval take effect every time.
> >
> > Signed-off-by: Ricky Wu <en-wei.wu@canonical.com>
> > ---
> >
> > In v2:
> > * Split the sleep codes into this patch
> >
> >   drivers/net/ethernet/intel/e1000e/phy.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/e1000e/phy.c b/drivers/net/ethernet/intel/e1000e/phy.c
> > index 93544f1cc2a5..4a58d56679c9 100644
> > --- a/drivers/net/ethernet/intel/e1000e/phy.c
> > +++ b/drivers/net/ethernet/intel/e1000e/phy.c
> > @@ -1777,6 +1777,11 @@ s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, u32 iterations,
> >
> >       *success = false;
> >       for (i = 0; i < iterations; i++) {
> > +             if (usec_interval >= 1000)
> > +                     msleep(usec_interval / 1000);
> > +             else
> > +                     udelay(usec_interval);
> > +
>
> I do not understand this approach. Why wait before first
> reading/accessing the PHY IEEE register?
>
> For further discussion, I would like to introduce Dima Ruinskiy (architect)
>
> >               /* Some PHYs require the MII_BMSR register to be read
> >                * twice due to the link bit being sticky.  No harm doing
> >                * it across the board.
> > @@ -1799,10 +1804,6 @@ s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, u32 iterations,
> >                       *success = true;
> >                       break;
> >               }
> > -             if (usec_interval >= 1000)
> > -                     msleep(usec_interval / 1000);
> > -             else
> > -                     udelay(usec_interval);
> >       }
> >
> >       return ret_val;
>