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

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Aug 23, 2024 at 03:48:30PM +0100, Jonathan Cameron wrote:
> On Wed, 21 Aug 2024 15:10:59 -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>
> 
> Trivial stuff only. Feel free to ignore if you really like the code
> the way it is.  I don't know the MLX5 parts, but based on just what
> is visible here and in this series.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> > diff --git a/drivers/fwctl/mlx5/main.c b/drivers/fwctl/mlx5/main.c
> > new file mode 100644
> > index 00000000000000..8839770fbe7ba5
> > --- /dev/null
> > +++ b/drivers/fwctl/mlx5/main.c
> 
> 
> > +
> > +static void *mlx5ctl_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
> > +			    void *rpc_in, size_t in_len, size_t *out_len)
> > +{
> > +	struct mlx5ctl_dev *mcdev =
> > +		container_of(uctx->fwctl, struct mlx5ctl_dev, fwctl);
> > +	struct mlx5ctl_uctx *mfd =
> > +		container_of(uctx, struct mlx5ctl_uctx, uctx);
> > +	void *rpc_alloc __free(kvfree) = NULL;
> 
> Whilst you can't completely pair this with destructor, I'd still
> move this as locally as possible.

Yeah, this is a troubling area for cleanup.h here.

I can't really move it as locally as possible because the assignment
is in a scope:

	} else {
		rpc_out = rpc_alloc = kvzalloc(*out_len, GFP_KERNEL);
		if (!rpc_alloc)
			return ERR_PTR(-ENOMEM);
	}

So given the choice of putting it at the top or put a NULL initialized
variable above the if, I'm feeling the top is more kernely?

Or this is just the wrong place to use a cleanup.h technique??

--- a/drivers/fwctl/mlx5/main.c
+++ b/drivers/fwctl/mlx5/main.c
@@ -226,7 +226,6 @@ static void *mlx5ctl_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
                container_of(uctx->fwctl, struct mlx5ctl_dev, fwctl);
        struct mlx5ctl_uctx *mfd =
                container_of(uctx, struct mlx5ctl_uctx, uctx);
-       void *rpc_alloc __free(kvfree) = NULL;
        void *rpc_out;
        int ret;
 
@@ -253,8 +252,8 @@ static void *mlx5ctl_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
        if (*out_len <= in_len) {
                rpc_out = rpc_in;
        } else {
-               rpc_out = rpc_alloc = kvzalloc(*out_len, GFP_KERNEL);
-               if (!rpc_alloc)
+               rpc_out = kvzalloc(*out_len, GFP_KERNEL);
+               if (!rpc_out)
                        return ERR_PTR(-ENOMEM);
        }
 
@@ -272,11 +271,12 @@ static void *mlx5ctl_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
         * but an error code was returned inside out. Everything else
         * means the RPC did not make it to the device.
         */
-       if (ret && ret != -EREMOTEIO)
+       if (ret && ret != -EREMOTEIO) {
+               if (rpc_out != rpc_in)
+                       kfree(rpc_out);
                return ERR_PTR(ret);
-       if (rpc_out == rpc_in)
-               return rpc_in;
-       return_ptr(rpc_alloc);
+       }
+       return rpc_out;
 }

Arguably it is clearer like above.. Let's go with the above, I think
this was too clever a use of cleanup.h, it seems to work alot better
with simpler cases.

> > +static void mlx5ctl_remove(struct auxiliary_device *adev)
> > +{
> > +	struct mlx5ctl_dev *mcdev __free(mlx5ctl) = auxiliary_get_drvdata(adev);
> 
> I'm not keen on the non constructor being paired with destructor
> but it's your code so you get keep the confusion if you really
> like it.
> 
> I'd just have an explicit put.

Yes, I thought I did that already.. Hum must have just thought about it

Jason