On Mon, Dec 16, 2024 at 10:12 AM Carlos Llamas <cmllamas@xxxxxxxxxx> wrote:
>
> On Thu, Dec 12, 2024 at 02:41:14PM -0800, Li Li wrote:
> > ---
> > Documentation/admin-guide/binder_genl.rst | 110 ++++++++
>
> Thanks for renaming to "Binder Netlink" this seems much better IMO.
> Also, I belive the documentation should also be binder_netlink.rst in
> such case?
>
Will also change it.
> > +++ b/Documentation/netlink/specs/binder_netlink.yaml
> > @@ -0,0 +1,108 @@
> > +# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
>
> I think you need a Copyright for this. I'm not sure if it would also be
> needed for the Documentation though.
>
Hmm, all other yaml files follow the same code style.
Netlink experts, can you please clarify this?
> > +/**
> > + * binder_find_proc() - set binder report flags
>
> the description of "binder report flags" is no longer accurate here.
Good catch! Will fix that.
>
> > + * @pid: the target process
> > + */
> > +static struct binder_proc *binder_find_proc(int pid)
> > +{
> > + struct binder_proc *proc;
> > +
> > + mutex_lock(&binder_procs_lock);
> > + hlist_for_each_entry(proc, &binder_procs, proc_node) {
> > + if (proc->pid == pid) {
> > + mutex_unlock(&binder_procs_lock);
> > + return proc;
>
> fwiw, the for_each stops when proc is NULL, so you can just break and
> return proc everytime. e.g.:
>
> mutex_lock(&binder_procs_lock);
> hlist_for_each_entry(proc, &binder_procs, proc_node) {
> if (proc->pid == pid)
> break;
> }
> mutex_unlock(&binder_procs_lock);
>
> return proc;
>
Got it. Thx
> > /**
> > * struct binder_device - information about a binder device node
> > - * @hlist: list of binder devices (only used for devices requested via
> > - * CONFIG_ANDROID_BINDER_DEVICES)
> > + * @hlist: list of binder devices
>
> This is the hunk that needs to go on the first 1/2 patch.
Sure.
> > /**
> > diff --git a/drivers/android/binder_netlink.c b/drivers/android/binder_netlink.c
> > new file mode 100644
> > index 000000000000..2081b4319268
> > --- /dev/null
> > +++ b/drivers/android/binder_netlink.c
> > @@ -0,0 +1,39 @@
> > +// SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
> > +/* Do not edit directly, auto-generated from: */
> > +/* Documentation/netlink/specs/binder_netlink.yaml */
> > +/* YNL-GEN kernel source */
> > +
> > +#include <net/netlink.h>
> > +#include <net/genetlink.h>
> > +
> > +#include "binder_netlink.h"
> > +
> > +#include <uapi/linux/android/binder_netlink.h>
> > +
> > +/* BINDER_NETLINK_CMD_REPORT_SETUP - do */
> > +static const struct nla_policy binder_netlink_report_setup_nl_policy[BINDER_NETLINK_A_CMD_FLAGS + 1] = {
> > + [BINDER_NETLINK_A_CMD_CONTEXT] = { .type = NLA_NUL_STRING, },
> > + [BINDER_NETLINK_A_CMD_PID] = { .type = NLA_U32, },
> > + [BINDER_NETLINK_A_CMD_FLAGS] = NLA_POLICY_MASK(NLA_U32, 0xf),
> > +};
> > +
> > +/* Ops table for binder_netlink */
> > +static const struct genl_split_ops binder_netlink_nl_ops[] = {
>
> not: There are several places where you have "netlink_nl" which seems
> kind of redundant to me. wdyt? IMO you should drop the "nl" in all of
> these cases.
>
These are automatically generated from the yaml file. So let's just
keep them as is.
But it's a good suggestion to the owner of yaml parser.
