Re: [patch net-next v2 03/12] mlxsw: core_linecards: Introduce per line card auxiliary device

[Jiri Pirko <jiri@resnulli.us>]

Wed, Jul 20, 2022 at 10:45:30AM CEST, idosch@nvidia.com wrote:
>On Tue, Jul 19, 2022 at 08:48:38AM +0200, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> In order to be eventually able to expose line card gearbox version and
>> possibility to flash FW, model the line card as a separate device on
>> auxiliary bus.
>
>What is the reason for adding the auxiliary device for the line card
>when it becomes 'provisioned' and not when it becomes 'active'? It needs
>to be explained in the commit message

Okay, done.


>
>> 
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>> ---
>> v1->v2:
>> - added auxdev removal to mlxsw_linecard_fini()
>> - adjusted mlxsw_linecard_bdev_del() to cope with bdev == NULL
>> ---
>>  drivers/net/ethernet/mellanox/mlxsw/Kconfig   |   1 +
>>  drivers/net/ethernet/mellanox/mlxsw/Makefile  |   2 +-
>>  drivers/net/ethernet/mellanox/mlxsw/core.c    |  13 +-
>>  drivers/net/ethernet/mellanox/mlxsw/core.h    |  10 ++
>>  .../mellanox/mlxsw/core_linecard_dev.c        | 155 ++++++++++++++++++
>>  .../ethernet/mellanox/mlxsw/core_linecards.c  |  11 ++
>>  6 files changed, 189 insertions(+), 3 deletions(-)
>>  create mode 100644 drivers/net/ethernet/mellanox/mlxsw/core_linecard_dev.c
>> 
>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/Kconfig b/drivers/net/ethernet/mellanox/mlxsw/Kconfig
>> index 4683312861ac..a510bf2cff2f 100644
>> --- a/drivers/net/ethernet/mellanox/mlxsw/Kconfig
>> +++ b/drivers/net/ethernet/mellanox/mlxsw/Kconfig
>> @@ -7,6 +7,7 @@ config MLXSW_CORE
>>  	tristate "Mellanox Technologies Switch ASICs support"
>>  	select NET_DEVLINK
>>  	select MLXFW
>> +	select AUXILIARY_BUS
>>  	help
>>  	  This driver supports Mellanox Technologies Switch ASICs family.
>>  
>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/Makefile b/drivers/net/ethernet/mellanox/mlxsw/Makefile
>> index c2d6d64ffe4b..3ca9fce759ea 100644
>> --- a/drivers/net/ethernet/mellanox/mlxsw/Makefile
>> +++ b/drivers/net/ethernet/mellanox/mlxsw/Makefile
>> @@ -2,7 +2,7 @@
>>  obj-$(CONFIG_MLXSW_CORE)	+= mlxsw_core.o
>>  mlxsw_core-objs			:= core.o core_acl_flex_keys.o \
>>  				   core_acl_flex_actions.o core_env.o \
>> -				   core_linecards.o
>> +				   core_linecards.o core_linecard_dev.o
>>  mlxsw_core-$(CONFIG_MLXSW_CORE_HWMON) += core_hwmon.o
>>  mlxsw_core-$(CONFIG_MLXSW_CORE_THERMAL) += core_thermal.o
>>  obj-$(CONFIG_MLXSW_PCI)		+= mlxsw_pci.o
>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
>> index 61eb96b93889..831b0d3472c6 100644
>> --- a/drivers/net/ethernet/mellanox/mlxsw/core.c
>> +++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
>> @@ -3334,9 +3334,15 @@ static int __init mlxsw_core_module_init(void)
>>  {
>>  	int err;
>>  
>> +	err = mlxsw_linecard_driver_register();
>> +	if (err)
>> +		return err;
>> +
>>  	mlxsw_wq = alloc_workqueue(mlxsw_core_driver_name, 0, 0);
>> -	if (!mlxsw_wq)
>> -		return -ENOMEM;
>> +	if (!mlxsw_wq) {
>> +		err = -ENOMEM;
>> +		goto err_alloc_workqueue;
>> +	}
>>  	mlxsw_owq = alloc_ordered_workqueue("%s_ordered", 0,
>>  					    mlxsw_core_driver_name);
>>  	if (!mlxsw_owq) {
>> @@ -3347,6 +3353,8 @@ static int __init mlxsw_core_module_init(void)
>>  
>>  err_alloc_ordered_workqueue:
>>  	destroy_workqueue(mlxsw_wq);
>> +err_alloc_workqueue:
>> +	mlxsw_linecard_driver_unregister();
>>  	return err;
>>  }
>>  
>> @@ -3354,6 +3362,7 @@ static void __exit mlxsw_core_module_exit(void)
>>  {
>>  	destroy_workqueue(mlxsw_owq);
>>  	destroy_workqueue(mlxsw_wq);
>> +	mlxsw_linecard_driver_unregister();
>>  }
>>  
>>  module_init(mlxsw_core_module_init);
>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.h b/drivers/net/ethernet/mellanox/mlxsw/core.h
>> index a3491ef2aa7e..b22db13fa547 100644
>> --- a/drivers/net/ethernet/mellanox/mlxsw/core.h
>> +++ b/drivers/net/ethernet/mellanox/mlxsw/core.h
>> @@ -12,6 +12,7 @@
>>  #include <linux/skbuff.h>
>>  #include <linux/workqueue.h>
>>  #include <linux/net_namespace.h>
>> +#include <linux/auxiliary_bus.h>
>>  #include <net/devlink.h>
>>  
>>  #include "trap.h"
>> @@ -561,6 +562,8 @@ enum mlxsw_linecard_status_event_type {
>>  	MLXSW_LINECARD_STATUS_EVENT_TYPE_UNPROVISION,
>>  };
>>  
>> +struct mlxsw_linecard_bdev;
>> +
>>  struct mlxsw_linecard {
>>  	u8 slot_index;
>>  	struct mlxsw_linecards *linecards;
>> @@ -575,6 +578,7 @@ struct mlxsw_linecard {
>>  	   active:1;
>>  	u16 hw_revision;
>>  	u16 ini_version;
>> +	struct mlxsw_linecard_bdev *bdev;
>>  };
>>  
>>  struct mlxsw_linecard_types_info;
>> @@ -614,4 +618,10 @@ void mlxsw_linecards_event_ops_unregister(struct mlxsw_core *mlxsw_core,
>>  					  struct mlxsw_linecards_event_ops *ops,
>>  					  void *priv);
>>  
>> +int mlxsw_linecard_bdev_add(struct mlxsw_linecard *linecard);
>> +void mlxsw_linecard_bdev_del(struct mlxsw_linecard *linecard);
>> +
>> +int mlxsw_linecard_driver_register(void);
>> +void mlxsw_linecard_driver_unregister(void);
>> +
>>  #endif
>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_linecard_dev.c b/drivers/net/ethernet/mellanox/mlxsw/core_linecard_dev.c
>> new file mode 100644
>> index 000000000000..bb6068b62df0
>> --- /dev/null
>> +++ b/drivers/net/ethernet/mellanox/mlxsw/core_linecard_dev.c
>> @@ -0,0 +1,155 @@
>> +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
>> +/* Copyright (c) 2022 NVIDIA Corporation and Mellanox Technologies. All rights reserved */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/err.h>
>> +#include <linux/types.h>
>> +#include <linux/err.h>
>> +#include <linux/auxiliary_bus.h>
>> +#include <linux/idr.h>
>> +#include <linux/gfp.h>
>> +#include <linux/slab.h>
>> +#include <net/devlink.h>
>> +#include "core.h"
>> +
>> +#define MLXSW_LINECARD_DEV_ID_NAME "lc"
>> +
>> +struct mlxsw_linecard_dev {
>> +	struct mlxsw_linecard *linecard;
>> +};
>> +
>> +struct mlxsw_linecard_bdev {
>> +	struct auxiliary_device adev;
>> +	struct mlxsw_linecard *linecard;
>> +	struct mlxsw_linecard_dev *linecard_dev;
>> +};
>> +
>> +static DEFINE_IDA(mlxsw_linecard_bdev_ida);
>> +
>> +static int mlxsw_linecard_bdev_id_alloc(void)
>> +{
>> +	return ida_alloc(&mlxsw_linecard_bdev_ida, GFP_KERNEL);
>> +}
>> +
>> +static void mlxsw_linecard_bdev_id_free(int id)
>> +{
>> +	ida_free(&mlxsw_linecard_bdev_ida, id);
>> +}
>> +
>> +static void mlxsw_linecard_bdev_release(struct device *device)
>> +{
>> +	struct auxiliary_device *adev =
>> +			container_of(device, struct auxiliary_device, dev);
>> +	struct mlxsw_linecard_bdev *linecard_bdev =
>> +			container_of(adev, struct mlxsw_linecard_bdev, adev);
>> +
>> +	mlxsw_linecard_bdev_id_free(adev->id);
>> +	kfree(linecard_bdev);
>> +}
>> +
>> +int mlxsw_linecard_bdev_add(struct mlxsw_linecard *linecard)
>> +{
>> +	struct mlxsw_linecard_bdev *linecard_bdev;
>> +	int err;
>> +	int id;
>> +
>> +	id = mlxsw_linecard_bdev_id_alloc();
>> +	if (id < 0)
>> +		return id;
>> +
>> +	linecard_bdev = kzalloc(sizeof(*linecard_bdev), GFP_KERNEL);
>> +	if (!linecard_bdev) {
>> +		mlxsw_linecard_bdev_id_free(id);
>> +		return -ENOMEM;
>> +	}
>> +	linecard_bdev->adev.id = id;
>> +	linecard_bdev->adev.name = MLXSW_LINECARD_DEV_ID_NAME;
>> +	linecard_bdev->adev.dev.release = mlxsw_linecard_bdev_release;
>> +	linecard_bdev->adev.dev.parent = linecard->linecards->bus_info->dev;
>> +	linecard_bdev->linecard = linecard;
>> +
>> +	err = auxiliary_device_init(&linecard_bdev->adev);
>> +	if (err) {
>> +		mlxsw_linecard_bdev_id_free(id);
>> +		kfree(linecard_bdev);
>> +		return err;
>> +	}
>> +
>> +	err = auxiliary_device_add(&linecard_bdev->adev);
>> +	if (err) {
>> +		auxiliary_device_uninit(&linecard_bdev->adev);
>> +		return err;
>> +	}
>> +
>> +	linecard->bdev = linecard_bdev;
>> +	return 0;
>> +}
>> +
>> +void mlxsw_linecard_bdev_del(struct mlxsw_linecard *linecard)
>> +{
>> +	struct mlxsw_linecard_bdev *linecard_bdev = linecard->bdev;
>> +
>> +	if (!linecard_bdev)
>
>This check is needed because of the invocation from
>mlxsw_linecard_fini() ? If so, please add a comment. Something like:
>
>/* Unprovisioned line cards do not have an auxiliary device. */

Okay, added.


>
>> +		return;
>> +	auxiliary_device_delete(&linecard_bdev->adev);
>> +	auxiliary_device_uninit(&linecard_bdev->adev);
>> +	linecard->bdev = NULL;
>> +}
>> +
>> +static const struct devlink_ops mlxsw_linecard_dev_devlink_ops = {
>> +};
>> +
>> +static int mlxsw_linecard_bdev_probe(struct auxiliary_device *adev,
>> +				     const struct auxiliary_device_id *id)
>> +{
>> +	struct mlxsw_linecard_bdev *linecard_bdev =
>> +			container_of(adev, struct mlxsw_linecard_bdev, adev);
>> +	struct mlxsw_linecard_dev *linecard_dev;
>> +	struct devlink *devlink;
>> +
>> +	devlink = devlink_alloc(&mlxsw_linecard_dev_devlink_ops,
>> +				sizeof(*linecard_dev), &adev->dev);
>> +	if (!devlink)
>> +		return -ENOMEM;
>> +	linecard_dev = devlink_priv(devlink);
>> +	linecard_dev->linecard = linecard_bdev->linecard;
>> +	linecard_bdev->linecard_dev = linecard_dev;
>> +
>> +	devlink_register(devlink);
>> +	return 0;
>> +}
>> +
>> +static void mlxsw_linecard_bdev_remove(struct auxiliary_device *adev)
>> +{
>> +	struct mlxsw_linecard_bdev *linecard_bdev =
>> +			container_of(adev, struct mlxsw_linecard_bdev, adev);
>> +	struct devlink *devlink = priv_to_devlink(linecard_bdev->linecard_dev);
>> +
>> +	devlink_unregister(devlink);
>> +	devlink_free(devlink);
>> +}
>> +
>> +static const struct auxiliary_device_id mlxsw_linecard_bdev_id_table[] = {
>> +	{ .name = KBUILD_MODNAME "." MLXSW_LINECARD_DEV_ID_NAME },
>> +	{},
>> +};
>> +
>> +MODULE_DEVICE_TABLE(auxiliary, mlxsw_linecard_bdev_id_table);
>> +
>> +static struct auxiliary_driver mlxsw_linecard_driver = {
>> +	.name = MLXSW_LINECARD_DEV_ID_NAME,
>> +	.probe = mlxsw_linecard_bdev_probe,
>> +	.remove = mlxsw_linecard_bdev_remove,
>> +	.id_table = mlxsw_linecard_bdev_id_table,
>> +};
>> +
>> +int mlxsw_linecard_driver_register(void)
>> +{
>> +	return auxiliary_driver_register(&mlxsw_linecard_driver);
>> +}
>> +
>> +void mlxsw_linecard_driver_unregister(void)
>> +{
>> +	auxiliary_driver_unregister(&mlxsw_linecard_driver);
>> +}
>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c b/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c
>> index 5c9869dcf674..43696d8badca 100644
>> --- a/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c
>> +++ b/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c
>> @@ -232,6 +232,7 @@ mlxsw_linecard_provision_set(struct mlxsw_linecard *linecard, u8 card_type,
>>  {
>>  	struct mlxsw_linecards *linecards = linecard->linecards;
>>  	const char *type;
>> +	int err;
>>  
>>  	type = mlxsw_linecard_types_lookup(linecards, card_type);
>>  	mlxsw_linecard_status_event_done(linecard,
>> @@ -252,6 +253,14 @@ mlxsw_linecard_provision_set(struct mlxsw_linecard *linecard, u8 card_type,
>>  	linecard->provisioned = true;
>>  	linecard->hw_revision = hw_revision;
>>  	linecard->ini_version = ini_version;
>> +
>> +	err = mlxsw_linecard_bdev_add(linecard);
>> +	if (err) {
>> +		linecard->provisioned = false;
>> +		mlxsw_linecard_provision_fail(linecard);
>> +		return err;
>> +	}
>> +
>>  	devlink_linecard_provision_set(linecard->devlink_linecard, type);
>>  	return 0;
>>  }
>> @@ -260,6 +269,7 @@ static void mlxsw_linecard_provision_clear(struct mlxsw_linecard *linecard)
>>  {
>>  	mlxsw_linecard_status_event_done(linecard,
>>  					 MLXSW_LINECARD_STATUS_EVENT_TYPE_UNPROVISION);
>> +	mlxsw_linecard_bdev_del(linecard);
>>  	linecard->provisioned = false;
>>  	devlink_linecard_provision_clear(linecard->devlink_linecard);
>>  }
>> @@ -885,6 +895,7 @@ static void mlxsw_linecard_fini(struct mlxsw_core *mlxsw_core,
>>  	mlxsw_core_flush_owq();
>>  	if (linecard->active)
>>  		mlxsw_linecard_active_clear(linecard);
>> +	mlxsw_linecard_bdev_del(linecard);
>>  	devlink_linecard_destroy(linecard->devlink_linecard);
>>  	mutex_destroy(&linecard->lock);
>>  }
>> -- 
>> 2.35.3
>>