On Mon, Aug 12, 2024 at 10:13 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote: > > On Mon, Aug 12, 2024 at 02:18:44PM -0700, Li Li wrote: > > From: Li Li <dualli@xxxxxxxxxx> > > Sorry, but I can not parse your Subject: line at all. Please use > vowels, we don't have a lack of them :) > > Also look at how other patches are formatted for these files to get an > idea of how to create a good subject line. Thank you for reviewing the patch! Sure, I'll find a more meaningful subject in v2. > > To prevent making the already bloated binder.c even bigger, a new source > > file binder_genl.c is created to host those generic netlink code. > > "genl" is a rough abbreviation that is not going to be easy to remember, > what's wrong with "binder_netlink.c"? It's just because genl has already been used in both of generic netlink kernel code (e.g. in linux/include/net/genetlink.h) and user space libraries https://man7.org/linux/man-pages/man8/genl.8.html. I'm happy to rename it to binder_netlink.c if that's easier to remember. > > > > > > Usage example (user space pseudo code): > > > > // enable binder report from the interested binder context(s) > > struct binder_report_info info = {0, BINDER_REPORT_ALL}; > > ioctl(binder1, BINDER_ENABLE_REPORT, &info); > > ioctl(binder2, BINDER_ENABLE_REPORT, &info); > > > > // set optional per-process report, overriding the global one > > struct binder_report_info info2; > > info2.pid = getpid(); > > info2.flags = BINDER_REPORT_FAILED | BINDER_REPORT_OVERRIDE; > > ioctl(binder2, BINDER_ENABLE_REPORT, &info2); > > > > // open netlink socket > > int fd = socket(AF_NETLINK, SOCK_RAW, NETLINK_GENERIC); > > > > // bind netlink socket > > bind(fd, struct socketaddr); > > > > // get the family id of binder generic netlink > > send(fd, CTRL_CMD_GETFAMILY, CTRL_ATTR_FAMILY_NAME); > > int id = recv(CTRL_ATTR_FAMILY_ID); > > > > // register the current process to receive binder reports > > send(fd, id, BINDER_GENL_CMD_REGISTER); > > > > // verify registration by reading back the registered pid > > recv(fd, BINDER_GENL_ATTR_PID, &pid); > > > > // wait and read all binder reports > > while (running) { > > struct binder_report report; > > recv(fd, BINDER_GENL_ATTR_REPORT, &report); > > > > // process struct binder_report > > do_something(&report); > > } > > > > // clean up > > close(fd); > > What userspace code is now going to use this and require it? How was it > tested? Where is the test code? Where is the new user/kernel api that > you created here documented? As mentioned in the commit message, binder is used in Android OS. But the user space administration process can do little to deal with binder transaction errors. This is tested with Android. I'll upload the user space code to AOSP. If there's a better option to host the test code, e.g. a smaller and simpler project that uses binder, please let me know. > > You added a new ioctl here as well, why not mention that? Why is it > needed? Why not always emit netlink messages? How do you turn them > off? The generic netlink message is a convenient way for the kernel driver to send information to user space. Technically it's possible to replace this new ioctl with a netlink message. But that requires much more unnecessary code parsing the raw byte streams and accessing internal binder_proc and other structures from netlink layer. It's much more elegant to use an ioctl with only a couple lines of source code. To turn them off, just call the same ioctl, resetting the flags to 0. That said, the name of this new ioctl (BINDER_ENABLE_REPORT) isn't good enough. What do you think if I change it to BINDER_SET_NETLINK_REPORT? > > And how does this deal with binder namespaces? Are these messages all > now "global" somehow? Or are they separated properly per namespace? The new ioctl BINDER_ENABLE_REPORT (again, it deserves a better name) sets the report_flags for its associated binder context. Each binder context has its own settings. The messages from all binder contexts are sent to user space via the same netlink socket. The user space code can enable the reports for each binder context separately and distinguish the netlink messages by the name of the binder context. It's also possible to have one netlink socket for each binder context. But it seems like overkill to me. Thanks, Li _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
