Re: [PATCH v2 7/8] fwctl/mlx5: Support for communicating with mlx5 fw

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 24 Jun 2024 19:47:31 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> From: Saeed Mahameed <saeedm@nvidia.com>
> 
> mlx5's fw has long provided a User Context concept. This has a long
> history in RDMA as part of the devx extended verbs programming
> interface. A User Context is a security envelope that contains objects and
> controls access. It contains the Protection Domain object from the
> InfiniBand Architecture and both togther provide the OS with the necessary
> tools to bind a security context like a process to the device.
> 
> The security context is restricted to not be able to touch the kernel or
> other processes. In the RDMA verbs case it is also restricted to not touch
> global device resources.
> 
> The fwctl_mlx5 takes this approach and builds a User Context per fwctl
> file descriptor and uses a FW security capability on the User Context to
> enable access to global device resources. This makes the context useful
> for provisioning and debugging the global device state.
> 
> mlx5 already has a robust infrastructure for delivering RPC messages to
> fw. Trivially connect fwctl's RPC mechanism to mlx5_cmd_do(). Enforce the
> User Context ID in every RPC header so the FW knows the security context
> of the issuing ID.
> 
> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

A few minor comments + a reference counting question.

> diff --git a/drivers/fwctl/Kconfig b/drivers/fwctl/Kconfig
> index 37147a695add9a..e5ee2d46d43126 100644
> --- a/drivers/fwctl/Kconfig
> +++ b/drivers/fwctl/Kconfig
> @@ -7,3 +7,17 @@ menuconfig FWCTL
>  	  support a wide range of lockdown compatible device behaviors including
>  	  manipulating device FLASH, debugging, and other activities that don't
>  	  fit neatly into an existing subsystem.
> +
> +if FWCTL

Why not use depends on FWCTL?

> +config FWCTL_MLX5
> +	tristate "mlx5 ConnectX control fwctl driver"
> +	depends on MLX5_CORE
> +	help
> +	  MLX5CTL provides interface for the user process to access the debug and
> +	  configuration registers of the ConnectX hardware family
> +	  (NICs, PCI switches and SmartNIC SoCs).
> +	  This will allow configuration and debug tools to work out of the box on
> +	  mainstream kernel.
> +
> +	  If you don't know what to do here, say N.
> +endif

> diff --git a/drivers/fwctl/mlx5/main.c b/drivers/fwctl/mlx5/main.c
> new file mode 100644
> index 00000000000000..5e64371d7e5508
> --- /dev/null
> +++ b/drivers/fwctl/mlx5/main.c



> +static void mlx5ctl_remove(struct auxiliary_device *adev)
> +{
> +	struct mlx5ctl_dev *mcdev __free(mlx5ctl) = auxiliary_get_drvdata(adev);

So this is calling fwctl_put(&mcdev->fwctl) on scope exit.

Why do you need to drop a reference beyond the one fwctl_unregister() is dropping
in cdev_device_del()?  Where am I missing a reference get?

> +
> +	fwctl_unregister(&mcdev->fwctl);
> +}
> +
> +static const struct auxiliary_device_id mlx5ctl_id_table[] = {
> +	{.name = MLX5_ADEV_NAME ".fwctl",},
> +	{},

No point in comma after terminating entries

> +};
> +MODULE_DEVICE_TABLE(auxiliary, mlx5ctl_id_table);
> +
> +static struct auxiliary_driver mlx5ctl_driver = {
> +	.name = "mlx5_fwctl",
> +	.probe = mlx5ctl_probe,
> +	.remove = mlx5ctl_remove,
> +	.id_table = mlx5ctl_id_table,
> +};
> +
> +module_auxiliary_driver(mlx5ctl_driver);
> +
> +MODULE_IMPORT_NS(FWCTL);
> +MODULE_DESCRIPTION("mlx5 ConnectX fwctl driver");
> +MODULE_AUTHOR("Saeed Mahameed <saeedm@nvidia.com>");
> +MODULE_LICENSE("Dual BSD/GPL");

> +#endif