Re: [net-next 13/15] ixgbe: implement SFF diagnostic monitoring via ethtool

["Michał Mirosław" <mirqus@gmail.com>]

2013/2/17 Tantilov, Emil S <emil.s.tantilov@intel.com>:
>>> @@ -2839,6 +2840,117 @@ static int ixgbe_set_channels(struct net_device
>>*dev,
>>>         return ixgbe_setup_tc(dev, netdev_get_num_tc(dev));
>>>  }
>>>
>>> +static int ixgbe_get_module_info(struct net_device *dev,
>>> +                                      struct ethtool_modinfo *modinfo)
>>> +{
>>> +       struct ixgbe_adapter *adapter = netdev_priv(dev);
>>> +       struct ixgbe_hw *hw = &adapter->hw;
>>> +       u32 status;
>>> +       u8 sff8472_rev, addr_mode;
>>> +       int ret_val = 0;
>>> +       bool page_swap = false;
>>> +
>>> +       /* avoid concurent i2c reads */
>>> +       while (test_bit(__IXGBE_IN_SFP_INIT, &adapter->state))
>>> +               msleep(100);
>>> +
>>> +       /* used by the service task */
>>> +       set_bit(__IXGBE_READ_I2C, &adapter->state);
>>
>>This is racy. Why do you need another bit?
>
> The I2C bit helps to reduce the delay in the service task relative to the initialization of the SFP modules.
>
>>
>> while (test_and_set_bit(__IXGBE_IN_SFP_INIT, &adapter->state))
>>       msleep(100);
>>...
>>  clear_bit(__IXGBE_IN_SFP_INIT, &adapter->state)
>
> This is what I had initially, but the i2c reads can take a long time on some parts and __IXGBE_IN_SFP_INIT protects portions of the code that have nothing to do with I2C reads. Setting __IXGBE_IN_SFP_INIT in ethtool while dumping the SFF data can introduce needlessly long delays in the SFP initialization path.

Maybe it would be enough to protect the body of read_i2c_eeprom()
usign a mutex? It seems you want __IXGBE_READ_I2C to work like a
mutex, but you test it in non-atomic way.

Best Regards,
Michał Mirosław