[PATCH] (resend) ixgbe: always initialize setup_fc

[PATCH] (resend) ixgbe: always initialize setup_fc

[Patrick McLean]

[-- Attachment #1: Type: text/plain, Size: 434 bytes --]

Gmail mangled my first message, sorry about that. Second attempt.

In ixgbe_init_mac_link_ops_X550em, the code has a special case for
backplane media type, but does not fall through to the default case,
so the setup_fc never gets initialized. This causes a panic when it
later tries to set up the card, and the kernel dereferences the null
pointer.

This patch lets the the function fall through, which initialized
setup_fc properly.

[-- Attachment #2: ixgbe-x550-always-initialize-setup_fc.patch --]
[-- Type: text/x-patch, Size: 559 bytes --]

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
index 19b75cd..73e2de7 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
@@ -1653,7 +1653,6 @@ static void ixgbe_init_mac_link_ops_X550em(struct ixgbe_hw *hw)
 		if (hw->device_id == IXGBE_DEV_ID_X550EM_A_SGMII ||
 		    hw->device_id == IXGBE_DEV_ID_X550EM_A_SGMII_L)
 			mac->ops.setup_link = ixgbe_setup_sgmii;
-		break;
 	default:
 		mac->ops.setup_fc = ixgbe_setup_fc_x550em;
 		break;

[Intel-wired-lan] [PATCH] (resend) ixgbe: always initialize setup_fc

[Patrick McLean]

Gmail mangled my first message, sorry about that. Second attempt.

In ixgbe_init_mac_link_ops_X550em, the code has a special case for
backplane media type, but does not fall through to the default case,
so the setup_fc never gets initialized. This causes a panic when it
later tries to set up the card, and the kernel dereferences the null
pointer.

This patch lets the the function fall through, which initialized
setup_fc properly.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ixgbe-x550-always-initialize-setup_fc.patch
Type: text/x-patch
Size: 559 bytes
Desc: not available
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20160701/a3c84a19/attachment.bin>

Re: [PATCH] (resend) ixgbe: always initialize setup_fc

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

Patrick McLean <patrickm@gaikai.com> wrote:

> Gmail mangled my first message, sorry about that. Second attempt.
>
> In ixgbe_init_mac_link_ops_X550em, the code has a special case for
> backplane media type, but does not fall through to the default case,
> so the setup_fc never gets initialized. This causes a panic when it
> later tries to set up the card, and the kernel dereferences the null
> pointer.
>
> This patch lets the the function fall through, which initialized
> setup_fc properly.

I don't think that this is the right fix. My memory is that fc autoneg is  
not supported in that configuration, so setup_fc is intended to be NULL. I  
think it is the reference that needs to have a check added.

[Intel-wired-lan] [PATCH] (resend) ixgbe: always initialize setup_fc

[Rustad, Mark D]

Patrick McLean <patrickm@gaikai.com> wrote:

> Gmail mangled my first message, sorry about that. Second attempt.
>
> In ixgbe_init_mac_link_ops_X550em, the code has a special case for
> backplane media type, but does not fall through to the default case,
> so the setup_fc never gets initialized. This causes a panic when it
> later tries to set up the card, and the kernel dereferences the null
> pointer.
>
> This patch lets the the function fall through, which initialized
> setup_fc properly.

I don't think that this is the right fix. My memory is that fc autoneg is  
not supported in that configuration, so setup_fc is intended to be NULL. I  
think it is the reference that needs to have a check added.

--
Mark Rustad, Networking Division, Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 841 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20160706/b46cb91d/attachment.asc>

RE: [Intel-wired-lan] [PATCH] (resend) ixgbe: always initialize setup_fc

[Tantilov, Emil S]

>-----Original Message-----
>From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On
>Behalf Of Rustad, Mark D
>Sent: Wednesday, July 06, 2016 4:01 PM
>To: Patrick McLean <patrickm@gaikai.com>
>Cc: netdev <netdev@vger.kernel.org>; intel-wired-lan <intel-wired-
>lan@lists.osuosl.org>
>Subject: Re: [Intel-wired-lan] [PATCH] (resend) ixgbe: always initialize
>setup_fc
>
>Patrick McLean <patrickm@gaikai.com> wrote:
>
>> Gmail mangled my first message, sorry about that. Second attempt.
>>
>> In ixgbe_init_mac_link_ops_X550em, the code has a special case for
>> backplane media type, but does not fall through to the default case,
>> so the setup_fc never gets initialized. This causes a panic when it
>> later tries to set up the card, and the kernel dereferences the null
>> pointer.
>>
>> This patch lets the the function fall through, which initialized
>> setup_fc properly.
>
>I don't think that this is the right fix. My memory is that fc autoneg is

setup_fc() does not configure FC autoneg and it should always be set.

I posted an alternative patch that simply sets setup_fc at the beginning of 
the function. The fall-through in the switch statement is not a good solution
because it won't work in case we need to add another case.

http://patchwork.ozlabs.org/patch/646228/

Thanks,
Emil

[Intel-wired-lan] [PATCH] (resend) ixgbe: always initialize setup_fc

[Tantilov, Emil S]

>-----Original Message-----
>From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
>Behalf Of Rustad, Mark D
>Sent: Wednesday, July 06, 2016 4:01 PM
>To: Patrick McLean <patrickm@gaikai.com>
>Cc: netdev <netdev@vger.kernel.org>; intel-wired-lan <intel-wired-
>lan at lists.osuosl.org>
>Subject: Re: [Intel-wired-lan] [PATCH] (resend) ixgbe: always initialize
>setup_fc
>
>Patrick McLean <patrickm@gaikai.com> wrote:
>
>> Gmail mangled my first message, sorry about that. Second attempt.
>>
>> In ixgbe_init_mac_link_ops_X550em, the code has a special case for
>> backplane media type, but does not fall through to the default case,
>> so the setup_fc never gets initialized. This causes a panic when it
>> later tries to set up the card, and the kernel dereferences the null
>> pointer.
>>
>> This patch lets the the function fall through, which initialized
>> setup_fc properly.
>
>I don't think that this is the right fix. My memory is that fc autoneg is

setup_fc() does not configure FC autoneg and it should always be set.

I posted an alternative patch that simply sets setup_fc at the beginning of 
the function. The fall-through in the switch statement is not a good solution
because it won't work in case we need to add another case.

http://patchwork.ozlabs.org/patch/646228/

Thanks,
Emil

[Intel-wired-lan] [PATCH] (resend) ixgbe: always initialize setup_fc

[zhuyj]

Sure. setup_fc should not be null. Emil, your patch can fix it well.

On Fri, Jul 8, 2016 at 8:18 AM, Tantilov, Emil S <emil.s.tantilov@intel.com>
wrote:

> >-----Original Message-----
> >From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org]
> On
> >Behalf Of Rustad, Mark D
> >Sent: Wednesday, July 06, 2016 4:01 PM
> >To: Patrick McLean <patrickm@gaikai.com>
> >Cc: netdev <netdev@vger.kernel.org>; intel-wired-lan <intel-wired-
> >lan at lists.osuosl.org>
> >Subject: Re: [Intel-wired-lan] [PATCH] (resend) ixgbe: always initialize
> >setup_fc
> >
> >Patrick McLean <patrickm@gaikai.com> wrote:
> >
> >> Gmail mangled my first message, sorry about that. Second attempt.
> >>
> >> In ixgbe_init_mac_link_ops_X550em, the code has a special case for
> >> backplane media type, but does not fall through to the default case,
> >> so the setup_fc never gets initialized. This causes a panic when it
> >> later tries to set up the card, and the kernel dereferences the null
> >> pointer.
> >>
> >> This patch lets the the function fall through, which initialized
> >> setup_fc properly.
> >
> >I don't think that this is the right fix. My memory is that fc autoneg is
>
> setup_fc() does not configure FC autoneg and it should always be set.
>
> I posted an alternative patch that simply sets setup_fc at the beginning of
> the function. The fall-through in the switch statement is not a good
> solution
> because it won't work in case we need to add another case.
>
> http://patchwork.ozlabs.org/patch/646228/
>
> Thanks,
> Emil
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20160708/c06a79a6/attachment.html>

Re: [Intel-wired-lan] [PATCH] (resend) ixgbe: always initialize setup_fc

[Patrick McLean]

[-- Attachment #1: Type: text/plain, Size: 1786 bytes --]

How about just initializing it when the rest of the struct is
initialized? This is what is done for every other model.

On Fri, Jul 8, 2016 at 2:47 AM, zhuyj <zyjzyj2000@gmail.com> wrote:
> Sure. setup_fc should not be null. Emil, your patch can fix it well.
>
> On Fri, Jul 8, 2016 at 8:18 AM, Tantilov, Emil S <emil.s.tantilov@intel.com>
> wrote:
>>
>> >-----Original Message-----
>> >From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org]
>> > On
>> >Behalf Of Rustad, Mark D
>> >Sent: Wednesday, July 06, 2016 4:01 PM
>> >To: Patrick McLean <patrickm@gaikai.com>
>> >Cc: netdev <netdev@vger.kernel.org>; intel-wired-lan <intel-wired-
>> >lan@lists.osuosl.org>
>> >Subject: Re: [Intel-wired-lan] [PATCH] (resend) ixgbe: always initialize
>> >setup_fc
>> >
>> >Patrick McLean <patrickm@gaikai.com> wrote:
>> >
>> >> Gmail mangled my first message, sorry about that. Second attempt.
>> >>
>> >> In ixgbe_init_mac_link_ops_X550em, the code has a special case for
>> >> backplane media type, but does not fall through to the default case,
>> >> so the setup_fc never gets initialized. This causes a panic when it
>> >> later tries to set up the card, and the kernel dereferences the null
>> >> pointer.
>> >>
>> >> This patch lets the the function fall through, which initialized
>> >> setup_fc properly.
>> >
>> >I don't think that this is the right fix. My memory is that fc autoneg is
>>
>> setup_fc() does not configure FC autoneg and it should always be set.
>>
>> I posted an alternative patch that simply sets setup_fc at the beginning
>> of
>> the function. The fall-through in the switch statement is not a good
>> solution
>> because it won't work in case we need to add another case.
>>
>> http://patchwork.ozlabs.org/patch/646228/
>>
>> Thanks,
>> Emil
>>
>

[-- Attachment #2: ixgbe-x550-always-initialize-setup_fc.patch --]
[-- Type: text/x-patch, Size: 673 bytes --]

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
index 19b75cd..cfc814a 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
@@ -2915,7 +2915,7 @@ static const struct ixgbe_mac_operations mac_ops_X550EM_x = {
 	.acquire_swfw_sync	= &ixgbe_acquire_swfw_sync_X550em,
 	.release_swfw_sync	= &ixgbe_release_swfw_sync_X550em,
 	.init_swfw_sync		= &ixgbe_init_swfw_sync_X540,
-	.setup_fc		= NULL, /* defined later */
+	.setup_fc		= ixgbe_setup_fc_x550em,
 	.read_iosf_sb_reg	= ixgbe_read_iosf_sb_reg_x550,
 	.write_iosf_sb_reg	= ixgbe_write_iosf_sb_reg_x550,
 };

[Intel-wired-lan] [PATCH] (resend) ixgbe: always initialize setup_fc

[Patrick McLean]

How about just initializing it when the rest of the struct is
initialized? This is what is done for every other model.

On Fri, Jul 8, 2016 at 2:47 AM, zhuyj <zyjzyj2000@gmail.com> wrote:
> Sure. setup_fc should not be null. Emil, your patch can fix it well.
>
> On Fri, Jul 8, 2016 at 8:18 AM, Tantilov, Emil S <emil.s.tantilov@intel.com>
> wrote:
>>
>> >-----Original Message-----
>> >From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org]
>> > On
>> >Behalf Of Rustad, Mark D
>> >Sent: Wednesday, July 06, 2016 4:01 PM
>> >To: Patrick McLean <patrickm@gaikai.com>
>> >Cc: netdev <netdev@vger.kernel.org>; intel-wired-lan <intel-wired-
>> >lan at lists.osuosl.org>
>> >Subject: Re: [Intel-wired-lan] [PATCH] (resend) ixgbe: always initialize
>> >setup_fc
>> >
>> >Patrick McLean <patrickm@gaikai.com> wrote:
>> >
>> >> Gmail mangled my first message, sorry about that. Second attempt.
>> >>
>> >> In ixgbe_init_mac_link_ops_X550em, the code has a special case for
>> >> backplane media type, but does not fall through to the default case,
>> >> so the setup_fc never gets initialized. This causes a panic when it
>> >> later tries to set up the card, and the kernel dereferences the null
>> >> pointer.
>> >>
>> >> This patch lets the the function fall through, which initialized
>> >> setup_fc properly.
>> >
>> >I don't think that this is the right fix. My memory is that fc autoneg is
>>
>> setup_fc() does not configure FC autoneg and it should always be set.
>>
>> I posted an alternative patch that simply sets setup_fc at the beginning
>> of
>> the function. The fall-through in the switch statement is not a good
>> solution
>> because it won't work in case we need to add another case.
>>
>> http://patchwork.ozlabs.org/patch/646228/
>>
>> Thanks,
>> Emil
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ixgbe-x550-always-initialize-setup_fc.patch
Type: text/x-patch
Size: 673 bytes
Desc: not available
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20160708/47f39e87/attachment.bin>

RE: [Intel-wired-lan] [PATCH] (resend) ixgbe: always initialize setup_fc

[Tantilov, Emil S]

>-----Original Message-----
>From: Patrick McLean [mailto:patrickm@gaikai.com]
>Sent: Friday, July 08, 2016 9:45 AM
>To: zhuyj <zyjzyj2000@gmail.com>
>Cc: Tantilov, Emil S <emil.s.tantilov@intel.com>; Rustad, Mark D
><mark.d.rustad@intel.com>; netdev <netdev@vger.kernel.org>; intel-wired-lan
><intel-wired-lan@lists.osuosl.org>
>Subject: Re: [Intel-wired-lan] [PATCH] (resend) ixgbe: always initialize
>setup_fc
>
>How about just initializing it when the rest of the struct is
>initialized? This is what is done for every other model.

That works as well.

Thanks,
Emil

[Intel-wired-lan] [PATCH] (resend) ixgbe: always initialize setup_fc

[Tantilov, Emil S]

>-----Original Message-----
>From: Patrick McLean [mailto:patrickm at gaikai.com]
>Sent: Friday, July 08, 2016 9:45 AM
>To: zhuyj <zyjzyj2000@gmail.com>
>Cc: Tantilov, Emil S <emil.s.tantilov@intel.com>; Rustad, Mark D
><mark.d.rustad@intel.com>; netdev <netdev@vger.kernel.org>; intel-wired-lan
><intel-wired-lan@lists.osuosl.org>
>Subject: Re: [Intel-wired-lan] [PATCH] (resend) ixgbe: always initialize
>setup_fc
>
>How about just initializing it when the rest of the struct is
>initialized? This is what is done for every other model.

That works as well.

Thanks,
Emil