[v3,1/2] net/ixgbe: fix x550 code to handle unidentified PHY
Checks
Commit Message
ixgbe_identify_sfp_module_X550em() was missing the code to handle
unidentified PHY that has been there in 82599 so it was not able to
complete initialization of ixgbe sequence if no sfp plugged in.
Port it over to return an appropriate type and complete init sequence
properly.
Fixes: d2e72774e58c ("ixgbe/base: support X550")
Cc: stable@dpdk.org
Signed-off-by: Luca Boccassi <bluca@debian.org>
---
v2: refresh to remove merge conflict with master
v3: coalesce fix into ixgbe_identify_sfp_module_X550em to avoid
code duplication, improve comment
drivers/net/ixgbe/base/ixgbe_x550.c | 6 ++++++
1 file changed, 6 insertions(+)
Comments
> -----Original Message-----
> From: Luca Boccassi [mailto:bluca@debian.org]
> Sent: Friday, November 2, 2018 8:19 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> 3chas3@gmail.com; Luca Boccassi <bluca@debian.org>; stable@dpdk.org
> Subject: [PATCH v3 1/2] net/ixgbe: fix x550 code to handle unidentified PHY
>
> ixgbe_identify_sfp_module_X550em() was missing the code to handle
> unidentified PHY that has been there in 82599 so it was not able to complete
> initialization of ixgbe sequence if no sfp plugged in.
> Port it over to return an appropriate type and complete init sequence
> properly.
>
> Fixes: d2e72774e58c ("ixgbe/base: support X550")
> Cc: stable@dpdk.org
>
> Signed-off-by: Luca Boccassi <bluca@debian.org>
> ---
> v2: refresh to remove merge conflict with master
> v3: coalesce fix into ixgbe_identify_sfp_module_X550em to avoid
> code duplication, improve comment
>
> drivers/net/ixgbe/base/ixgbe_x550.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/ixgbe/base/ixgbe_x550.c
> b/drivers/net/ixgbe/base/ixgbe_x550.c
> index f7b98af52..a88d5c86a 100644
> --- a/drivers/net/ixgbe/base/ixgbe_x550.c
> +++ b/drivers/net/ixgbe/base/ixgbe_x550.c
> @@ -1561,6 +1561,12 @@ s32 ixgbe_identify_sfp_module_X550em(struct
> ixgbe_hw *hw)
>
> status = ixgbe_identify_module_generic(hw);
>
> + /* Set PHY type none if no PHY detected to allow init without SFP */
> + if (hw->phy.type == ixgbe_phy_unknown) {
> + hw->phy.type = ixgbe_phy_none;
Set PHY type to none for a device that does have PHY looks weird.
does ixgeb_phy_generic works here?
Where is failure you met with ixgbe_phy_unknown?
> + return IXGBE_SUCCESS;
> + }
> +
> if (status != IXGBE_SUCCESS)
> return status;
>
> --
> 2.19.1
On Mon, 2018-11-05 at 17:41 +0000, Zhang, Qi Z wrote:
> > -----Original Message-----
> > From: Luca Boccassi [mailto:bluca@debian.org]
> > Sent: Friday, November 2, 2018 8:19 AM
> > To: dev@dpdk.org
> > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> > 3chas3@gmail.com; Luca Boccassi <bluca@debian.org>; stable@dpdk.org
> > Subject: [PATCH v3 1/2] net/ixgbe: fix x550 code to handle
> > unidentified PHY
> >
> > ixgbe_identify_sfp_module_X550em() was missing the code to handle
> > unidentified PHY that has been there in 82599 so it was not able to
> > complete
> > initialization of ixgbe sequence if no sfp plugged in.
> > Port it over to return an appropriate type and complete init
> > sequence
> > properly.
> >
> > Fixes: d2e72774e58c ("ixgbe/base: support X550")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > ---
> > v2: refresh to remove merge conflict with master
> > v3: coalesce fix into ixgbe_identify_sfp_module_X550em to avoid
> > code duplication, improve comment
> >
> > drivers/net/ixgbe/base/ixgbe_x550.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/net/ixgbe/base/ixgbe_x550.c
> > b/drivers/net/ixgbe/base/ixgbe_x550.c
> > index f7b98af52..a88d5c86a 100644
> > --- a/drivers/net/ixgbe/base/ixgbe_x550.c
> > +++ b/drivers/net/ixgbe/base/ixgbe_x550.c
> > @@ -1561,6 +1561,12 @@ s32 ixgbe_identify_sfp_module_X550em(struct
> > ixgbe_hw *hw)
> >
> > status = ixgbe_identify_module_generic(hw);
> >
> > + /* Set PHY type none if no PHY detected to allow init
> > without SFP */
> > + if (hw->phy.type == ixgbe_phy_unknown) {
> > + hw->phy.type = ixgbe_phy_none;
>
> Set PHY type to none for a device that does have PHY looks weird.
> does ixgeb_phy_generic works here?
>
> Where is failure you met with ixgbe_phy_unknown?
Yes it is a bit weird, but it works :-)
The issue is that the PMD fails to initialise when there is no SFP
plugged, and then it will always stay in that failed state when an SFP
is later plugged in, and it won't work until the machine is rebooted
with the SFP plugged in.
The logs (with an older DPDK version):
PMD: eth_ixgbe_dev_init(): Hardware Initialization Failure: -20
EAL: Requested device 0000:04:00.0 cannot be used
The kernel driver didn't like it either:
[ 7.579782] ixgbe 0000:04:00.0: Multiqueue Enabled: Rx Queue count = 8, Tx Queue count = 8
[ 7.649766] ixgbe 0000:04:00.0: MAC: 5, PHY: 0, PBA No: 020A00-000
[ 7.649774] ixgbe 0000:04:00.0: 00:25:90:5e:05:20
[ 8.763790] ixgbe 0000:04:00.0 0000:04:00.0 (uninitialized): CS4227 reset did not complete
[ 8.772051] ixgbe 0000:04:00.0 0000:04:00.0 (uninitialized): CS4227 reset failed: -3
[ 9.059374] ixgbe 0000:04:00.0: Intel(R) 10 Gigabit Network Connection
But with the kernel driver, if an SFP is plugged in later then the
interface works correctly.
With this series, an SFP can be plugged in after booting and
initialising the DPDK application.
On 11/05/2018 12:41 PM, Zhang, Qi Z wrote:
>
>
>> -----Original Message-----
>> From: Luca Boccassi [mailto:bluca@debian.org]
>> Sent: Friday, November 2, 2018 8:19 AM
>> To: dev@dpdk.org
>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
>> 3chas3@gmail.com; Luca Boccassi <bluca@debian.org>; stable@dpdk.org
>> Subject: [PATCH v3 1/2] net/ixgbe: fix x550 code to handle unidentified PHY
>>
>> ixgbe_identify_sfp_module_X550em() was missing the code to handle
>> unidentified PHY that has been there in 82599 so it was not able to complete
>> initialization of ixgbe sequence if no sfp plugged in.
>> Port it over to return an appropriate type and complete init sequence
>> properly.
>>
>> Fixes: d2e72774e58c ("ixgbe/base: support X550")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Luca Boccassi <bluca@debian.org>
>> ---
>> v2: refresh to remove merge conflict with master
>> v3: coalesce fix into ixgbe_identify_sfp_module_X550em to avoid
>> code duplication, improve comment
>>
>> drivers/net/ixgbe/base/ixgbe_x550.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/net/ixgbe/base/ixgbe_x550.c
>> b/drivers/net/ixgbe/base/ixgbe_x550.c
>> index f7b98af52..a88d5c86a 100644
>> --- a/drivers/net/ixgbe/base/ixgbe_x550.c
>> +++ b/drivers/net/ixgbe/base/ixgbe_x550.c
>> @@ -1561,6 +1561,12 @@ s32 ixgbe_identify_sfp_module_X550em(struct
>> ixgbe_hw *hw)
>>
>> status = ixgbe_identify_module_generic(hw);
>>
>> + /* Set PHY type none if no PHY detected to allow init without SFP */
>> + if (hw->phy.type == ixgbe_phy_unknown) {
>> + hw->phy.type = ixgbe_phy_none;
>
> Set PHY type to none for a device that does have PHY looks weird.
> does ixgeb_phy_generic works here?
Yes, it does seem strange but that's what ixgbe_identify_sfp_module_generic
seems to do:
err_read_i2c_eeprom:
hw->phy.sfp_type = ixgbe_sfp_type_not_present;
if (hw->phy.type != ixgbe_phy_nl) {
hw->phy.id = 0;
hw->phy.type = ixgbe_phy_unknown;
}
The QSFP version a little more forceful:
err_read_i2c_eeprom:
hw->phy.sfp_type = ixgbe_sfp_type_not_present;
hw->phy.id = 0;
hw->phy.type = ixgbe_phy_unknown;
If we go forward without setting the phy_type to none, we will eventually
run into issues calling other phy routines.
So should a lack of SFP, reset the PHY type? It's hazy because the
difference
between PHY and SFP isn't that clear to me here.
> Where is failure you met with ixgbe_phy_unknown?
>
>> + return IXGBE_SUCCESS;
>> + }
>> +
>> if (status != IXGBE_SUCCESS)
>> return status;
>>
>> --
>> 2.19.1
>
> -----Original Message-----
> From: Chas Williams [mailto:3chas3@gmail.com]
> Sent: Monday, November 5, 2018 11:19 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Luca Boccassi <bluca@debian.org>;
> dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH v3 1/2] net/ixgbe: fix x550 code to handle unidentified
> PHY
>
>
>
> On 11/05/2018 12:41 PM, Zhang, Qi Z wrote:
> >
> >
> >> -----Original Message-----
> >> From: Luca Boccassi [mailto:bluca@debian.org]
> >> Sent: Friday, November 2, 2018 8:19 AM
> >> To: dev@dpdk.org
> >> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >> <konstantin.ananyev@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> >> 3chas3@gmail.com; Luca Boccassi <bluca@debian.org>; stable@dpdk.org
> >> Subject: [PATCH v3 1/2] net/ixgbe: fix x550 code to handle
> >> unidentified PHY
> >>
> >> ixgbe_identify_sfp_module_X550em() was missing the code to handle
> >> unidentified PHY that has been there in 82599 so it was not able to
> >> complete initialization of ixgbe sequence if no sfp plugged in.
> >> Port it over to return an appropriate type and complete init sequence
> >> properly.
> >>
> >> Fixes: d2e72774e58c ("ixgbe/base: support X550")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Luca Boccassi <bluca@debian.org>
> >> ---
> >> v2: refresh to remove merge conflict with master
> >> v3: coalesce fix into ixgbe_identify_sfp_module_X550em to avoid
> >> code duplication, improve comment
> >>
> >> drivers/net/ixgbe/base/ixgbe_x550.c | 6 ++++++
> >> 1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/net/ixgbe/base/ixgbe_x550.c
> >> b/drivers/net/ixgbe/base/ixgbe_x550.c
> >> index f7b98af52..a88d5c86a 100644
> >> --- a/drivers/net/ixgbe/base/ixgbe_x550.c
> >> +++ b/drivers/net/ixgbe/base/ixgbe_x550.c
> >> @@ -1561,6 +1561,12 @@ s32
> ixgbe_identify_sfp_module_X550em(struct
> >> ixgbe_hw *hw)
> >>
> >> status = ixgbe_identify_module_generic(hw);
> >>
> >> + /* Set PHY type none if no PHY detected to allow init without SFP */
> >> + if (hw->phy.type == ixgbe_phy_unknown) {
> >> + hw->phy.type = ixgbe_phy_none;
> >
> > Set PHY type to none for a device that does have PHY looks weird.
> > does ixgeb_phy_generic works here?
>
> Yes, it does seem strange but that's what ixgbe_identify_sfp_module_generic
> seems to do:
>
> err_read_i2c_eeprom:
> hw->phy.sfp_type = ixgbe_sfp_type_not_present;
> if (hw->phy.type != ixgbe_phy_nl) {
> hw->phy.id = 0;
> hw->phy.type = ixgbe_phy_unknown;
> }
>
> The QSFP version a little more forceful:
>
> err_read_i2c_eeprom:
> hw->phy.sfp_type = ixgbe_sfp_type_not_present;
> hw->phy.id = 0;
> hw->phy.type = ixgbe_phy_unknown;
>
> If we go forward without setting the phy_type to none, we will eventually run
> into issues calling other phy routines.
>
> So should a lack of SFP, reset the PHY type? It's hazy because the difference
> between PHY and SFP isn't that clear to me here.
I'm not sure that's the same case:).
Just feel that it's better to handle ixgbe_phy_unknown directly for some device id as a special case than just replace it to ixgbe_phy_none to cheat the check path, since that rely on we never change the way to handle ixgbe_phy_none.
So still have the question?
What is the failure if you go with ixgbe_phy_unknown?
Is that possible to work around this like
if (phy_type == ixgbe_phy_unknown && dev_id == xxxx)
...
>
> > Where is failure you met with ixgbe_phy_unknown?
> >
> >> + return IXGBE_SUCCESS;
> >> + }
> >> +
> >> if (status != IXGBE_SUCCESS)
> >> return status;
> >>
> >> --
> >> 2.19.1
> >
On Tue, 2018-11-06 at 23:31 +0000, Zhang, Qi Z wrote:
> > -----Original Message-----
> > From: Chas Williams [mailto:3chas3@gmail.com]
> > Sent: Monday, November 5, 2018 11:19 AM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Luca Boccassi <bluca@debian
> > .org>;
> > dev@dpdk.org
> > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; stable@dpdk.org
> > Subject: Re: [PATCH v3 1/2] net/ixgbe: fix x550 code to handle
> > unidentified
> > PHY
> >
> >
> >
> > On 11/05/2018 12:41 PM, Zhang, Qi Z wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Luca Boccassi [mailto:bluca@debian.org]
> > > > Sent: Friday, November 2, 2018 8:19 AM
> > > > To: dev@dpdk.org
> > > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> > > > <konstantin.ananyev@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.c
> > > > om>;
> > > > 3chas3@gmail.com; Luca Boccassi <bluca@debian.org>; stable@dpdk
> > > > .org
> > > > Subject: [PATCH v3 1/2] net/ixgbe: fix x550 code to handle
> > > > unidentified PHY
> > > >
> > > > ixgbe_identify_sfp_module_X550em() was missing the code to
> > > > handle
> > > > unidentified PHY that has been there in 82599 so it was not
> > > > able to
> > > > complete initialization of ixgbe sequence if no sfp plugged in.
> > > > Port it over to return an appropriate type and complete init
> > > > sequence
> > > > properly.
> > > >
> > > > Fixes: d2e72774e58c ("ixgbe/base: support X550")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > > > ---
> > > > v2: refresh to remove merge conflict with master
> > > > v3: coalesce fix into ixgbe_identify_sfp_module_X550em to avoid
> > > > code duplication, improve comment
> > > >
> > > > drivers/net/ixgbe/base/ixgbe_x550.c | 6 ++++++
> > > > 1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > b/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > index f7b98af52..a88d5c86a 100644
> > > > --- a/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > +++ b/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > @@ -1561,6 +1561,12 @@ s32
> >
> > ixgbe_identify_sfp_module_X550em(struct
> > > > ixgbe_hw *hw)
> > > >
> > > > status = ixgbe_identify_module_generic(hw);
> > > >
> > > > + /* Set PHY type none if no PHY detected to allow init
> > > > without SFP */
> > > > + if (hw->phy.type == ixgbe_phy_unknown) {
> > > > + hw->phy.type = ixgbe_phy_none;
> > >
> > > Set PHY type to none for a device that does have PHY looks weird.
> > > does ixgeb_phy_generic works here?
> >
> > Yes, it does seem strange but that's what
> > ixgbe_identify_sfp_module_generic
> > seems to do:
> >
> > err_read_i2c_eeprom:
> > hw->phy.sfp_type = ixgbe_sfp_type_not_present;
> > if (hw->phy.type != ixgbe_phy_nl) {
> > hw->phy.id = 0;
> > hw->phy.type = ixgbe_phy_unknown;
> > }
> >
> > The QSFP version a little more forceful:
> >
> > err_read_i2c_eeprom:
> > hw->phy.sfp_type = ixgbe_sfp_type_not_present;
> > hw->phy.id = 0;
> > hw->phy.type = ixgbe_phy_unknown;
> >
> > If we go forward without setting the phy_type to none, we will
> > eventually run
> > into issues calling other phy routines.
> >
> > So should a lack of SFP, reset the PHY type? It's hazy because the
> > difference
> > between PHY and SFP isn't that clear to me here.
>
> I'm not sure that's the same case:).
> Just feel that it's better to handle ixgbe_phy_unknown directly for
> some device id as a special case than just replace it to
> ixgbe_phy_none to cheat the check path, since that rely on we never
> change the way to handle ixgbe_phy_none.
>
> So still have the question?
> What is the failure if you go with ixgbe_phy_unknown?
> Is that possible to work around this like
> if (phy_type == ixgbe_phy_unknown && dev_id == xxxx)
> ...
Hi,
Thanks for having a look at this again. If you could please see the
other answer, from myself, I've quoted the exact error we see and the
issue it causes.
> -----Original Message-----
> From: Luca Boccassi [mailto:bluca@debian.org]
> Sent: Wednesday, November 7, 2018 5:55 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Chas Williams <3chas3@gmail.com>;
> dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH v3 1/2] net/ixgbe: fix x550 code to handle unidentified
> PHY
>
> On Tue, 2018-11-06 at 23:31 +0000, Zhang, Qi Z wrote:
> > > -----Original Message-----
> > > From: Chas Williams [mailto:3chas3@gmail.com]
> > > Sent: Monday, November 5, 2018 11:19 AM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Luca Boccassi <bluca@debian
> > > .org>; dev@dpdk.org
> > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> > > <konstantin.ananyev@intel.com>; stable@dpdk.org
> > > Subject: Re: [PATCH v3 1/2] net/ixgbe: fix x550 code to handle
> > > unidentified PHY
> > >
> > >
> > >
> > > On 11/05/2018 12:41 PM, Zhang, Qi Z wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Luca Boccassi [mailto:bluca@debian.org]
> > > > > Sent: Friday, November 2, 2018 8:19 AM
> > > > > To: dev@dpdk.org
> > > > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> > > > > <konstantin.ananyev@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.c
> > > > > om>;
> > > > > 3chas3@gmail.com; Luca Boccassi <bluca@debian.org>; stable@dpdk
> > > > > .org
> > > > > Subject: [PATCH v3 1/2] net/ixgbe: fix x550 code to handle
> > > > > unidentified PHY
> > > > >
> > > > > ixgbe_identify_sfp_module_X550em() was missing the code to
> > > > > handle
> > > > > unidentified PHY that has been there in 82599 so it was not
> > > > > able to
> > > > > complete initialization of ixgbe sequence if no sfp plugged in.
> > > > > Port it over to return an appropriate type and complete init
> > > > > sequence
> > > > > properly.
> > > > >
> > > > > Fixes: d2e72774e58c ("ixgbe/base: support X550")
> > > > > Cc: stable@dpdk.org
> > > > >
> > > > > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > > > > ---
> > > > > v2: refresh to remove merge conflict with master
> > > > > v3: coalesce fix into ixgbe_identify_sfp_module_X550em to avoid
> > > > > code duplication, improve comment
> > > > >
> > > > > drivers/net/ixgbe/base/ixgbe_x550.c | 6 ++++++
> > > > > 1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > > b/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > > index f7b98af52..a88d5c86a 100644
> > > > > --- a/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > > +++ b/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > > @@ -1561,6 +1561,12 @@ s32
> > >
> > > ixgbe_identify_sfp_module_X550em(struct
> > > > > ixgbe_hw *hw)
> > > > >
> > > > > status = ixgbe_identify_module_generic(hw);
> > > > >
> > > > > + /* Set PHY type none if no PHY detected to allow init
> > > > > without SFP */
> > > > > + if (hw->phy.type == ixgbe_phy_unknown) {
> > > > > + hw->phy.type = ixgbe_phy_none;
> > > >
> > > > Set PHY type to none for a device that does have PHY looks weird.
> > > > does ixgeb_phy_generic works here?
> > >
> > > Yes, it does seem strange but that's what
> > > ixgbe_identify_sfp_module_generic
> > > seems to do:
> > >
> > > err_read_i2c_eeprom:
> > > hw->phy.sfp_type = ixgbe_sfp_type_not_present;
> > > if (hw->phy.type != ixgbe_phy_nl) {
> > > hw->phy.id = 0;
> > > hw->phy.type = ixgbe_phy_unknown;
> > > }
> > >
> > > The QSFP version a little more forceful:
> > >
> > > err_read_i2c_eeprom:
> > > hw->phy.sfp_type = ixgbe_sfp_type_not_present;
> > > hw->phy.id = 0;
> > > hw->phy.type = ixgbe_phy_unknown;
> > >
> > > If we go forward without setting the phy_type to none, we will
> > > eventually run
> > > into issues calling other phy routines.
> > >
> > > So should a lack of SFP, reset the PHY type? It's hazy because the
> > > difference
> > > between PHY and SFP isn't that clear to me here.
> >
> > I'm not sure that's the same case:).
> > Just feel that it's better to handle ixgbe_phy_unknown directly for
> > some device id as a special case than just replace it to
> > ixgbe_phy_none to cheat the check path, since that rely on we never
> > change the way to handle ixgbe_phy_none.
> >
> > So still have the question?
> > What is the failure if you go with ixgbe_phy_unknown?
> > Is that possible to work around this like
> > if (phy_type == ixgbe_phy_unknown && dev_id == xxxx)
> > ...
>
> Hi,
>
> Thanks for having a look at this again. If you could please see the
> other answer, from myself, I've quoted the exact error we see and the
> issue it causes.
Yes, I see it failed at eth_ixgbe_dev_init, it will be better if you can provide more detail for the call stack, so we can figure out if we can work around this by handle ixgbe_phy_unknown directly with some special case.
>
> --
> Kind regards,
> Luca Boccassi
On Wed, 2018-11-07 at 18:27 +0000, Zhang, Qi Z wrote:
> > -----Original Message-----
> > From: Luca Boccassi [mailto:bluca@debian.org]
> > Sent: Wednesday, November 7, 2018 5:55 AM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Chas Williams <3chas3@gmail
> > .com>;
> > dev@dpdk.org
> > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; stable@dpdk.org
> > Subject: Re: [PATCH v3 1/2] net/ixgbe: fix x550 code to handle
> > unidentified
> > PHY
> >
> > On Tue, 2018-11-06 at 23:31 +0000, Zhang, Qi Z wrote:
> > > > -----Original Message-----
> > > > From: Chas Williams [mailto:3chas3@gmail.com]
> > > > Sent: Monday, November 5, 2018 11:19 AM
> > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Luca Boccassi <bluca@de
> > > > bian
> > > > .org>; dev@dpdk.org
> > > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> > > > <konstantin.ananyev@intel.com>; stable@dpdk.org
> > > > Subject: Re: [PATCH v3 1/2] net/ixgbe: fix x550 code to handle
> > > > unidentified PHY
> > > >
> > > >
> > > >
> > > > On 11/05/2018 12:41 PM, Zhang, Qi Z wrote:
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Luca Boccassi [mailto:bluca@debian.org]
> > > > > > Sent: Friday, November 2, 2018 8:19 AM
> > > > > > To: dev@dpdk.org
> > > > > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> > > > > > <konstantin.ananyev@intel.com>; Zhang, Qi Z <qi.z.zhang@int
> > > > > > el.c
> > > > > > om>;
> > > > > > 3chas3@gmail.com; Luca Boccassi <bluca@debian.org>; stable@
> > > > > > dpdk
> > > > > > .org
> > > > > > Subject: [PATCH v3 1/2] net/ixgbe: fix x550 code to handle
> > > > > > unidentified PHY
> > > > > >
> > > > > > ixgbe_identify_sfp_module_X550em() was missing the code to
> > > > > > handle
> > > > > > unidentified PHY that has been there in 82599 so it was not
> > > > > > able to
> > > > > > complete initialization of ixgbe sequence if no sfp plugged
> > > > > > in.
> > > > > > Port it over to return an appropriate type and complete
> > > > > > init
> > > > > > sequence
> > > > > > properly.
> > > > > >
> > > > > > Fixes: d2e72774e58c ("ixgbe/base: support X550")
> > > > > > Cc: stable@dpdk.org
> > > > > >
> > > > > > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > > > > > ---
> > > > > > v2: refresh to remove merge conflict with master
> > > > > > v3: coalesce fix into ixgbe_identify_sfp_module_X550em to
> > > > > > avoid
> > > > > > code duplication, improve comment
> > > > > >
> > > > > > drivers/net/ixgbe/base/ixgbe_x550.c | 6 ++++++
> > > > > > 1 file changed, 6 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > > > b/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > > > index f7b98af52..a88d5c86a 100644
> > > > > > --- a/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > > > +++ b/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > > > @@ -1561,6 +1561,12 @@ s32
> > > >
> > > > ixgbe_identify_sfp_module_X550em(struct
> > > > > > ixgbe_hw *hw)
> > > > > >
> > > > > > status = ixgbe_identify_module_generic(hw);
> > > > > >
> > > > > > + /* Set PHY type none if no PHY detected to allow
> > > > > > init
> > > > > > without SFP */
> > > > > > + if (hw->phy.type == ixgbe_phy_unknown) {
> > > > > > + hw->phy.type = ixgbe_phy_none;
> > > > >
> > > > > Set PHY type to none for a device that does have PHY looks
> > > > > weird.
> > > > > does ixgeb_phy_generic works here?
> > > >
> > > > Yes, it does seem strange but that's what
> > > > ixgbe_identify_sfp_module_generic
> > > > seems to do:
> > > >
> > > > err_read_i2c_eeprom:
> > > > hw->phy.sfp_type = ixgbe_sfp_type_not_present;
> > > > if (hw->phy.type != ixgbe_phy_nl) {
> > > > hw->phy.id = 0;
> > > > hw->phy.type = ixgbe_phy_unknown;
> > > > }
> > > >
> > > > The QSFP version a little more forceful:
> > > >
> > > > err_read_i2c_eeprom:
> > > > hw->phy.sfp_type = ixgbe_sfp_type_not_present;
> > > > hw->phy.id = 0;
> > > > hw->phy.type = ixgbe_phy_unknown;
> > > >
> > > > If we go forward without setting the phy_type to none, we will
> > > > eventually run
> > > > into issues calling other phy routines.
> > > >
> > > > So should a lack of SFP, reset the PHY type? It's hazy because
> > > > the
> > > > difference
> > > > between PHY and SFP isn't that clear to me here.
> > >
> > > I'm not sure that's the same case:).
> > > Just feel that it's better to handle ixgbe_phy_unknown directly
> > > for
> > > some device id as a special case than just replace it to
> > > ixgbe_phy_none to cheat the check path, since that rely on we
> > > never
> > > change the way to handle ixgbe_phy_none.
> > >
> > > So still have the question?
> > > What is the failure if you go with ixgbe_phy_unknown?
> > > Is that possible to work around this like
> > > if (phy_type == ixgbe_phy_unknown && dev_id == xxxx)
> > > ...
> >
> > Hi,
> >
> > Thanks for having a look at this again. If you could please see the
> > other answer, from myself, I've quoted the exact error we see and
> > the
> > issue it causes.
>
> Yes, I see it failed at eth_ixgbe_dev_init, it will be better if you
> can provide more detail for the call stack, so we can figure out if
> we can work around this by handle ixgbe_phy_unknown directly with
> some special case.
Hi,
The original problem was found and fixed internally a while ago, so
it's taking some time to get hold of the same hardware again. I hope to
get back with more details next week, sorry for the delay.
On Fri, 2018-11-09 at 13:18 +0000, Luca Boccassi wrote:
> On Wed, 2018-11-07 at 18:27 +0000, Zhang, Qi Z wrote:
> > > -----Original Message-----
> > > From: Luca Boccassi [mailto:bluca@debian.org]
> > > Sent: Wednesday, November 7, 2018 5:55 AM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Chas Williams <3chas3@gma
> > > il
> > > .com>;
> > > dev@dpdk.org
> > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> > > <konstantin.ananyev@intel.com>; stable@dpdk.org
> > > Subject: Re: [PATCH v3 1/2] net/ixgbe: fix x550 code to handle
> > > unidentified
> > > PHY
> > >
> > > On Tue, 2018-11-06 at 23:31 +0000, Zhang, Qi Z wrote:
> > > > > -----Original Message-----
> > > > > From: Chas Williams [mailto:3chas3@gmail.com]
> > > > > Sent: Monday, November 5, 2018 11:19 AM
> > > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Luca Boccassi <bluca@
> > > > > de
> > > > > bian
> > > > > .org>; dev@dpdk.org
> > > > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> > > > > <konstantin.ananyev@intel.com>; stable@dpdk.org
> > > > > Subject: Re: [PATCH v3 1/2] net/ixgbe: fix x550 code to
> > > > > handle
> > > > > unidentified PHY
> > > > >
> > > > >
> > > > >
> > > > > On 11/05/2018 12:41 PM, Zhang, Qi Z wrote:
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Luca Boccassi [mailto:bluca@debian.org]
> > > > > > > Sent: Friday, November 2, 2018 8:19 AM
> > > > > > > To: dev@dpdk.org
> > > > > > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev,
> > > > > > > Konstantin
> > > > > > > <konstantin.ananyev@intel.com>; Zhang, Qi Z <qi.z.zhang@i
> > > > > > > nt
> > > > > > > el.c
> > > > > > > om>;
> > > > > > > 3chas3@gmail.com; Luca Boccassi <bluca@debian.org>;
> > > > > > > stable@
> > > > > > > dpdk
> > > > > > > .org
> > > > > > > Subject: [PATCH v3 1/2] net/ixgbe: fix x550 code to
> > > > > > > handle
> > > > > > > unidentified PHY
> > > > > > >
> > > > > > > ixgbe_identify_sfp_module_X550em() was missing the code
> > > > > > > to
> > > > > > > handle
> > > > > > > unidentified PHY that has been there in 82599 so it was
> > > > > > > not
> > > > > > > able to
> > > > > > > complete initialization of ixgbe sequence if no sfp
> > > > > > > plugged
> > > > > > > in.
> > > > > > > Port it over to return an appropriate type and complete
> > > > > > > init
> > > > > > > sequence
> > > > > > > properly.
> > > > > > >
> > > > > > > Fixes: d2e72774e58c ("ixgbe/base: support X550")
> > > > > > > Cc: stable@dpdk.org
> > > > > > >
> > > > > > > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > > > > > > ---
> > > > > > > v2: refresh to remove merge conflict with master
> > > > > > > v3: coalesce fix into ixgbe_identify_sfp_module_X550em to
> > > > > > > avoid
> > > > > > > code duplication, improve comment
> > > > > > >
> > > > > > > drivers/net/ixgbe/base/ixgbe_x550.c | 6 ++++++
> > > > > > > 1 file changed, 6 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > > > > b/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > > > > index f7b98af52..a88d5c86a 100644
> > > > > > > --- a/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > > > > +++ b/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > > > > @@ -1561,6 +1561,12 @@ s32
> > > > >
> > > > > ixgbe_identify_sfp_module_X550em(struct
> > > > > > > ixgbe_hw *hw)
> > > > > > >
> > > > > > > status = ixgbe_identify_module_generic(hw);
> > > > > > >
> > > > > > > + /* Set PHY type none if no PHY detected to allow
> > > > > > > init
> > > > > > > without SFP */
> > > > > > > + if (hw->phy.type == ixgbe_phy_unknown) {
> > > > > > > + hw->phy.type = ixgbe_phy_none;
> > > > > >
> > > > > > Set PHY type to none for a device that does have PHY looks
> > > > > > weird.
> > > > > > does ixgeb_phy_generic works here?
> > > > >
> > > > > Yes, it does seem strange but that's what
> > > > > ixgbe_identify_sfp_module_generic
> > > > > seems to do:
> > > > >
> > > > > err_read_i2c_eeprom:
> > > > > hw->phy.sfp_type = ixgbe_sfp_type_not_present;
> > > > > if (hw->phy.type != ixgbe_phy_nl) {
> > > > > hw->phy.id = 0;
> > > > > hw->phy.type = ixgbe_phy_unknown;
> > > > > }
> > > > >
> > > > > The QSFP version a little more forceful:
> > > > >
> > > > > err_read_i2c_eeprom:
> > > > > hw->phy.sfp_type = ixgbe_sfp_type_not_present;
> > > > > hw->phy.id = 0;
> > > > > hw->phy.type = ixgbe_phy_unknown;
> > > > >
> > > > > If we go forward without setting the phy_type to none, we
> > > > > will
> > > > > eventually run
> > > > > into issues calling other phy routines.
> > > > >
> > > > > So should a lack of SFP, reset the PHY type? It's hazy
> > > > > because
> > > > > the
> > > > > difference
> > > > > between PHY and SFP isn't that clear to me here.
> > > >
> > > > I'm not sure that's the same case:).
> > > > Just feel that it's better to handle ixgbe_phy_unknown directly
> > > > for
> > > > some device id as a special case than just replace it to
> > > > ixgbe_phy_none to cheat the check path, since that rely on we
> > > > never
> > > > change the way to handle ixgbe_phy_none.
> > > >
> > > > So still have the question?
> > > > What is the failure if you go with ixgbe_phy_unknown?
> > > > Is that possible to work around this like
> > > > if (phy_type == ixgbe_phy_unknown && dev_id == xxxx)
> > > > ...
> > >
> > > Hi,
> > >
> > > Thanks for having a look at this again. If you could please see
> > > the
> > > other answer, from myself, I've quoted the exact error we see and
> > > the
> > > issue it causes.
> >
> > Yes, I see it failed at eth_ixgbe_dev_init, it will be better if
> > you
> > can provide more detail for the call stack, so we can figure out if
> > we can work around this by handle ixgbe_phy_unknown directly with
> > some special case.
>
> Hi,
>
> The original problem was found and fixed internally a while ago, so
> it's taking some time to get hold of the same hardware again. I hope
> to
> get back with more details next week, sorry for the delay.
Hi,
I had the chance to rebase the application on 18.11 and test again, and
it seems like the first patch is no longer necessary. Which is good!
Unfortunately I don't have time to bisect and find exactly when it was
fixed.
I have sent a v4 dropping the first patch.
@@ -1561,6 +1561,12 @@ s32 ixgbe_identify_sfp_module_X550em(struct ixgbe_hw *hw)
status = ixgbe_identify_module_generic(hw);
+ /* Set PHY type none if no PHY detected to allow init without SFP */
+ if (hw->phy.type == ixgbe_phy_unknown) {
+ hw->phy.type = ixgbe_phy_none;
+ return IXGBE_SUCCESS;
+ }
+
if (status != IXGBE_SUCCESS)
return status;