Re: [RFC 1/2] 6lowpan: Add stateful compression for IPHC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Lukasz,

> The patch adds stateful compression (context ids based compression) to the
> 6lowpan module [RFC6282]. The code does not impact current stateless
> compression but only introducing a new feature for IPHC.
> 
> Stateful Compression users allocate context tables through the functions in
> the context.c using public APIs declared in 6lowpan.h. The context.c is
> extending the 6lowpan module and not created as a separate module.
> 
> A debugfs for maintaining the context tables is implemented
> (/sys/kernel/debug/6lowpan/context).
> 
> As context.c is not a separate module the logical place to put this is in the
> iphc.c. In the long term this is not needed and can be removed.
> (See the discussion section below).
> 
> Command usage for the debugfs:
> 
> echo "<COMMAND> <INTERFACE> <CONTEXT_ID> <CONTEXT_PREFIX> <CONTEXT_PREFIX_LENGTH> <COMPRESSION_ENABLE_FLAG" > /sys/kernel/debug/6lowpan/context
> 
> Examples:
> 
> echo "add bt0 3 2001:db8:: 64 1" > /sys/kernel/debug/6lowpan/context
> 
> echo "remove bt0 3" > /sys/kernel/debug/6lowpan/context
> 
> cat /sys/kernel/debug/6lowpan/context
> 
> e.g.
> Context table of interface bt0
> 
> CID Prefix      Len CF
> 3   2001:db8::  64  1
> 4   2001:db8::1 128 1
> 
> Signed-off-by: Lukasz Duda <lukasz.duda@xxxxxxxxxxxxx>
> Signed-off-by: Glenn Ruben Bakke <glenn.ruben.bakke@xxxxxxxxxxxxx>
> ---
> include/net/6lowpan.h |  37 ++++
> net/6lowpan/Makefile  |   2 +-
> net/6lowpan/context.c | 523 ++++++++++++++++++++++++++++++++++++++++++++++++++
> net/6lowpan/context.h |  13 ++
> net/6lowpan/iphc.c    | 259 +++++++++++++++++++++----
> 5 files changed, 791 insertions(+), 43 deletions(-)
> create mode 100644 net/6lowpan/context.c
> create mode 100644 net/6lowpan/context.h
> 
> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
> index dc03d77..27dc870 100644
> --- a/include/net/6lowpan.h
> +++ b/include/net/6lowpan.h
> @@ -197,6 +197,28 @@
> #define LOWPAN_NHC_UDP_CS_P_11	0xF3 /* source & dest = 0xF0B + 4bit inline */
> #define LOWPAN_NHC_UDP_CS_C	0x04 /* checksum elided */
> 
> +struct lowpan_context_table {
> +	struct list_head list;
> +
> +	/* Context entries */
> +	struct net_device *netdev;
> +	struct list_head context_entries;
> +	atomic_t context_count;
> +};
> +
> +struct lowpan_context_entry {
> +	struct list_head list;
> +	struct rcu_head rcu;
> +	struct lowpan_context_table *ctx_table;
> +
> +	/* Context parameters */
> +	u8 cid;
> +	struct in6_addr prefix;
> +	unsigned int prefix_len;
> +	bool compression_flag;
> +};
> +
> +
> #ifdef DEBUG
> /* print data in line */
> static inline void raw_dump_inline(const char *caller, char *msg,
> @@ -335,6 +357,10 @@ lowpan_uncompress_size(const struct sk_buff *skb, u16 *dgram_offset)
> 	if (!(iphc0 & 0x03))
> 		ret++;
> 
> +	if (iphc1 & LOWPAN_IPHC_CID) {
> +		ret++;
> +	}
> +

Single line if statements do not use { }

> 	ret += lowpan_addr_mode_size((iphc1 & LOWPAN_IPHC_SAM) >>
> 				     LOWPAN_IPHC_SAM_BIT);
> 
> @@ -372,6 +398,17 @@ lowpan_uncompress_size(const struct sk_buff *skb, u16 *dgram_offset)
> 	return skb->len + uncomp_header - ret;
> }
> 
> +int lowpan_context_table_alloc(struct net_device *netdev);
> +int lowpan_context_table_free(struct net_device *netdev);
> +int lowpan_context_free_all(void);
> +int lowpan_context_add(struct net_device *netdev,
> +		       struct lowpan_context_entry *entry);
> +int lowpan_context_remove(struct lowpan_context_entry *entry);
> +struct lowpan_context_entry *lowpan_context_decompress(struct net_device *netdev,
> +						       u8 cid);
> +struct lowpan_context_entry *lowpan_context_compress(struct net_device *netdev,
> +						     struct in6_addr *addr);
> +
> int
> lowpan_header_decompress(struct sk_buff *skb, struct net_device *dev,
> 			 const u8 *saddr, const u8 saddr_type,
> diff --git a/net/6lowpan/Makefile b/net/6lowpan/Makefile
> index eb8baa7..bda5be0 100644
> --- a/net/6lowpan/Makefile
> +++ b/net/6lowpan/Makefile
> @@ -1,6 +1,6 @@
> obj-$(CONFIG_6LOWPAN) += 6lowpan.o
> 
> -6lowpan-y := iphc.o nhc.o
> +6lowpan-y := iphc.o nhc.o context.o
> 
> #rfc6282 nhcs
> obj-$(CONFIG_6LOWPAN_NHC_DEST) += nhc_dest.o
> diff --git a/net/6lowpan/context.c b/net/6lowpan/context.c
> new file mode 100644
> index 0000000..b38dcf3
> --- /dev/null
> +++ b/net/6lowpan/context.c
> @@ -0,0 +1,523 @@
> +/**
> +   Copyright (c)  2015 Nordic Semiconductor. All Rights Reserved.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License version 2 and
> +   only version 2 as published by the Free Software Foundation.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> + */
> +
> +#include <linux/if_arp.h>
> +#include <linux/netdevice.h>
> +#include <linux/module.h>
> +#include <linux/debugfs.h>
> +#include <linux/inet.h>
> +
> +#include <net/ipv6.h>
> +#include <net/addrconf.h>
> +#include <net/6lowpan.h>
> +
> +#include <asm/atomic.h>
> +
> +#define LOWPAN_DBG(fmt, ...)  pr_debug(fmt "\n", ##__VA_ARGS__)
> +
> +#define LOWPAN_CONTEXT_TABLE_MAX_SIZE 16
> +#define LOWPAN_CONTEXT_COMMAND_ADD    "add"
> +#define LOWPAN_CONTEXT_COMMAND_REMOVE "remove"
> +
> +#define CHECK_CID_RANGE(cid) ((cid) > 0 && (cid) < LOWPAN_CONTEXT_TABLE_MAX_SIZE)
> +#define CHECK_PREFIX_RANGE(len) ((len) > 0 && (len) <= 128)
> +
> +static LIST_HEAD(context_tables);
> +static DEFINE_SPINLOCK(module_lock);
> +
> +static inline void entry_update(struct lowpan_context_entry *entry,
> +				u8 cid,
> +				struct in6_addr prefix,
> +				unsigned int prefix_len,
> +				bool compression_flag)
> +{
> +	/* Fill context entry. */
> +	entry->cid = cid;
> +	entry->compression_flag = compression_flag;
> +	entry->prefix = prefix;
> +	entry->prefix_len = prefix_len;
> +}
> +
> +static inline void entry_free(struct lowpan_context_entry *entry)
> +{
> +	/* Increase number of available contexts. */
> +	atomic_dec(&entry->ctx_table->context_count);
> +
> +	list_del_rcu(&entry->list);
> +	kfree_rcu(entry, rcu);
> +}
> +
> +static int entry_add(struct lowpan_context_table *ctx_table,
> +		    u8 cid,
> +		    struct in6_addr prefix,
> +		    unsigned int prefix_len,
> +		    bool compression_flag)

All of these should align with "struct .."

> +{
> +	struct lowpan_context_entry *new_entry = NULL;
> +
> +	new_entry = kzalloc(sizeof(*new_entry), GFP_ATOMIC);
> +	if (!new_entry)
> +		return -ENOMEM;
> +
> +	/* Fill context entry. */
> +	new_entry->ctx_table = ctx_table;
> +	entry_update(new_entry, cid, prefix, prefix_len, compression_flag);
> +
> +	INIT_LIST_HEAD(&new_entry->list);
> +	list_add_rcu(&new_entry->list, &ctx_table->context_entries);
> +
> +	/* Increase number of available contexts. */
> +	atomic_inc(&ctx_table->context_count);
> +
> +	return 0;
> +}
> +
> +static void table_free(struct lowpan_context_table *ctx_table)
> +{
> +	struct lowpan_context_entry *entry = NULL;
> +
> +	rcu_read_lock();
> +
> +	/* Clear context table. */
> +	if (atomic_read(&ctx_table->context_count)) {
> +		list_for_each_entry_rcu(entry, &ctx_table->context_entries, list) {
> +			entry_free(entry);
> +		}

Do we need the { } here?

> +	}
> +
> +	rcu_read_unlock();
> +
> +	/* Remove context table. */
> +	list_del(&ctx_table->list);
> +	kfree(ctx_table);
> +}
> +
> +static inline struct net_device *find_netdev(const char *name)
> +{
> +	struct net_device *netdev;
> +
> +	rcu_read_lock();
> +	netdev = dev_get_by_name_rcu(&init_net, name);
> +	rcu_read_unlock();
> +
> +	return netdev;
> +}
> +
> +static struct lowpan_context_table *get_table(struct net_device *netdev)
> +{
> +	struct lowpan_context_table *entry;
> +
> +	spin_lock(&module_lock);
> +
> +	list_for_each_entry(entry, &context_tables, list) {
> +			if (entry->netdev == netdev) {
> +				/* Table has been found. */
> +				spin_unlock(&module_lock);
> +				return entry;
> +			}

Too many indentations here.

And personally why need initialize entry = NULL and just call break here. Lot easier to check if there is only one lock and one unlock.

> +	}
> +
> +	spin_unlock(&module_lock);
> +
> +	return NULL;
> +}
> +
> +static struct lowpan_context_entry *get_ctx_by_cid(struct lowpan_context_table *table,
> +						   u8 cid)
> +{
> +	struct lowpan_context_entry *entry = NULL;
> +
> +	rcu_read_lock();
> +
> +	list_for_each_entry_rcu(entry, &table->context_entries, list) {
> +		if (entry->cid == cid) {
> +			/* Context entry has been found. */
> +			rcu_read_unlock();
> +			return entry;
> +		}

Comment as above.

> +	}
> +
> +	rcu_read_unlock();
> +
> +	return NULL;
> +}
> +
> +static struct lowpan_context_entry *get_ctx_by_addr(struct lowpan_context_table *table,
> +						    struct in6_addr *addr)
> +{
> +	struct lowpan_context_entry *entry;
> +	struct lowpan_context_entry *best_match = NULL;
> +
> +	rcu_read_lock();
> +
> +	list_for_each_entry_rcu(entry, &table->context_entries, list) {
> +		if (entry->compression_flag) {

		if (!something)
			continue;

> +			if (ipv6_prefix_equal(addr, &entry->prefix,
> +					(entry->prefix_len >= 64) ? entry->prefix_len : 64)) {
> +				/* Valid context entry has been found. Check if prefix can cover
> +				 * more bytes than previous one.
> +				 */
> +				if (!best_match || (best_match->prefix_len < entry->prefix_len)) {
> +					best_match = entry;
> +				}

No { } for single line if statements.

> +			}
> +		}
> +	}
> +
> +	rcu_read_unlock();
> +
> +	return best_match;
> +}
> +
> +ssize_t lowpan_context_dbgfs_write(struct file *fp,
> +				   const char __user *user_buffer,
> +				   size_t count,
> +				   loff_t *position)
> +{
> +	char buf[128], str_operation[16], str_interface[16], str_ipv6addr[40];
> +	size_t buf_size = min(count, sizeof(buf) - 1);
> +	int n, ret;
> +	u8 cid;
> +	unsigned int prefix_length, c_flag;
> +	struct in6_addr ctx_prefix;
> +	struct net_device *netdev;
> +	struct lowpan_context_table *ctx_table;
> +	struct lowpan_context_entry *entry;
> +
> +	memset(&ctx_prefix, 0, sizeof(ctx_prefix));
> +
> +	if (copy_from_user(buf, user_buffer, buf_size))
> +		return -EFAULT;
> +
> +	buf[buf_size] = '\0';
> +
> +	/* Parse input parameters. */
> +	n = sscanf(buf, "%s %s %hhd %s %d %d",
> +			str_operation, str_interface, &cid, str_ipv6addr,
> +			&prefix_length, &c_flag);

Alignment is wrong. It needs to align with buf.

> +
> +	/* Look up for net device. */
> +	if (!(netdev = find_netdev(str_interface))) {

In general the (!(x = y)) are not preferred. Assign it first and then check. Run checkpatch.pl.

> +		LOWPAN_DBG("Cannot find given interface");

Why are these debugs and don't actually print a real error.

> +		return -EINVAL;
> +	}
> +
> +	if (!CHECK_CID_RANGE(cid)) {
> +		LOWPAN_DBG("CID value is out of range");
> +		return -EINVAL;
> +	}
> +
> +	if (!(ctx_table = get_table(netdev))){
> +		LOWPAN_DBG("Cannot find context table for %s interface",
> +				str_interface);

Alignment.

> +		return -EINVAL;
> +	}
> +
> +	entry = get_ctx_by_cid(ctx_table, cid);
> +
> +	/* Execute command. */
> +	if (memcmp(str_operation, LOWPAN_CONTEXT_COMMAND_ADD,
> +			sizeof(LOWPAN_CONTEXT_COMMAND_ADD)) == 0) {

I prefer !memcmp and alignment please.

> +		/* Parse IPv6 address. */
> +		in6_pton(str_ipv6addr,
> +			 sizeof(str_ipv6addr),
> +			 ctx_prefix.s6_addr,
> +			 '\0',
> +			 NULL);
> +
> +		/* Check parameters. */
> +		if(ipv6_addr_any(&ctx_prefix) || !CHECK_PREFIX_RANGE(prefix_length)) {

If_space_(

> +			LOWPAN_DBG("Given parameters are not valid");
> +			return -EINVAL;
> +		}
> +
> +		LOWPAN_DBG("Got new add request: I:%s CID:%d Prefix:%pI6c/%d C:%d",
> +				str_interface, cid, &ctx_prefix, prefix_length, c_flag);


You need to follow the networking subsystem alignment rules. See checkpatch.pl --strict.

> +
> +		/* Check if entry for given CID already exists. */
> +		if (entry) {
> +			/* Just update parameters. */
> +			rcu_read_lock();
> +			entry_update(entry, cid, ctx_prefix, prefix_length,
> +					(bool)c_flag);
> +			rcu_read_unlock();
> +		}
> +		else {

} else {

> +			/* Create a new entry. */
> +			if((ret = entry_add(ctx_table, cid, ctx_prefix, prefix_length,

If_space_(

And assignment.

> +					(bool)c_flag)) < 0) {


Oh, and alignment.

I am also not loving the (bool) cast here. Why do we need that. !!c_flag not good enough?

Please fix all the spacing, alignment and coding style issues since I stop mentioning them from now on. There are just too many.

> +				LOWPAN_DBG("Cannot add context entry");
> +				return ret;
> +			}
> +		}
> +	}
> +	else if (memcmp(str_operation, LOWPAN_CONTEXT_COMMAND_REMOVE,
> +			sizeof(LOWPAN_CONTEXT_COMMAND_REMOVE)) == 0) {
> +		LOWPAN_DBG("Got new remove request: I:%s CID:%d",
> +				str_interface, cid);
> +
> +		if (!entry) {
> +			LOWPAN_DBG("Cannot find context entry");
> +			return -EINVAL;
> +		}
> +
> +		rcu_read_lock();
> +		entry_free(entry);
> +		rcu_read_unlock();
> +	}
> +	else {
> +		LOWPAN_DBG("Invalid command - Cannot process the request");
> +		return -EINVAL;
> +	}
> +
> +	return count;
> +}
> +
> +static int lowpan_context_show(struct seq_file *f, void *ptr)
> +{
> +	struct lowpan_context_table *table;
> +	struct lowpan_context_entry *entry;
> +
> +	spin_lock(&module_lock);

Really? module_lock? Why do we need that?

> +
> +	list_for_each_entry(table, &context_tables, list) {
> +		if (atomic_read(&table->context_count)) {
> +			seq_printf(f, "Context table of interface %s\r\n\r\n",
> +					table->netdev->name);
> +			seq_printf(f, "%-3s %-39s %-3s %-3s \r\n",
> +					"CID", "Prefix", "Len", "CF");
> +
> +			list_for_each_entry_rcu(entry, &table->context_entries, list) {
> +				seq_printf(f, "%-3d %-39pI6c %-3d %-3s \r\n",
> +					entry->cid,
> +					&entry->prefix,
> +					entry->prefix_len,
> +					entry->compression_flag ? "1" : "0");
> +			}
> +
> +			seq_printf(f, "\r\n\r\n");
> +		}
> +	}
> +
> +	spin_unlock(&module_lock);
> +
> +	return 0;
> +}
> +
> +int lowpan_context_dbgfs_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, lowpan_context_show, inode->i_private);
> +}
> +
> +int lowpan_context_free_all(void)
> +{
> +	struct lowpan_context_table *entry = NULL;
> +
> +	spin_lock(&module_lock);
> +
> +	list_for_each_entry(entry, &context_tables, list) {
> +		table_free(entry);
> +	}
> +
> +	spin_unlock(&module_lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(lowpan_context_free_all);
> +
> +int lowpan_context_table_alloc(struct net_device *netdev)
> +{
> +	struct lowpan_context_table *ctx_table = NULL;
> +
> +	if (!netdev) {
> +		LOWPAN_DBG("Network device has to be specified");
> +		return -EPERM;
> +	}
> +
> +	/* Look for context table. */
> +	ctx_table = get_table(netdev);
> +
> +	if (ctx_table) {
> +		LOWPAN_DBG("Context table already exists for interface %s %p",
> +				netdev->name, ctx_table);
> +		return -EEXIST;
> +	}
> +
> +	ctx_table = kzalloc(sizeof(*ctx_table), GFP_ATOMIC);
> +	if (!ctx_table)
> +		return -ENOMEM;
> +
> +	/* Assign network device to context table. */
> +	ctx_table->netdev = netdev;
> +
> +	/* Initialize contexts list. */
> +	INIT_LIST_HEAD(&ctx_table->context_entries);
> +
> +	spin_lock(&module_lock);
> +	INIT_LIST_HEAD(&ctx_table->list);
> +	list_add_rcu(&ctx_table->list, &context_tables);
> +	spin_unlock(&module_lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(lowpan_context_table_alloc);
> +
> +int lowpan_context_table_free(struct net_device *netdev)
> +{
> +	struct lowpan_context_table *ctx_table = NULL;
> +
> +	if (!netdev) {
> +		LOWPAN_DBG("Network device has to be specified");
> +		return -EPERM;
> +	}
> +
> +	/* Look for context table. */
> +	ctx_table = get_table(netdev);
> +	if (!ctx_table) {
> +		LOWPAN_DBG("Cannot find context table for interface %s",
> +				netdev->name);
> +		return -ENOENT;
> +	}
> +
> +	spin_lock(&module_lock);
> +	table_free(ctx_table);
> +	spin_unlock(&module_lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(lowpan_context_table_free);
> +
> +int lowpan_context_add(struct net_device *netdev,
> +		       struct lowpan_context_entry *entry)
> +{
> +	int ret;
> +	struct lowpan_context_table *ctx_table = NULL;
> +	struct lowpan_context_entry *stored_entry = NULL;
> +
> +	if (!entry || !netdev) {
> +		LOWPAN_DBG("Parameters are incorrect");
> +		return -EPERM;
> +	}
> +
> +	if (!(ctx_table = get_table(netdev))){
> +		LOWPAN_DBG("Cannot find context table for %s interface", netdev->name);
> +		return -EINVAL;
> +	}
> +
> +	stored_entry = get_ctx_by_cid(ctx_table, entry->cid);
> +
> +	/* Check if entry for given CID already exists. */
> +	if (entry) {
> +		/* Just update parameters. */
> +		rcu_read_lock();
> +		entry_update(stored_entry,
> +			     entry->cid,
> +			     entry->prefix,
> +			     entry->prefix_len,
> +			     entry->compression_flag);
> +		rcu_read_unlock();
> +	}
> +	else {
> +		/* Create a new entry. */
> +		if ((ret = entry_add(ctx_table, entry->cid, entry->prefix,
> +				entry->prefix_len, entry->compression_flag)) < 0) {
> +			LOWPAN_DBG("Cannot add context entry");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(lowpan_context_add);
> +
> +int lowpan_context_remove(struct lowpan_context_entry *entry)
> +{
> +	if (!entry) {
> +		LOWPAN_DBG("Given entry is NULL");
> +		return -EPERM;
> +	}
> +
> +	/* Free given entry. */
> +	rcu_read_lock();
> +	entry_free(entry);
> +	rcu_read_unlock();
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(lowpan_context_remove);
> +
> +struct lowpan_context_entry *lowpan_context_decompress(struct net_device *netdev,
> +						       u8 cid)
> +{
> +	struct lowpan_context_table *ctx_table = NULL;
> +	struct lowpan_context_entry *entry = NULL;
> +
> +	if (!netdev) {
> +		LOWPAN_DBG("Network device has to be specified");
> +		return NULL;
> +	}
> +
> +	/* Get proper interface table. */
> +	ctx_table = get_table(netdev);
> +	if (!ctx_table) {
> +		LOWPAN_DBG("Cannot find context table for interface %s", netdev->name);
> +		return NULL;
> +	}
> +
> +	/* Search for given CID. */
> +	entry = get_ctx_by_cid(ctx_table, cid);
> +	if (!entry) {
> +		LOWPAN_DBG("There is no context of id:%d for interface %s",
> +				cid, netdev->name);
> +		return NULL;
> +	}
> +
> +	LOWPAN_DBG("Found context prefix for context of id:%d - %pI6c",
> +			entry->cid, &entry->prefix);
> +
> +	return entry;
> +}
> +EXPORT_SYMBOL_GPL(lowpan_context_decompress);
> +
> +struct lowpan_context_entry *lowpan_context_compress(struct net_device *netdev,
> +						     struct in6_addr *addr)
> +{
> +	struct lowpan_context_table *ctx_table = NULL;
> +	struct lowpan_context_entry *entry = NULL;
> +
> +	if (!netdev || !addr) {
> +		LOWPAN_DBG("Network device and address has to be specified");
> +	}
> +
> +	/* Get proper interface table. */
> +	ctx_table = get_table(netdev);
> +	if (!ctx_table) {
> +		LOWPAN_DBG("Cannot find context table for interface %s",
> +				netdev->name);
> +		return NULL;
> +	}
> +
> +	/* Search for given address. */
> +	entry = get_ctx_by_addr(ctx_table, addr);
> +	if (!entry) {
> +		LOWPAN_DBG("There is no context for address %pI6c for interface %s",
> +				addr, netdev->name);
> +		return NULL;
> +	}
> +
> +	LOWPAN_DBG("Found context of id:%d", entry->cid);
> +
> +	/* Return null in case no compression capability in found context */
> +	return entry;
> +}
> +EXPORT_SYMBOL_GPL(lowpan_context_compress);
> diff --git a/net/6lowpan/context.h b/net/6lowpan/context.h
> new file mode 100644
> index 0000000..b9e26e5
> --- /dev/null
> +++ b/net/6lowpan/context.h
> @@ -0,0 +1,13 @@
> +#ifndef __6LOWPAN_CONTEXT_H
> +#define __6LOWPAN_CONTEXT_H
> +
> +#include <linux/debugfs.h>
> +
> +ssize_t lowpan_context_dbgfs_write(struct file *fp,
> +				   const char __user *user_buffer,
> +				   size_t count,
> +				   loff_t *position);
> +
> +int lowpan_context_dbgfs_open(struct inode *inode, struct file *file);
> +
> +#endif /* __6LOWPAN_CONTEXT_H */
> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
> index 9055d7b..b777fd3 100644
> --- a/net/6lowpan/iphc.c
> +++ b/net/6lowpan/iphc.c
> @@ -55,6 +55,62 @@
> #include <net/af_ieee802154.h>
> 
> #include "nhc.h"
> +#include "context.h"
> +
> +#define CTX_SRC(context)  ((context & 0xf0) >> 4)
> +#define CTX_DEST(context) (context & 0x0f)
> +#define CID_NOT_USED      0xff
> +
> +static struct dentry *lowpan_debugfs;
> +static struct dentry *lowpan_context_debugfs;
> +
> +/* Add context prefix into given IPv6 address.
> + * Context prefix is always used, therefore any original bits can be overwritten.
> + */
> +static inline void add_cid_prefix(struct in6_addr *ipaddr,
> +				  struct lowpan_context_entry *ctx_entry)
> +{
> +	int o = ctx_entry->prefix_len >> 3,
> +		b = ctx_entry->prefix_len & 0x7;

No multiline variable declarations.

	int a;
	int b;

> +
> +	memcpy(ipaddr->s6_addr, ctx_entry->prefix.s6_addr, o);
> +	if (b != 0) {
> +		ipaddr->s6_addr[o] = ctx_entry->prefix.s6_addr[o] & (0xff00 >> b);
> +	}
> +}
> +
> +/* Check if address can be fully compressed with stateful compression. */
> +static inline bool lowpan_address_is_covered_by_cid(struct in6_addr *ipaddr,
> +						    struct in6_addr *ctx_addr,
> +						    unsigned int ctx_length,
> +						    u8 *lladdr)
> +{
> +	unsigned int start_byte, offset;
> +
> +	/* Context covers IPv6 address by its size. */
> +	if (ctx_length == 128) {
> +		return true;
> +	}
> +
> +	/* Check if IID can be retrieved in case of longer prefix than 64 bits. */
> +	if (ctx_length > 64) {
> +	/* Check only IID fields that are not covered by context prefix. */

Indentation of the comment is wrong.

> +		start_byte = ctx_length << 3;
> +		offset = ctx_length % 8;
> +
> +		/* Check all bytes from the second one. */
> +		if (start_byte == 15 ||
> +		    0 == memcmp(&ipaddr->s6_addr[start_byte+1],
> +				&lladdr[start_byte-7],
> +				15-start_byte)) {

Never value == variable. It is always the other way around. And here !memcmp is preferred.

> +			/* Then check first byte. */
> +			return (u8)(ipaddr->s6_addr[start_byte] << offset) ==
> +				(u8)(lladdr[start_byte] << offset);
> +		}
> +	}
> +
> +	return false;
> +}
> 
> /* Uncompress address function for source and
>  * destination address(non-multicast).
> @@ -137,39 +193,84 @@ static int uncompress_addr(struct sk_buff *skb,
> 	return 0;
> }
> 
> -/* Uncompress address function for source context
> - * based address(non-multicast).
> - */
> -static int uncompress_context_based_src_addr(struct sk_buff *skb,
> -					     struct in6_addr *ipaddr,
> -					     const u8 sam)
> +/* Uncompress address after stateful compression. */
> +static int uncompress_context_based_addr(struct sk_buff *skb, struct net_device *dev,
> +					 struct in6_addr *ipaddr, const u8 address_mode,
> +					 const u8 *lladdr, const u8 addr_type,
> +					 const u8 addr_len, const u8 context_id)
> {
> -	switch (sam) {
> -	case LOWPAN_IPHC_ADDR_00:
> -		/* unspec address ::
> +	bool fail = false;
> +	struct lowpan_context_entry *ctx_entry;
> +
> +	if (address_mode == LOWPAN_IPHC_ADDR_00) {
> +		/* Unspecified address ::
> 		 * Do nothing, address is already ::
> 		 */
> -		break;
> -	case LOWPAN_IPHC_ADDR_01:
> -		/* TODO */
> -	case LOWPAN_IPHC_ADDR_02:
> -		/* TODO */
> -	case LOWPAN_IPHC_ADDR_03:
> -		/* TODO */
> -		netdev_warn(skb->dev, "SAM value 0x%x not supported\n", sam);
> -		return -EINVAL;
> -	default:
> -		pr_debug("Invalid sam value: 0x%x\n", sam);
> -		return -EINVAL;
> +	} else {

You can save yourself a lot of indentation if you either leave the function directly or just goto to the end of it.

> +		/* Look for prefix assigned to given context id. */
> +		ctx_entry = lowpan_context_decompress(dev, context_id);
> +		if (!ctx_entry) {
> +			pr_debug("Unknown Context ID. Unable to decompress the address\n");
> +			return -EINVAL;
> +		}
> +
> +		switch (address_mode) {
> +			case LOWPAN_IPHC_ADDR_01:
> +				/* CID Prefix + 64-bits in-line. */
> +				fail = lowpan_fetch_skb(skb, &ipaddr->s6_addr[8], 8);
> +			break;
> +			case LOWPAN_IPHC_ADDR_02:
> +				/* CID Prefix + 16-bits in-line. */
> +				ipaddr->s6_addr[11] = 0xFF;
> +				ipaddr->s6_addr[12] = 0xFE;
> +				fail = lowpan_fetch_skb(skb, &ipaddr->s6_addr[14], 2);
> +			break;
> +			case LOWPAN_IPHC_ADDR_03:
> +				switch (addr_type) {
> +				case IEEE802154_ADDR_LONG:
> +					/* CID Prefix + 64-bits from LL Address. */
> +					memcpy(&ipaddr->s6_addr[8],
> +					       lladdr,
> +					       addr_len);
> +
> +					/* Second bit-flip (Universe/Local)
> +					* is done according RFC2464
> +					*/
> +					ipaddr->s6_addr[8] ^= 0x02;
> +					break;
> +				case IEEE802154_ADDR_SHORT:
> +					/* CID Prefix + 16-bits from LL Address. */
> +					ipaddr->s6_addr[11] = 0xFF;
> +					ipaddr->s6_addr[12] = 0xFE;
> +					ipaddr->s6_addr16[7] = htons(*((u16 *)lladdr));
> +					break;
> +				default:
> +					pr_debug("Invalid addr_type set\n");
> +					return -EINVAL;
> +				}
> +			break;
> +			default:
> +				pr_debug("Invalid address mode value: 0x%x\n",
> +						address_mode);
> +				return -EINVAL;
> +		}
> +
> +		if (fail) {
> +			pr_debug("Failed to fetch skb data\n");
> +			return -EIO;
> +		}
> +
> +		/* Add context prefix to IPv6 address. Context bits are always used. */
> +		add_cid_prefix(ipaddr, ctx_entry);
> 	}
> 
> -	raw_dump_inline(NULL,
> -			"Reconstructed context based ipv6 src addr is",
> -			ipaddr->s6_addr, 16);
> +	raw_dump_inline(NULL, "Reconstructed ipv6 addr is",
> +		ipaddr->s6_addr, 16);
> 
> -	return 0;
> +    return 0;

We are still using tabs ;)

> }
> 
> +
> /* Uncompress function for multicast destination address,
>  * when M bit is set.
>  */
> @@ -320,7 +421,8 @@ lowpan_header_decompress(struct sk_buff *skb, struct net_device *dev,
> 	if (iphc1 & LOWPAN_IPHC_SAC) {
> 		/* Source address context based uncompression */
> 		pr_debug("SAC bit is set. Handle context based source address.\n");
> -		err = uncompress_context_based_src_addr(skb, &hdr.saddr, tmp);
> +		err = uncompress_context_based_addr(skb, dev, &hdr.saddr, tmp,
> +			saddr, saddr_type, saddr_len, CTX_SRC(num_context));
> 	} else {
> 		/* Source address uncompression */
> 		pr_debug("source address stateless compression\n");
> @@ -348,10 +450,18 @@ lowpan_header_decompress(struct sk_buff *skb, struct net_device *dev,
> 				return -EINVAL;
> 		}
> 	} else {
> -		err = uncompress_addr(skb, &hdr.daddr, tmp, daddr,
> -				      daddr_type, daddr_len);
> -		pr_debug("dest: stateless compression mode %d dest %pI6c\n",
> -			 tmp, &hdr.daddr);
> +		if (iphc1 & LOWPAN_IPHC_DAC) {
> +			/* Destination address context based uncompression */
> +			pr_debug("DAC bit is set. Handle context based source address.\n");
> +			err = uncompress_context_based_addr(skb, dev, &hdr.daddr, tmp,
> +				daddr, daddr_type, daddr_len, CTX_DEST(num_context));
> +		} else {
> +			/* Destination address uncompression */
> +			pr_debug("destination address stateless compression\n");
> +			err = uncompress_addr(skb, &hdr.daddr, tmp, daddr,
> +				daddr_type, daddr_len);
> +		}
> +
> 		if (err)
> 			return -EINVAL;
> 	}
> @@ -389,11 +499,12 @@ EXPORT_SYMBOL_GPL(lowpan_header_decompress);
> 
> static u8 lowpan_compress_addr_64(u8 **hc_ptr, u8 shift,
> 				  const struct in6_addr *ipaddr,
> -				  const unsigned char *lladdr)
> +				  const unsigned char *lladdr,
> +				  bool cover_by_context)
> {
> 	u8 val = 0;
> 
> -	if (is_addr_mac_addr_based(ipaddr, lladdr)) {
> +	if (is_addr_mac_addr_based(ipaddr, lladdr) || cover_by_context) {
> 		val = 3; /* 0-bits */
> 		pr_debug("address compression 0 bits\n");
> 	} else if (lowpan_is_iid_16_bit_compressable(ipaddr)) {
> @@ -418,9 +529,13 @@ int lowpan_header_compress(struct sk_buff *skb, struct net_device *dev,
> 			   const void *_saddr, unsigned int len)
> {
> 	u8 tmp, iphc0, iphc1, *hc_ptr;
> +	u8 cid = 0;
> 	struct ipv6hdr *hdr;
> 	u8 head[100] = {};
> 	int ret, addr_type;
> +	struct lowpan_context_entry *entry;
> +	bool scid_used = false, dcid_used = false,
> +	 scid_cover = false, dcid_cover = false;
> 
> 	if (type != ETH_P_IPV6)
> 		return -EINVAL;
> @@ -444,7 +559,26 @@ int lowpan_header_compress(struct sk_buff *skb, struct net_device *dev,
> 	iphc0 = LOWPAN_DISPATCH_IPHC;
> 	iphc1 = 0;
> 
> -	/* TODO: context lookup */
> +	/* Look for source context. */
> +	entry = lowpan_context_compress(dev, &hdr->saddr);
> +	if (entry) {
> +		cid = entry->cid << 4;
> +		scid_used = true;
> +	}
> +
> +	/* Look for destination context. */
> +	entry = lowpan_context_compress(dev, &hdr->daddr);
> +	if (entry) {
> +		cid |= entry->cid;
> +		dcid_used = true;
> +	}
> +
> +	if (scid_used || dcid_used) {
> +		/* Add CID flag. */
> +		iphc1 |= LOWPAN_IPHC_CID;
> +
> +		lowpan_push_hc_data(&hc_ptr, &cid, 1);
> +	}
> 
> 	raw_dump_inline(__func__, "saddr",
> 			(unsigned char *)_saddr, IEEE802154_ADDR_LEN);
> @@ -531,12 +665,20 @@ int lowpan_header_compress(struct sk_buff *skb, struct net_device *dev,
> 		pr_debug("source address is unspecified, setting SAC\n");
> 		iphc1 |= LOWPAN_IPHC_SAC;
> 	} else {
> -		if (addr_type & IPV6_ADDR_LINKLOCAL) {
> +		if ((addr_type & IPV6_ADDR_LINKLOCAL) || scid_used) {
> +			/* In case of stateful source address compression, set SAC bit. */
> +			if (scid_used) {
> +				iphc1 |= LOWPAN_IPHC_SAC;
> +				scid_cover = lowpan_address_is_covered_by_cid(&hdr->saddr,
> +					&entry->prefix, entry->prefix_len, (u8 *)_saddr);
> +			}
> +
> 			iphc1 |= lowpan_compress_addr_64(&hc_ptr,
> 							 LOWPAN_IPHC_SAM_BIT,
> -							 &hdr->saddr, _saddr);
> -			pr_debug("source address unicast link-local %pI6c iphc1 0x%02x\n",
> -				 &hdr->saddr, iphc1);
> +							 &hdr->saddr, _saddr,
> +							 scid_cover);
> +			pr_debug("source ipv6 address %pI6c iphc1 0x%02x\n",
> +				&hdr->saddr, iphc1);
> 		} else {
> 			pr_debug("send the full source address\n");
> 			lowpan_push_hc_data(&hc_ptr, hdr->saddr.s6_addr, 16);
> @@ -576,12 +718,20 @@ int lowpan_header_compress(struct sk_buff *skb, struct net_device *dev,
> 			lowpan_push_hc_data(&hc_ptr, hdr->daddr.s6_addr, 16);
> 		}
> 	} else {
> -		if (addr_type & IPV6_ADDR_LINKLOCAL) {
> -			/* TODO: context lookup */
> +		if (addr_type & IPV6_ADDR_LINKLOCAL || dcid_used) {
> +			/* In case of stateful destination address compression, set DAC bit. */
> +			if (dcid_used) {
> +				iphc1 |= LOWPAN_IPHC_DAC;
> +				dcid_cover = lowpan_address_is_covered_by_cid(&hdr->daddr,
> +					&entry->prefix, entry->prefix_len, (u8 *)_daddr);
> +			}
> +
> 			iphc1 |= lowpan_compress_addr_64(&hc_ptr,
> -				LOWPAN_IPHC_DAM_BIT, &hdr->daddr, _daddr);
> -			pr_debug("dest address unicast link-local %pI6c "
> -				 "iphc1 0x%02x\n", &hdr->daddr, iphc1);
> +							 LOWPAN_IPHC_DAM_BIT,
> +							 &hdr->daddr, _daddr,
> +							 dcid_cover);
> +			pr_debug("destination ipv6 address %pI6c iphc1 0x%02x\n",
> +				&hdr->daddr, iphc1);
> 		} else {
> 			pr_debug("dest address unicast %pI6c\n", &hdr->daddr);
> 			lowpan_push_hc_data(&hc_ptr, hdr->daddr.s6_addr, 16);
> @@ -611,8 +761,23 @@ int lowpan_header_compress(struct sk_buff *skb, struct net_device *dev,
> }
> EXPORT_SYMBOL_GPL(lowpan_header_compress);
> 
> +const struct file_operations lowpan_context_fops = {
> +	.open	= lowpan_context_dbgfs_open,
> +	.read	= seq_read,
> +	.write   = lowpan_context_dbgfs_write,
> +	.llseek  = seq_lseek,
> +	.release = single_release,

I really need to start seeing proper alignments since otherwise code becomes hard to review.

> +};
> +
> static int __init lowpan_module_init(void)
> {
> +
> +	lowpan_debugfs = debugfs_create_dir("6lowpan", NULL);
> +
> +	lowpan_context_debugfs = debugfs_create_file("context", 0644,
> +						     lowpan_debugfs, NULL,
> +						     &lowpan_context_fops);
> +

I wonder why not just export the lowpan_debugfs entry and move the actual debugfs entries into context.c.

> 	request_module_nowait("ipv6");
> 
> 	request_module_nowait("nhc_dest");
> @@ -625,6 +790,16 @@ static int __init lowpan_module_init(void)
> 
> 	return 0;
> }
> +
> +static void __exit lowpan_module_exit(void)
> +{
> +	debugfs_remove(lowpan_context_debugfs);
> +	debugfs_remove(lowpan_debugfs);
> +
> +	lowpan_context_free_all();
> +}
> +
> module_init(lowpan_module_init);
> +module_exit(lowpan_module_exit);

I would also start splitting the patches. One first one could be to introduce the lowpan_debugfs dentry and with that the module exit function. The smaller the patches and logically split, the easier they are to review and with that also to apply.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux