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
