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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 17 Feb 2013 14:41:23 +0100
From:	Michał Mirosław <mirqus@...il.com>
To:	"Tantilov, Emil S" <emil.s.tantilov@...el.com>
Cc:	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	Aurélien Guillaume <footplus@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"gospo@...hat.com" <gospo@...hat.com>,
	"sassmann@...hat.com" <sassmann@...hat.com>
Subject: Re: [net-next 13/15] ixgbe: implement SFF diagnostic monitoring via ethtool

2013/2/17 Tantilov, Emil S <emil.s.tantilov@...el.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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html