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@xxxxxxxxxx> wrote:
>
> > From: Saeed Mahameed <saeedm@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
>
> > 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
