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

[Jason Gunthorpe <jgg@nvidia.com>]

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@nvidia.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.

It is not mandatory, drivers could call fwctl_register() directly from
a pci probe function. That is just very undesirable for a secondary
subsystem like fwctl for reasons Leon explained. We want loose
coupling controled by modules and driver binding, not strong coupling
where if you load, say, mlx5_core, you get a million other modules
automatically pulled in as well. Users should have control over this.

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

The design of the FW interface would have to be really off to motivate
an OOT one. IMHO you are more likely to see an intree fwctl driver and
maybe an OOT netdev or something.
 
> > +++ 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

Time flies, thanks

> 
> > + */
> > +#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

It is part of the auxdev methodology. These headers pre-exist for all
the other mlx5 family aux devices to use.

> >   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?

Yes.

> 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, 

I think we want people to have to ask though, don't we? We don't want
to make it easy to write OOT drivers.

> that would also resolve problem of conflicting IDs (my-driver-id
> prior-to and after upstreaming)

Yes it would, but I suggest people get their driver posted before they
start shipping it :)

Jonathan had suggested using a uuid IIRC for the same reason.

Jason