Re: [PATCH v2 5/8] fwctl: FWCTL_RPC to execute a Remote Procedure Call to device firmware

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Jul 26, 2024 at 04:30:09PM +0100, Jonathan Cameron wrote:
> > @@ -8,16 +8,20 @@
> >  #include <linux/slab.h>
> >  #include <linux/container_of.h>
> >  #include <linux/fs.h>
> > +#include <linux/sizes.h>
> >  
> >  #include <uapi/fwctl/fwctl.h>
> >  
> >  enum {
> >  	FWCTL_MAX_DEVICES = 256,
> > +	MAX_RPC_LEN = SZ_2M,
> >  };
> 
> In what way is that usefully handled as an enum?
> I'd just use #defines

I generally am not so keen on defines for constants.. There is some
advantage with clangd and gdb, for instance. Enum is the only other
option even though it is a bit of abuse to use it like this.

> >  DEFINE_FREE(kfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T));
> > +DEFINE_FREE(kvfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kvfree(_T));
> kvfree define free already defined as this since 6.9

Ok
 
> > +	void *inbuf __free(kvfree) =
> > +		kvzalloc(cmd->in_len, GFP_KERNEL | GFP_KERNEL_ACCOUNT);
> 
> As before
> #define GFP_KERNEL_ACCOUNT (GFP_KERNEL | __GFP_ACCOUNT)
> so don't need both.

Yep
 
> > +	if (outbuf == inbuf) {
> > +		/* The driver can re-use inbuf as outbuf */
> > +		inbuf = NULL;
> I wish no_free_ptr() didn't have __must_check. Can do something ugly
> like
> 		outbuf = no_free_ptr(inbuf);
> probably but maybe just setting it NULL is simpler.

Yeah NULL seems clearer, the outbuf assignment is a bit too odd, IMHO

> > +	/**
> > +	 * @FWCTL_RPC_DEBUG_READ_ONLY: Read only access to debug information
> > +	 *
> > +	 * Readable debug information. Debug information is compatible with
> > +	 * kernel lockdown, and does not disclose any sensitive information. For
> > +	 * instance exposing any encryption secrets from this information is
> > +	 * forbidden.
> > +	 */
> > +	FWCTL_RPC_DEBUG_READ_ONLY = 1,
> > +	/**
> > +	 * @FWCTL_RPC_DEBUG_WRITE: Writable access to lockdown compatible debug information
> 
> Write access
> probably rather than writeable.

Sure

Thanks,
Jason