Re: [PATCH net-next 07/10] mlx4: Register mlx4 devices to an auxiliary virtual bus

[Petr Pavlu <petr.pavlu@suse.com>]

On 8/6/23 05:16, Zhu Yanjun wrote:
> 在 2023/8/4 23:05, Petr Pavlu 写道:
>> Add an auxiliary virtual bus to model the mlx4 driver structure. The
>> code is added along the current custom device management logic.
>> Subsequent patches switch mlx4_en and mlx4_ib to the auxiliary bus and
>> the old interface is then removed.
>>
>> Structure mlx4_priv gains a new adev dynamic array to keep track of its
>> auxiliary devices. Access to the array is protected by the global
>> mlx4_intf mutex.
>>
>> Functions mlx4_register_device() and mlx4_unregister_device() are
>> updated to expose auxiliary devices on the bus in order to load mlx4_en
>> and/or mlx4_ib. Functions mlx4_register_auxiliary_driver() and
>> mlx4_unregister_auxiliary_driver() are added to substitute
>> mlx4_register_interface() and mlx4_unregister_interface(), respectively.
>> Function mlx4_do_bond() is adjusted to walk over the adev array and
>> re-adds a specific auxiliary device if its driver sets the
>> MLX4_INTFF_BONDING flag.
>>
>> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
>> Tested-by: Leon Romanovsky <leon@kernel.org>
>> ---
>>   drivers/net/ethernet/mellanox/mlx4/Kconfig |   1 +
>>   drivers/net/ethernet/mellanox/mlx4/intf.c  | 230 ++++++++++++++++++++-
>>   drivers/net/ethernet/mellanox/mlx4/main.c  |  17 +-
>>   drivers/net/ethernet/mellanox/mlx4/mlx4.h  |   6 +
>>   include/linux/mlx4/device.h                |   7 +
>>   include/linux/mlx4/driver.h                |  11 +
>>   6 files changed, 268 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/Kconfig b/drivers/net/ethernet/mellanox/mlx4/Kconfig
>> index 1b4b1f642317..825e05fb8607 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/Kconfig
>> +++ b/drivers/net/ethernet/mellanox/mlx4/Kconfig
>> @@ -27,6 +27,7 @@ config MLX4_EN_DCB
>>   config MLX4_CORE
>>   	tristate
>>   	depends on PCI
>> +	select AUXILIARY_BUS
>>   	select NET_DEVLINK
>>   	default n
>>   
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/intf.c b/drivers/net/ethernet/mellanox/mlx4/intf.c
>> index 30aead34ce08..4b1e18e4a682 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/intf.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/intf.c
>> @@ -48,6 +48,89 @@ struct mlx4_device_context {
>>   static LIST_HEAD(intf_list);
>>   static LIST_HEAD(dev_list);
>>   static DEFINE_MUTEX(intf_mutex);
>> +static DEFINE_IDA(mlx4_adev_ida);
>> +
>> +static const struct mlx4_adev_device {
>> +	const char *suffix;
>> +	bool (*is_supported)(struct mlx4_dev *dev);
>> +} mlx4_adev_devices[1] = {};
>> +
>> +int mlx4_adev_init(struct mlx4_dev *dev)
>> +{
>> +	struct mlx4_priv *priv = mlx4_priv(dev);
>> +
>> +	priv->adev_idx = ida_alloc(&mlx4_adev_ida, GFP_KERNEL);
>> +	if (priv->adev_idx < 0)
>> +		return priv->adev_idx;
>> +
>> +	priv->adev = kcalloc(ARRAY_SIZE(mlx4_adev_devices),
>> +			     sizeof(struct mlx4_adev *), GFP_KERNEL);
>> +	if (!priv->adev) {
>> +		ida_free(&mlx4_adev_ida, priv->adev_idx);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +void mlx4_adev_cleanup(struct mlx4_dev *dev)
>> +{
>> +	struct mlx4_priv *priv = mlx4_priv(dev);
>> +
>> +	kfree(priv->adev);
>> +	ida_free(&mlx4_adev_ida, priv->adev_idx);
>> +}
>> +
>> +static void adev_release(struct device *dev)
>> +{
>> +	struct mlx4_adev *mlx4_adev =
>> +		container_of(dev, struct mlx4_adev, adev.dev);
>> +	struct mlx4_priv *priv = mlx4_priv(mlx4_adev->mdev);
>> +	int idx = mlx4_adev->idx;
>> +
>> +	kfree(mlx4_adev);
>> +	priv->adev[idx] = NULL;
>> +}
>> +
>> +static struct mlx4_adev *add_adev(struct mlx4_dev *dev, int idx)
>> +{
>> +	struct mlx4_priv *priv = mlx4_priv(dev);
>> +	const char *suffix = mlx4_adev_devices[idx].suffix;
>> +	struct auxiliary_device *adev;
>> +	struct mlx4_adev *madev;
>> +	int ret;
>> +
>> +	madev = kzalloc(sizeof(*madev), GFP_KERNEL);
>> +	if (!madev)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	adev = &madev->adev;
>> +	adev->id = priv->adev_idx;
>> +	adev->name = suffix;
>> +	adev->dev.parent = &dev->persist->pdev->dev;
>> +	adev->dev.release = adev_release;
>> +	madev->mdev = dev;
>> +	madev->idx = idx;
>> +
>> +	ret = auxiliary_device_init(adev);
>> +	if (ret) {
>> +		kfree(madev);
>> +		return ERR_PTR(ret);
>> +	}
>> +
>> +	ret = auxiliary_device_add(adev);
>> +	if (ret) {
> 
> madev is allocated, but it is not handled here when auxiliary_device_add 
> error. It should be freed, too?
> That is, add "kfree(madev);" here?
> 
> If madev will be handled in other place, please add some comments here 
> to indicate madev is handled in other place.

A successful call to auxiliary_device_init() registers the device's
.release callback. The madev storage is freed by calling
auxiliary_device_uninit() which invokes adev_release() -> kfree().

Thanks,
Petr