Re: Intel ixgbe: uninitialized variable use in ixgbe_non_sfp_link_config()

Re: Intel ixgbe: uninitialized variable use in ixgbe_non_sfp_link_config()

[Jeff Kirsher]

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

On Sun, 2012-01-08 at 22:21 +0100, Jesper Juhl wrote:
> Hi
> 
> In ixgbe_non_sfp_link_config(), the variable 'negotiation' is declared
> without initializer and unless we take the true branch in the 'if
> ((!autoneg) && (hw->mac.ops.get_link_capabilities))' statement it will
> remain uninitialized when it is subsequently read in the 'ret =
> hw->mac.ops.setup_link(hw, autoneg, negotiation, link_up)' statement.
> 
> The test of 'ret' after the 'if ((!autoneg) &&
> (hw->mac.ops.get_link_capabilities))' statement also looks fairly
> pointless if we do not take the true branch, since then 'ret' will not
> have been changed since the previous identical test.
> 
> The correct fix escapes me since I don't really know this code (and don't 
> plan to spend the time to get to know it), but I thought I'd just report 
> what I had noticed and then someone else can hopefully come up with a good 
> fix :-)
> 
> 
> PS. Please CC me on replies.
> 

Adding netdev mailing list and Don Skidmore (ixgbe maintainer)
Removed Bruce Allan (e1000e maintainer)

I see the potential issue you are referring to, I will defer to Don to
either explain the reasoning in the logic or suggest a fix.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: Intel ixgbe: uninitialized variable use in ixgbe_non_sfp_link_config()

[Skidmore, Donald C]

>-----Original Message-----
>From: Kirsher, Jeffrey T
>Sent: Sunday, January 08, 2012 9:07 PM
>To: Jesper Juhl; Skidmore, Donald C
>Cc: e1000-devel Mailing List; David S. Miller; Brandeburg, Jesse; netdev
>Subject: Re: Intel ixgbe: uninitialized variable use in
>ixgbe_non_sfp_link_config()
>
>On Sun, 2012-01-08 at 22:21 +0100, Jesper Juhl wrote:
>> Hi
>>
>> In ixgbe_non_sfp_link_config(), the variable 'negotiation' is declared
>> without initializer and unless we take the true branch in the 'if
>> ((!autoneg) && (hw->mac.ops.get_link_capabilities))' statement it will
>> remain uninitialized when it is subsequently read in the 'ret =
>> hw->mac.ops.setup_link(hw, autoneg, negotiation, link_up)' statement.
>>
>> The test of 'ret' after the 'if ((!autoneg) &&
>> (hw->mac.ops.get_link_capabilities))' statement also looks fairly
>> pointless if we do not take the true branch, since then 'ret' will not
>> have been changed since the previous identical test.
>>
>> The correct fix escapes me since I don't really know this code (and
>don't
>> plan to spend the time to get to know it), but I thought I'd just
>report
>> what I had noticed and then someone else can hopefully come up with a
>good
>> fix :-)
>>
>>
>> PS. Please CC me on replies.
>>
>
>Adding netdev mailing list and Don Skidmore (ixgbe maintainer)
>Removed Bruce Allan (e1000e maintainer)
>
>I see the potential issue you are referring to, I will defer to Don to
>either explain the reasoning in the logic or suggest a fix.


Thanks for bringing this up Jasper.

I've actually noticed this before and have it on my list of things that need refactoring.  I'll try to get to it much sooner now. :)

The good news is that his doesn't actually cause a problem.  Since all the possible functions that the setup_link pointer points to don't actually use the 'negotiation' variable until it has been initialized in the same said function.  This does begs the question "why do we even pass it in then", well the short answer is we shouldn't.  I plan on refactoring the code to remove it.  The only reason I haven't done it yet is we have other drivers that use this same code which makes the change a bit more complicated coordinating the effort.  

I'll also fix the redundant conditional around the goto while I'm at it.

Thanks again,
-Don Skidmore <donald.c.skidmore@intel.com>