| Message ID | 20240528043813.1342483-6-michal.swiatkowski@linux.intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | ice: support devlink subfunction | expand |
On 5/28/24 06:38, Michal Swiatkowski wrote: > From: Piotr Raczynski <piotr.raczynski@intel.com> > > Make devlink allocation function generic to use it for PF and for SF. > > Add function for SF devlink port creation. It will be used in next > patch. > > Create header file for subfunction device. Define subfunction device > structure there as it is needed for devlink allocation and port > creation. > > Signed-off-by: Piotr Raczynski <piotr.raczynski@intel.com> > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > --- > .../net/ethernet/intel/ice/devlink/devlink.c | 33 +++++++++++++++ > .../net/ethernet/intel/ice/devlink/devlink.h | 1 + > .../ethernet/intel/ice/devlink/devlink_port.c | 41 +++++++++++++++++++ > .../ethernet/intel/ice/devlink/devlink_port.h | 3 ++ > drivers/net/ethernet/intel/ice/ice_sf_eth.h | 21 ++++++++++ > 5 files changed, 99 insertions(+) > create mode 100644 drivers/net/ethernet/intel/ice/ice_sf_eth.h just two minor nitpicks, so: Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > > diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c > index bfb3d5b59a62..58196c170b1b 100644 > --- a/drivers/net/ethernet/intel/ice/devlink/devlink.c > +++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c > @@ -10,6 +10,7 @@ > #include "ice_eswitch.h" > #include "ice_fw_update.h" > #include "ice_dcb_lib.h" > +#include "ice_sf_eth.h" > > /* context for devlink info version reporting */ > struct ice_info_ctx { > @@ -1282,6 +1283,8 @@ static const struct devlink_ops ice_devlink_ops = { > .port_new = ice_devlink_port_new, > }; > > +static const struct devlink_ops ice_sf_devlink_ops; > + > static int > ice_devlink_enable_roce_get(struct devlink *devlink, u32 id, > struct devlink_param_gset_ctx *ctx) > @@ -1422,6 +1425,7 @@ static void ice_devlink_free(void *devlink_ptr) > * Allocate a devlink instance for this device and return the private area as > * the PF structure. The devlink memory is kept track of through devres by > * adding an action to remove it when unwinding. > + * > */ > struct ice_pf *ice_allocate_pf(struct device *dev) > { > @@ -1438,6 +1442,35 @@ struct ice_pf *ice_allocate_pf(struct device *dev) > return devlink_priv(devlink); > } > > +/** > + * ice_allocate_sf - Allocate devlink and return SF structure pointer > + * @dev: the device to allocate for > + * @pf: pointer to the PF structure > + * > + * Allocate a devlink instance for SF. > + * > + * Return: void pointer to allocated memory nit: it's not void; you could add "or ERR_PTR in case of error" > + */ > +struct ice_sf_priv *ice_allocate_sf(struct device *dev, struct ice_pf *pf) > +{ > + struct devlink *devlink; > + int err; > + > + devlink = devlink_alloc_ns(&ice_sf_devlink_ops, > + sizeof(struct ice_sf_priv), > + devlink_net(priv_to_devlink(pf)), dev); > + if (!devlink) > + return NULL; ERR_PTR(-ENOMEM) would be more consistent with the other error exit path > + > + err = devl_nested_devlink_set(priv_to_devlink(pf), devlink); > + if (err) { > + devlink_free(devlink); > + return ERR_PTR(err); > + } > + > + return devlink_priv(devlink); > +} > + > /** > * ice_devlink_register - Register devlink interface for this PF > * @pf: the PF to register the devlink for. > diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.h b/drivers/net/ethernet/intel/ice/devlink/devlink.h > index d291c0e2e17b..1af3b0763fbb 100644 > --- a/drivers/net/ethernet/intel/ice/devlink/devlink.h > +++ b/drivers/net/ethernet/intel/ice/devlink/devlink.h > @@ -5,6 +5,7 @@ > #define _ICE_DEVLINK_H_ > > struct ice_pf *ice_allocate_pf(struct device *dev); > +struct ice_sf_priv *ice_allocate_sf(struct device *dev, struct ice_pf *pf); > > void ice_devlink_register(struct ice_pf *pf); > void ice_devlink_unregister(struct ice_pf *pf); > diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink_port.c b/drivers/net/ethernet/intel/ice/devlink/devlink_port.c > index 5d1fe08e4bab..f06baabd0112 100644 > --- a/drivers/net/ethernet/intel/ice/devlink/devlink_port.c > +++ b/drivers/net/ethernet/intel/ice/devlink/devlink_port.c > @@ -489,6 +489,47 @@ void ice_devlink_destroy_vf_port(struct ice_vf *vf) > devl_port_unregister(&vf->devlink_port); > } > > +/** > + * ice_devlink_create_sf_dev_port - Register virtual port for a subfunction > + * @sf_dev: the subfunction device to create a devlink port for > + * > + * Register virtual flavour devlink port for the subfunction auxiliary device > + * created after activating a dynamically added devlink port. > + * > + * Return: zero on success or an error code on failure. > + */ > +int ice_devlink_create_sf_dev_port(struct ice_sf_dev *sf_dev) > +{ > + struct devlink_port_attrs attrs = {}; > + struct ice_dynamic_port *dyn_port; > + struct devlink_port *devlink_port; > + struct devlink *devlink; > + struct ice_vsi *vsi; > + > + dyn_port = sf_dev->dyn_port; > + vsi = dyn_port->vsi; > + > + devlink_port = &sf_dev->priv->devlink_port; > + > + attrs.flavour = DEVLINK_PORT_FLAVOUR_VIRTUAL; (just comment, not an issue) we have (among others): 198│ enum devlink_port_flavour { 199│ DEVLINK_PORT_FLAVOUR_PHYSICAL, /* Any kind of a port physically 200│ * facing the user. 201│ */ 210│ DEVLINK_PORT_FLAVOUR_PCI_VF, /* Represents eswitch port 211│ * for the PCI VF. It is an internal 212│ * port that faces the PCI VF. 213│ */ 214│ DEVLINK_PORT_FLAVOUR_VIRTUAL, /* Any virtual port facing the user. */ 216│ * is not used in any way. 217│ */ 218│ DEVLINK_PORT_FLAVOUR_PCI_SF, /* Represents eswitch port 219│ * for the PCI SF. It is an internal 220│ * port that faces the PCI SF. 221│ */ from that I conclude that _PCI_ ones are internal, and you are adding user-facing port, so your choice is good, even if there is one with SF in the name. Perhaps the enum should have this piece of documentation ;) > + > + devlink_port_attrs_set(devlink_port, &attrs); > + devlink = priv_to_devlink(sf_dev->priv); > + > + return devl_port_register(devlink, devlink_port, vsi->idx); > +} > + > +/** > + * ice_devlink_destroy_sf_dev_port - Destroy virtual port for a subfunction > + * @sf_dev: the subfunction device to create a devlink port for > + * > + * Unregisters the virtual port associated with this subfunction. > + */ > +void ice_devlink_destroy_sf_dev_port(struct ice_sf_dev *sf_dev) > +{ > + devl_port_unregister(&sf_dev->priv->devlink_port); > +} > + > /** > * ice_dealloc_dynamic_port - Deallocate and remove a dynamic port > * @dyn_port: dynamic port instance to deallocate > diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink_port.h b/drivers/net/ethernet/intel/ice/devlink/devlink_port.h > index 08ebf56664a5..97b21b58c300 100644 > --- a/drivers/net/ethernet/intel/ice/devlink/devlink_port.h > +++ b/drivers/net/ethernet/intel/ice/devlink/devlink_port.h > @@ -5,6 +5,7 @@ > #define _DEVLINK_PORT_H_ > > #include "../ice.h" > +#include "../ice_sf_eth.h" > > /** > * struct ice_dynamic_port - Track dynamically added devlink port instance > @@ -34,6 +35,8 @@ int ice_devlink_create_vf_port(struct ice_vf *vf); > void ice_devlink_destroy_vf_port(struct ice_vf *vf); > int ice_devlink_create_sf_port(struct ice_dynamic_port *dyn_port); > void ice_devlink_destroy_sf_port(struct ice_dynamic_port *dyn_port); > +int ice_devlink_create_sf_dev_port(struct ice_sf_dev *sf_dev); > +void ice_devlink_destroy_sf_dev_port(struct ice_sf_dev *sf_dev); > > #define ice_devlink_port_to_dyn(port) \ > container_of(port, struct ice_dynamic_port, devlink_port) > diff --git a/drivers/net/ethernet/intel/ice/ice_sf_eth.h b/drivers/net/ethernet/intel/ice/ice_sf_eth.h > new file mode 100644 > index 000000000000..a08f8b2bceef > --- /dev/null > +++ b/drivers/net/ethernet/intel/ice/ice_sf_eth.h > @@ -0,0 +1,21 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Copyright (c) 2024, Intel Corporation. */ > + > +#ifndef _ICE_SF_ETH_H_ > +#define _ICE_SF_ETH_H_ > + > +#include <linux/auxiliary_bus.h> > +#include "ice.h" > + > +struct ice_sf_dev { > + struct auxiliary_device adev; > + struct ice_dynamic_port *dyn_port; > + struct ice_sf_priv *priv; > +}; > + > +struct ice_sf_priv { > + struct ice_sf_dev *dev; > + struct devlink_port devlink_port; > +}; > + > +#endif /* _ICE_SF_ETH_H_ */
On Tue, May 28, 2024 at 09:11:09AM +0200, Przemek Kitszel wrote: > On 5/28/24 06:38, Michal Swiatkowski wrote: > > From: Piotr Raczynski <piotr.raczynski@intel.com> > > > > Make devlink allocation function generic to use it for PF and for SF. > > > > Add function for SF devlink port creation. It will be used in next > > patch. > > > > Create header file for subfunction device. Define subfunction device > > structure there as it is needed for devlink allocation and port > > creation. > > > > Signed-off-by: Piotr Raczynski <piotr.raczynski@intel.com> > > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > > --- > > .../net/ethernet/intel/ice/devlink/devlink.c | 33 +++++++++++++++ > > .../net/ethernet/intel/ice/devlink/devlink.h | 1 + > > .../ethernet/intel/ice/devlink/devlink_port.c | 41 +++++++++++++++++++ > > .../ethernet/intel/ice/devlink/devlink_port.h | 3 ++ > > drivers/net/ethernet/intel/ice/ice_sf_eth.h | 21 ++++++++++ > > 5 files changed, 99 insertions(+) > > create mode 100644 drivers/net/ethernet/intel/ice/ice_sf_eth.h > > just two minor nitpicks, so: > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > > > > > diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c > > index bfb3d5b59a62..58196c170b1b 100644 > > --- a/drivers/net/ethernet/intel/ice/devlink/devlink.c > > +++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c > > @@ -10,6 +10,7 @@ > > #include "ice_eswitch.h" > > #include "ice_fw_update.h" > > #include "ice_dcb_lib.h" > > +#include "ice_sf_eth.h" > > /* context for devlink info version reporting */ > > struct ice_info_ctx { > > @@ -1282,6 +1283,8 @@ static const struct devlink_ops ice_devlink_ops = { > > .port_new = ice_devlink_port_new, > > }; > > +static const struct devlink_ops ice_sf_devlink_ops; > > + > > static int > > ice_devlink_enable_roce_get(struct devlink *devlink, u32 id, > > struct devlink_param_gset_ctx *ctx) > > @@ -1422,6 +1425,7 @@ static void ice_devlink_free(void *devlink_ptr) > > * Allocate a devlink instance for this device and return the private area as > > * the PF structure. The devlink memory is kept track of through devres by > > * adding an action to remove it when unwinding. > > + * > > */ > > struct ice_pf *ice_allocate_pf(struct device *dev) > > { > > @@ -1438,6 +1442,35 @@ struct ice_pf *ice_allocate_pf(struct device *dev) > > return devlink_priv(devlink); > > } > > +/** > > + * ice_allocate_sf - Allocate devlink and return SF structure pointer > > + * @dev: the device to allocate for > > + * @pf: pointer to the PF structure > > + * > > + * Allocate a devlink instance for SF. > > + * > > + * Return: void pointer to allocated memory > > nit: it's not void; you could add "or ERR_PTR in case of error" > Right, will fix > > + */ > > +struct ice_sf_priv *ice_allocate_sf(struct device *dev, struct ice_pf *pf) > > +{ > > + struct devlink *devlink; > > + int err; > > + > > + devlink = devlink_alloc_ns(&ice_sf_devlink_ops, > > + sizeof(struct ice_sf_priv), > > + devlink_net(priv_to_devlink(pf)), dev); > > + if (!devlink) > > + return NULL; > > ERR_PTR(-ENOMEM) would be more consistent with the other error exit path > Ok > > + > > + err = devl_nested_devlink_set(priv_to_devlink(pf), devlink); > > + if (err) { > > + devlink_free(devlink); > > + return ERR_PTR(err); > > + } > > + > > + return devlink_priv(devlink); > > +} > > + > > /** > > * ice_devlink_register - Register devlink interface for this PF > > * @pf: the PF to register the devlink for. > > diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.h b/drivers/net/ethernet/intel/ice/devlink/devlink.h > > index d291c0e2e17b..1af3b0763fbb 100644 > > --- a/drivers/net/ethernet/intel/ice/devlink/devlink.h > > +++ b/drivers/net/ethernet/intel/ice/devlink/devlink.h > > @@ -5,6 +5,7 @@ > > #define _ICE_DEVLINK_H_ > > struct ice_pf *ice_allocate_pf(struct device *dev); > > +struct ice_sf_priv *ice_allocate_sf(struct device *dev, struct ice_pf *pf); > > void ice_devlink_register(struct ice_pf *pf); > > void ice_devlink_unregister(struct ice_pf *pf); > > diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink_port.c b/drivers/net/ethernet/intel/ice/devlink/devlink_port.c > > index 5d1fe08e4bab..f06baabd0112 100644 > > --- a/drivers/net/ethernet/intel/ice/devlink/devlink_port.c > > +++ b/drivers/net/ethernet/intel/ice/devlink/devlink_port.c > > @@ -489,6 +489,47 @@ void ice_devlink_destroy_vf_port(struct ice_vf *vf) > > devl_port_unregister(&vf->devlink_port); > > } > > +/** > > + * ice_devlink_create_sf_dev_port - Register virtual port for a subfunction > > + * @sf_dev: the subfunction device to create a devlink port for > > + * > > + * Register virtual flavour devlink port for the subfunction auxiliary device > > + * created after activating a dynamically added devlink port. > > + * > > + * Return: zero on success or an error code on failure. > > + */ > > +int ice_devlink_create_sf_dev_port(struct ice_sf_dev *sf_dev) > > +{ > > + struct devlink_port_attrs attrs = {}; > > + struct ice_dynamic_port *dyn_port; > > + struct devlink_port *devlink_port; > > + struct devlink *devlink; > > + struct ice_vsi *vsi; > > + > > + dyn_port = sf_dev->dyn_port; > > + vsi = dyn_port->vsi; > > + > > + devlink_port = &sf_dev->priv->devlink_port; > > + > > + attrs.flavour = DEVLINK_PORT_FLAVOUR_VIRTUAL; > > (just comment, not an issue) > we have (among others): > 198│ enum devlink_port_flavour { > 199│ DEVLINK_PORT_FLAVOUR_PHYSICAL, /* Any kind of a port physically > 200│ * facing the user. > 201│ */ > 210│ DEVLINK_PORT_FLAVOUR_PCI_VF, /* Represents eswitch port > 211│ * for the PCI VF. It is an > internal > 212│ * port that faces the PCI VF. > 213│ */ > 214│ DEVLINK_PORT_FLAVOUR_VIRTUAL, /* Any virtual port facing the > user. */ > 216│ * is not used in any way. > 217│ */ > 218│ DEVLINK_PORT_FLAVOUR_PCI_SF, /* Represents eswitch port > 219│ * for the PCI SF. It is an > internal > 220│ * port that faces the PCI SF. > 221│ */ > > from that I conclude that _PCI_ ones are internal, and you are adding > user-facing port, so your choice is good, even if there is one with SF > in the name. Perhaps the enum should have this piece of documentation ;) > DEVLINK_PORT_FLAVOUR_PCI_SF is created during port representor creation and linked with his netdev. According to the documentation: Documentation/networking/devlink/devlink-port.rst Thanks, Michal [...]
diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c index bfb3d5b59a62..58196c170b1b 100644 --- a/drivers/net/ethernet/intel/ice/devlink/devlink.c +++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c @@ -10,6 +10,7 @@ #include "ice_eswitch.h" #include "ice_fw_update.h" #include "ice_dcb_lib.h" +#include "ice_sf_eth.h" /* context for devlink info version reporting */ struct ice_info_ctx { @@ -1282,6 +1283,8 @@ static const struct devlink_ops ice_devlink_ops = { .port_new = ice_devlink_port_new, }; +static const struct devlink_ops ice_sf_devlink_ops; + static int ice_devlink_enable_roce_get(struct devlink *devlink, u32 id, struct devlink_param_gset_ctx *ctx) @@ -1422,6 +1425,7 @@ static void ice_devlink_free(void *devlink_ptr) * Allocate a devlink instance for this device and return the private area as * the PF structure. The devlink memory is kept track of through devres by * adding an action to remove it when unwinding. + * */ struct ice_pf *ice_allocate_pf(struct device *dev) { @@ -1438,6 +1442,35 @@ struct ice_pf *ice_allocate_pf(struct device *dev) return devlink_priv(devlink); } +/** + * ice_allocate_sf - Allocate devlink and return SF structure pointer + * @dev: the device to allocate for + * @pf: pointer to the PF structure + * + * Allocate a devlink instance for SF. + * + * Return: void pointer to allocated memory + */ +struct ice_sf_priv *ice_allocate_sf(struct device *dev, struct ice_pf *pf) +{ + struct devlink *devlink; + int err; + + devlink = devlink_alloc_ns(&ice_sf_devlink_ops, + sizeof(struct ice_sf_priv), + devlink_net(priv_to_devlink(pf)), dev); + if (!devlink) + return NULL; + + err = devl_nested_devlink_set(priv_to_devlink(pf), devlink); + if (err) { + devlink_free(devlink); + return ERR_PTR(err); + } + + return devlink_priv(devlink); +} + /** * ice_devlink_register - Register devlink interface for this PF * @pf: the PF to register the devlink for. diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.h b/drivers/net/ethernet/intel/ice/devlink/devlink.h index d291c0e2e17b..1af3b0763fbb 100644 --- a/drivers/net/ethernet/intel/ice/devlink/devlink.h +++ b/drivers/net/ethernet/intel/ice/devlink/devlink.h @@ -5,6 +5,7 @@ #define _ICE_DEVLINK_H_ struct ice_pf *ice_allocate_pf(struct device *dev); +struct ice_sf_priv *ice_allocate_sf(struct device *dev, struct ice_pf *pf); void ice_devlink_register(struct ice_pf *pf); void ice_devlink_unregister(struct ice_pf *pf); diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink_port.c b/drivers/net/ethernet/intel/ice/devlink/devlink_port.c index 5d1fe08e4bab..f06baabd0112 100644 --- a/drivers/net/ethernet/intel/ice/devlink/devlink_port.c +++ b/drivers/net/ethernet/intel/ice/devlink/devlink_port.c @@ -489,6 +489,47 @@ void ice_devlink_destroy_vf_port(struct ice_vf *vf) devl_port_unregister(&vf->devlink_port); } +/** + * ice_devlink_create_sf_dev_port - Register virtual port for a subfunction + * @sf_dev: the subfunction device to create a devlink port for + * + * Register virtual flavour devlink port for the subfunction auxiliary device + * created after activating a dynamically added devlink port. + * + * Return: zero on success or an error code on failure. + */ +int ice_devlink_create_sf_dev_port(struct ice_sf_dev *sf_dev) +{ + struct devlink_port_attrs attrs = {}; + struct ice_dynamic_port *dyn_port; + struct devlink_port *devlink_port; + struct devlink *devlink; + struct ice_vsi *vsi; + + dyn_port = sf_dev->dyn_port; + vsi = dyn_port->vsi; + + devlink_port = &sf_dev->priv->devlink_port; + + attrs.flavour = DEVLINK_PORT_FLAVOUR_VIRTUAL; + + devlink_port_attrs_set(devlink_port, &attrs); + devlink = priv_to_devlink(sf_dev->priv); + + return devl_port_register(devlink, devlink_port, vsi->idx); +} + +/** + * ice_devlink_destroy_sf_dev_port - Destroy virtual port for a subfunction + * @sf_dev: the subfunction device to create a devlink port for + * + * Unregisters the virtual port associated with this subfunction. + */ +void ice_devlink_destroy_sf_dev_port(struct ice_sf_dev *sf_dev) +{ + devl_port_unregister(&sf_dev->priv->devlink_port); +} + /** * ice_dealloc_dynamic_port - Deallocate and remove a dynamic port * @dyn_port: dynamic port instance to deallocate diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink_port.h b/drivers/net/ethernet/intel/ice/devlink/devlink_port.h index 08ebf56664a5..97b21b58c300 100644 --- a/drivers/net/ethernet/intel/ice/devlink/devlink_port.h +++ b/drivers/net/ethernet/intel/ice/devlink/devlink_port.h @@ -5,6 +5,7 @@ #define _DEVLINK_PORT_H_ #include "../ice.h" +#include "../ice_sf_eth.h" /** * struct ice_dynamic_port - Track dynamically added devlink port instance @@ -34,6 +35,8 @@ int ice_devlink_create_vf_port(struct ice_vf *vf); void ice_devlink_destroy_vf_port(struct ice_vf *vf); int ice_devlink_create_sf_port(struct ice_dynamic_port *dyn_port); void ice_devlink_destroy_sf_port(struct ice_dynamic_port *dyn_port); +int ice_devlink_create_sf_dev_port(struct ice_sf_dev *sf_dev); +void ice_devlink_destroy_sf_dev_port(struct ice_sf_dev *sf_dev); #define ice_devlink_port_to_dyn(port) \ container_of(port, struct ice_dynamic_port, devlink_port) diff --git a/drivers/net/ethernet/intel/ice/ice_sf_eth.h b/drivers/net/ethernet/intel/ice/ice_sf_eth.h new file mode 100644 index 000000000000..a08f8b2bceef --- /dev/null +++ b/drivers/net/ethernet/intel/ice/ice_sf_eth.h @@ -0,0 +1,21 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright (c) 2024, Intel Corporation. */ + +#ifndef _ICE_SF_ETH_H_ +#define _ICE_SF_ETH_H_ + +#include <linux/auxiliary_bus.h> +#include "ice.h" + +struct ice_sf_dev { + struct auxiliary_device adev; + struct ice_dynamic_port *dyn_port; + struct ice_sf_priv *priv; +}; + +struct ice_sf_priv { + struct ice_sf_dev *dev; + struct devlink_port devlink_port; +}; + +#endif /* _ICE_SF_ETH_H_ */