RE: [net-next 07/15] ixgbe: fix SFF data dumps of SFP+ modules

["Tantilov, Emil S" <emil.s.tantilov@intel.com>]

>-----Original Message-----
>From: netdev-owner@vger.kernel.org [mailto:netdev-
>owner@vger.kernel.org] On Behalf Of Ben Hutchings
>Sent: Friday, August 02, 2013 2:09 AM
>To: Kirsher, Jeffrey T
>Cc: davem@davemloft.net; Tantilov, Emil S;
>netdev@vger.kernel.org; gospo@redhat.com;
>sassmann@redhat.com
>Subject: Re: [net-next 07/15] ixgbe: fix SFF data dumps of
>SFP+ modules
>
>On Mon, 2013-07-29 at 05:52 -0700, Jeff Kirsher wrote:
>> From: Emil Tantilov <emil.s.tantilov@intel.com>
>>
>> This patch fixes several issues with the previous
>implementation of the
>> SFF data dump of SFP+ modules:
>>
>> - removed the __IXGBE_READ_I2C flag - I2C access locking
>is handled in the
>>   HW specific routines
>>
>> - fixed the read loop to read data from ee->offset to ee-
>>len
>>
>> - the reads fail if __IXGBE_IN_SFP_INIT is set in the
>process - this is
>>   needed because on some HW I2C operations can take long
>time and disrupt
>>   the SFP and link detection process
>>
>> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
>> Reported-by: Ben Hutchings <bhutchings@solarflare.com>
>> Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>[...]
>> @@ -2969,48 +2955,25 @@ static int
>ixgbe_get_module_eeprom(struct net_device *dev,
>>  	int i = 0;
>>  	int ret_val = 0;
>>
>> -	/* ixgbe_get_module_info is called before this function
>in all
>> -	 * cases, so we do not need any checks we already do
>above,
>> -	 * and can trust ee->len to be a known value.
>> -	 */
>> +	if (ee->len == 0)
>> +		return -EINVAL;
>>
>> -	while (test_bit(__IXGBE_IN_SFP_INIT, &adapter->state))
>> -		msleep(100);
>> -	set_bit(__IXGBE_READ_I2C, &adapter->state);
>> +	for (i = ee->offset; i < ee->len; i++) {
>
>                               i < ee->offset + ee->len
>
>> +		/* I2C reads can take long time */
>> +		if (test_bit(__IXGBE_IN_SFP_INIT, &adapter-
>>state))
>> +			return -EBUSY;
>>
>> -	/* Read the first block, SFF-8079 */
>> -	for (i = 0; i < ETH_MODULE_SFF_8079_LEN; i++) {
>> -		status = hw->phy.ops.read_i2c_eeprom(hw, i,
>&databyte);
>> -		if (status != 0) {
>> -			/* Error occured while reading module */
>> +		if (i < ETH_MODULE_SFF_8079_LEN)
>> +			status  = hw->phy.ops.read_i2c_eeprom(hw, i,
>&databyte);
>> +		else
>> +			status = hw->phy.ops.read_i2c_sff8472(hw, i,
>&databyte);
>[...]
>                                                     i -
>ETH_MODULE_SFF_8079_LEN ?
>
>But this works anyway because the address is truncated to
>u8.

Actually as is the loop will only work when offset is 0, so it looks like it should be:

for (i = ee->offset; i < ee->offset + ee->len; i++) {


Unless you had something else in mind.

I will submit a fix for this.

Thanks,
Emil

>
>Ben.