On Wed, 2009-06-03 at 14:52 +0400, Dmitry Eremin-Solenikov wrote:
> + IEEE802154_ATTR_CAPABILITY, /* FIXME: this is association */
Fix what?
> +#define IEEE802154_ATTR_MAX (__IEEE802154_ATTR_MAX - 1)
> +#define NLA_HW_ADDR NLA_U64
> +#define NLA_GET_HW_ADDR(attr, addr) do { u64 _temp = nla_get_u64(attr); memcpy(addr, &_temp, 8); } while (0)
> +#define NLA_PUT_HW_ADDR(msg, attr, addr) do { u64 _temp; memcpy(&_temp, addr, 8); NLA_PUT_U64(msg, attr, _temp); } while (0)
I really don't like this namespace pollution.
> +#ifdef IEEE802154_NL_WANT_POLICY
> +static struct nla_policy ieee802154_policy[IEEE802154_ATTR_MAX + 1] = {
Ho humm. This shouldn't be in a header file. Not even with an #ifdef
that exactly one C file then sets.
> + [IEEE802154_ATTR_DURATION] = { .type = NLA_U8, },
> +#ifdef __KERNEL__
> + [IEEE802154_ATTR_ED_LIST] = { .len = 27 },
> +#else
Ick.
> +/* commands */
> +/* REQ should be responded with CONF
> + * and INDIC with RESP
> + */
> +enum {
kernel-doc explaining the commands would be immensely helpful.
> + IEEE802154_GTS_REQ, /* Not supported yet */
> + IEEE802154_GTS_INDIC, /* Not supported yet */
> + IEEE802154_GTS_CONF, /* Not supported yet */
> + IEEE802154_RX_ENABLE_REQ, /* Not supported yet */
> + IEEE802154_RX_ENABLE_CONF, /* Not supported yet */
Just leave it out then. You're fixing ABI here.
> +#ifdef __KERNEL__
> +struct net_device;
> +
> +int ieee802154_nl_assoc_indic(struct net_device *dev, struct ieee802154_addr *addr, u8 cap);
> +int ieee802154_nl_assoc_confirm(struct net_device *dev, u16 short_addr, u8 status);
> +int ieee802154_nl_disassoc_indic(struct net_device *dev, struct ieee802154_addr *addr, u8 reason);
> +int ieee802154_nl_disassoc_confirm(struct net_device *dev, u8 status);
> +int ieee802154_nl_scan_confirm(struct net_device *dev, u8 status, u8 scan_type, u32 unscanned,
> + u8 *edl/*, struct list_head *pan_desc_list */);
> +int ieee802154_nl_beacon_indic(struct net_device *dev, u16 panid, u16 coord_addr); /* TODO */
> +#endif
Why not just use two header files, one in net/ and one in linux/?
> -obj-$(CONFIG_IEEE802154) += af_802154.o
> +obj-$(CONFIG_IEEE802154) += nl802154.o af_802154.o
> +nl802154-objs := netlink.o
That doesn't make any sense. Why the indirection?
> +#include <net/ieee802154/af_ieee802154.h>
> +#define IEEE802154_NL_WANT_POLICY
> +#include <net/ieee802154/nl802154.h>
Like I thought. That's ugly.
> +static int ieee802154_nl_put_failure(struct sk_buff *msg)
> +{
> + void *hdr = genlmsg_data(NLMSG_DATA(msg->data)); /* XXX: nlh is right at the start of msg */
> + genlmsg_cancel(msg, hdr);
> + nlmsg_free(msg);
> + return -ENOBUFS;
> +}
This seems weird.
> +int ieee802154_nl_assoc_indic(struct net_device *dev, struct ieee802154_addr *addr, u8 cap)
> +{
> + struct sk_buff *msg;
> +
> + pr_debug("%s\n", __func__);
> +
> + msg = ieee802154_nl_create(/* flags*/ 0, IEEE802154_ASSOCIATE_INDIC);
> + if (!msg)
> + return -ENOBUFS;
> +
> + NLA_PUT_STRING(msg, IEEE802154_ATTR_DEV_NAME, dev->name);
> + NLA_PUT_U32(msg, IEEE802154_ATTR_DEV_INDEX, dev->ifindex);
> + NLA_PUT_HW_ADDR(msg, IEEE802154_ATTR_HW_ADDR, dev->dev_addr);
> +
> + /* FIXME: check that we really received hw address */
> + NLA_PUT_HW_ADDR(msg, IEEE802154_ATTR_SRC_HW_ADDR, addr->hwaddr);
?
> +static int ieee802154_associate_req(struct sk_buff *skb, struct genl_info *info)
> +{
> + struct net_device *dev;
> + struct ieee802154_addr addr;
> + int ret = -EINVAL;
> +
> + if (!info->attrs[IEEE802154_ATTR_CHANNEL]
> + || !info->attrs[IEEE802154_ATTR_COORD_PAN_ID]
> + || (!info->attrs[IEEE802154_ATTR_COORD_HW_ADDR] && !info->attrs[IEEE802154_ATTR_COORD_SHORT_ADDR])
> + || !info->attrs[IEEE802154_ATTR_CAPABILITY])
> + return -EINVAL;
That's some odd coding style.
johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
