Re: [Intel-wired-lan] [iwl-next v1 04/15] ice: add basic devlink subfunctions support

[Jacob Keller <jacob.e.keller@intel.com>]

On 2/13/2024 12:55 AM, Jiri Pirko wrote:
> Tue, Feb 13, 2024 at 08:27:13AM CET, michal.swiatkowski@linux.intel.com wrote:
>> From: Piotr Raczynski <piotr.raczynski@intel.com>
>>
>> Implement devlink port handlers responsible for ethernet type devlink
>> subfunctions. Create subfunction devlink port and setup all resources
>> needed for a subfunction netdev to operate. Configure new VSI for each
>> new subfunction, initialize and configure interrupts and Tx/Rx resources.
>> Set correct MAC filters and create new netdev.
>>
>> For now, subfunction is limited to only one Tx/Rx queue pair.
>>
>> Only allocate new subfunction VSI with devlink port new command.
>> This makes sure that real resources are configured only when a new
>> subfunction gets activated. Allocate and free subfunction MSIX
>> interrupt vectors using new API calls with pci_msix_alloc_irq_at
>> and pci_msix_free_irq.
>>
>> Temporarily, before adding auxiliary bus driver for subfunctions,
>> configure subfunction netdev directly for the created devlink
>> port. This will be modified in the next patch to properly that handle
>> devlink port as the port representor.
>>
>> Support both automatic and manual subfunction numbers. If no subfunction
>> number is provided, use xa_alloc to pick a number automatically. This
>> will find the first free index and use that as the number. This reduces
>> burden on users in the simple case where a specific number is not
>> required. It may also be slightly faster to check that a number exists
>> since xarray lookup should be faster than a linear scan of the dyn_ports
>> xarray.
>>
>> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
>> Co-developed-by: Jacob Keller <jacob.e.keller@intel.com>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> Signed-off-by: Piotr Raczynski <piotr.raczynski@intel.com>
>> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
>> ---
>> drivers/net/ethernet/intel/ice/Makefile       |   1 +
>> .../intel/ice/devlink/ice_devlink_port.c      | 508 ++++++++++++++++++
>> .../intel/ice/devlink/ice_devlink_port.h      |  30 ++
>> drivers/net/ethernet/intel/ice/ice.h          |   4 +
>> drivers/net/ethernet/intel/ice/ice_devlink.c  |   3 +
>> drivers/net/ethernet/intel/ice/ice_lib.c      |   5 +-
>> drivers/net/ethernet/intel/ice/ice_lib.h      |   2 +
>> drivers/net/ethernet/intel/ice/ice_main.c     |  14 +-
>> drivers/net/ethernet/intel/ice/ice_sf_eth.c   | 138 +++++
>> drivers/net/ethernet/intel/ice/ice_sf_eth.h   |  15 +
>> 10 files changed, 716 insertions(+), 4 deletions(-)
>> create mode 100644 drivers/net/ethernet/intel/ice/ice_sf_eth.c
>> create mode 100644 drivers/net/ethernet/intel/ice/ice_sf_eth.h
>>
>> diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile
>> index cd4ab46d72a7..d56a7165df95 100644
>> --- a/drivers/net/ethernet/intel/ice/Makefile
>> +++ b/drivers/net/ethernet/intel/ice/Makefile
>> @@ -31,6 +31,7 @@ ice-y := ice_main.o	\
>> 	 ice_idc.o	\
>> 	 ice_devlink.o	\
>> 	 devlink/ice_devlink_port.o	\
>> +	 ice_sf_eth.o	\
>> 	 ice_ddp.o	\
>> 	 ice_fw_update.o \
>> 	 ice_lag.o	\
>> diff --git a/drivers/net/ethernet/intel/ice/devlink/ice_devlink_port.c b/drivers/net/ethernet/intel/ice/devlink/ice_devlink_port.c
>> index c8c823467fcf..90efceaddb02 100644
>> --- a/drivers/net/ethernet/intel/ice/devlink/ice_devlink_port.c
>> +++ b/drivers/net/ethernet/intel/ice/devlink/ice_devlink_port.c
>> @@ -10,6 +10,8 @@
>> #include "ice_eswitch.h"
>> #include "ice_fw_update.h"
>> #include "ice_dcb_lib.h"
>> +#include "ice_sf_eth.h"
>> +#include "ice_fltr.h"
>>
>> static int ice_active_port_option = -1;
>>
>> @@ -432,3 +434,509 @@ void ice_devlink_destroy_vf_port(struct ice_vf *vf)
>> 	devlink_port_unregister(&vf->devlink_port);
>> }
>>
>> +/**
>> + * ice_activate_dynamic_port - Activate a dynamic port
>> + * @dyn_port: dynamic port instance to activate
>> + * @extack: extack for reporting error messages
>> + *
>> + * Activate the dynamic port based on its flavour.
>> + *
>> + * Return: zero on success or an error code on failure.
>> + */
>> +static int
>> +ice_activate_dynamic_port(struct ice_dynamic_port *dyn_port,
>> +			  struct netlink_ext_ack *extack)
>> +{
>> +	int err;
>> +
>> +	switch (dyn_port->devlink_port.attrs.flavour) {
>> +	case DEVLINK_PORT_FLAVOUR_PCI_SF:
> 
> Pointless switch case.
> 
> It looks like you have odd habbit of checking things that cannot happen
> all over this patch :) See more below...
> 
> 

I remember asking for this kind of split because I have some work for
supporting dynamic creation of Scalable IOV VFs which create a VF port
instead of a SF port. Since it only makes sense in that context, it
makes sense to remove it from this series and add it when its actually
necessary.

That's where the whole "dynamic_port" stuff comes from rather than just
calling it all "sf_port".