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 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;
> +}
> +
>  /**
>   *  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.  If nothing else maybe you should define a function that
takes in the netdev and the pci_dev and computes the hash based on those
values.  Otherwise you are going to end up with each driver doing its'
own version of this function and that will make things rather messy.

Also the port_id has a high likelihood of most fields canceling each
other out.  For example, I'm not sure if you really want to try and find
the offset of the hw_addr within the adapter structure but that is
likely what you are getting by casting the adapter pointer and he
hw_addr arrary as longs and XORing them together.  From what I can tell
the only fields that are providing any real value in distinguishing
between ports on a 4 port adapter would be the dev_addr, flash_address,
and io_base.

Thanks,

Alex