Re: [patch net-next v4 4/4] igb/igbvf: implement ndo_get_phys_port_id

[Alexander Duyck <alexander.h.duyck@intel.com>]

On 07/25/2013 12:58 PM, John Fastabend wrote:
> On 07/25/2013 12:15 PM, Ben Hutchings wrote:
>> On Thu, 2013-07-25 at 12:03 -0700, Alexander Duyck wrote:
>>> On 07/25/2013 11:51 AM, Ben Hutchings wrote:
>>>> On Thu, 2013-07-25 at 11:00 -0700, Alexander Duyck wrote:
>>>>> On 07/25/2013 09:44 AM, Ben Hutchings wrote:
>>>>>> On Thu, 2013-07-25 at 09:17 -0700, Alexander Duyck wrote:
>>>>>>> On 07/25/2013 06:03 AM, Jiri Pirko wrote:
>>>>>>>> @@ -1982,6 +2001,21 @@ static s32 igb_init_i2c(struct
>>>>>>>> igb_adapter *adapter)
>>>>>>>>       return status;
>>>>>>>>   }
>>>>>>>>
>>>>>>>> +static void igb_compute_phys_port_id(struct igb_adapter *adapter)
>>>>>>>> +{
>>>>>>>> +    adapter->phys_port_id = *((u32 *) adapter->netdev->dev_addr);
>>>>>>>> +    adapter->phys_port_id ^= *((u32 *)
>>>>>>>> adapter->netdev->dev_addr + 4);
>>>>>>>> +    adapter->phys_port_id ^= (long) adapter;
>>>>>>>> +    adapter->phys_port_id ^= (long) adapter->hw.hw_addr;
>>>>>>>> +    adapter->phys_port_id ^= (long) adapter->hw.flash_address;
>>>>>>>> +    adapter->phys_port_id ^= (u32) adapter->hw.io_base;
>>>>>>>> +    adapter->phys_port_id ^= adapter->hw.device_id;
>>>>>>>> +    adapter->phys_port_id ^= adapter->hw.subsystem_vendor_id;
>>>>>>>> +    adapter->phys_port_id ^= adapter->hw.subsystem_device_id;
>>>>>>>> +    adapter->phys_port_id ^= adapter->hw.vendor_id;
>>>>>>>> +    adapter->phys_port_id ^= adapter->hw.revision_id;
>>>>>> I didn't look at Jiri's patch initially, but... what's wrong with
>>>>>> using
>>>>>> the MAC address from NVRAM (should already be in netdev->perm_addr)?
>>>>>> That's what I'm expecting to do for the SFC9000 family in sfc.
>>>>>>
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>   /**
>>>>>>>>    *  igb_probe - Device Initialization Routine
>>>>>>>>    *  @pdev: PCI device information struct
>>>>>>> I really think this bit here should be standardized and made
>>>>>>> available
>>>>>>> to all drivers.
>>>>>> [...]
>>>>>>
>>>>>> I think it's a bad example and should not be used in any drivers!
>>>>>>
>>>>>> Ben.
>>>>>>
>>>>> I agree.  That is why the second paragraph started listing what was
>>>>> wrong with this implementation.  What I said was meant in the more
>>>>> general sense that whatever solution should be used should be the
>>>>> same
>>>>> across multiple drivers, not up to each driver to compute.
>>>> I would love to know how you think this can be done generically. 
>>>> If it
>>>> can then we don't need the driver operation at all.
>>>>
>>>> Ben.
>>>>
>>>
>>> Well like you mentioned, you could just pull this out out of
>>> netdev->perm_addr.  You don't need to have anything driver specific as
>>> long as that is the field you are using generic netdev or pci_dev
>>> attributes to do the identification.
>>
>> For a PF driver that never shares the port with another PF, yes.  There
>> are a handful of those drivers so it might be worth sharing an
>> implementation that uses perm_addr, but it's so trivial...
>>
>>> All you need is something that
>>> uniquely identifies the device in the system correct?
>>
>> The spec is that it is universally unique.  I don't know who's going to
>> need that property but I think it's achievable since each physical port
>> normally has at least one globally unique MAC address assigned at
>> manufacturing time.
>>
>>> For that matter
>>> it seems like you could probably just pull the domain, bus, device, and
>>> function number out of the PCI device and that would probably work as
>>> well as long as you cannot somehow have PFs running inside of guests.
>>
>> Some NICs have multiple PFs for the same port, and those could be
>> assigned to guest VMs.
>>
>> All VF drivers would need some way to get the port ID, and that can't be
>> done generically from inside a guest VM.
>>
>> Ben.
>>
>
> I doubt you'll be able to think up a way to do this generically as Ben
> points out. But also no reason for the complicated hash just use the
> perm address of the PF and if you have multiple PFs elect one of the
> n perm address to be the stand-in for the unique one.
>
> .John
>

I think there may have been some miscommunication.  I wasn't talking
about the VF drivers having a generic means of getting the ID.  I would
just want all of the drivers to be using a similar approach for
generating the port ID so that we reduce the risk of any kind of port ID
collision.  We need to make sure we are all stuffing the 6 bytes of
perm_addr into the 4 byte port ID using the same approach.

Thanks,

Alex