[Intel-wired-lan] [PATCH v3 07/11] igc: Add HW initialization code

[Shannon Nelson]

On 7/4/2018 6:39 AM, Neftin, Sasha wrote:
> On 6/29/2018 01:25, Shannon Nelson wrote:
>> On 6/24/2018 1:45 AM, Sasha Neftin wrote:

[...]

>>> +
>>> +/**
>>> + *  igc_release_swfw_sync_i225 - Release SW/FW semaphore
>>> + *  @hw: pointer to the HW structure
>>> + *  @mask: specifies which semaphore to acquire
>>> + *
>>> + *  Release the SW/FW semaphore used to access the PHY or NVM.  The 
>>> mask
>>> + *  will also specify which port we're releasing the lock for.
>>> + **/
>>> +void igc_release_swfw_sync_i225(struct e1000_hw *hw, u16 mask)
>>> +{
>>> +    u32 swfw_sync;
>>> +
>>> +    while (igc_get_hw_semaphore_i225(hw))
>>> +        ; /* Empty */
>>
>> This loop needs a bounding condition
>>
> I prefer keep and be alight with another drivers. Any benefit if we will 
> change?

When the firmware is "broken" the semaphore get will never succeed and 
the thread will hang.  We shouldn't be creating opportunities for a 
driver to hang a kernel thread.


[...]

>>>   /**
>>> + *  igc_get_hw_dev - return device
>>> + *  @hw: pointer to hardware structure
>>> + *
>>> + *  used by hardware layer to print debugging information
>>> + **/
>>> +struct net_device *igc_get_hw_dev(struct e1000_hw *hw)
>>> +{
>>> +    struct igc_adapter *adapter = hw->back;
>>> +
>>> +    return adapter->netdev;
>>
>> I think this can either become a #define or an inline in a .h file
>>
> Declared in .h Move it will cause to mess. What is benefit move it?

This is a very simple accessor function to get at the netdev, but 
because it is in a separate code file from those that use it, the 
compiler can't optimize it well - there will likely be an expensive 
function call each time it is used.  If it was set up as a "static 
inline" function in a .h file, the compiler can optimize out the 
function call and save a lot of cycles.

sln