On Tue, Oct 22, 2024 at 11:11:13AM -0700, Li Li wrote: > On Tue, Oct 22, 2024 at 10:06 AM Simon Horman <horms@xxxxxxxxxx> wrote: > > > > On Mon, Oct 21, 2024 at 12:12:33PM -0700, Li Li wrote: > > > + * @pid: the target process > > > + * @flags: the flags to set > > > + * > > > + * If pid is 0, the flags are applied to the whole binder context. > > > + * Otherwise, the flags are applied to the specific process only. > > > + */ > > > +static int binder_genl_set_report(struct binder_context *context, u32 pid, u32 flags) > > > > ... > > > > > static int __init init_binder_device(const char *name) > > > { > > > int ret; > > > @@ -6920,6 +7196,11 @@ static int __init init_binder_device(const char *name) > > > > The code above this hunk looks like this: > > > > > > ret = misc_register(&binder_device->miscdev); > > if (ret < 0) { > > kfree(binder_device); > > return ret; > > } > > > > > > > > hlist_add_head(&binder_device->hlist, &binder_devices); > > > > > > + binder_device->context.report_seq = (atomic_t)ATOMIC_INIT(0); > > > + ret = binder_genl_init(&binder_device->context.genl_family, name); > > > + if (ret < 0) > > > + kfree(binder_device); > > > > So I think that binder_device->miscdev needs to be misc_deregister'ed > > if we hit this error condition. > > > > > + > > > return ret; > > > > Probably adding an unwind ladder like this makes sense (completely untested!): > > > > ret = misc_register(&binder_device->miscdev); > > if (ret < 0) > > goto err_misc_deregister; This should be: goto err_free_dev; > > > > hlist_add_head(&binder_device->hlist, &binder_devices); > > > > binder_device->context.report_seq = (atomic_t)ATOMIC_INIT(0); > > ret = binder_genl_init(&binder_device->context.genl_family, name); > > if (ret < 0); > > goto err_misc_deregister; > > > > return 0; > > > > err_misc_deregister: > > misc_deregister(&binder_device->miscdev); > > err_free_dev: > > kfree(binder_device); > > return ret; > > > > Good catch! Will fix it in the next revision. Thanks. > > > ... > > > > > diff --git a/drivers/android/binder_genl.h b/drivers/android/binder_genl.h > > > > Perhaps it is because of a different version of net-next, > > but with this patch applied on top of the current head commit > > 13feb6074a9f ("binder: report txn errors via generic netlink (genl)") > > I see: > > > > $ ./tools/net/ynl/ynl-regen.sh -f > > $ git diff > > > > diff --git a/include/uapi/linux/android/binder_genl.h b/include/uapi/linux/android/binder_genl.h > > index ef5289133be5..93e58b370420 100644 > > --- a/include/uapi/linux/android/binder_genl.h > > +++ b/include/uapi/linux/android/binder_genl.h > > @@ -3,12 +3,17 @@ > > /* Documentation/netlink/specs/binder_genl.yaml */ > > /* YNL-GEN uapi header */ > > > > -#ifndef _UAPI_LINUX_BINDER_GENL_H > > -#define _UAPI_LINUX_BINDER_GENL_H > > +#ifndef _UAPI_LINUX_ANDROID/BINDER_GENL_H > > +#define _UAPI_LINUX_ANDROID/BINDER_GENL_H > > > > #define BINDER_GENL_FAMILY_NAME "binder_genl" > > #define BINDER_GENL_FAMILY_VERSION 1 > > > > +/** > > + * enum binder_genl_flag - Used with "set" and "reply" command below, defining > > + * what kind \ of binder transactions should be reported to the user space \ > > + * administration process. > > + */ > > enum binder_genl_flag { > > BINDER_GENL_FLAG_FAILED = 1, > > BINDER_GENL_FLAG_DELAYED = 2, > > @@ -34,4 +39,4 @@ enum { > > BINDER_GENL_CMD_MAX = (__BINDER_GENL_CMD_MAX - 1) > > }; > > > > -#endif /* _UAPI_LINUX_BINDER_GENL_H */ > > +#endif /* _UAPI_LINUX_ANDROID/BINDER_GENL_H */ > > The patch was based on the top of Linus's tree instead of net-next. > I'll investigate this difference. Likewise, thanks. If this patch is targeted at net-next you should base it on that tree. Also, on that topic, if so the target should be noted in the subject. Subject: [PATCH net-next vX] ...