From: "Zhang, Qi Z" <qi.z.zhang@intel.com>
To: "Daly, Jeff" <jeffd@silicom-usa.com>,
"dev@dpdk.org" <dev@dpdk.org>,
"Yang, Qiming" <qiming.yang@intel.com>,
"Wu, Wenjun1" <wenjun1.wu@intel.com>
Cc: Stephen Douthit <stephend@silicom-usa.com>
Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
Date: Tue, 31 May 2022 13:03:13 +0000 [thread overview]
Message-ID: <DM4PR11MB5994E0F80DF0270DA4AAA755D7DC9@DM4PR11MB5994.namprd11.prod.outlook.com> (raw)
In-Reply-To: <VI1PR0402MB3517C4CF5D2B9E19E9280451EADC9@VI1PR0402MB3517.eurprd04.prod.outlook.com>
> -----Original Message-----
> From: Jeff Daly <jeffd@silicom-usa.com>
> Sent: Tuesday, May 31, 2022 8:25 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org; Yang, Qiming
> <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> Cc: Stephen Douthit <stephend@silicom-usa.com>
> Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
>
>
>
> > -----Original Message-----
> > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Sent: Monday, May 30, 2022 10:21 AM
> > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org; Yang, Qiming
> > <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> > Cc: Stephen Douthit <stephend@silicom-usa.com>
> > Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
> >
> > Caution: This is an external email. Please take care when clicking
> > links or opening attachments.
> >
> >
> > > -----Original Message-----
> > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > Sent: Monday, May 30, 2022 9:47 PM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org; Yang, Qiming
> > > <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> > > Cc: Stephen Douthit <stephend@silicom-usa.com>
> > > Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > Sent: Sunday, May 29, 2022 7:25 PM
> > > > To: Jeff Daly <jeffd@silicom-usa.com>; dev@dpdk.org; Yang, Qiming
> > > > <qiming.yang@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
> > > > Cc: Stephen Douthit <stephend@silicom-usa.com>
> > > > Subject: RE: [PATCH 1/3] ixgbe: make link update thread periodic
> > > >
> > > > Caution: This is an external email. Please take care when clicking
> > > > links or opening attachments.
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > > > Sent: Friday, May 20, 2022 2:03 AM
> > > > > To: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Wu,
> > > > > Wenjun1 <wenjun1.wu@intel.com>
> > > > > Cc: Stephen Douthit <stephend@silicom-usa.com>
> > > > > Subject: [PATCH 1/3] ixgbe: make link update thread periodic
> > > > >
> > > > > Rather than run-to-completion, allow the link update thread to
> > > > > be
> > > > periodic.
> > > > > This will set the stage for properly handling hot-plugging.
> > > >
> > > > Could you explain more about what's the hot-plugging issue with
> > > > run-to- completion you try to fix?
> > > >
> > >
> > > it doesn't work right when you have SFPs. (at least not on our
> > > platform or on an
> > > 82599 dual SFP add-in card we have). run-to-completion only works 1x.
> > > if you remove and plug in a different SFP it doesn't work. This
> > > patch series should have been taking in context with the original
> > > SFP hotplug patch but apparently since I can't ever seem to get the
> > > patch submission threading to do what I mean perhaps some context
> > > has gone missing. the SFP hotplug fix has been in the queue since
> > > Dec 2021, has been reworked several times, has gone through a change
> > > in Intel maintainership. this patch series makes the SFP hot
> > > plugging work like the
> > Intel kernel driver does.
> > >
> > > > >
> > > > > Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> > > > > Inspired-by: Stephen Douthit <stephend@silicom-usa.com>
> > > > > ---
> > > > > drivers/net/ixgbe/base/ixgbe_common.c | 4 +-
> > > > > drivers/net/ixgbe/ixgbe_ethdev.c | 180 ++++++++++----------------
> > > > > 2 files changed, 71 insertions(+), 113 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/ixgbe/base/ixgbe_common.c
> > > > > b/drivers/net/ixgbe/base/ixgbe_common.c
> > > > > index aa843bd5c4a5..712062306491 100644
> > > > > --- a/drivers/net/ixgbe/base/ixgbe_common.c
> > > > > +++ b/drivers/net/ixgbe/base/ixgbe_common.c
> > > > > @@ -4154,8 +4154,8 @@ s32 ixgbe_check_mac_link_generic(struct
> > > > > ixgbe_hw *hw, ixgbe_link_speed *speed,
> > > > > break;
> > > > > case ixgbe_mac_X550EM_x:
> > > > > case ixgbe_mac_X550EM_a:
> > > > > - sfp_cage_full = IXGBE_READ_REG(hw, IXGBE_ESDP)
> > > > > &
> > > > > - IXGBE_ESDP_SDP0;
> > > > > + sfp_cage_full = !(IXGBE_READ_REG(hw,
> > > > > + IXGBE_ESDP)
> > > > > &
> > > > > + IXGBE_ESDP_SDP0);
> > > > > break;
> > > > > default:
> > > > > /* sanity check - No SFP+ devices here */
> > > > > diff --git
> > > >
> > > > Looks like you change the behavior of link status check for x550.
> > > > I'm not an ixgbe expert, but I know this is not kernel driver's
> > > > implementation.
> > > >
> > >
> > > sigh. this was supposed to be part of a different patch which also
> > > had some question about functionality. the SDP0 bit check doesn't
> > > specifically need to be a check for a '1', since the bit reflects
> > > the state of
> > the pin on the platform.
> > > Intel's platform implementations have an inverter on the board to
> > > switch the state.
> > > MOD_ABS from an SFP will be '0' when an SFP is plugged in. with an
> > > inverter in the platform the signal will be '1' when an SFP is
> > > plugged in. there's no guidance from Intel's platform design guide
> > > that an inverter needs to be between the SFP and the NIC SDP pin so
> > > having it only follow Intel's platform implementations is hard to justify.
> >
> > OK, I assume the existing code should be proved for the normal
> > scenario (remove and plug in with the same SFP) So how can we
> > guarantee this change will not break something?
> >
> > Could you help me to understand why we should ignore the difference
> > when
> > SDP0 is 1 in normal scenario?
> >
>
> 'normal scenario'? meaning on any Intel-implemented platform?
>
> the code needs a devargs-like option so that platforms that don't implement
> the inverter can still use this code in the expected way.
Yes, please move this change into a separate patch and use a devargs for SDP pin invert case.
>
> > Before change
> > we will continue to read the link register and return the link speed.
> >
> > after change
> > we return SPEED_UNKNOWN immediately .
> >
> >
> >
> >
> > >
> > > > So do you think this is a fix for both DPDK and kernel driver? if
> > > > it is, please move this change into a separate patch and we need
> > > > to reach the right expert to approve this.
> > > >
> > > >
> > >
> > > no, as explained above.
next prev parent reply other threads:[~2022-05-31 13:03 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-19 18:02 [PATCH 0/3] ixgbe: Fix SFP hotplug detection Jeff Daly
2022-05-19 18:02 ` [PATCH 1/3] ixgbe: make link update thread periodic Jeff Daly
2022-05-29 23:25 ` Zhang, Qi Z
2022-05-30 13:46 ` Jeff Daly
2022-05-30 14:21 ` Zhang, Qi Z
2022-05-30 14:38 ` Zhang, Qi Z
2022-05-31 12:25 ` Jeff Daly
2022-05-31 13:03 ` Zhang, Qi Z [this message]
2022-05-19 18:02 ` [PATCH 2/3] ixgbe: move periodic link service work into separate function Jeff Daly
2022-05-19 18:02 ` [PATCH 3/3] ixgbe: make hotplug detection aware of changed SFPs Jeff Daly
-- strict thread matches above, loose matches on Subject: below --
2022-05-19 19:25 [PATCH 0/3] ixgbe: fix SFP hotplug detection Jeff Daly
2022-05-19 19:25 ` [PATCH 1/3] ixgbe: make link update thread periodic Jeff Daly
2022-05-19 17:43 [PATCH 0/3] ixgbe: Fix SFP hotplug detection/removal Jeff Daly
2022-05-19 17:43 ` [PATCH 1/3] ixgbe: make link update thread periodic Jeff Daly
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DM4PR11MB5994E0F80DF0270DA4AAA755D7DC9@DM4PR11MB5994.namprd11.prod.outlook.com \
--to=qi.z.zhang@intel.com \
--cc=dev@dpdk.org \
--cc=jeffd@silicom-usa.com \
--cc=qiming.yang@intel.com \
--cc=stephend@silicom-usa.com \
--cc=wenjun1.wu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).