netdev - Re: [PATCH v4 07/10] fwctl/mlx5: Support for communicating with mlx5 fw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <daca71bd-547d-406c-83d8-05f8508703b2@intel.com>
Date: Thu, 13 Feb 2025 14:19:38 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Jason Gunthorpe <jgg@...dia.com>
CC: Andy Gospodarek <andrew.gospodarek@...adcom.com>, Aron Silverton
	<aron.silverton@...cle.com>, Dan Williams <dan.j.williams@...el.com>, "Daniel
 Vetter" <daniel.vetter@...ll.ch>, Dave Jiang <dave.jiang@...el.com>, "David
 Ahern" <dsahern@...nel.org>, Andy Gospodarek <gospo@...adcom.com>, "Christoph
 Hellwig" <hch@...radead.org>, Itay Avraham <itayavr@...dia.com>, Jiri Pirko
	<jiri@...dia.com>, Jonathan Cameron <Jonathan.Cameron@...wei.com>, "Jakub
 Kicinski" <kuba@...nel.org>, Leonid Bloch <lbloch@...dia.com>, Leon
 Romanovsky <leonro@...dia.com>, <linux-cxl@...r.kernel.org>,
	<linux-rdma@...r.kernel.org>, <netdev@...r.kernel.org>, Saeed Mahameed
	<saeedm@...dia.com>, "Nelson, Shannon" <shannon.nelson@....com>
Subject: Re: [PATCH v4 07/10] fwctl/mlx5: Support for communicating with mlx5
 fw

On 2/7/25 01:13, Jason Gunthorpe wrote:
> From: Saeed Mahameed <saeedm@...dia.com>

In part this is a general feedback for the subsystem too.

> +FWCTL MLX5 DRIVER

I don't like this design.
That way each and every real driver would need to make another one to
just use fwctl.

Why not just require the real driver to call fwctl_register(opsstruct),
with the required .validate, .do_cmd, etc commands backed there?
There will be much less scaffolding.

Or the intention is to have this little driver replaced by OOT one,
but keep the real (say networking) driver as-is from intree?

> +++ b/drivers/fwctl/mlx5/main.c
> @@ -0,0 +1,340 @@
> +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> +/*
> + * Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES

-2025

> + */
> +#include <linux/fwctl.h>
> +#include <linux/auxiliary_bus.h>
> +#include <linux/mlx5/device.h>
> +#include <linux/mlx5/driver.h>

this breaks abstraction (at least your headers are in nice place, but
this is rather uncommon, typical solution is to have them backed inside
the driver directory) - the two drivers will be tightly coupled

> +module_auxiliary_driver(mlx5ctl_driver);
> +
> +MODULE_IMPORT_NS("FWCTL");
> +MODULE_DESCRIPTION("mlx5 ConnectX fwctl driver");
> +MODULE_AUTHOR("Saeed Mahameed <saeedm@...dia.com>");
> +MODULE_LICENSE("Dual BSD/GPL");
> diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h
> index 7a21f2f011917a..0790b8291ee1bd 100644
> --- a/include/uapi/fwctl/fwctl.h
> +++ b/include/uapi/fwctl/fwctl.h
> @@ -42,6 +42,7 @@ enum {
>   
>   enum fwctl_device_type {
>   	FWCTL_DEVICE_TYPE_ERROR = 0,
> +	FWCTL_DEVICE_TYPE_MLX5 = 1,

is that for fwctl info to be able to properly report what device user
has asked ioctl on? Would be great to embed 32byte long cstring of
DRIVER_NAME, to don't need each and every device to come to you and
ask for inclusion, that would also resolve problem of conflicting IDs
(my-driver-id prior-to and after upstreaming)