On Thu, Feb 13, 2025 at 02:19:38PM +0100, Przemek Kitszel wrote:
> On 2/7/25 01:13, Jason Gunthorpe wrote:
> > From: Saeed Mahameed <saeedm@xxxxxxxxxx>
>
> 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.
We invented auxiliary_bus to actually reduce scaffolding. The auxiliary
devices allow split of complex, multi-subsystem devices without need
to create hard binding of their drivers.
It allows for every subsystem to have its own independent driver, which
can be loaded separately, something that is not possible with your idea.
>
> Or the intention is to have this little driver replaced by OOT one,
> but keep the real (say networking) driver as-is from intree?
No, please read the purpose here drivers/base/auxiliary.c:
....
23 * In some subsystems, the functionality of the core device (PCI/ACPI/other) is
24 * too complex for a single device to be managed by a monolithic driver (e.g.
25 * Sound Open Firmware), multiple devices might implement a common intersection
26 * of functionality (e.g. NICs + RDMA), or a driver may want to export an
27 * interface for another subsystem to drive (e.g. SIOV Physical Function export
28 * Virtual Function management). A split of the functionality into child-
29 * devices representing sub-domains of functionality makes it possible to
30 * compartmentalize, layer, and distribute domain-specific concerns via a Linux
31 * device-driver model.
....
>
> > +++ 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
FWCTL driver is connected to auxiliary device which is managed by some
other driver core (in our case mlx5_core). It is coupled by design.
>
> > +module_auxiliary_driver(mlx5ctl_driver);
> > +
> > +MODULE_IMPORT_NS("FWCTL");
> > +MODULE_DESCRIPTION("mlx5 ConnectX fwctl driver");
> > +MODULE_AUTHOR("Saeed Mahameed <saeedm@xxxxxxxxxx>");
> > +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)
Yes, we do want to make sure that FWCTL is used for upstream code and
don't want to open it for any out-of-tree drivers, which wants to use
this interface but didn't send it to upstream.
Thanks
>
>
