Message-Id: <1182811210.6644.22.camel@johannes.berg>
Date:	Tue, 26 Jun 2007 00:40:10 +0200
From:	Johannes Berg <johannes@...solutions.net>
To:	Zhang Rui <rui.zhang@...el.com>
Cc:	hadi@...erus.ca, netdev@...r.kernel.org,
	"linux-acpi@...r" <linux-acpi@...r.kernel.org>, lenb@...nel.org,
	Thomas Graf <tgraf@...g.ch>
Subject: Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

On Tue, 2007-06-19 at 11:32 +0800, Zhang Rui wrote:

> > Ok, by inspection (sorry, still dont have much time) - your kernel code
> > is sending to group 1; i.e
> > 
> > genlmsg_multicast(skb, 0, 1, GFP_ATOMIC);
> > 
> > you need to change that to send to your assigned id, i.e:
> > genlmsg_multicast(skb, 0, acpi_event_genl_family.id, GFP_ATOMIC);
> > 
> Oh, that's the problem.
> Great, now it works happily. :).
> Jamal, thanks for your help!

I wonder if we should hold off on this API until we've worked out the
multicast issue.

Right now we have (mostly by convention afaict) in generic netlink that
everybody has the same group ID as the family ID but that breaks down as
soon as somebody needs more groups than that, which nl80211 will most
likely need.

Hence, the proposal Jamal had was to have a dynamic multicast number
allocator and (if I understood correctly) look up multicast numbers by
family ID/name. This is fairly extensive API/ABI change, but luckily
there are no generic netlink multicast users yet except for the
controller which luckily has the fixed ID "1".

Therefore, if we hold off on this patch until we've written the code for
dynamic multicast groups, we can hardcode the group for controller and
have all others dynamically assigned; if we merge the ACPI events now
we'll have to hardcode the ACPI family ID (and thus multicast group) to
a small number to avoid problems with dynamic multicast groups where the
numbers will be != family ID.



My proposition for the actual dynamic registration interface would be to
add a .groups array to pointers to struct genl_family with that just
being

struct genl_multicast_group {
	char *name;
	u32 id;
}
(as usual, NULL signifies array termination)

and the controller is responsible for assigning the ID and exporting it
to userspace. "name" is a per-family field, something like this patch:

---
 include/linux/genetlink.h |    3 +
 include/net/genetlink.h   |   15 ++++++
 net/netlink/genetlink.c   |  111 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 129 insertions(+)

--- wireless-dev.orig/include/net/genetlink.h	2007-06-25 23:56:59.085732308 +0200
+++ wireless-dev/include/net/genetlink.h	2007-06-26 00:01:43.935732308 +0200
@@ -5,12 +5,26 @@
 #include <net/netlink.h>
 
 /**
+ * struct genl_multicast_group - generic netlink multicast group
+ * @name: name of the multicast group, names are per-family
+ * @id: multicast group ID, assigned by the core, to use with
+ *      genlmsg_multicast().
+ */
+struct genl_multicast_group
+{
+	char	name[GENL_NAMSIZ];
+	u32	id;
+};
+
+/**
  * struct genl_family - generic netlink family
  * @id: protocol family idenfitier
  * @hdrsize: length of user specific header in bytes
  * @name: name of family
  * @version: protocol version
  * @maxattr: maximum number of attributes supported
+ * @multicast_groups: multicast groups to be registered
+ *	for this family (%NULL-terminated array)
  * @attrbuf: buffer to store parsed attributes
  * @ops_list: list of all assigned operations
  * @family_list: family list
@@ -22,6 +36,7 @@ struct genl_family
 	char			name[GENL_NAMSIZ];
 	unsigned int		version;
 	unsigned int		maxattr;
+	struct genl_multicast_group **multicast_groups;
 	struct nlattr **	attrbuf;	/* private */
 	struct list_head	ops_list;	/* private */
 	struct list_head	family_list;	/* private */
--- wireless-dev.orig/net/netlink/genetlink.c	2007-06-25 23:56:02.805732308 +0200
+++ wireless-dev/net/netlink/genetlink.c	2007-06-26 00:39:26.985732308 +0200
@@ -3,6 +3,7 @@
  *
  * 		Authors:	Jamal Hadi Salim
  * 				Thomas Graf <tgraf@...g.ch>
+ *				Johannes Berg <johannes@...solutions.net>
  */
 
 #include <linux/module.h>
@@ -13,6 +14,7 @@
 #include <linux/string.h>
 #include <linux/skbuff.h>
 #include <linux/mutex.h>
+#include <linux/bitmap.h>
 #include <net/sock.h>
 #include <net/genetlink.h>
 
@@ -42,6 +44,15 @@ static void genl_unlock(void)
 #define GENL_FAM_TAB_MASK	(GENL_FAM_TAB_SIZE - 1)
 
 static struct list_head family_ht[GENL_FAM_TAB_SIZE];
+/*
+ * To avoid an allocation at boot of just one unsigned long,
+ * declare it global instead.
+ * Bit 0 (special?) and bit 1 are marked as already used
+ * since group 1 is the controller group.
+ */
+static unsigned long mcast_group_start = 0x3;
+static unsigned long *multicast_groups = &mcast_group_start;
+static unsigned long multicast_group_bits = BITS_PER_LONG;
 
 static int genl_ctrl_event(int event, void *data);
 
@@ -116,6 +127,76 @@ static inline u16 genl_generate_id(void)
 	return id_gen_idx;
 }
 
+static int genl_register_mcast_group(struct genl_multicast_group *grp)
+{
+	int id = find_first_zero_bit(multicast_groups, multicast_group_bits);
+
+	if (id >= multicast_group_bits) {
+		if (multicast_groups == &mcast_group_start) {
+			multicast_groups =
+				kzalloc(multicast_group_bits/BITS_PER_LONG + 1,
+					GFP_KERNEL);
+			if (!multicast_groups)
+				return -ENOMEM;
+			*multicast_groups = mcast_group_start;
+		} else {
+			multicast_groups =
+				krealloc(multicast_groups,
+					 multicast_group_bits/BITS_PER_LONG + 1,
+					 GFP_KERNEL);
+			if (!multicast_groups)
+				return -ENOMEM;
+			multicast_groups[multicast_group_bits/BITS_PER_LONG] = 0;
+		}
+		multicast_group_bits += BITS_PER_LONG;
+	}
+
+	grp->id = id;
+	set_bit(id, multicast_groups);
+	return 0;
+}
+
+static void genl_unregister_mcast_group(struct genl_multicast_group *grp)
+{
+	if (grp->id) {
+		clear_bit(grp->id, multicast_groups);
+		grp->id = 0;
+	}
+}
+
+static int genl_register_mcast_groups(struct genl_family *family)
+{
+	struct genl_multicast_group **grp;
+	int err;
+
+	if (!family->multicast_groups)
+		return 0;
+
+	for (grp = family->multicast_groups; *grp; grp++) {
+		err = genl_register_mcast_group(*grp);
+		if (err) {
+			while (grp != family->multicast_groups) {
+				genl_unregister_mcast_group(*grp);
+				grp--;
+			}
+			return -ENOMEM;
+		}
+	}
+
+	return 0;
+}
+
+static void genl_unregister_mcast_groups(struct genl_family *family)
+{
+	struct genl_multicast_group **grp;
+
+	if (!family->multicast_groups)
+		return;
+
+	for (grp = family->multicast_groups; *grp; grp++)
+		genl_unregister_mcast_group(*grp);
+}
+
 /**
  * genl_register_ops - register generic netlink operations
  * @family: generic netlink family
@@ -240,6 +321,10 @@ int genl_register_family(struct genl_fam
 		family->id = newid;
 	}
 
+	err = genl_register_mcast_groups(family);
+	if (err)
+		goto errout_locked;
+
 	if (family->maxattr) {
 		family->attrbuf = kmalloc((family->maxattr+1) *
 					sizeof(struct nlattr *), GFP_KERNEL);
@@ -290,6 +375,8 @@ int genl_unregister_family(struct genl_f
 		return 0;
 	}
 
+	genl_unregister_mcast_groups(family);
+
 	genl_unlock();
 
 	return -ENOENT;
@@ -374,6 +461,7 @@ static int ctrl_fill_info(struct genl_fa
 			  u32 flags, struct sk_buff *skb, u8 cmd)
 {
 	void *hdr;
+	struct genl_multicast_group **grp;
 
 	hdr = genlmsg_put(skb, pid, seq, &genl_ctrl, flags, cmd);
 	if (hdr == NULL)
@@ -410,6 +498,29 @@ static int ctrl_fill_info(struct genl_fa
 		nla_nest_end(skb, nla_ops);
 	}
 
+	if (family->multicast_groups) {
+		struct nlattr *nla_grps;
+		int idx = 1;
+
+		nla_grps = nla_nest_start(skb, CTRL_ATTR_MCAST_GROUPS);
+		if (nla_grps == NULL)
+			goto nla_put_failure;
+
+		for (grp = family->multicast_groups; *grp; grp++) {
+			struct nlattr *nest;
+
+			nest = nla_nest_start(skb, idx++);
+			if (nest == NULL)
+				goto nla_put_failure;
+
+			NLA_PUT_U32(skb, CTRL_ATTR_MCAST_GRP_ID, (*grp)->id);
+			NLA_PUT_STRING(skb, CTRL_ATTR_MCAST_GRP_NAME, (*grp)->name);
+
+			nla_nest_end(skb, nest);
+		}
+		nla_nest_end(skb, nla_grps);
+	}
+
 	return genlmsg_end(skb, hdr);
 
 nla_put_failure:
--- wireless-dev.orig/include/linux/genetlink.h	2007-06-25 23:56:19.895732308 +0200
+++ wireless-dev/include/linux/genetlink.h	2007-06-26 00:33:36.785732308 +0200
@@ -52,6 +52,9 @@ enum {
 	CTRL_ATTR_HDRSIZE,
 	CTRL_ATTR_MAXATTR,
 	CTRL_ATTR_OPS,
+	CTRL_ATTR_MCAST_GROUPS,
+	CTRL_ATTR_MCAST_GRP_NAME,
+	CTRL_ATTR_MCAST_GRP_ID,
 	__CTRL_ATTR_MAX,
 };
 



johannes

Download attachment "signature.asc" of type "application/pgp-signature" (191 bytes)

Powered by blists - more mailing lists