[00/14] can: cleanup of af_can/raw + simplifying of ndev->ml_priv handling

[00/14] can: cleanup of af_can/raw + simplifying of ndev->ml_priv handling

[Marc Kleine-Budde]

Hello,

while reviwing and cleaning up the j1939 stack I digged a bit deeper into
af_can and raw implementation.

The first patch adds a missing error check and will probably go into -stable.
Patches 2-8 change some struct and variable names, making the code more
readble, IMHO.
Patch 9 removed the need for struct raw_sock::ifindex from the raw sock, by
using struct sock::sk_bound_dev_if from the generic socket structure.
Patch 10: Checks if can_family is AF_CAN in raw's bind function.
Patch 11: Cleans up the newly integrated CAN net namespace support.

Patches 13-14: Where to put the per device protocol specific memory? af_can
allocated it's memory during a netdev_notifier call, life cycle proves to be
rather complicated (see remove_on_zero_entries, etc...), adding the j1939
memory makes it even more compilcated. So I decided to allocate the memory
during the allocation if net_device. And this seems to work. More details in
the individual patches.

This series applies to net-next/master, but should work on any recent kernel.
Please review, test and comment.

regards,
Marc

[PATCH 01/14] can: af_can: can_pernet_init(): add missing error handling for kzalloc returning NULL

[Marc Kleine-Budde]

This patch adds the missing check and error handling for out-of-memory
situations, when kzalloc cannot allocate memory.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/af_can.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index 88edac0f3e36..0896e2facd50 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -875,9 +875,14 @@ static int can_pernet_init(struct net *net)
 	spin_lock_init(&net->can.can_rcvlists_lock);
 	net->can.can_rx_alldev_list =
 		kzalloc(sizeof(struct dev_rcv_lists), GFP_KERNEL);
-
-	net->can.can_stats = kzalloc(sizeof(struct s_stats), GFP_KERNEL);
-	net->can.can_pstats = kzalloc(sizeof(struct s_pstats), GFP_KERNEL);
+	if (!net->can.can_rx_alldev_list)
+		goto out;
+	net->can.can_stats = kzalloc(sizeof(struct can_stats), GFP_KERNEL);
+	if (!net->can.can_stats)
+		goto out_free_alldev_list;
+	net->can.can_pstats = kzalloc(sizeof(struct can_pstats), GFP_KERNEL);
+	if (!net->can.can_pstats)
+		goto out_free_can_stats;
 
 	if (IS_ENABLED(CONFIG_PROC_FS)) {
 		/* the statistics are updated every second (timer triggered) */
@@ -892,6 +897,13 @@ static int can_pernet_init(struct net *net)
 	}
 
 	return 0;
+
+ out_free_can_stats:
+	kfree(net->can.can_stats);
+ out_free_alldev_list:
+	kfree(net->can.can_rx_alldev_list);
+ out:
+	return -ENOMEM;
 }
 
 static void can_pernet_exit(struct net *net)
-- 
2.13.2

Re: [PATCH 01/14] can: af_can: can_pernet_init(): add missing error handling for kzalloc returning NULL

[Oliver Hartkopp]

On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
> This patch adds the missing check and error handling for out-of-memory
> situations, when kzalloc cannot allocate memory.
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

> ---
>   net/can/af_can.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index 88edac0f3e36..0896e2facd50 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -875,9 +875,14 @@ static int can_pernet_init(struct net *net)
>   	spin_lock_init(&net->can.can_rcvlists_lock);
>   	net->can.can_rx_alldev_list =
>   		kzalloc(sizeof(struct dev_rcv_lists), GFP_KERNEL);
> -
> -	net->can.can_stats = kzalloc(sizeof(struct s_stats), GFP_KERNEL);
> -	net->can.can_pstats = kzalloc(sizeof(struct s_pstats), GFP_KERNEL);
> +	if (!net->can.can_rx_alldev_list)
> +		goto out;
> +	net->can.can_stats = kzalloc(sizeof(struct can_stats), GFP_KERNEL);
> +	if (!net->can.can_stats)
> +		goto out_free_alldev_list;
> +	net->can.can_pstats = kzalloc(sizeof(struct can_pstats), GFP_KERNEL);
> +	if (!net->can.can_pstats)
> +		goto out_free_can_stats;
>   
>   	if (IS_ENABLED(CONFIG_PROC_FS)) {
>   		/* the statistics are updated every second (timer triggered) */
> @@ -892,6 +897,13 @@ static int can_pernet_init(struct net *net)
>   	}
>   
>   	return 0;
> +
> + out_free_can_stats:
> +	kfree(net->can.can_stats);
> + out_free_alldev_list:
> +	kfree(net->can.can_rx_alldev_list);
> + out:
> +	return -ENOMEM;
>   }
>   
>   static void can_pernet_exit(struct net *net)
> 

Re: [PATCH 01/14] can: af_can: can_pernet_init(): add missing error handling for kzalloc returning NULL

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 649 bytes --]

On 08/24/2017 02:42 PM, Oliver Hartkopp wrote:
> 
> 
> On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
>> This patch adds the missing check and error handling for out-of-memory
>> situations, when kzalloc cannot allocate memory.
>>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

Tnx.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 02/14] can: give structs holding the CAN statistics a sensible name

[Marc Kleine-Budde]

This patch renames both "struct s_stats" and "struct s_pstats" by
replacing the leading "s" by "can" to better reflect their meaning and
improve code readability.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 include/net/netns/can.h |  8 ++++----
 net/can/af_can.c        | 18 +++++++++---------
 net/can/af_can.h        |  4 ++--
 net/can/proc.c          | 16 ++++++++--------
 4 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/include/net/netns/can.h b/include/net/netns/can.h
index b106e6ae2e5b..5ee0f88ebbac 100644
--- a/include/net/netns/can.h
+++ b/include/net/netns/can.h
@@ -8,8 +8,8 @@
 #include <linux/spinlock.h>
 
 struct dev_rcv_lists;
-struct s_stats;
-struct s_pstats;
+struct can_stats;
+struct can_pstats;
 
 struct netns_can {
 #if IS_ENABLED(CONFIG_PROC_FS)
@@ -30,8 +30,8 @@ struct netns_can {
 	struct dev_rcv_lists *can_rx_alldev_list;
 	spinlock_t can_rcvlists_lock;
 	struct timer_list can_stattimer;/* timer for statistics update */
-	struct s_stats *can_stats;	/* packet statistics */
-	struct s_pstats *can_pstats;	/* receive list statistics */
+	struct can_stats *can_stats; /* packet statistics */
+	struct can_pstats *can_pstats; /* receive list statistics */
 
 	/* CAN GW per-net gateway jobs */
 	struct hlist_head cgw_list;
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 0896e2facd50..bbd8a9bcf28d 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -217,7 +217,7 @@ int can_send(struct sk_buff *skb, int loop)
 {
 	struct sk_buff *newskb = NULL;
 	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
-	struct s_stats *can_stats = dev_net(skb->dev)->can.can_stats;
+	struct can_stats *can_stats = dev_net(skb->dev)->can.can_stats;
 	int err = -EINVAL;
 
 	if (skb->len == CAN_MTU) {
@@ -321,8 +321,8 @@ EXPORT_SYMBOL(can_send);
  * af_can rx path
  */
 
-static struct dev_rcv_lists *find_dev_rcv_lists(struct net *net,
-						struct net_device *dev)
+static struct dev_rcv_lists *can_dev_rcv_lists_find(struct net *net,
+						    struct net_device *dev)
 {
 	if (!dev)
 		return net->can.can_rx_alldev_list;
@@ -465,7 +465,7 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 	struct receiver *r;
 	struct hlist_head *rl;
 	struct dev_rcv_lists *d;
-	struct s_pstats *can_pstats = net->can.can_pstats;
+	struct can_pstats *can_pstats = net->can.can_pstats;
 	int err = 0;
 
 	/* insert new receiver  (dev,canid,mask) -> (func,data) */
@@ -482,7 +482,7 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 
 	spin_lock(&net->can.can_rcvlists_lock);
 
-	d = find_dev_rcv_lists(net, dev);
+	d = can_dev_rcv_lists_find(net, dev);
 	if (d) {
 		rl = find_rcv_list(&can_id, &mask, d);
 
@@ -541,7 +541,7 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 {
 	struct receiver *r = NULL;
 	struct hlist_head *rl;
-	struct s_pstats *can_pstats = net->can.can_pstats;
+	struct can_pstats *can_pstats = net->can.can_pstats;
 	struct dev_rcv_lists *d;
 
 	if (dev && dev->type != ARPHRD_CAN)
@@ -552,7 +552,7 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 
 	spin_lock(&net->can.can_rcvlists_lock);
 
-	d = find_dev_rcv_lists(net, dev);
+	d = can_dev_rcv_lists_find(net, dev);
 	if (!d) {
 		pr_err("BUG: receive list not found for "
 		       "dev %s, id %03X, mask %03X\n",
@@ -684,7 +684,7 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
 {
 	struct dev_rcv_lists *d;
 	struct net *net = dev_net(dev);
-	struct s_stats *can_stats = net->can.can_stats;
+	struct can_stats *can_stats = net->can.can_stats;
 	int matches;
 
 	/* update statistics */
@@ -701,7 +701,7 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
 	matches = can_rcv_filter(net->can.can_rx_alldev_list, skb);
 
 	/* find receive list for this device */
-	d = find_dev_rcv_lists(net, dev);
+	d = can_dev_rcv_lists_find(net, dev);
 	if (d)
 		matches += can_rcv_filter(d, skb);
 
diff --git a/net/can/af_can.h b/net/can/af_can.h
index d0ef45bb2a72..4e2cb14c1af7 100644
--- a/net/can/af_can.h
+++ b/net/can/af_can.h
@@ -78,7 +78,7 @@ struct dev_rcv_lists {
 /* statistic structures */
 
 /* can be reset e.g. by can_init_stats() */
-struct s_stats {
+struct can_stats {
 	unsigned long jiffies_init;
 
 	unsigned long rx_frames;
@@ -103,7 +103,7 @@ struct s_stats {
 };
 
 /* persistent statistics */
-struct s_pstats {
+struct can_pstats {
 	unsigned long stats_reset;
 	unsigned long user_reset;
 	unsigned long rcv_entries;
diff --git a/net/can/proc.c b/net/can/proc.c
index 83045f00c63c..559fca8035aa 100644
--- a/net/can/proc.c
+++ b/net/can/proc.c
@@ -77,14 +77,14 @@ static const char rx_list_name[][8] = {
 
 static void can_init_stats(struct net *net)
 {
-	struct s_stats *can_stats = net->can.can_stats;
-	struct s_pstats *can_pstats = net->can.can_pstats;
+	struct can_stats *can_stats = net->can.can_stats;
+	struct can_pstats *can_pstats = net->can.can_pstats;
 	/*
 	 * This memset function is called from a timer context (when
 	 * can_stattimer is active which is the default) OR in a process
 	 * context (reading the proc_fs when can_stattimer is disabled).
 	 */
-	memset(can_stats, 0, sizeof(struct s_stats));
+	memset(can_stats, 0, sizeof(struct can_stats));
 	can_stats->jiffies_init = jiffies;
 
 	can_pstats->stats_reset++;
@@ -118,7 +118,7 @@ static unsigned long calc_rate(unsigned long oldjif, unsigned long newjif,
 void can_stat_update(unsigned long data)
 {
 	struct net *net = (struct net *)data;
-	struct s_stats *can_stats = net->can.can_stats;
+	struct can_stats *can_stats = net->can.can_stats;
 	unsigned long j = jiffies; /* snapshot */
 
 	/* restart counting in timer context on user request */
@@ -211,8 +211,8 @@ static void can_print_recv_banner(struct seq_file *m)
 static int can_stats_proc_show(struct seq_file *m, void *v)
 {
 	struct net *net = m->private;
-	struct s_stats *can_stats = net->can.can_stats;
-	struct s_pstats *can_pstats = net->can.can_pstats;
+	struct can_stats *can_stats = net->can.can_stats;
+	struct can_pstats *can_pstats = net->can.can_pstats;
 
 	seq_putc(m, '\n');
 	seq_printf(m, " %8ld transmitted frames (TXF)\n", can_stats->tx_frames);
@@ -286,8 +286,8 @@ static const struct file_operations can_stats_proc_fops = {
 static int can_reset_stats_proc_show(struct seq_file *m, void *v)
 {
 	struct net *net = m->private;
-	struct s_pstats *can_pstats = net->can.can_pstats;
-	struct s_stats *can_stats = net->can.can_stats;
+	struct can_pstats *can_pstats = net->can.can_pstats;
+	struct can_stats *can_stats = net->can.can_stats;
 
 	user_reset = 1;
 
-- 
2.13.2

Re: [PATCH 02/14] can: give structs holding the CAN statistics a sensible name

[Oliver Hartkopp]

Naming structs and its instances identically is bad.

On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:

>   struct netns_can {
>   #if IS_ENABLED(CONFIG_PROC_FS)
> @@ -30,8 +30,8 @@ struct netns_can {
>   	struct dev_rcv_lists *can_rx_alldev_list;
>   	spinlock_t can_rcvlists_lock;
>   	struct timer_list can_stattimer;/* timer for statistics update */
> -	struct s_stats *can_stats;	/* packet statistics */
> -	struct s_pstats *can_pstats;	/* receive list statistics */
> +	struct can_stats *can_stats; /* packet statistics */
> +	struct can_pstats *can_pstats; /* receive list statistics */

If you really want to rename this stuff, what about this:

struct can_pkt_stats *can_stats; /* packet statistics */
struct can_rxl_stats *can_rstats; /* receive list statistics */

Regards,
Oliver

>   
>   	/* CAN GW per-net gateway jobs */
>   	struct hlist_head cgw_list;
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index 0896e2facd50..bbd8a9bcf28d 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -217,7 +217,7 @@ int can_send(struct sk_buff *skb, int loop)
>   {
>   	struct sk_buff *newskb = NULL;
>   	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> -	struct s_stats *can_stats = dev_net(skb->dev)->can.can_stats;
> +	struct can_stats *can_stats = dev_net(skb->dev)->can.can_stats;
>   	int err = -EINVAL;
>   
>   	if (skb->len == CAN_MTU) {
> @@ -321,8 +321,8 @@ EXPORT_SYMBOL(can_send);
>    * af_can rx path
>    */
>   
> -static struct dev_rcv_lists *find_dev_rcv_lists(struct net *net,
> -						struct net_device *dev)
> +static struct dev_rcv_lists *can_dev_rcv_lists_find(struct net *net,
> +						    struct net_device *dev)
>   {
>   	if (!dev)
>   		return net->can.can_rx_alldev_list;
> @@ -465,7 +465,7 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
>   	struct receiver *r;
>   	struct hlist_head *rl;
>   	struct dev_rcv_lists *d;
> -	struct s_pstats *can_pstats = net->can.can_pstats;
> +	struct can_pstats *can_pstats = net->can.can_pstats;
>   	int err = 0;
>   
>   	/* insert new receiver  (dev,canid,mask) -> (func,data) */
> @@ -482,7 +482,7 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
>   
>   	spin_lock(&net->can.can_rcvlists_lock);
>   
> -	d = find_dev_rcv_lists(net, dev);
> +	d = can_dev_rcv_lists_find(net, dev);
>   	if (d) {
>   		rl = find_rcv_list(&can_id, &mask, d);
>   
> @@ -541,7 +541,7 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
>   {
>   	struct receiver *r = NULL;
>   	struct hlist_head *rl;
> -	struct s_pstats *can_pstats = net->can.can_pstats;
> +	struct can_pstats *can_pstats = net->can.can_pstats;
>   	struct dev_rcv_lists *d;
>   
>   	if (dev && dev->type != ARPHRD_CAN)
> @@ -552,7 +552,7 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
>   
>   	spin_lock(&net->can.can_rcvlists_lock);
>   
> -	d = find_dev_rcv_lists(net, dev);
> +	d = can_dev_rcv_lists_find(net, dev);
>   	if (!d) {
>   		pr_err("BUG: receive list not found for "
>   		       "dev %s, id %03X, mask %03X\n",
> @@ -684,7 +684,7 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
>   {
>   	struct dev_rcv_lists *d;
>   	struct net *net = dev_net(dev);
> -	struct s_stats *can_stats = net->can.can_stats;
> +	struct can_stats *can_stats = net->can.can_stats;
>   	int matches;
>   
>   	/* update statistics */
> @@ -701,7 +701,7 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
>   	matches = can_rcv_filter(net->can.can_rx_alldev_list, skb);
>   
>   	/* find receive list for this device */
> -	d = find_dev_rcv_lists(net, dev);
> +	d = can_dev_rcv_lists_find(net, dev);
>   	if (d)
>   		matches += can_rcv_filter(d, skb);
>   
> diff --git a/net/can/af_can.h b/net/can/af_can.h
> index d0ef45bb2a72..4e2cb14c1af7 100644
> --- a/net/can/af_can.h
> +++ b/net/can/af_can.h
> @@ -78,7 +78,7 @@ struct dev_rcv_lists {
>   /* statistic structures */
>   
>   /* can be reset e.g. by can_init_stats() */
> -struct s_stats {
> +struct can_stats {
>   	unsigned long jiffies_init;
>   
>   	unsigned long rx_frames;
> @@ -103,7 +103,7 @@ struct s_stats {
>   };
>   
>   /* persistent statistics */
> -struct s_pstats {
> +struct can_pstats {
>   	unsigned long stats_reset;
>   	unsigned long user_reset;
>   	unsigned long rcv_entries;
> diff --git a/net/can/proc.c b/net/can/proc.c
> index 83045f00c63c..559fca8035aa 100644
> --- a/net/can/proc.c
> +++ b/net/can/proc.c
> @@ -77,14 +77,14 @@ static const char rx_list_name[][8] = {
>   
>   static void can_init_stats(struct net *net)
>   {
> -	struct s_stats *can_stats = net->can.can_stats;
> -	struct s_pstats *can_pstats = net->can.can_pstats;
> +	struct can_stats *can_stats = net->can.can_stats;
> +	struct can_pstats *can_pstats = net->can.can_pstats;
>   	/*
>   	 * This memset function is called from a timer context (when
>   	 * can_stattimer is active which is the default) OR in a process
>   	 * context (reading the proc_fs when can_stattimer is disabled).
>   	 */
> -	memset(can_stats, 0, sizeof(struct s_stats));
> +	memset(can_stats, 0, sizeof(struct can_stats));
>   	can_stats->jiffies_init = jiffies;
>   
>   	can_pstats->stats_reset++;
> @@ -118,7 +118,7 @@ static unsigned long calc_rate(unsigned long oldjif, unsigned long newjif,
>   void can_stat_update(unsigned long data)
>   {
>   	struct net *net = (struct net *)data;
> -	struct s_stats *can_stats = net->can.can_stats;
> +	struct can_stats *can_stats = net->can.can_stats;
>   	unsigned long j = jiffies; /* snapshot */
>   
>   	/* restart counting in timer context on user request */
> @@ -211,8 +211,8 @@ static void can_print_recv_banner(struct seq_file *m)
>   static int can_stats_proc_show(struct seq_file *m, void *v)
>   {
>   	struct net *net = m->private;
> -	struct s_stats *can_stats = net->can.can_stats;
> -	struct s_pstats *can_pstats = net->can.can_pstats;
> +	struct can_stats *can_stats = net->can.can_stats;
> +	struct can_pstats *can_pstats = net->can.can_pstats;
>   
>   	seq_putc(m, '\n');
>   	seq_printf(m, " %8ld transmitted frames (TXF)\n", can_stats->tx_frames);
> @@ -286,8 +286,8 @@ static const struct file_operations can_stats_proc_fops = {
>   static int can_reset_stats_proc_show(struct seq_file *m, void *v)
>   {
>   	struct net *net = m->private;
> -	struct s_pstats *can_pstats = net->can.can_pstats;
> -	struct s_stats *can_stats = net->can.can_stats;
> +	struct can_pstats *can_pstats = net->can.can_pstats;
> +	struct can_stats *can_stats = net->can.can_stats;
>   
>   	user_reset = 1;
>   
> 

Re: [PATCH 02/14] can: give structs holding the CAN statistics a sensible name

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2777 bytes --]

On 08/24/2017 02:58 PM, Oliver Hartkopp wrote:
> Naming structs and its instances identically is bad.
> 
> On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
> 
>>   struct netns_can {
>>   #if IS_ENABLED(CONFIG_PROC_FS)
>> @@ -30,8 +30,8 @@ struct netns_can {
>>   	struct dev_rcv_lists *can_rx_alldev_list;
>>   	spinlock_t can_rcvlists_lock;
>>   	struct timer_list can_stattimer;/* timer for statistics update */
>> -	struct s_stats *can_stats;	/* packet statistics */
>> -	struct s_pstats *can_pstats;	/* receive list statistics */
>> +	struct can_stats *can_stats; /* packet statistics */
>> +	struct can_pstats *can_pstats; /* receive list statistics */
                               ^
Now I found out what that "p" originally means:

> /* persistent statistics */
> struct can_pstats {
> 	unsigned long stats_reset;
> 	unsigned long user_reset;
> 	unsigned long rcv_entries;
> 	unsigned long rcv_entries_max;
> };

\o/

> If you really want to rename this stuff, what about this:
> 
> struct can_pkt_stats *can_stats; /* packet statistics */
> struct can_rxl_stats *can_rstats; /* receive list statistics */
             ^^^
We're using rcv_list(s) instead of rxl in various other places.

If we give the variables speaking names, the comments are obsolete:

struct can_pkg_stats *can_pkg_stats;
struct can_rcv_list_stats *can_rcv_list_stats;

We can remove the "can_" prefix of the variables in the struct
netns_can, as the struct already has the "can" in it.

So the struct becomes:

> struct netns_can {
> #if IS_ENABLED(CONFIG_PROC_FS)
> 	struct proc_dir_entry *proc_dir;
> 	struct proc_dir_entry *pde_version;
> 	struct proc_dir_entry *pde_stats;
> 	struct proc_dir_entry *pde_reset_stats;
> 	struct proc_dir_entry *pde_rcvlist_all;
> 	struct proc_dir_entry *pde_rcvlist_fil;
> 	struct proc_dir_entry *pde_rcvlist_inv;
> 	struct proc_dir_entry *pde_rcvlist_sff;
> 	struct proc_dir_entry *pde_rcvlist_eff;
> 	struct proc_dir_entry *pde_rcvlist_err;
> 	struct proc_dir_entry *bcmproc_dir;
> #endif
> 
> 	/* receive filters subscribed for 'all' CAN devices */
> 	struct can_dev_rcv_lists *rx_alldev_list;
> 	spinlock_t rcvlists_lock;
> 	struct timer_list stattimer; /* timer for statistics update */
> 	struct can_pkg_stats *pkg_stats;
> 	struct can_rcv_lists_stats *rcv_lists_stats;
> 
> #if IS_ENABLED(CONFIG_CAN_GW)
> 	/* CAN GW per-net gateway jobs */
> 	struct hlist_head cgw_list;
> #endif
> };

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 02/14] can: give structs holding the CAN statistics a sensible name

[Oliver Hartkopp]

On 08/25/2017 03:08 PM, Marc Kleine-Budde wrote:
> On 08/24/2017 02:58 PM, Oliver Hartkopp wrote:
>> Naming structs and its instances identically is bad.
>>
>> On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
>>
>>>    struct netns_can {
>>>    #if IS_ENABLED(CONFIG_PROC_FS)
>>> @@ -30,8 +30,8 @@ struct netns_can {
>>>    	struct dev_rcv_lists *can_rx_alldev_list;
>>>    	spinlock_t can_rcvlists_lock;
>>>    	struct timer_list can_stattimer;/* timer for statistics update */
>>> -	struct s_stats *can_stats;	/* packet statistics */
>>> -	struct s_pstats *can_pstats;	/* receive list statistics */
>>> +	struct can_stats *can_stats; /* packet statistics */
>>> +	struct can_pstats *can_pstats; /* receive list statistics */
>                                 ^
> Now I found out what that "p" originally means:
> 
>> /* persistent statistics */
>> struct can_pstats {
>> 	unsigned long stats_reset;
>> 	unsigned long user_reset;
>> 	unsigned long rcv_entries;
>> 	unsigned long rcv_entries_max;
>> };
> 
> \o/
> 
>> If you really want to rename this stuff, what about this:
>>
>> struct can_pkt_stats *can_stats; /* packet statistics */
>> struct can_rxl_stats *can_rstats; /* receive list statistics */
>               ^^^
> We're using rcv_list(s) instead of rxl in various other places.
> 
> If we give the variables speaking names, the comments are obsolete:
> 
> struct can_pkg_stats *can_pkg_stats;
> struct can_rcv_list_stats *can_rcv_list_stats;
> 
> We can remove the "can_" prefix of the variables in the struct
> netns_can, as the struct already has the "can" in it.

Yes. Good idea.

> 
> So the struct becomes:
> 
>> struct netns_can {
>> #if IS_ENABLED(CONFIG_PROC_FS)
>> 	struct proc_dir_entry *proc_dir;
>> 	struct proc_dir_entry *pde_version;
>> 	struct proc_dir_entry *pde_stats;
>> 	struct proc_dir_entry *pde_reset_stats;
>> 	struct proc_dir_entry *pde_rcvlist_all;
>> 	struct proc_dir_entry *pde_rcvlist_fil;
>> 	struct proc_dir_entry *pde_rcvlist_inv;
>> 	struct proc_dir_entry *pde_rcvlist_sff;
>> 	struct proc_dir_entry *pde_rcvlist_eff;
>> 	struct proc_dir_entry *pde_rcvlist_err;
>> 	struct proc_dir_entry *bcmproc_dir;
>> #endif
>>
>> 	/* receive filters subscribed for 'all' CAN devices */
>> 	struct can_dev_rcv_lists *rx_alldev_list;
>> 	spinlock_t rcvlists_lock;
>> 	struct timer_list stattimer; /* timer for statistics update */
>> 	struct can_pkg_stats *pkg_stats;
>> 	struct can_rcv_lists_stats *rcv_lists_stats;

Agreed :-)

So it would look like this then:

diff --git a/net/can/proc.c b/net/can/proc.c
index 83045f00c63c..559fca8035aa 100644
--- a/net/can/proc.c
+++ b/net/can/proc.c
@@ -77,14 +77,14 @@ static const char rx_list_name[][8] = {

  static void can_init_stats(struct net *net)
  {
-	struct s_stats *can_stats = net->can.can_stats;
-	struct s_pstats *can_pstats = net->can.can_pstats;
+	struct can_pkg_stats *pkg_stats = net->can.pkg_stats;
+	struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats;

Right?

Best regards,
Oliver

>>
>> #if IS_ENABLED(CONFIG_CAN_GW)
>> 	/* CAN GW per-net gateway jobs */
>> 	struct hlist_head cgw_list;
>> #endif
>> };
> 
> regards,
> Marc
> 

Re: [PATCH 02/14] can: give structs holding the CAN statistics a sensible name

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 3534 bytes --]

On 08/25/2017 07:32 PM, Oliver Hartkopp wrote:
> On 08/25/2017 03:08 PM, Marc Kleine-Budde wrote:
>> On 08/24/2017 02:58 PM, Oliver Hartkopp wrote:
>>> Naming structs and its instances identically is bad.
>>>
>>> On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
>>>
>>>>    struct netns_can {
>>>>    #if IS_ENABLED(CONFIG_PROC_FS)
>>>> @@ -30,8 +30,8 @@ struct netns_can {
>>>>    	struct dev_rcv_lists *can_rx_alldev_list;
>>>>    	spinlock_t can_rcvlists_lock;
>>>>    	struct timer_list can_stattimer;/* timer for statistics update */
>>>> -	struct s_stats *can_stats;	/* packet statistics */
>>>> -	struct s_pstats *can_pstats;	/* receive list statistics */
>>>> +	struct can_stats *can_stats; /* packet statistics */
>>>> +	struct can_pstats *can_pstats; /* receive list statistics */
>>                                 ^
>> Now I found out what that "p" originally means:
>>
>>> /* persistent statistics */
>>> struct can_pstats {
>>> 	unsigned long stats_reset;
>>> 	unsigned long user_reset;
>>> 	unsigned long rcv_entries;
>>> 	unsigned long rcv_entries_max;
>>> };
>>
>> \o/

I'll remove that comment then.

>>> If you really want to rename this stuff, what about this:
>>>
>>> struct can_pkt_stats *can_stats; /* packet statistics */
>>> struct can_rxl_stats *can_rstats; /* receive list statistics */
>>               ^^^
>> We're using rcv_list(s) instead of rxl in various other places.
>>
>> If we give the variables speaking names, the comments are obsolete:
>>
>> struct can_pkg_stats *can_pkg_stats;
>> struct can_rcv_list_stats *can_rcv_list_stats;
>>
>> We can remove the "can_" prefix of the variables in the struct
>> netns_can, as the struct already has the "can" in it.
> 
> Yes. Good idea.

>> So the struct becomes:
>>
>>> struct netns_can {
>>> #if IS_ENABLED(CONFIG_PROC_FS)
>>> 	struct proc_dir_entry *proc_dir;
>>> 	struct proc_dir_entry *pde_version;
>>> 	struct proc_dir_entry *pde_stats;
>>> 	struct proc_dir_entry *pde_reset_stats;
>>> 	struct proc_dir_entry *pde_rcvlist_all;
>>> 	struct proc_dir_entry *pde_rcvlist_fil;
>>> 	struct proc_dir_entry *pde_rcvlist_inv;
>>> 	struct proc_dir_entry *pde_rcvlist_sff;
>>> 	struct proc_dir_entry *pde_rcvlist_eff;
>>> 	struct proc_dir_entry *pde_rcvlist_err;
>>> 	struct proc_dir_entry *bcmproc_dir;
>>> #endif
>>>
>>> 	/* receive filters subscribed for 'all' CAN devices */
>>> 	struct can_dev_rcv_lists *rx_alldev_list;
>>> 	spinlock_t rcvlists_lock;
>>> 	struct timer_list stattimer; /* timer for statistics update */
>>> 	struct can_pkg_stats *pkg_stats;
>>> 	struct can_rcv_lists_stats *rcv_lists_stats;
> 
> Agreed :-)
> 
> So it would look like this then:
> 
> diff --git a/net/can/proc.c b/net/can/proc.c
> index 83045f00c63c..559fca8035aa 100644
> --- a/net/can/proc.c
> +++ b/net/can/proc.c
> @@ -77,14 +77,14 @@ static const char rx_list_name[][8] = {
> 
>   static void can_init_stats(struct net *net)
>   {
> -	struct s_stats *can_stats = net->can.can_stats;
> -	struct s_pstats *can_pstats = net->can.can_pstats;
> +	struct can_pkg_stats *pkg_stats = net->can.pkg_stats;
> +	struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats;
> 
> Right?

ACK

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 03/14] can: af_can: give struct holding the CAN per device receive lists a sensible name

[Marc Kleine-Budde]

This patch adds a "can_" prefix to the "struct dev_rcv_lists" to better
reflect the meaning and improbe code readability.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 include/net/netns/can.h |  4 ++--
 net/can/af_can.c        | 22 +++++++++++-----------
 net/can/af_can.h        |  2 +-
 net/can/proc.c          |  8 ++++----
 4 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/net/netns/can.h b/include/net/netns/can.h
index 5ee0f88ebbac..0594d9e7e309 100644
--- a/include/net/netns/can.h
+++ b/include/net/netns/can.h
@@ -7,7 +7,7 @@
 
 #include <linux/spinlock.h>
 
-struct dev_rcv_lists;
+struct can_dev_rcv_lists;
 struct can_stats;
 struct can_pstats;
 
@@ -27,7 +27,7 @@ struct netns_can {
 #endif
 
 	/* receive filters subscribed for 'all' CAN devices */
-	struct dev_rcv_lists *can_rx_alldev_list;
+	struct can_dev_rcv_lists *can_rx_alldev_list;
 	spinlock_t can_rcvlists_lock;
 	struct timer_list can_stattimer;/* timer for statistics update */
 	struct can_stats *can_stats; /* packet statistics */
diff --git a/net/can/af_can.c b/net/can/af_can.c
index bbd8a9bcf28d..2b12f713f4ca 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -321,13 +321,13 @@ EXPORT_SYMBOL(can_send);
  * af_can rx path
  */
 
-static struct dev_rcv_lists *can_dev_rcv_lists_find(struct net *net,
-						    struct net_device *dev)
+static struct can_dev_rcv_lists *can_dev_rcv_lists_find(struct net *net,
+							struct net_device *dev)
 {
 	if (!dev)
 		return net->can.can_rx_alldev_list;
 	else
-		return (struct dev_rcv_lists *)dev->ml_priv;
+		return (struct can_dev_rcv_lists *)dev->ml_priv;
 }
 
 /**
@@ -381,7 +381,7 @@ static unsigned int effhash(canid_t can_id)
  *  Reduced can_id to have a preprocessed filter compare value.
  */
 static struct hlist_head *find_rcv_list(canid_t *can_id, canid_t *mask,
-					struct dev_rcv_lists *d)
+					struct can_dev_rcv_lists *d)
 {
 	canid_t inv = *can_id & CAN_INV_FILTER; /* save flag before masking */
 
@@ -464,7 +464,7 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 {
 	struct receiver *r;
 	struct hlist_head *rl;
-	struct dev_rcv_lists *d;
+	struct can_dev_rcv_lists *d;
 	struct can_pstats *can_pstats = net->can.can_pstats;
 	int err = 0;
 
@@ -542,7 +542,7 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	struct receiver *r = NULL;
 	struct hlist_head *rl;
 	struct can_pstats *can_pstats = net->can.can_pstats;
-	struct dev_rcv_lists *d;
+	struct can_dev_rcv_lists *d;
 
 	if (dev && dev->type != ARPHRD_CAN)
 		return;
@@ -615,7 +615,7 @@ static inline void deliver(struct sk_buff *skb, struct receiver *r)
 	r->matches++;
 }
 
-static int can_rcv_filter(struct dev_rcv_lists *d, struct sk_buff *skb)
+static int can_rcv_filter(struct can_dev_rcv_lists *d, struct sk_buff *skb)
 {
 	struct receiver *r;
 	int matches = 0;
@@ -682,7 +682,7 @@ static int can_rcv_filter(struct dev_rcv_lists *d, struct sk_buff *skb)
 
 static void can_receive(struct sk_buff *skb, struct net_device *dev)
 {
-	struct dev_rcv_lists *d;
+	struct can_dev_rcv_lists *d;
 	struct net *net = dev_net(dev);
 	struct can_stats *can_stats = net->can.can_stats;
 	int matches;
@@ -829,7 +829,7 @@ static int can_notifier(struct notifier_block *nb, unsigned long msg,
 			void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
-	struct dev_rcv_lists *d;
+	struct can_dev_rcv_lists *d;
 
 	if (dev->type != ARPHRD_CAN)
 		return NOTIFY_DONE;
@@ -874,7 +874,7 @@ static int can_pernet_init(struct net *net)
 {
 	spin_lock_init(&net->can.can_rcvlists_lock);
 	net->can.can_rx_alldev_list =
-		kzalloc(sizeof(struct dev_rcv_lists), GFP_KERNEL);
+		kzalloc(sizeof(struct can_dev_rcv_lists), GFP_KERNEL);
 	if (!net->can.can_rx_alldev_list)
 		goto out;
 	net->can.can_stats = kzalloc(sizeof(struct can_stats), GFP_KERNEL);
@@ -920,7 +920,7 @@ static void can_pernet_exit(struct net *net)
 	rcu_read_lock();
 	for_each_netdev_rcu(net, dev) {
 		if (dev->type == ARPHRD_CAN && dev->ml_priv) {
-			struct dev_rcv_lists *d = dev->ml_priv;
+			struct can_dev_rcv_lists *d = dev->ml_priv;
 
 			BUG_ON(d->entries);
 			kfree(d);
diff --git a/net/can/af_can.h b/net/can/af_can.h
index 4e2cb14c1af7..4b77c7951f17 100644
--- a/net/can/af_can.h
+++ b/net/can/af_can.h
@@ -67,7 +67,7 @@ struct receiver {
 enum { RX_ERR, RX_ALL, RX_FIL, RX_INV, RX_MAX };
 
 /* per device receive filters linked at dev->ml_priv */
-struct dev_rcv_lists {
+struct can_dev_rcv_lists {
 	struct hlist_head rx[RX_MAX];
 	struct hlist_head rx_sff[CAN_SFF_RCV_ARRAY_SZ];
 	struct hlist_head rx_eff[CAN_EFF_RCV_ARRAY_SZ];
diff --git a/net/can/proc.c b/net/can/proc.c
index 559fca8035aa..b06ea3823b07 100644
--- a/net/can/proc.c
+++ b/net/can/proc.c
@@ -338,7 +338,7 @@ static const struct file_operations can_version_proc_fops = {
 
 static inline void can_rcvlist_proc_show_one(struct seq_file *m, int idx,
 					     struct net_device *dev,
-					     struct dev_rcv_lists *d)
+					     struct can_dev_rcv_lists *d)
 {
 	if (!hlist_empty(&d->rx[idx])) {
 		can_print_recv_banner(m);
@@ -353,7 +353,7 @@ static int can_rcvlist_proc_show(struct seq_file *m, void *v)
 	/* double cast to prevent GCC warning */
 	int idx = (int)(long)PDE_DATA(m->file->f_inode);
 	struct net_device *dev;
-	struct dev_rcv_lists *d;
+	struct can_dev_rcv_lists *d;
 	struct net *net = m->private;
 
 	seq_printf(m, "\nreceive list '%s':\n", rx_list_name[idx]);
@@ -417,7 +417,7 @@ static inline void can_rcvlist_proc_show_array(struct seq_file *m,
 static int can_rcvlist_sff_proc_show(struct seq_file *m, void *v)
 {
 	struct net_device *dev;
-	struct dev_rcv_lists *d;
+	struct can_dev_rcv_lists *d;
 	struct net *net = m->private;
 
 	/* RX_SFF */
@@ -461,7 +461,7 @@ static const struct file_operations can_rcvlist_sff_proc_fops = {
 static int can_rcvlist_eff_proc_show(struct seq_file *m, void *v)
 {
 	struct net_device *dev;
-	struct dev_rcv_lists *d;
+	struct can_dev_rcv_lists *d;
 	struct net *net = m->private;
 
 	/* RX_EFF */
-- 
2.13.2

Re: [PATCH 03/14] can: af_can: give struct holding the CAN per device receive lists a sensible name

[Oliver Hartkopp]

On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
> This patch adds a "can_" prefix to the "struct dev_rcv_lists" to better
> reflect the meaning and improbe code readability.
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

> ---
>   include/net/netns/can.h |  4 ++--
>   net/can/af_can.c        | 22 +++++++++++-----------
>   net/can/af_can.h        |  2 +-
>   net/can/proc.c          |  8 ++++----
>   4 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/include/net/netns/can.h b/include/net/netns/can.h
> index 5ee0f88ebbac..0594d9e7e309 100644
> --- a/include/net/netns/can.h
> +++ b/include/net/netns/can.h
> @@ -7,7 +7,7 @@
>   
>   #include <linux/spinlock.h>
>   
> -struct dev_rcv_lists;
> +struct can_dev_rcv_lists;
>   struct can_stats;
>   struct can_pstats;
>   
> @@ -27,7 +27,7 @@ struct netns_can {
>   #endif
>   
>   	/* receive filters subscribed for 'all' CAN devices */
> -	struct dev_rcv_lists *can_rx_alldev_list;
> +	struct can_dev_rcv_lists *can_rx_alldev_list;
>   	spinlock_t can_rcvlists_lock;
>   	struct timer_list can_stattimer;/* timer for statistics update */
>   	struct can_stats *can_stats; /* packet statistics */
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index bbd8a9bcf28d..2b12f713f4ca 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -321,13 +321,13 @@ EXPORT_SYMBOL(can_send);
>    * af_can rx path
>    */
>   
> -static struct dev_rcv_lists *can_dev_rcv_lists_find(struct net *net,
> -						    struct net_device *dev)
> +static struct can_dev_rcv_lists *can_dev_rcv_lists_find(struct net *net,
> +							struct net_device *dev)
>   {
>   	if (!dev)
>   		return net->can.can_rx_alldev_list;
>   	else
> -		return (struct dev_rcv_lists *)dev->ml_priv;
> +		return (struct can_dev_rcv_lists *)dev->ml_priv;
>   }
>   
>   /**
> @@ -381,7 +381,7 @@ static unsigned int effhash(canid_t can_id)
>    *  Reduced can_id to have a preprocessed filter compare value.
>    */
>   static struct hlist_head *find_rcv_list(canid_t *can_id, canid_t *mask,
> -					struct dev_rcv_lists *d)
> +					struct can_dev_rcv_lists *d)
>   {
>   	canid_t inv = *can_id & CAN_INV_FILTER; /* save flag before masking */
>   
> @@ -464,7 +464,7 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
>   {
>   	struct receiver *r;
>   	struct hlist_head *rl;
> -	struct dev_rcv_lists *d;
> +	struct can_dev_rcv_lists *d;
>   	struct can_pstats *can_pstats = net->can.can_pstats;
>   	int err = 0;
>   
> @@ -542,7 +542,7 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
>   	struct receiver *r = NULL;
>   	struct hlist_head *rl;
>   	struct can_pstats *can_pstats = net->can.can_pstats;
> -	struct dev_rcv_lists *d;
> +	struct can_dev_rcv_lists *d;
>   
>   	if (dev && dev->type != ARPHRD_CAN)
>   		return;
> @@ -615,7 +615,7 @@ static inline void deliver(struct sk_buff *skb, struct receiver *r)
>   	r->matches++;
>   }
>   
> -static int can_rcv_filter(struct dev_rcv_lists *d, struct sk_buff *skb)
> +static int can_rcv_filter(struct can_dev_rcv_lists *d, struct sk_buff *skb)
>   {
>   	struct receiver *r;
>   	int matches = 0;
> @@ -682,7 +682,7 @@ static int can_rcv_filter(struct dev_rcv_lists *d, struct sk_buff *skb)
>   
>   static void can_receive(struct sk_buff *skb, struct net_device *dev)
>   {
> -	struct dev_rcv_lists *d;
> +	struct can_dev_rcv_lists *d;
>   	struct net *net = dev_net(dev);
>   	struct can_stats *can_stats = net->can.can_stats;
>   	int matches;
> @@ -829,7 +829,7 @@ static int can_notifier(struct notifier_block *nb, unsigned long msg,
>   			void *ptr)
>   {
>   	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> -	struct dev_rcv_lists *d;
> +	struct can_dev_rcv_lists *d;
>   
>   	if (dev->type != ARPHRD_CAN)
>   		return NOTIFY_DONE;
> @@ -874,7 +874,7 @@ static int can_pernet_init(struct net *net)
>   {
>   	spin_lock_init(&net->can.can_rcvlists_lock);
>   	net->can.can_rx_alldev_list =
> -		kzalloc(sizeof(struct dev_rcv_lists), GFP_KERNEL);
> +		kzalloc(sizeof(struct can_dev_rcv_lists), GFP_KERNEL);
>   	if (!net->can.can_rx_alldev_list)
>   		goto out;
>   	net->can.can_stats = kzalloc(sizeof(struct can_stats), GFP_KERNEL);
> @@ -920,7 +920,7 @@ static void can_pernet_exit(struct net *net)
>   	rcu_read_lock();
>   	for_each_netdev_rcu(net, dev) {
>   		if (dev->type == ARPHRD_CAN && dev->ml_priv) {
> -			struct dev_rcv_lists *d = dev->ml_priv;
> +			struct can_dev_rcv_lists *d = dev->ml_priv;
>   
>   			BUG_ON(d->entries);
>   			kfree(d);
> diff --git a/net/can/af_can.h b/net/can/af_can.h
> index 4e2cb14c1af7..4b77c7951f17 100644
> --- a/net/can/af_can.h
> +++ b/net/can/af_can.h
> @@ -67,7 +67,7 @@ struct receiver {
>   enum { RX_ERR, RX_ALL, RX_FIL, RX_INV, RX_MAX };
>   
>   /* per device receive filters linked at dev->ml_priv */
> -struct dev_rcv_lists {
> +struct can_dev_rcv_lists {
>   	struct hlist_head rx[RX_MAX];
>   	struct hlist_head rx_sff[CAN_SFF_RCV_ARRAY_SZ];
>   	struct hlist_head rx_eff[CAN_EFF_RCV_ARRAY_SZ];
> diff --git a/net/can/proc.c b/net/can/proc.c
> index 559fca8035aa..b06ea3823b07 100644
> --- a/net/can/proc.c
> +++ b/net/can/proc.c
> @@ -338,7 +338,7 @@ static const struct file_operations can_version_proc_fops = {
>   
>   static inline void can_rcvlist_proc_show_one(struct seq_file *m, int idx,
>   					     struct net_device *dev,
> -					     struct dev_rcv_lists *d)
> +					     struct can_dev_rcv_lists *d)
>   {
>   	if (!hlist_empty(&d->rx[idx])) {
>   		can_print_recv_banner(m);
> @@ -353,7 +353,7 @@ static int can_rcvlist_proc_show(struct seq_file *m, void *v)
>   	/* double cast to prevent GCC warning */
>   	int idx = (int)(long)PDE_DATA(m->file->f_inode);
>   	struct net_device *dev;
> -	struct dev_rcv_lists *d;
> +	struct can_dev_rcv_lists *d;
>   	struct net *net = m->private;
>   
>   	seq_printf(m, "\nreceive list '%s':\n", rx_list_name[idx]);
> @@ -417,7 +417,7 @@ static inline void can_rcvlist_proc_show_array(struct seq_file *m,
>   static int can_rcvlist_sff_proc_show(struct seq_file *m, void *v)
>   {
>   	struct net_device *dev;
> -	struct dev_rcv_lists *d;
> +	struct can_dev_rcv_lists *d;
>   	struct net *net = m->private;
>   
>   	/* RX_SFF */
> @@ -461,7 +461,7 @@ static const struct file_operations can_rcvlist_sff_proc_fops = {
>   static int can_rcvlist_eff_proc_show(struct seq_file *m, void *v)
>   {
>   	struct net_device *dev;
> -	struct dev_rcv_lists *d;
> +	struct can_dev_rcv_lists *d;
>   	struct net *net = m->private;
>   
>   	/* RX_EFF */
> 

Re: [PATCH 03/14] can: af_can: give struct holding the CAN per device receive lists a sensible name

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 651 bytes --]

On 08/24/2017 02:59 PM, Oliver Hartkopp wrote:
> 
> 
> On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
>> This patch adds a "can_" prefix to the "struct dev_rcv_lists" to better
>> reflect the meaning and improbe code readability.
>>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

Tnx.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 04/14] can: af_can: give variable holding the CAN per device receive lists a sensible name

[Marc Kleine-Budde]

This patch gives the variables holding the CAN receive filter lists a
better name by renaming them from "d" to "dev_rcv_lists".

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/af_can.c | 87 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 43 insertions(+), 44 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index 2b12f713f4ca..43f4f51d5a73 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -381,7 +381,7 @@ static unsigned int effhash(canid_t can_id)
  *  Reduced can_id to have a preprocessed filter compare value.
  */
 static struct hlist_head *find_rcv_list(canid_t *can_id, canid_t *mask,
-					struct can_dev_rcv_lists *d)
+					struct can_dev_rcv_lists *dev_rcv_lists)
 {
 	canid_t inv = *can_id & CAN_INV_FILTER; /* save flag before masking */
 
@@ -389,7 +389,7 @@ static struct hlist_head *find_rcv_list(canid_t *can_id, canid_t *mask,
 	if (*mask & CAN_ERR_FLAG) {
 		/* clear CAN_ERR_FLAG in filter entry */
 		*mask &= CAN_ERR_MASK;
-		return &d->rx[RX_ERR];
+		return &dev_rcv_lists->rx[RX_ERR];
 	}
 
 	/* with cleared CAN_ERR_FLAG we have a simple mask/value filterpair */
@@ -405,11 +405,11 @@ static struct hlist_head *find_rcv_list(canid_t *can_id, canid_t *mask,
 
 	/* inverse can_id/can_mask filter */
 	if (inv)
-		return &d->rx[RX_INV];
+		return &dev_rcv_lists->rx[RX_INV];
 
 	/* mask == 0 => no condition testing at receive time */
 	if (!(*mask))
-		return &d->rx[RX_ALL];
+		return &dev_rcv_lists->rx[RX_ALL];
 
 	/* extra filterlists for the subscription of a single non-RTR can_id */
 	if (((*mask & CAN_EFF_RTR_FLAGS) == CAN_EFF_RTR_FLAGS) &&
@@ -417,15 +417,15 @@ static struct hlist_head *find_rcv_list(canid_t *can_id, canid_t *mask,
 
 		if (*can_id & CAN_EFF_FLAG) {
 			if (*mask == (CAN_EFF_MASK | CAN_EFF_RTR_FLAGS))
-				return &d->rx_eff[effhash(*can_id)];
+				return &dev_rcv_lists->rx_eff[effhash(*can_id)];
 		} else {
 			if (*mask == (CAN_SFF_MASK | CAN_EFF_RTR_FLAGS))
-				return &d->rx_sff[*can_id];
+				return &dev_rcv_lists->rx_sff[*can_id];
 		}
 	}
 
 	/* default: filter via can_id/can_mask */
-	return &d->rx[RX_FIL];
+	return &dev_rcv_lists->rx[RX_FIL];
 }
 
 /**
@@ -464,7 +464,7 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 {
 	struct receiver *r;
 	struct hlist_head *rl;
-	struct can_dev_rcv_lists *d;
+	struct can_dev_rcv_lists *dev_rcv_lists;
 	struct can_pstats *can_pstats = net->can.can_pstats;
 	int err = 0;
 
@@ -482,9 +482,9 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 
 	spin_lock(&net->can.can_rcvlists_lock);
 
-	d = can_dev_rcv_lists_find(net, dev);
-	if (d) {
-		rl = find_rcv_list(&can_id, &mask, d);
+	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
+	if (dev_rcv_lists) {
+		rl = find_rcv_list(&can_id, &mask, dev_rcv_lists);
 
 		r->can_id  = can_id;
 		r->mask    = mask;
@@ -495,7 +495,7 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 		r->sk      = sk;
 
 		hlist_add_head_rcu(&r->list, rl);
-		d->entries++;
+		dev_rcv_lists->entries++;
 
 		can_pstats->rcv_entries++;
 		if (can_pstats->rcv_entries_max < can_pstats->rcv_entries)
@@ -542,7 +542,7 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	struct receiver *r = NULL;
 	struct hlist_head *rl;
 	struct can_pstats *can_pstats = net->can.can_pstats;
-	struct can_dev_rcv_lists *d;
+	struct can_dev_rcv_lists *dev_rcv_lists;
 
 	if (dev && dev->type != ARPHRD_CAN)
 		return;
@@ -552,22 +552,21 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 
 	spin_lock(&net->can.can_rcvlists_lock);
 
-	d = can_dev_rcv_lists_find(net, dev);
-	if (!d) {
+	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
+	if (!dev_rcv_lists) {
 		pr_err("BUG: receive list not found for "
 		       "dev %s, id %03X, mask %03X\n",
 		       DNAME(dev), can_id, mask);
 		goto out;
 	}
 
-	rl = find_rcv_list(&can_id, &mask, d);
+	rl = find_rcv_list(&can_id, &mask, dev_rcv_lists);
 
 	/*
 	 * Search the receiver list for the item to delete.  This should
 	 * exist, since no receiver may be unregistered that hasn't
 	 * been registered before.
 	 */
-
 	hlist_for_each_entry_rcu(r, rl, list) {
 		if (r->can_id == can_id && r->mask == mask &&
 		    r->func == func && r->data == data)
@@ -586,14 +585,14 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	}
 
 	hlist_del_rcu(&r->list);
-	d->entries--;
+	dev_rcv_lists->entries--;
 
 	if (can_pstats->rcv_entries > 0)
 		can_pstats->rcv_entries--;
 
 	/* remove device structure requested by NETDEV_UNREGISTER */
-	if (d->remove_on_zero_entries && !d->entries) {
-		kfree(d);
+	if (dev_rcv_lists->remove_on_zero_entries && !dev_rcv_lists->entries) {
+		kfree(dev_rcv_lists);
 		dev->ml_priv = NULL;
 	}
 
@@ -615,19 +614,19 @@ static inline void deliver(struct sk_buff *skb, struct receiver *r)
 	r->matches++;
 }
 
-static int can_rcv_filter(struct can_dev_rcv_lists *d, struct sk_buff *skb)
+static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buff *skb)
 {
 	struct receiver *r;
 	int matches = 0;
 	struct can_frame *cf = (struct can_frame *)skb->data;
 	canid_t can_id = cf->can_id;
 
-	if (d->entries == 0)
+	if (dev_rcv_lists->entries == 0)
 		return 0;
 
 	if (can_id & CAN_ERR_FLAG) {
 		/* check for error message frame entries only */
-		hlist_for_each_entry_rcu(r, &d->rx[RX_ERR], list) {
+		hlist_for_each_entry_rcu(r, &dev_rcv_lists->rx[RX_ERR], list) {
 			if (can_id & r->mask) {
 				deliver(skb, r);
 				matches++;
@@ -637,13 +636,13 @@ static int can_rcv_filter(struct can_dev_rcv_lists *d, struct sk_buff *skb)
 	}
 
 	/* check for unfiltered entries */
-	hlist_for_each_entry_rcu(r, &d->rx[RX_ALL], list) {
+	hlist_for_each_entry_rcu(r, &dev_rcv_lists->rx[RX_ALL], list) {
 		deliver(skb, r);
 		matches++;
 	}
 
 	/* check for can_id/mask entries */
-	hlist_for_each_entry_rcu(r, &d->rx[RX_FIL], list) {
+	hlist_for_each_entry_rcu(r, &dev_rcv_lists->rx[RX_FIL], list) {
 		if ((can_id & r->mask) == r->can_id) {
 			deliver(skb, r);
 			matches++;
@@ -651,7 +650,7 @@ static int can_rcv_filter(struct can_dev_rcv_lists *d, struct sk_buff *skb)
 	}
 
 	/* check for inverted can_id/mask entries */
-	hlist_for_each_entry_rcu(r, &d->rx[RX_INV], list) {
+	hlist_for_each_entry_rcu(r, &dev_rcv_lists->rx[RX_INV], list) {
 		if ((can_id & r->mask) != r->can_id) {
 			deliver(skb, r);
 			matches++;
@@ -663,7 +662,7 @@ static int can_rcv_filter(struct can_dev_rcv_lists *d, struct sk_buff *skb)
 		return matches;
 
 	if (can_id & CAN_EFF_FLAG) {
-		hlist_for_each_entry_rcu(r, &d->rx_eff[effhash(can_id)], list) {
+		hlist_for_each_entry_rcu(r, &dev_rcv_lists->rx_eff[effhash(can_id)], list) {
 			if (r->can_id == can_id) {
 				deliver(skb, r);
 				matches++;
@@ -671,7 +670,7 @@ static int can_rcv_filter(struct can_dev_rcv_lists *d, struct sk_buff *skb)
 		}
 	} else {
 		can_id &= CAN_SFF_MASK;
-		hlist_for_each_entry_rcu(r, &d->rx_sff[can_id], list) {
+		hlist_for_each_entry_rcu(r, &dev_rcv_lists->rx_sff[can_id], list) {
 			deliver(skb, r);
 			matches++;
 		}
@@ -682,7 +681,7 @@ static int can_rcv_filter(struct can_dev_rcv_lists *d, struct sk_buff *skb)
 
 static void can_receive(struct sk_buff *skb, struct net_device *dev)
 {
-	struct can_dev_rcv_lists *d;
+	struct can_dev_rcv_lists *dev_rcv_lists;
 	struct net *net = dev_net(dev);
 	struct can_stats *can_stats = net->can.can_stats;
 	int matches;
@@ -701,9 +700,9 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
 	matches = can_rcv_filter(net->can.can_rx_alldev_list, skb);
 
 	/* find receive list for this device */
-	d = can_dev_rcv_lists_find(net, dev);
-	if (d)
-		matches += can_rcv_filter(d, skb);
+	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
+	if (dev_rcv_lists)
+		matches += can_rcv_filter(dev_rcv_lists, skb);
 
 	rcu_read_unlock();
 
@@ -829,7 +828,7 @@ static int can_notifier(struct notifier_block *nb, unsigned long msg,
 			void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
-	struct can_dev_rcv_lists *d;
+	struct can_dev_rcv_lists *dev_rcv_lists;
 
 	if (dev->type != ARPHRD_CAN)
 		return NOTIFY_DONE;
@@ -839,23 +838,23 @@ static int can_notifier(struct notifier_block *nb, unsigned long msg,
 	case NETDEV_REGISTER:
 
 		/* create new dev_rcv_lists for this device */
-		d = kzalloc(sizeof(*d), GFP_KERNEL);
-		if (!d)
+		dev_rcv_lists = kzalloc(sizeof(*dev_rcv_lists), GFP_KERNEL);
+		if (!dev_rcv_lists)
 			return NOTIFY_DONE;
 		BUG_ON(dev->ml_priv);
-		dev->ml_priv = d;
+		dev->ml_priv = dev_rcv_lists;
 
 		break;
 
 	case NETDEV_UNREGISTER:
 		spin_lock(&dev_net(dev)->can.can_rcvlists_lock);
 
-		d = dev->ml_priv;
-		if (d) {
-			if (d->entries)
-				d->remove_on_zero_entries = 1;
+		dev_rcv_lists = dev->ml_priv;
+		if (dev_rcv_lists) {
+			if (dev_rcv_lists->entries)
+				dev_rcv_lists->remove_on_zero_entries = 1;
 			else {
-				kfree(d);
+				kfree(dev_rcv_lists);
 				dev->ml_priv = NULL;
 			}
 		} else
@@ -920,10 +919,10 @@ static void can_pernet_exit(struct net *net)
 	rcu_read_lock();
 	for_each_netdev_rcu(net, dev) {
 		if (dev->type == ARPHRD_CAN && dev->ml_priv) {
-			struct can_dev_rcv_lists *d = dev->ml_priv;
+			struct can_dev_rcv_lists *dev_rcv_lists = dev->ml_priv;
 
-			BUG_ON(d->entries);
-			kfree(d);
+			BUG_ON(dev_rcv_lists->entries);
+			kfree(dev_rcv_lists);
 			dev->ml_priv = NULL;
 		}
 	}
-- 
2.13.2

Re: [PATCH 04/14] can: af_can: give variable holding the CAN per device receive lists a sensible name

[Oliver Hartkopp]

On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
> This patch gives the variables holding the CAN receive filter lists a
> better name by renaming them from "d" to "dev_rcv_lists".
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>   net/can/af_can.c | 87 ++++++++++++++++++++++++++++----------------------------
>   1 file changed, 43 insertions(+), 44 deletions(-)
> 
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index 2b12f713f4ca..43f4f51d5a73 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -381,7 +381,7 @@ static unsigned int effhash(canid_t can_id)
>    *  Reduced can_id to have a preprocessed filter compare value.
>    */
>   static struct hlist_head *find_rcv_list(canid_t *can_id, canid_t *mask,
> -					struct can_dev_rcv_lists *d)
> +					struct can_dev_rcv_lists *dev_rcv_lists)

					struct can_dev_rcv_lists *cdrl)

Ok. 'd' might be too short and not be easy to bring into connection with 
can_dev_rcv_lists ... but 'dev_rcv_lists' smells like just another 
structure definition an not a handy variable name, like e.g. cdrl ;-)

Regards,
Oliver

Re: [PATCH 04/14] can: af_can: give variable holding the CAN per device receive lists a sensible name

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1534 bytes --]

On 08/24/2017 03:03 PM, Oliver Hartkopp wrote:
> On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
>> This patch gives the variables holding the CAN receive filter lists a
>> better name by renaming them from "d" to "dev_rcv_lists".
>>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> ---
>>   net/can/af_can.c | 87 ++++++++++++++++++++++++++++----------------------------
>>   1 file changed, 43 insertions(+), 44 deletions(-)
>>
>> diff --git a/net/can/af_can.c b/net/can/af_can.c
>> index 2b12f713f4ca..43f4f51d5a73 100644
>> --- a/net/can/af_can.c
>> +++ b/net/can/af_can.c
>> @@ -381,7 +381,7 @@ static unsigned int effhash(canid_t can_id)
>>    *  Reduced can_id to have a preprocessed filter compare value.
>>    */
>>   static struct hlist_head *find_rcv_list(canid_t *can_id, canid_t *mask,
>> -					struct can_dev_rcv_lists *d)
>> +					struct can_dev_rcv_lists *dev_rcv_lists)
> 
> 					struct can_dev_rcv_lists *cdrl)
> 
> Ok. 'd' might be too short and not be easy to bring into connection with 
> can_dev_rcv_lists ... but 'dev_rcv_lists' smells like just another 
> structure definition an not a handy variable name, like e.g. cdrl ;-)

"d" is short - yes, but "cdrl" doesn't speak to me either.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 05/14] can: proc: give variable holding the CAN per device receive lists a sensible name

[Marc Kleine-Budde]

This patch gives the variables holding the CAN per device receive filter lists
a better name by renaming them from "d" to "dev_rcv_lists".

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/proc.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/net/can/proc.c b/net/can/proc.c
index b06ea3823b07..966dd53f8fa4 100644
--- a/net/can/proc.c
+++ b/net/can/proc.c
@@ -338,11 +338,11 @@ static const struct file_operations can_version_proc_fops = {
 
 static inline void can_rcvlist_proc_show_one(struct seq_file *m, int idx,
 					     struct net_device *dev,
-					     struct can_dev_rcv_lists *d)
+					     struct can_dev_rcv_lists *dev_rcv_lists)
 {
-	if (!hlist_empty(&d->rx[idx])) {
+	if (!hlist_empty(&dev_rcv_lists->rx[idx])) {
 		can_print_recv_banner(m);
-		can_print_rcvlist(m, &d->rx[idx], dev);
+		can_print_rcvlist(m, &dev_rcv_lists->rx[idx], dev);
 	} else
 		seq_printf(m, "  (%s: no entry)\n", DNAME(dev));
 
@@ -353,7 +353,7 @@ static int can_rcvlist_proc_show(struct seq_file *m, void *v)
 	/* double cast to prevent GCC warning */
 	int idx = (int)(long)PDE_DATA(m->file->f_inode);
 	struct net_device *dev;
-	struct can_dev_rcv_lists *d;
+	struct can_dev_rcv_lists *dev_rcv_lists;
 	struct net *net = m->private;
 
 	seq_printf(m, "\nreceive list '%s':\n", rx_list_name[idx]);
@@ -361,8 +361,8 @@ static int can_rcvlist_proc_show(struct seq_file *m, void *v)
 	rcu_read_lock();
 
 	/* receive list for 'all' CAN devices (dev == NULL) */
-	d = net->can.can_rx_alldev_list;
-	can_rcvlist_proc_show_one(m, idx, NULL, d);
+	dev_rcv_lists = net->can.can_rx_alldev_list;
+	can_rcvlist_proc_show_one(m, idx, NULL, dev_rcv_lists);
 
 	/* receive list for registered CAN devices */
 	for_each_netdev_rcu(net, dev) {
@@ -417,7 +417,7 @@ static inline void can_rcvlist_proc_show_array(struct seq_file *m,
 static int can_rcvlist_sff_proc_show(struct seq_file *m, void *v)
 {
 	struct net_device *dev;
-	struct can_dev_rcv_lists *d;
+	struct can_dev_rcv_lists *dev_rcv_lists;
 	struct net *net = m->private;
 
 	/* RX_SFF */
@@ -426,15 +426,16 @@ static int can_rcvlist_sff_proc_show(struct seq_file *m, void *v)
 	rcu_read_lock();
 
 	/* sff receive list for 'all' CAN devices (dev == NULL) */
-	d = net->can.can_rx_alldev_list;
-	can_rcvlist_proc_show_array(m, NULL, d->rx_sff, ARRAY_SIZE(d->rx_sff));
+	dev_rcv_lists = net->can.can_rx_alldev_list;
+	can_rcvlist_proc_show_array(m, NULL, dev_rcv_lists->rx_sff,
+				    ARRAY_SIZE(dev_rcv_lists->rx_sff));
 
 	/* sff receive list for registered CAN devices */
 	for_each_netdev_rcu(net, dev) {
 		if (dev->type == ARPHRD_CAN && dev->ml_priv) {
-			d = dev->ml_priv;
-			can_rcvlist_proc_show_array(m, dev, d->rx_sff,
-						    ARRAY_SIZE(d->rx_sff));
+			dev_rcv_lists = dev->ml_priv;
+			can_rcvlist_proc_show_array(m, dev, dev_rcv_lists->rx_sff,
+						    ARRAY_SIZE(dev_rcv_lists->rx_sff));
 		}
 	}
 
@@ -461,7 +462,7 @@ static const struct file_operations can_rcvlist_sff_proc_fops = {
 static int can_rcvlist_eff_proc_show(struct seq_file *m, void *v)
 {
 	struct net_device *dev;
-	struct can_dev_rcv_lists *d;
+	struct can_dev_rcv_lists *dev_rcv_lists;
 	struct net *net = m->private;
 
 	/* RX_EFF */
@@ -470,15 +471,16 @@ static int can_rcvlist_eff_proc_show(struct seq_file *m, void *v)
 	rcu_read_lock();
 
 	/* eff receive list for 'all' CAN devices (dev == NULL) */
-	d = net->can.can_rx_alldev_list;
-	can_rcvlist_proc_show_array(m, NULL, d->rx_eff, ARRAY_SIZE(d->rx_eff));
+	dev_rcv_lists = net->can.can_rx_alldev_list;
+	can_rcvlist_proc_show_array(m, NULL, dev_rcv_lists->rx_eff,
+				    ARRAY_SIZE(dev_rcv_lists->rx_eff));
 
 	/* eff receive list for registered CAN devices */
 	for_each_netdev_rcu(net, dev) {
 		if (dev->type == ARPHRD_CAN && dev->ml_priv) {
-			d = dev->ml_priv;
-			can_rcvlist_proc_show_array(m, dev, d->rx_eff,
-						    ARRAY_SIZE(d->rx_eff));
+			dev_rcv_lists = dev->ml_priv;
+			can_rcvlist_proc_show_array(m, dev, dev_rcv_lists->rx_eff,
+						    ARRAY_SIZE(dev_rcv_lists->rx_eff));
 		}
 	}
 
-- 
2.13.2

Re: [PATCH 05/14] can: proc: give variable holding the CAN per device receive lists a sensible name

[Oliver Hartkopp]

On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
> This patch gives the variables holding the CAN per device receive filter lists
> a better name by renaming them from "d" to "dev_rcv_lists".

Same comment as in patch 04/14:

'd' might be too short and not be easy to bring into connection with 
can_dev_rcv_lists ... but 'dev_rcv_lists' smells like just another 
structure definition an not a handy variable name, like e.g. cdrl

Regards,
Oliver

> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>   net/can/proc.c | 38 ++++++++++++++++++++------------------
>   1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/net/can/proc.c b/net/can/proc.c
> index b06ea3823b07..966dd53f8fa4 100644
> --- a/net/can/proc.c
> +++ b/net/can/proc.c
> @@ -338,11 +338,11 @@ static const struct file_operations can_version_proc_fops = {
>   
>   static inline void can_rcvlist_proc_show_one(struct seq_file *m, int idx,
>   					     struct net_device *dev,
> -					     struct can_dev_rcv_lists *d)
> +					     struct can_dev_rcv_lists *dev_rcv_lists)
>   {
> -	if (!hlist_empty(&d->rx[idx])) {
> +	if (!hlist_empty(&dev_rcv_lists->rx[idx])) {
>   		can_print_recv_banner(m);
> -		can_print_rcvlist(m, &d->rx[idx], dev);
> +		can_print_rcvlist(m, &dev_rcv_lists->rx[idx], dev);
>   	} else
>   		seq_printf(m, "  (%s: no entry)\n", DNAME(dev));
>   
> @@ -353,7 +353,7 @@ static int can_rcvlist_proc_show(struct seq_file *m, void *v)
>   	/* double cast to prevent GCC warning */
>   	int idx = (int)(long)PDE_DATA(m->file->f_inode);
>   	struct net_device *dev;
> -	struct can_dev_rcv_lists *d;
> +	struct can_dev_rcv_lists *dev_rcv_lists;
>   	struct net *net = m->private;
>   
>   	seq_printf(m, "\nreceive list '%s':\n", rx_list_name[idx]);
> @@ -361,8 +361,8 @@ static int can_rcvlist_proc_show(struct seq_file *m, void *v)
>   	rcu_read_lock();
>   
>   	/* receive list for 'all' CAN devices (dev == NULL) */
> -	d = net->can.can_rx_alldev_list;
> -	can_rcvlist_proc_show_one(m, idx, NULL, d);
> +	dev_rcv_lists = net->can.can_rx_alldev_list;
> +	can_rcvlist_proc_show_one(m, idx, NULL, dev_rcv_lists);
>   
>   	/* receive list for registered CAN devices */
>   	for_each_netdev_rcu(net, dev) {
> @@ -417,7 +417,7 @@ static inline void can_rcvlist_proc_show_array(struct seq_file *m,
>   static int can_rcvlist_sff_proc_show(struct seq_file *m, void *v)
>   {
>   	struct net_device *dev;
> -	struct can_dev_rcv_lists *d;
> +	struct can_dev_rcv_lists *dev_rcv_lists;
>   	struct net *net = m->private;
>   
>   	/* RX_SFF */
> @@ -426,15 +426,16 @@ static int can_rcvlist_sff_proc_show(struct seq_file *m, void *v)
>   	rcu_read_lock();
>   
>   	/* sff receive list for 'all' CAN devices (dev == NULL) */
> -	d = net->can.can_rx_alldev_list;
> -	can_rcvlist_proc_show_array(m, NULL, d->rx_sff, ARRAY_SIZE(d->rx_sff));
> +	dev_rcv_lists = net->can.can_rx_alldev_list;
> +	can_rcvlist_proc_show_array(m, NULL, dev_rcv_lists->rx_sff,
> +				    ARRAY_SIZE(dev_rcv_lists->rx_sff));
>   
>   	/* sff receive list for registered CAN devices */
>   	for_each_netdev_rcu(net, dev) {
>   		if (dev->type == ARPHRD_CAN && dev->ml_priv) {
> -			d = dev->ml_priv;
> -			can_rcvlist_proc_show_array(m, dev, d->rx_sff,
> -						    ARRAY_SIZE(d->rx_sff));
> +			dev_rcv_lists = dev->ml_priv;
> +			can_rcvlist_proc_show_array(m, dev, dev_rcv_lists->rx_sff,
> +						    ARRAY_SIZE(dev_rcv_lists->rx_sff));
>   		}
>   	}
>   
> @@ -461,7 +462,7 @@ static const struct file_operations can_rcvlist_sff_proc_fops = {
>   static int can_rcvlist_eff_proc_show(struct seq_file *m, void *v)
>   {
>   	struct net_device *dev;
> -	struct can_dev_rcv_lists *d;
> +	struct can_dev_rcv_lists *dev_rcv_lists;
>   	struct net *net = m->private;
>   
>   	/* RX_EFF */
> @@ -470,15 +471,16 @@ static int can_rcvlist_eff_proc_show(struct seq_file *m, void *v)
>   	rcu_read_lock();
>   
>   	/* eff receive list for 'all' CAN devices (dev == NULL) */
> -	d = net->can.can_rx_alldev_list;
> -	can_rcvlist_proc_show_array(m, NULL, d->rx_eff, ARRAY_SIZE(d->rx_eff));
> +	dev_rcv_lists = net->can.can_rx_alldev_list;
> +	can_rcvlist_proc_show_array(m, NULL, dev_rcv_lists->rx_eff,
> +				    ARRAY_SIZE(dev_rcv_lists->rx_eff));
>   
>   	/* eff receive list for registered CAN devices */
>   	for_each_netdev_rcu(net, dev) {
>   		if (dev->type == ARPHRD_CAN && dev->ml_priv) {
> -			d = dev->ml_priv;
> -			can_rcvlist_proc_show_array(m, dev, d->rx_eff,
> -						    ARRAY_SIZE(d->rx_eff));
> +			dev_rcv_lists = dev->ml_priv;
> +			can_rcvlist_proc_show_array(m, dev, dev_rcv_lists->rx_eff,
> +						    ARRAY_SIZE(dev_rcv_lists->rx_eff));
>   		}
>   	}
>   
> 

[PATCH 06/14] can: af_can: rename find_rcv_list() to can_rcv_list_find()

[Marc Kleine-Budde]

This patch add the commonly used prefix "can_" to the find_rcv_list()
function and add the "find" to the end, as the function returns a struct
rcv_list. This improves the overall readability of the code.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/af_can.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index 43f4f51d5a73..0f1334803aea 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -354,7 +354,7 @@ static unsigned int effhash(canid_t can_id)
 }
 
 /**
- * find_rcv_list - determine optimal filterlist inside device filter struct
+ * can_rcv_list_find - determine optimal filterlist inside device filter struct
  * @can_id: pointer to CAN identifier of a given can_filter
  * @mask: pointer to CAN mask of a given can_filter
  * @d: pointer to the device filter struct
@@ -380,8 +380,8 @@ static unsigned int effhash(canid_t can_id)
  *  Constistency checked mask.
  *  Reduced can_id to have a preprocessed filter compare value.
  */
-static struct hlist_head *find_rcv_list(canid_t *can_id, canid_t *mask,
-					struct can_dev_rcv_lists *dev_rcv_lists)
+static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
+					    struct can_dev_rcv_lists *dev_rcv_lists)
 {
 	canid_t inv = *can_id & CAN_INV_FILTER; /* save flag before masking */
 
@@ -484,7 +484,7 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 
 	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
 	if (dev_rcv_lists) {
-		rl = find_rcv_list(&can_id, &mask, dev_rcv_lists);
+		rl = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);
 
 		r->can_id  = can_id;
 		r->mask    = mask;
@@ -560,7 +560,7 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 		goto out;
 	}
 
-	rl = find_rcv_list(&can_id, &mask, dev_rcv_lists);
+	rl = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);
 
 	/*
 	 * Search the receiver list for the item to delete.  This should
-- 
2.13.2

Re: [PATCH 06/14] can: af_can: rename find_rcv_list() to can_rcv_list_find()

[Oliver Hartkopp]

On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
> This patch add the commonly used prefix "can_" to the find_rcv_list()
> function and add the "find" to the end, as the function returns a struct
> rcv_list. This improves the overall readability of the code.

The function should be named according to it's natural function.

If you want to move it to some kind of 'can' namespace I would suggest

find_can_rcv_list()

> -		rl = find_rcv_list(&can_id, &mask, dev_rcv_lists);
> +		rl = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);

find_... is better to understand than can_...

And this is a local function not visible to anyone else outside of 
af_can.c .

Regards,
Oliver

Re: [PATCH 06/14] can: af_can: rename find_rcv_list() to can_rcv_list_find()

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1556 bytes --]

On 08/24/2017 03:11 PM, Oliver Hartkopp wrote:
> On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
>> This patch add the commonly used prefix "can_" to the find_rcv_list()
>> function and add the "find" to the end, as the function returns a struct
>> rcv_list. This improves the overall readability of the code.
> 
> The function should be named according to it's natural function.
> 
> If you want to move it to some kind of 'can' namespace I would suggest
> 
> find_can_rcv_list()

Sadly C doesn't have native namespaces (and objects) like C++, so an
option is to put a can_ in front of every CAN related function....

>> -		rl = find_rcv_list(&can_id, &mask, dev_rcv_lists);
>> +		rl = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);
> 
> find_... is better to understand than can_...

...then the "object" or rather the variable the functions deals with,
here a "rcv_list" and then what it does "find". Looks a bit like RPN
calculator :)

> And this is a local function not visible to anyone else outside of 
> af_can.c .

If you know the code, then it's clear, but if you don't know the code
you immediately see that this is a CAN specific function and not a
"generic" one from rest of the networking stack.

Convinced? :D

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 07/14] can: af_can: give variable holding the CAN receiver and the receiver list a sensible name

[Marc Kleine-Budde]

This patch gives the variables holding the CAN receiver and the receiver
list a better name by renaming them from "r to "rcv" and "rl" to
"recv_list".

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/af_can.c | 101 +++++++++++++++++++++++++++----------------------------
 1 file changed, 50 insertions(+), 51 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index 0f1334803aea..cb079d2dcde2 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -462,8 +462,8 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 		    canid_t mask, void (*func)(struct sk_buff *, void *),
 		    void *data, char *ident, struct sock *sk)
 {
-	struct receiver *r;
-	struct hlist_head *rl;
+	struct receiver *rcv;
+	struct hlist_head *rcv_list;
 	struct can_dev_rcv_lists *dev_rcv_lists;
 	struct can_pstats *can_pstats = net->can.can_pstats;
 	int err = 0;
@@ -476,32 +476,32 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 	if (dev && !net_eq(net, dev_net(dev)))
 		return -ENODEV;
 
-	r = kmem_cache_alloc(rcv_cache, GFP_KERNEL);
-	if (!r)
+	rcv = kmem_cache_alloc(rcv_cache, GFP_KERNEL);
+	if (!rcv)
 		return -ENOMEM;
 
 	spin_lock(&net->can.can_rcvlists_lock);
 
 	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
 	if (dev_rcv_lists) {
-		rl = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);
+		rcv_list = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);
 
-		r->can_id  = can_id;
-		r->mask    = mask;
-		r->matches = 0;
-		r->func    = func;
-		r->data    = data;
-		r->ident   = ident;
-		r->sk      = sk;
+		rcv->can_id = can_id;
+		rcv->mask = mask;
+		rcv->matches = 0;
+		rcv->func = func;
+		rcv->data = data;
+		rcv->ident = ident;
+		rcv->sk = sk;
 
-		hlist_add_head_rcu(&r->list, rl);
+		hlist_add_head_rcu(&rcv->list, rcv_list);
 		dev_rcv_lists->entries++;
 
 		can_pstats->rcv_entries++;
 		if (can_pstats->rcv_entries_max < can_pstats->rcv_entries)
 			can_pstats->rcv_entries_max = can_pstats->rcv_entries;
 	} else {
-		kmem_cache_free(rcv_cache, r);
+		kmem_cache_free(rcv_cache, rcv);
 		err = -ENODEV;
 	}
 
@@ -516,10 +516,10 @@ EXPORT_SYMBOL(can_rx_register);
  */
 static void can_rx_delete_receiver(struct rcu_head *rp)
 {
-	struct receiver *r = container_of(rp, struct receiver, rcu);
-	struct sock *sk = r->sk;
+	struct receiver *rcv = container_of(rp, struct receiver, rcu);
+	struct sock *sk = rcv->sk;
 
-	kmem_cache_free(rcv_cache, r);
+	kmem_cache_free(rcv_cache, rcv);
 	if (sk)
 		sock_put(sk);
 }
@@ -539,8 +539,8 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 		       canid_t mask, void (*func)(struct sk_buff *, void *),
 		       void *data)
 {
-	struct receiver *r = NULL;
-	struct hlist_head *rl;
+	struct receiver *rcv = NULL;
+	struct hlist_head *rcv_list;
 	struct can_pstats *can_pstats = net->can.can_pstats;
 	struct can_dev_rcv_lists *dev_rcv_lists;
 
@@ -560,31 +560,30 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 		goto out;
 	}
 
-	rl = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);
+	rcv_list = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);
 
 	/*
 	 * Search the receiver list for the item to delete.  This should
 	 * exist, since no receiver may be unregistered that hasn't
 	 * been registered before.
 	 */
-	hlist_for_each_entry_rcu(r, rl, list) {
-		if (r->can_id == can_id && r->mask == mask &&
-		    r->func == func && r->data == data)
+	hlist_for_each_entry_rcu(rcv, rcv_list, list) {
+		if (rcv->can_id == can_id && rcv->mask == mask &&
+		    rcv->func == func && rcv->data == data)
 			break;
 	}
 
 	/*
 	 * Check for bugs in CAN protocol implementations using af_can.c:
-	 * 'r' will be NULL if no matching list item was found for removal.
+	 * 'rcv' will be NULL if no matching list item was found for removal.
 	 */
-
-	if (!r) {
+	if (!rcv) {
 		WARN(1, "BUG: receive list entry not found for dev %s, "
 		     "id %03X, mask %03X\n", DNAME(dev), can_id, mask);
 		goto out;
 	}
 
-	hlist_del_rcu(&r->list);
+	hlist_del_rcu(&rcv->list);
 	dev_rcv_lists->entries--;
 
 	if (can_pstats->rcv_entries > 0)
@@ -600,23 +599,23 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	spin_unlock(&net->can.can_rcvlists_lock);
 
 	/* schedule the receiver item for deletion */
-	if (r) {
-		if (r->sk)
-			sock_hold(r->sk);
-		call_rcu(&r->rcu, can_rx_delete_receiver);
+	if (rcv) {
+		if (rcv->sk)
+			sock_hold(rcv->sk);
+		call_rcu(&rcv->rcu, can_rx_delete_receiver);
 	}
 }
 EXPORT_SYMBOL(can_rx_unregister);
 
-static inline void deliver(struct sk_buff *skb, struct receiver *r)
+static inline void deliver(struct sk_buff *skb, struct receiver *rcv)
 {
-	r->func(skb, r->data);
-	r->matches++;
+	rcv->func(skb, rcv->data);
+	rcv->matches++;
 }
 
 static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buff *skb)
 {
-	struct receiver *r;
+	struct receiver *rcv;
 	int matches = 0;
 	struct can_frame *cf = (struct can_frame *)skb->data;
 	canid_t can_id = cf->can_id;
@@ -626,9 +625,9 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
 
 	if (can_id & CAN_ERR_FLAG) {
 		/* check for error message frame entries only */
-		hlist_for_each_entry_rcu(r, &dev_rcv_lists->rx[RX_ERR], list) {
-			if (can_id & r->mask) {
-				deliver(skb, r);
+		hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_ERR], list) {
+			if (can_id & rcv->mask) {
+				deliver(skb, rcv);
 				matches++;
 			}
 		}
@@ -636,23 +635,23 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
 	}
 
 	/* check for unfiltered entries */
-	hlist_for_each_entry_rcu(r, &dev_rcv_lists->rx[RX_ALL], list) {
-		deliver(skb, r);
+	hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_ALL], list) {
+		deliver(skb, rcv);
 		matches++;
 	}
 
 	/* check for can_id/mask entries */
-	hlist_for_each_entry_rcu(r, &dev_rcv_lists->rx[RX_FIL], list) {
-		if ((can_id & r->mask) == r->can_id) {
-			deliver(skb, r);
+	hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_FIL], list) {
+		if ((can_id & rcv->mask) == rcv->can_id) {
+			deliver(skb, rcv);
 			matches++;
 		}
 	}
 
 	/* check for inverted can_id/mask entries */
-	hlist_for_each_entry_rcu(r, &dev_rcv_lists->rx[RX_INV], list) {
-		if ((can_id & r->mask) != r->can_id) {
-			deliver(skb, r);
+	hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_INV], list) {
+		if ((can_id & rcv->mask) != rcv->can_id) {
+			deliver(skb, rcv);
 			matches++;
 		}
 	}
@@ -662,16 +661,16 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
 		return matches;
 
 	if (can_id & CAN_EFF_FLAG) {
-		hlist_for_each_entry_rcu(r, &dev_rcv_lists->rx_eff[effhash(can_id)], list) {
-			if (r->can_id == can_id) {
-				deliver(skb, r);
+		hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx_eff[effhash(can_id)], list) {
+			if (rcv->can_id == can_id) {
+				deliver(skb, rcv);
 				matches++;
 			}
 		}
 	} else {
 		can_id &= CAN_SFF_MASK;
-		hlist_for_each_entry_rcu(r, &dev_rcv_lists->rx_sff[can_id], list) {
-			deliver(skb, r);
+		hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx_sff[can_id], list) {
+			deliver(skb, rcv);
 			matches++;
 		}
 	}
-- 
2.13.2

Re: [PATCH 07/14] can: af_can: give variable holding the CAN receiver and the receiver list a sensible name

[Oliver Hartkopp]

On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
> This patch gives the variables holding the CAN receiver and the receiver
> list a better name by renaming them from "r to "rcv" and "rl" to
> "recv_list".

Comment not consistent to code.

> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

Indeed I would suggest 'rcv' and 'rcvlist'

> -		hlist_add_head_rcu(&r->list, rl);
> +		hlist_add_head_rcu(&rcv->list, rcv_list);

would lead to

 > +		hlist_add_head_rcu(&rcv->list, rcvlist);

which looks better to me.

Regards,
Oliver

Re: [PATCH 07/14] can: af_can: give variable holding the CAN receiver and the receiver list a sensible name

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 3330 bytes --]

On 08/24/2017 03:27 PM, Oliver Hartkopp wrote:
> 
> 
> On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
>> This patch gives the variables holding the CAN receiver and the receiver
>> list a better name by renaming them from "r to "rcv" and "rl" to
>> "recv_list".
> 
> Comment not consistent to code.
> 
>>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> Indeed I would suggest 'rcv' and 'rcvlist'
> 
>> -		hlist_add_head_rcu(&r->list, rl);
>> +		hlist_add_head_rcu(&rcv->list, rcv_list);
> 
> would lead to
> 
>  > +		hlist_add_head_rcu(&rcv->list, rcvlist);
> 
> which looks better to me.

We have the spelling with an underscore already in the code:

> diff --git a/include/net/netns/can.h b/include/net/netns/can.h                                                                                                                                                                                                                            
> index b106e6ae2e5b..1fcd395ed506 100644                                                                                                                                                                                                                                                   
> --- a/include/net/netns/can.h                                                                                                                                                                                                                                                             
> +++ b/include/net/netns/can.h                                                                                                                                                                                                                                                             
> @@ -9,6 +9,6 @@                                                                                                                                                                                                                                                                           
> 
>  struct dev_rcv_lists;
> -struct s_stats;                                                                                                                                                                                                                                                                          
> -struct s_pstats;                                                                                                                                                                                                                                                                         
> +struct can_pkg_stats;                                                                                                                                                                                                                                                                    
> +struct can_rcv_lists_stats; 

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 08/14] can: af_can: can_rx_register(): use max() instead of open coding it

[Marc Kleine-Budde]

This patch replaces an open coded max by the proper kernel define max().

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/af_can.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index cb079d2dcde2..e3df12cd2cae 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -498,8 +498,8 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 		dev_rcv_lists->entries++;
 
 		can_pstats->rcv_entries++;
-		if (can_pstats->rcv_entries_max < can_pstats->rcv_entries)
-			can_pstats->rcv_entries_max = can_pstats->rcv_entries;
+		can_pstats->rcv_entries_max = max(can_pstats->rcv_entries_max,
+						  can_pstats->rcv_entries);
 	} else {
 		kmem_cache_free(rcv_cache, rcv);
 		err = -ENODEV;
-- 
2.13.2

Re: [PATCH 08/14] can: af_can: can_rx_register(): use max() instead of open coding it

[Oliver Hartkopp]

On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
> This patch replaces an open coded max by the proper kernel define max().
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

Acked-by: Oliver Hartkopp <soketcan@hartkopp.net>

> ---
>   net/can/af_can.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index cb079d2dcde2..e3df12cd2cae 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -498,8 +498,8 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
>   		dev_rcv_lists->entries++;
>   
>   		can_pstats->rcv_entries++;
> -		if (can_pstats->rcv_entries_max < can_pstats->rcv_entries)
> -			can_pstats->rcv_entries_max = can_pstats->rcv_entries;
> +		can_pstats->rcv_entries_max = max(can_pstats->rcv_entries_max,
> +						  can_pstats->rcv_entries);
>   	} else {
>   		kmem_cache_free(rcv_cache, rcv);
>   		err = -ENODEV;
> 

Re: [PATCH 08/14] can: af_can: can_rx_register(): use max() instead of open coding it

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 595 bytes --]

On 08/24/2017 03:28 PM, Oliver Hartkopp wrote:
> 
> 
> On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
>> This patch replaces an open coded max by the proper kernel define max().
>>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> Acked-by: Oliver Hartkopp <soketcan@hartkopp.net>

Tnx,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 09/14] can: raw: use struct sock::sk_bound_dev_if instead of struct raw_sock::ifindex

[Marc Kleine-Budde]

This patch removes the struct raw_sock::ifindex from the CAN raw socket
and uses the already existing sock::sk_bound_dev_if instead.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/raw.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/net/can/raw.c b/net/can/raw.c
index 864c80dbdb72..80a1545bf51a 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -83,7 +83,6 @@ struct uniqframe {
 struct raw_sock {
 	struct sock sk;
 	int bound;
-	int ifindex;
 	struct notifier_block notifier;
 	int loopback;
 	int recv_own_msgs;
@@ -279,7 +278,7 @@ static int raw_notifier(struct notifier_block *nb,
 	if (dev->type != ARPHRD_CAN)
 		return NOTIFY_DONE;
 
-	if (ro->ifindex != dev->ifindex)
+	if (sk->sk_bound_dev_if != dev->ifindex)
 		return NOTIFY_DONE;
 
 	switch (msg) {
@@ -293,7 +292,7 @@ static int raw_notifier(struct notifier_block *nb,
 		if (ro->count > 1)
 			kfree(ro->filter);
 
-		ro->ifindex = 0;
+		sk->sk_bound_dev_if = 0;
 		ro->bound   = 0;
 		ro->count   = 0;
 		release_sock(sk);
@@ -318,7 +317,6 @@ static int raw_init(struct sock *sk)
 	struct raw_sock *ro = raw_sk(sk);
 
 	ro->bound            = 0;
-	ro->ifindex          = 0;
 
 	/* set default filter to single entry dfilter */
 	ro->dfilter.can_id   = 0;
@@ -361,10 +359,10 @@ static int raw_release(struct socket *sock)
 
 	/* remove current filters & unregister */
 	if (ro->bound) {
-		if (ro->ifindex) {
+		if (sk->sk_bound_dev_if) {
 			struct net_device *dev;
 
-			dev = dev_get_by_index(sock_net(sk), ro->ifindex);
+			dev = dev_get_by_index(sock_net(sk), sk->sk_bound_dev_if);
 			if (dev) {
 				raw_disable_allfilters(dev_net(dev), dev, sk);
 				dev_put(dev);
@@ -376,7 +374,7 @@ static int raw_release(struct socket *sock)
 	if (ro->count > 1)
 		kfree(ro->filter);
 
-	ro->ifindex = 0;
+	sk->sk_bound_dev_if = 0;
 	ro->bound   = 0;
 	ro->count   = 0;
 	free_percpu(ro->uniq);
@@ -404,7 +402,7 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
 
 	lock_sock(sk);
 
-	if (ro->bound && addr->can_ifindex == ro->ifindex)
+	if (ro->bound && addr->can_ifindex == sk->sk_bound_dev_if)
 		goto out;
 
 	if (addr->can_ifindex) {
@@ -438,11 +436,11 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
 	if (!err) {
 		if (ro->bound) {
 			/* unregister old filters */
-			if (ro->ifindex) {
+			if (sk->sk_bound_dev_if) {
 				struct net_device *dev;
 
 				dev = dev_get_by_index(sock_net(sk),
-						       ro->ifindex);
+						       sk->sk_bound_dev_if);
 				if (dev) {
 					raw_disable_allfilters(dev_net(dev),
 							       dev, sk);
@@ -451,7 +449,7 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
 			} else
 				raw_disable_allfilters(sock_net(sk), NULL, sk);
 		}
-		ro->ifindex = ifindex;
+		sk->sk_bound_dev_if = ifindex;
 		ro->bound = 1;
 	}
 
@@ -472,14 +470,13 @@ static int raw_getname(struct socket *sock, struct sockaddr *uaddr,
 {
 	struct sockaddr_can *addr = (struct sockaddr_can *)uaddr;
 	struct sock *sk = sock->sk;
-	struct raw_sock *ro = raw_sk(sk);
 
 	if (peer)
 		return -EOPNOTSUPP;
 
 	memset(addr, 0, sizeof(*addr));
 	addr->can_family  = AF_CAN;
-	addr->can_ifindex = ro->ifindex;
+	addr->can_ifindex = sk->sk_bound_dev_if;
 
 	*len = sizeof(*addr);
 
@@ -524,8 +521,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 
 		lock_sock(sk);
 
-		if (ro->bound && ro->ifindex)
-			dev = dev_get_by_index(sock_net(sk), ro->ifindex);
+		if (ro->bound && sk->sk_bound_dev_if)
+			dev = dev_get_by_index(sock_net(sk), sk->sk_bound_dev_if);
 
 		if (ro->bound) {
 			/* (try to) register the new filters */
@@ -578,8 +575,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 
 		lock_sock(sk);
 
-		if (ro->bound && ro->ifindex)
-			dev = dev_get_by_index(sock_net(sk), ro->ifindex);
+		if (ro->bound && sk->sk_bound_dev_if)
+			dev = dev_get_by_index(sock_net(sk), sk->sk_bound_dev_if);
 
 		/* remove current error mask */
 		if (ro->bound) {
@@ -743,7 +740,7 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 
 		ifindex = addr->can_ifindex;
 	} else
-		ifindex = ro->ifindex;
+		ifindex = sk->sk_bound_dev_if;
 
 	if (ro->fd_frames) {
 		if (unlikely(size != CANFD_MTU && size != CAN_MTU))
-- 
2.13.2

Re: [PATCH 09/14] can: raw: use struct sock::sk_bound_dev_if instead of struct raw_sock::ifindex

[Oliver Hartkopp]

On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
> This patch removes the struct raw_sock::ifindex from the CAN raw socket
> and uses the already existing sock::sk_bound_dev_if instead.

I would like to omit this patch.

1. It does NOT increase the readability ;-)

ifindex points out that we are handling an interface index.
sk_bound_dev_if could be anything.

2. I have bad feelings on using data structures from the general 
networking code as I had various discussions with e.g. iif in struct 
netdevice so that we needed to store our CAN specific incoming interface 
in a separated space before skb->data.

I would skip that patch for these reasons.

Regards,
Oliver

> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>   net/can/raw.c | 33 +++++++++++++++------------------
>   1 file changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/net/can/raw.c b/net/can/raw.c
> index 864c80dbdb72..80a1545bf51a 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -83,7 +83,6 @@ struct uniqframe {
>   struct raw_sock {
>   	struct sock sk;
>   	int bound;
> -	int ifindex;
>   	struct notifier_block notifier;
>   	int loopback;
>   	int recv_own_msgs;
> @@ -279,7 +278,7 @@ static int raw_notifier(struct notifier_block *nb,
>   	if (dev->type != ARPHRD_CAN)
>   		return NOTIFY_DONE;
>   
> -	if (ro->ifindex != dev->ifindex)
> +	if (sk->sk_bound_dev_if != dev->ifindex)
>   		return NOTIFY_DONE;
>   
>   	switch (msg) {
> @@ -293,7 +292,7 @@ static int raw_notifier(struct notifier_block *nb,
>   		if (ro->count > 1)
>   			kfree(ro->filter);
>   
> -		ro->ifindex = 0;
> +		sk->sk_bound_dev_if = 0;
>   		ro->bound   = 0;
>   		ro->count   = 0;
>   		release_sock(sk);
> @@ -318,7 +317,6 @@ static int raw_init(struct sock *sk)
>   	struct raw_sock *ro = raw_sk(sk);
>   
>   	ro->bound            = 0;
> -	ro->ifindex          = 0;
>   
>   	/* set default filter to single entry dfilter */
>   	ro->dfilter.can_id   = 0;
> @@ -361,10 +359,10 @@ static int raw_release(struct socket *sock)
>   
>   	/* remove current filters & unregister */
>   	if (ro->bound) {
> -		if (ro->ifindex) {
> +		if (sk->sk_bound_dev_if) {
>   			struct net_device *dev;
>   
> -			dev = dev_get_by_index(sock_net(sk), ro->ifindex);
> +			dev = dev_get_by_index(sock_net(sk), sk->sk_bound_dev_if);
>   			if (dev) {
>   				raw_disable_allfilters(dev_net(dev), dev, sk);
>   				dev_put(dev);
> @@ -376,7 +374,7 @@ static int raw_release(struct socket *sock)
>   	if (ro->count > 1)
>   		kfree(ro->filter);
>   
> -	ro->ifindex = 0;
> +	sk->sk_bound_dev_if = 0;
>   	ro->bound   = 0;
>   	ro->count   = 0;
>   	free_percpu(ro->uniq);
> @@ -404,7 +402,7 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
>   
>   	lock_sock(sk);
>   
> -	if (ro->bound && addr->can_ifindex == ro->ifindex)
> +	if (ro->bound && addr->can_ifindex == sk->sk_bound_dev_if)
>   		goto out;
>   
>   	if (addr->can_ifindex) {
> @@ -438,11 +436,11 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
>   	if (!err) {
>   		if (ro->bound) {
>   			/* unregister old filters */
> -			if (ro->ifindex) {
> +			if (sk->sk_bound_dev_if) {
>   				struct net_device *dev;
>   
>   				dev = dev_get_by_index(sock_net(sk),
> -						       ro->ifindex);
> +						       sk->sk_bound_dev_if);
>   				if (dev) {
>   					raw_disable_allfilters(dev_net(dev),
>   							       dev, sk);
> @@ -451,7 +449,7 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
>   			} else
>   				raw_disable_allfilters(sock_net(sk), NULL, sk);
>   		}
> -		ro->ifindex = ifindex;
> +		sk->sk_bound_dev_if = ifindex;
>   		ro->bound = 1;
>   	}
>   
> @@ -472,14 +470,13 @@ static int raw_getname(struct socket *sock, struct sockaddr *uaddr,
>   {
>   	struct sockaddr_can *addr = (struct sockaddr_can *)uaddr;
>   	struct sock *sk = sock->sk;
> -	struct raw_sock *ro = raw_sk(sk);
>   
>   	if (peer)
>   		return -EOPNOTSUPP;
>   
>   	memset(addr, 0, sizeof(*addr));
>   	addr->can_family  = AF_CAN;
> -	addr->can_ifindex = ro->ifindex;
> +	addr->can_ifindex = sk->sk_bound_dev_if;
>   
>   	*len = sizeof(*addr);
>   
> @@ -524,8 +521,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>   
>   		lock_sock(sk);
>   
> -		if (ro->bound && ro->ifindex)
> -			dev = dev_get_by_index(sock_net(sk), ro->ifindex);
> +		if (ro->bound && sk->sk_bound_dev_if)
> +			dev = dev_get_by_index(sock_net(sk), sk->sk_bound_dev_if);
>   
>   		if (ro->bound) {
>   			/* (try to) register the new filters */
> @@ -578,8 +575,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>   
>   		lock_sock(sk);
>   
> -		if (ro->bound && ro->ifindex)
> -			dev = dev_get_by_index(sock_net(sk), ro->ifindex);
> +		if (ro->bound && sk->sk_bound_dev_if)
> +			dev = dev_get_by_index(sock_net(sk), sk->sk_bound_dev_if);
>   
>   		/* remove current error mask */
>   		if (ro->bound) {
> @@ -743,7 +740,7 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>   
>   		ifindex = addr->can_ifindex;
>   	} else
> -		ifindex = ro->ifindex;
> +		ifindex = sk->sk_bound_dev_if;
>   
>   	if (ro->fd_frames) {
>   		if (unlikely(size != CANFD_MTU && size != CAN_MTU))
> 

Re: [PATCH 09/14] can: raw: use struct sock::sk_bound_dev_if instead of struct raw_sock::ifindex

[Kurt Van Dijck]

> >This patch removes the struct raw_sock::ifindex from the CAN raw socket
> >and uses the already existing sock::sk_bound_dev_if instead.
> 
> I would like to omit this patch.
> 
> 1. It does NOT increase the readability ;-)
> 
> ifindex points out that we are handling an interface index.
> sk_bound_dev_if could be anything.

See http://elixir.free-electrons.com/linux/v4.12.8/source/net/core/sock.c#L617
sk_bound_dev_if may be a difficult name, but could not be anything.
It allow to use SO_BINDTODEVICE (man 7 socket) to work properly for can raw sockets too.

> 
> 2. I have bad feelings on using data structures from the general networking
> code as I had various discussions with e.g. iif in struct netdevice so that
> we needed to store our CAN specific incoming interface in a separated space
> before skb->data.

sock::sk_bound_dev_if happens to be semantically identical to raw_sock::ifindex.
The grounds for discussions are IMHO very limited in this matter.
It also helps to check for the net device being a CAN device first.

Kind regards,
Kurt
> 
> Regards,
> Oliver
> 
> >
> >Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> >---
> >  net/can/raw.c | 33 +++++++++++++++------------------
> >  1 file changed, 15 insertions(+), 18 deletions(-)
> >
> >diff --git a/net/can/raw.c b/net/can/raw.c
> >index 864c80dbdb72..80a1545bf51a 100644
> >--- a/net/can/raw.c
> >+++ b/net/can/raw.c
> >@@ -83,7 +83,6 @@ struct uniqframe {
> >  struct raw_sock {
> >  	struct sock sk;
> >  	int bound;
> >-	int ifindex;
> >  	struct notifier_block notifier;
> >  	int loopback;
> >  	int recv_own_msgs;
> >@@ -279,7 +278,7 @@ static int raw_notifier(struct notifier_block *nb,
> >  	if (dev->type != ARPHRD_CAN)
> >  		return NOTIFY_DONE;
> >-	if (ro->ifindex != dev->ifindex)
> >+	if (sk->sk_bound_dev_if != dev->ifindex)
> >  		return NOTIFY_DONE;
> >  	switch (msg) {
> >@@ -293,7 +292,7 @@ static int raw_notifier(struct notifier_block *nb,
> >  		if (ro->count > 1)
> >  			kfree(ro->filter);
> >-		ro->ifindex = 0;
> >+		sk->sk_bound_dev_if = 0;
> >  		ro->bound   = 0;
> >  		ro->count   = 0;
> >  		release_sock(sk);
> >@@ -318,7 +317,6 @@ static int raw_init(struct sock *sk)
> >  	struct raw_sock *ro = raw_sk(sk);
> >  	ro->bound            = 0;
> >-	ro->ifindex          = 0;
> >  	/* set default filter to single entry dfilter */
> >  	ro->dfilter.can_id   = 0;
> >@@ -361,10 +359,10 @@ static int raw_release(struct socket *sock)
> >  	/* remove current filters & unregister */
> >  	if (ro->bound) {
> >-		if (ro->ifindex) {
> >+		if (sk->sk_bound_dev_if) {
> >  			struct net_device *dev;
> >-			dev = dev_get_by_index(sock_net(sk), ro->ifindex);
> >+			dev = dev_get_by_index(sock_net(sk), sk->sk_bound_dev_if);
> >  			if (dev) {
> >  				raw_disable_allfilters(dev_net(dev), dev, sk);
> >  				dev_put(dev);
> >@@ -376,7 +374,7 @@ static int raw_release(struct socket *sock)
> >  	if (ro->count > 1)
> >  		kfree(ro->filter);
> >-	ro->ifindex = 0;
> >+	sk->sk_bound_dev_if = 0;
> >  	ro->bound   = 0;
> >  	ro->count   = 0;
> >  	free_percpu(ro->uniq);
> >@@ -404,7 +402,7 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
> >  	lock_sock(sk);
> >-	if (ro->bound && addr->can_ifindex == ro->ifindex)
> >+	if (ro->bound && addr->can_ifindex == sk->sk_bound_dev_if)
> >  		goto out;
> >  	if (addr->can_ifindex) {
> >@@ -438,11 +436,11 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
> >  	if (!err) {
> >  		if (ro->bound) {
> >  			/* unregister old filters */
> >-			if (ro->ifindex) {
> >+			if (sk->sk_bound_dev_if) {
> >  				struct net_device *dev;
> >  				dev = dev_get_by_index(sock_net(sk),
> >-						       ro->ifindex);
> >+						       sk->sk_bound_dev_if);
> >  				if (dev) {
> >  					raw_disable_allfilters(dev_net(dev),
> >  							       dev, sk);
> >@@ -451,7 +449,7 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
> >  			} else
> >  				raw_disable_allfilters(sock_net(sk), NULL, sk);
> >  		}
> >-		ro->ifindex = ifindex;
> >+		sk->sk_bound_dev_if = ifindex;
> >  		ro->bound = 1;
> >  	}
> >@@ -472,14 +470,13 @@ static int raw_getname(struct socket *sock, struct sockaddr *uaddr,
> >  {
> >  	struct sockaddr_can *addr = (struct sockaddr_can *)uaddr;
> >  	struct sock *sk = sock->sk;
> >-	struct raw_sock *ro = raw_sk(sk);
> >  	if (peer)
> >  		return -EOPNOTSUPP;
> >  	memset(addr, 0, sizeof(*addr));
> >  	addr->can_family  = AF_CAN;
> >-	addr->can_ifindex = ro->ifindex;
> >+	addr->can_ifindex = sk->sk_bound_dev_if;
> >  	*len = sizeof(*addr);
> >@@ -524,8 +521,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
> >  		lock_sock(sk);
> >-		if (ro->bound && ro->ifindex)
> >-			dev = dev_get_by_index(sock_net(sk), ro->ifindex);
> >+		if (ro->bound && sk->sk_bound_dev_if)
> >+			dev = dev_get_by_index(sock_net(sk), sk->sk_bound_dev_if);
> >  		if (ro->bound) {
> >  			/* (try to) register the new filters */
> >@@ -578,8 +575,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
> >  		lock_sock(sk);
> >-		if (ro->bound && ro->ifindex)
> >-			dev = dev_get_by_index(sock_net(sk), ro->ifindex);
> >+		if (ro->bound && sk->sk_bound_dev_if)
> >+			dev = dev_get_by_index(sock_net(sk), sk->sk_bound_dev_if);
> >  		/* remove current error mask */
> >  		if (ro->bound) {
> >@@ -743,7 +740,7 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
> >  		ifindex = addr->can_ifindex;
> >  	} else
> >-		ifindex = ro->ifindex;
> >+		ifindex = sk->sk_bound_dev_if;
> >  	if (ro->fd_frames) {
> >  		if (unlikely(size != CANFD_MTU && size != CAN_MTU))
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/14] can: raw: use struct sock::sk_bound_dev_if instead of struct raw_sock::ifindex

[Oliver Hartkopp]

On 08/24/2017 04:11 PM, Kurt Van Dijck wrote:
>>> This patch removes the struct raw_sock::ifindex from the CAN raw socket
>>> and uses the already existing sock::sk_bound_dev_if instead.
>>
>> I would like to omit this patch.
>>
>> 1. It does NOT increase the readability ;-)
>>
>> ifindex points out that we are handling an interface index.
>> sk_bound_dev_if could be anything.
> 
> See http://elixir.free-electrons.com/linux/v4.12.8/source/net/core/sock.c#L617
> sk_bound_dev_if may be a difficult name, but could not be anything.
> It allow to use SO_BINDTODEVICE (man 7 socket) to work properly for can raw sockets too.

When ro->ifindex = 0 it can still be bound in the case of CAN sockets.
There is a different semantic as ifindex = zero means 'all CAN 
interfaces' and not 'no interface'.

So I would really wonder whether SO_BINDTODEVICE (man 7 socket) would 
ever work properly for can raw sockets too.

Thanks for the reference to sock.c - it just proved my guts feeling.

Regards,
Oliver

> 
>>
>> 2. I have bad feelings on using data structures from the general networking
>> code as I had various discussions with e.g. iif in struct netdevice so that
>> we needed to store our CAN specific incoming interface in a separated space
>> before skb->data.
> 
> sock::sk_bound_dev_if happens to be semantically identical to raw_sock::ifindex.
> The grounds for discussions are IMHO very limited in this matter.
> It also helps to check for the net device being a CAN device first.
> 
> Kind regards,
> Kurt
>>
>> Regards,
>> Oliver
>>
>>>
>>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>>> ---
>>>   net/can/raw.c | 33 +++++++++++++++------------------
>>>   1 file changed, 15 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/net/can/raw.c b/net/can/raw.c
>>> index 864c80dbdb72..80a1545bf51a 100644
>>> --- a/net/can/raw.c
>>> +++ b/net/can/raw.c
>>> @@ -83,7 +83,6 @@ struct uniqframe {
>>>   struct raw_sock {
>>>   	struct sock sk;
>>>   	int bound;
>>> -	int ifindex;
>>>   	struct notifier_block notifier;
>>>   	int loopback;
>>>   	int recv_own_msgs;
>>> @@ -279,7 +278,7 @@ static int raw_notifier(struct notifier_block *nb,
>>>   	if (dev->type != ARPHRD_CAN)
>>>   		return NOTIFY_DONE;
>>> -	if (ro->ifindex != dev->ifindex)
>>> +	if (sk->sk_bound_dev_if != dev->ifindex)
>>>   		return NOTIFY_DONE;
>>>   	switch (msg) {
>>> @@ -293,7 +292,7 @@ static int raw_notifier(struct notifier_block *nb,
>>>   		if (ro->count > 1)
>>>   			kfree(ro->filter);
>>> -		ro->ifindex = 0;
>>> +		sk->sk_bound_dev_if = 0;
>>>   		ro->bound   = 0;
>>>   		ro->count   = 0;
>>>   		release_sock(sk);
>>> @@ -318,7 +317,6 @@ static int raw_init(struct sock *sk)
>>>   	struct raw_sock *ro = raw_sk(sk);
>>>   	ro->bound            = 0;
>>> -	ro->ifindex          = 0;
>>>   	/* set default filter to single entry dfilter */
>>>   	ro->dfilter.can_id   = 0;
>>> @@ -361,10 +359,10 @@ static int raw_release(struct socket *sock)
>>>   	/* remove current filters & unregister */
>>>   	if (ro->bound) {
>>> -		if (ro->ifindex) {
>>> +		if (sk->sk_bound_dev_if) {
>>>   			struct net_device *dev;
>>> -			dev = dev_get_by_index(sock_net(sk), ro->ifindex);
>>> +			dev = dev_get_by_index(sock_net(sk), sk->sk_bound_dev_if);
>>>   			if (dev) {
>>>   				raw_disable_allfilters(dev_net(dev), dev, sk);
>>>   				dev_put(dev);
>>> @@ -376,7 +374,7 @@ static int raw_release(struct socket *sock)
>>>   	if (ro->count > 1)
>>>   		kfree(ro->filter);
>>> -	ro->ifindex = 0;
>>> +	sk->sk_bound_dev_if = 0;
>>>   	ro->bound   = 0;
>>>   	ro->count   = 0;
>>>   	free_percpu(ro->uniq);
>>> @@ -404,7 +402,7 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
>>>   	lock_sock(sk);
>>> -	if (ro->bound && addr->can_ifindex == ro->ifindex)
>>> +	if (ro->bound && addr->can_ifindex == sk->sk_bound_dev_if)
>>>   		goto out;
>>>   	if (addr->can_ifindex) {
>>> @@ -438,11 +436,11 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
>>>   	if (!err) {
>>>   		if (ro->bound) {
>>>   			/* unregister old filters */
>>> -			if (ro->ifindex) {
>>> +			if (sk->sk_bound_dev_if) {
>>>   				struct net_device *dev;
>>>   				dev = dev_get_by_index(sock_net(sk),
>>> -						       ro->ifindex);
>>> +						       sk->sk_bound_dev_if);
>>>   				if (dev) {
>>>   					raw_disable_allfilters(dev_net(dev),
>>>   							       dev, sk);
>>> @@ -451,7 +449,7 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
>>>   			} else
>>>   				raw_disable_allfilters(sock_net(sk), NULL, sk);
>>>   		}
>>> -		ro->ifindex = ifindex;
>>> +		sk->sk_bound_dev_if = ifindex;
>>>   		ro->bound = 1;
>>>   	}
>>> @@ -472,14 +470,13 @@ static int raw_getname(struct socket *sock, struct sockaddr *uaddr,
>>>   {
>>>   	struct sockaddr_can *addr = (struct sockaddr_can *)uaddr;
>>>   	struct sock *sk = sock->sk;
>>> -	struct raw_sock *ro = raw_sk(sk);
>>>   	if (peer)
>>>   		return -EOPNOTSUPP;
>>>   	memset(addr, 0, sizeof(*addr));
>>>   	addr->can_family  = AF_CAN;
>>> -	addr->can_ifindex = ro->ifindex;
>>> +	addr->can_ifindex = sk->sk_bound_dev_if;
>>>   	*len = sizeof(*addr);
>>> @@ -524,8 +521,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>>>   		lock_sock(sk);
>>> -		if (ro->bound && ro->ifindex)
>>> -			dev = dev_get_by_index(sock_net(sk), ro->ifindex);
>>> +		if (ro->bound && sk->sk_bound_dev_if)
>>> +			dev = dev_get_by_index(sock_net(sk), sk->sk_bound_dev_if);
>>>   		if (ro->bound) {
>>>   			/* (try to) register the new filters */
>>> @@ -578,8 +575,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>>>   		lock_sock(sk);
>>> -		if (ro->bound && ro->ifindex)
>>> -			dev = dev_get_by_index(sock_net(sk), ro->ifindex);
>>> +		if (ro->bound && sk->sk_bound_dev_if)
>>> +			dev = dev_get_by_index(sock_net(sk), sk->sk_bound_dev_if);
>>>   		/* remove current error mask */
>>>   		if (ro->bound) {
>>> @@ -743,7 +740,7 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>>>   		ifindex = addr->can_ifindex;
>>>   	} else
>>> -		ifindex = ro->ifindex;
>>> +		ifindex = sk->sk_bound_dev_if;
>>>   	if (ro->fd_frames) {
>>>   		if (unlikely(size != CANFD_MTU && size != CAN_MTU))
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-can" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/14] can: raw: use struct sock::sk_bound_dev_if instead of struct raw_sock::ifindex

[Kurt Van Dijck]

> On 08/24/2017 04:11 PM, Kurt Van Dijck wrote:
> >>>This patch removes the struct raw_sock::ifindex from the CAN raw socket
> >>>and uses the already existing sock::sk_bound_dev_if instead.
> >>
> >>I would like to omit this patch.
> >>
> >>1. It does NOT increase the readability ;-)
> >>
> >>ifindex points out that we are handling an interface index.
> >>sk_bound_dev_if could be anything.
> >
> >See http://elixir.free-electrons.com/linux/v4.12.8/source/net/core/sock.c#L617
> >sk_bound_dev_if may be a difficult name, but could not be anything.
> >It allow to use SO_BINDTODEVICE (man 7 socket) to work properly for can raw sockets too.
> 
> When ro->ifindex = 0 it can still be bound in the case of CAN sockets.
> There is a different semantic as ifindex = zero means 'all CAN interfaces'
> and not 'no interface'.

ro->ifindex = 0: accept traffic from all interfaces
ro->ifindex != 0: accept traffic only from 1 iface

sk_bound_dev_if = 0: accept traffic from all interfaces
sk_bound_dev_if != 0: accept traffic only from 1 iface

What is the semantic difference?

> 
> So I would really wonder whether SO_BINDTODEVICE (man 7 socket) would ever
> work properly for can raw sockets too.

I guess that it's up to us to make it work or not.
Due to the can filter design which is not tied to sockets, we must
implement it per socket type. Yet, AF_CAN has already defined the
functionality, so it shouldn't be too hard.

Regards,
Kurt

> 
> Thanks for the reference to sock.c - it just proved my guts feeling.
> 
> Regards,
> Oliver
> 
> >
> >>
> >>2. I have bad feelings on using data structures from the general networking
> >>code as I had various discussions with e.g. iif in struct netdevice so that
> >>we needed to store our CAN specific incoming interface in a separated space
> >>before skb->data.
> >
> >sock::sk_bound_dev_if happens to be semantically identical to raw_sock::ifindex.
> >The grounds for discussions are IMHO very limited in this matter.
> >It also helps to check for the net device being a CAN device first.
> >
> >Kind regards,
> >Kurt
> >>
> >>Regards,
> >>Oliver
> >>
> >>>
> >>>Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> >>>---
> >>>  net/can/raw.c | 33 +++++++++++++++------------------
> >>>  1 file changed, 15 insertions(+), 18 deletions(-)
> >>>
> >>>diff --git a/net/can/raw.c b/net/can/raw.c
> >>>index 864c80dbdb72..80a1545bf51a 100644
> >>>--- a/net/can/raw.c
> >>>+++ b/net/can/raw.c
> >>>@@ -83,7 +83,6 @@ struct uniqframe {
> >>>  struct raw_sock {
> >>>  	struct sock sk;
> >>>  	int bound;
> >>>-	int ifindex;
> >>>  	struct notifier_block notifier;
> >>>  	int loopback;
> >>>  	int recv_own_msgs;
> >>>@@ -279,7 +278,7 @@ static int raw_notifier(struct notifier_block *nb,
> >>>  	if (dev->type != ARPHRD_CAN)
> >>>  		return NOTIFY_DONE;
> >>>-	if (ro->ifindex != dev->ifindex)
> >>>+	if (sk->sk_bound_dev_if != dev->ifindex)
> >>>  		return NOTIFY_DONE;
> >>>  	switch (msg) {
> >>>@@ -293,7 +292,7 @@ static int raw_notifier(struct notifier_block *nb,
> >>>  		if (ro->count > 1)
> >>>  			kfree(ro->filter);
> >>>-		ro->ifindex = 0;
> >>>+		sk->sk_bound_dev_if = 0;
> >>>  		ro->bound   = 0;
> >>>  		ro->count   = 0;
> >>>  		release_sock(sk);
> >>>@@ -318,7 +317,6 @@ static int raw_init(struct sock *sk)
> >>>  	struct raw_sock *ro = raw_sk(sk);
> >>>  	ro->bound            = 0;
> >>>-	ro->ifindex          = 0;
> >>>  	/* set default filter to single entry dfilter */
> >>>  	ro->dfilter.can_id   = 0;
> >>>@@ -361,10 +359,10 @@ static int raw_release(struct socket *sock)
> >>>  	/* remove current filters & unregister */
> >>>  	if (ro->bound) {
> >>>-		if (ro->ifindex) {
> >>>+		if (sk->sk_bound_dev_if) {
> >>>  			struct net_device *dev;
> >>>-			dev = dev_get_by_index(sock_net(sk), ro->ifindex);
> >>>+			dev = dev_get_by_index(sock_net(sk), sk->sk_bound_dev_if);
> >>>  			if (dev) {
> >>>  				raw_disable_allfilters(dev_net(dev), dev, sk);
> >>>  				dev_put(dev);
> >>>@@ -376,7 +374,7 @@ static int raw_release(struct socket *sock)
> >>>  	if (ro->count > 1)
> >>>  		kfree(ro->filter);
> >>>-	ro->ifindex = 0;
> >>>+	sk->sk_bound_dev_if = 0;
> >>>  	ro->bound   = 0;
> >>>  	ro->count   = 0;
> >>>  	free_percpu(ro->uniq);
> >>>@@ -404,7 +402,7 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
> >>>  	lock_sock(sk);
> >>>-	if (ro->bound && addr->can_ifindex == ro->ifindex)
> >>>+	if (ro->bound && addr->can_ifindex == sk->sk_bound_dev_if)
> >>>  		goto out;
> >>>  	if (addr->can_ifindex) {
> >>>@@ -438,11 +436,11 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
> >>>  	if (!err) {
> >>>  		if (ro->bound) {
> >>>  			/* unregister old filters */
> >>>-			if (ro->ifindex) {
> >>>+			if (sk->sk_bound_dev_if) {
> >>>  				struct net_device *dev;
> >>>  				dev = dev_get_by_index(sock_net(sk),
> >>>-						       ro->ifindex);
> >>>+						       sk->sk_bound_dev_if);
> >>>  				if (dev) {
> >>>  					raw_disable_allfilters(dev_net(dev),
> >>>  							       dev, sk);
> >>>@@ -451,7 +449,7 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
> >>>  			} else
> >>>  				raw_disable_allfilters(sock_net(sk), NULL, sk);
> >>>  		}
> >>>-		ro->ifindex = ifindex;
> >>>+		sk->sk_bound_dev_if = ifindex;
> >>>  		ro->bound = 1;
> >>>  	}
> >>>@@ -472,14 +470,13 @@ static int raw_getname(struct socket *sock, struct sockaddr *uaddr,
> >>>  {
> >>>  	struct sockaddr_can *addr = (struct sockaddr_can *)uaddr;
> >>>  	struct sock *sk = sock->sk;
> >>>-	struct raw_sock *ro = raw_sk(sk);
> >>>  	if (peer)
> >>>  		return -EOPNOTSUPP;
> >>>  	memset(addr, 0, sizeof(*addr));
> >>>  	addr->can_family  = AF_CAN;
> >>>-	addr->can_ifindex = ro->ifindex;
> >>>+	addr->can_ifindex = sk->sk_bound_dev_if;
> >>>  	*len = sizeof(*addr);
> >>>@@ -524,8 +521,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
> >>>  		lock_sock(sk);
> >>>-		if (ro->bound && ro->ifindex)
> >>>-			dev = dev_get_by_index(sock_net(sk), ro->ifindex);
> >>>+		if (ro->bound && sk->sk_bound_dev_if)
> >>>+			dev = dev_get_by_index(sock_net(sk), sk->sk_bound_dev_if);
> >>>  		if (ro->bound) {
> >>>  			/* (try to) register the new filters */
> >>>@@ -578,8 +575,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
> >>>  		lock_sock(sk);
> >>>-		if (ro->bound && ro->ifindex)
> >>>-			dev = dev_get_by_index(sock_net(sk), ro->ifindex);
> >>>+		if (ro->bound && sk->sk_bound_dev_if)
> >>>+			dev = dev_get_by_index(sock_net(sk), sk->sk_bound_dev_if);
> >>>  		/* remove current error mask */
> >>>  		if (ro->bound) {
> >>>@@ -743,7 +740,7 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
> >>>  		ifindex = addr->can_ifindex;
> >>>  	} else
> >>>-		ifindex = ro->ifindex;
> >>>+		ifindex = sk->sk_bound_dev_if;
> >>>  	if (ro->fd_frames) {
> >>>  		if (unlikely(size != CANFD_MTU && size != CAN_MTU))
> >>>
> >>--
> >>To unsubscribe from this list: send the line "unsubscribe linux-can" in
> >>the body of a message to majordomo@vger.kernel.org
> >>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/14] can: raw: use struct sock::sk_bound_dev_if instead of struct raw_sock::ifindex

[Oliver Hartkopp]

On 08/25/2017 11:34 AM, Kurt Van Dijck wrote:
>> On 08/24/2017 04:11 PM, Kurt Van Dijck wrote:
>>>>> This patch removes the struct raw_sock::ifindex from the CAN raw socket
>>>>> and uses the already existing sock::sk_bound_dev_if instead.
>>>>
>>>> I would like to omit this patch.
>>>>
>>>> 1. It does NOT increase the readability ;-)
>>>>
>>>> ifindex points out that we are handling an interface index.
>>>> sk_bound_dev_if could be anything.
>>>
>>> See http://elixir.free-electrons.com/linux/v4.12.8/source/net/core/sock.c#L617
>>> sk_bound_dev_if may be a difficult name, but could not be anything.
>>> It allow to use SO_BINDTODEVICE (man 7 socket) to work properly for can raw sockets too.
>>
>> When ro->ifindex = 0 it can still be bound in the case of CAN sockets.
>> There is a different semantic as ifindex = zero means 'all CAN interfaces'
>> and not 'no interface'.
> 
> ro->ifindex = 0: accept traffic from all interfaces
> ro->ifindex != 0: accept traffic only from 1 iface
> 
> sk_bound_dev_if = 0: accept traffic from all interfaces
> sk_bound_dev_if != 0: accept traffic only from 1 iface
> 
> What is the semantic difference?

man 7 socket

SO_BINDTODEVICE

Bind this socket to a particular device like “eth0”, as specified in the 
passed interface name.  If the name  is  an empty  string  or the option 
length is zero, the socket device binding is removed.  The passed option 
is a variable-length null-terminated interface name string with the 
maximum size of IFNAMSIZ.  If a socket is bound to  an  interface,  only 
  packets received from that particular interface are processed by the 
socket. Note that this works only for some socket types, particularly 
AF_INET sockets.  It is not supported for packet  sockets  (use  normal 
bind(2) there).

Especially "If the name  is  an empty  string  or the option length is 
zero, the socket device binding is removed." is wrong.

A bound CAN_RAW socket can have ifindex = 0.

And "It is not supported for packet  sockets  (use  normal  bind(2) 
there)" - the CAN_RAW socket is based on PF_PACKET sockets with the 
device binding and addressing concept.

The more I look into this, the more I'm sure that we should leave it as-is.

Regards,
Oliver

Re: [PATCH 09/14] can: raw: use struct sock::sk_bound_dev_if instead of struct raw_sock::ifindex

[Kurt Van Dijck]

Hey Oliver,

> On 08/25/2017 11:34 AM, Kurt Van Dijck wrote:
> >>On 08/24/2017 04:11 PM, Kurt Van Dijck wrote:
> >>>>>This patch removes the struct raw_sock::ifindex from the CAN raw socket
> >>>>>and uses the already existing sock::sk_bound_dev_if instead.
> >>>>
> >>>>I would like to omit this patch.
> >>>>
> >>>>1. It does NOT increase the readability ;-)
> >>>>
> >>>>ifindex points out that we are handling an interface index.
> >>>>sk_bound_dev_if could be anything.
> >>>
> >>>See http://elixir.free-electrons.com/linux/v4.12.8/source/net/core/sock.c#L617
> >>>sk_bound_dev_if may be a difficult name, but could not be anything.
> >>>It allow to use SO_BINDTODEVICE (man 7 socket) to work properly for can raw sockets too.
> >>
> >>When ro->ifindex = 0 it can still be bound in the case of CAN sockets.
> >>There is a different semantic as ifindex = zero means 'all CAN interfaces'
> >>and not 'no interface'.
> >
> >ro->ifindex = 0: accept traffic from all interfaces
> >ro->ifindex != 0: accept traffic only from 1 iface
> >
> >sk_bound_dev_if = 0: accept traffic from all interfaces
> >sk_bound_dev_if != 0: accept traffic only from 1 iface
> >
> >What is the semantic difference?
> 
> man 7 socket
> 
> SO_BINDTODEVICE
> 
> Bind this socket to a particular device like “eth0”, as specified in the
> passed interface name.  If the name  is  an empty  string  or the option
> length is zero, the socket device binding is removed.  The passed option is
> a variable-length null-terminated interface name string with the maximum
> size of IFNAMSIZ.  If a socket is bound to  an  interface,  only  packets
> received from that particular interface are processed by the socket. Note
> that this works only for some socket types, particularly AF_INET sockets.
> It is not supported for packet  sockets  (use  normal bind(2) there).

A verbatim copy of the man page isn't helping. I have read it.

> 
> Especially "If the name  is  an empty  string  or the option length is zero,
> the socket device binding is removed." is wrong.

Do you mean the man page is wrong?
Or do you mean that the semantic difference is wrong?
Or ?

> 
> A bound CAN_RAW socket can have ifindex = 0.

I see no reason why a bound UDP socket can not have sk_bound_dev_if = 0.
I see no reason why a bound CAN_RAW socket can not either.
I start to think here that the word 'device binding' confuses you with
bind(2), but I have not seen any reference that SO_BINDTODEVICE
conflicts/interacts with bind() so far.

> 
> And "It is not supported for packet  sockets  (use  normal  bind(2) there)"
> - the CAN_RAW socket is based on PF_PACKET sockets with the device binding
> and addressing concept.
> 
> The more I look into this, the more I'm sure that we should leave it as-is.

> >>>>I would like to omit this patch.

Well, I feel it may block the others, so indeed, omitting may be the
best by lack of consensus.

Regards,
Kurt

Re: [PATCH 09/14] can: raw: use struct sock::sk_bound_dev_if instead of struct raw_sock::ifindex

[Oliver Hartkopp]

On 08/25/2017 08:46 PM, Kurt Van Dijck wrote:
> Hey Oliver,
> 
>> On 08/25/2017 11:34 AM, Kurt Van Dijck wrote:
>>>> On 08/24/2017 04:11 PM, Kurt Van Dijck wrote:
>>>>>>> This patch removes the struct raw_sock::ifindex from the CAN raw socket
>>>>>>> and uses the already existing sock::sk_bound_dev_if instead.
>>>>>>
>>>>>> I would like to omit this patch.
>>>>>>
>>>>>> 1. It does NOT increase the readability ;-)
>>>>>>
>>>>>> ifindex points out that we are handling an interface index.
>>>>>> sk_bound_dev_if could be anything.
>>>>>
>>>>> See http://elixir.free-electrons.com/linux/v4.12.8/source/net/core/sock.c#L617
>>>>> sk_bound_dev_if may be a difficult name, but could not be anything.
>>>>> It allow to use SO_BINDTODEVICE (man 7 socket) to work properly for can raw sockets too.
>>>>
>>>> When ro->ifindex = 0 it can still be bound in the case of CAN sockets.
>>>> There is a different semantic as ifindex = zero means 'all CAN interfaces'
>>>> and not 'no interface'.
>>>
>>> ro->ifindex = 0: accept traffic from all interfaces
>>> ro->ifindex != 0: accept traffic only from 1 iface
>>>
>>> sk_bound_dev_if = 0: accept traffic from all interfaces
>>> sk_bound_dev_if != 0: accept traffic only from 1 iface
>>>
>>> What is the semantic difference?
>>
>> man 7 socket
>>
>> SO_BINDTODEVICE
>>
>> Bind this socket to a particular device like “eth0”, as specified in the
>> passed interface name.  If the name  is  an empty  string  or the option
>> length is zero, the socket device binding is removed.  The passed option is
>> a variable-length null-terminated interface name string with the maximum
>> size of IFNAMSIZ.  If a socket is bound to  an  interface,  only  packets
>> received from that particular interface are processed by the socket. Note
>> that this works only for some socket types, particularly AF_INET sockets.
>> It is not supported for packet  sockets  (use  normal bind(2) there).
> 
> A verbatim copy of the man page isn't helping. I have read it.
> 
>>
>> Especially "If the name  is  an empty  string  or the option length is zero,
>> the socket device binding is removed." is wrong.
> 
> Do you mean the man page is wrong?
> Or do you mean that the semantic difference is wrong?

Yes. It tells us:

"If the name  is  an empty  string  or the option length is zero, the 
socket device binding is removed."

Which means the status of 'bind' is NOT decoupled from the number in 
ifindex.

A provided empty string leads to an unbound socket - and not to a socket 
bound to ifindex=0 which is intended with CAN sockets.

> Or ?
> 
>>
>> A bound CAN_RAW socket can have ifindex = 0.
> 
> I see no reason why a bound UDP socket can not have sk_bound_dev_if = 0.
> I see no reason why a bound CAN_RAW socket can not either.
> I start to think here that the word 'device binding' confuses you with
> bind(2), but I have not seen any reference that SO_BINDTODEVICE
> conflicts/interacts with bind() so far.
> 
>>
>> And "It is not supported for packet  sockets  (use  normal  bind(2) there)"
>> - the CAN_RAW socket is based on PF_PACKET sockets with the device binding
>> and addressing concept.
>>
>> The more I look into this, the more I'm sure that we should leave it as-is.
> 
>>>>>> I would like to omit this patch.
> 
> Well, I feel it may block the others, so indeed, omitting may be the
> best by lack of consensus.

Ok. We should omit this patch.

But I'm more interested in convincing you than just omitting this patch 
to not block the upstream process ;-)

Best regards,
Oliver

[PATCH 10/14] can: raw: raw_bind: bail out if can_family is not AF_CAN

[Marc Kleine-Budde]

Until now CAN raw's bind() doesn't check if the can_familiy in the
struct sockaddr_can is set to AF_CAN. This patch adds the missing check.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/raw.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/can/raw.c b/net/can/raw.c
index 80a1545bf51a..014874b11def 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -399,6 +399,8 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
 
 	if (len < sizeof(*addr))
 		return -EINVAL;
+	if (addr->can_family != AF_CAN)
+		return -EINVAL;
 
 	lock_sock(sk);
 
-- 
2.13.2

Re: [PATCH 10/14] can: raw: raw_bind: bail out if can_family is not AF_CAN

[Oliver Hartkopp]

On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
> Until now CAN raw's bind() doesn't check if the can_familiy in the
> struct sockaddr_can is set to AF_CAN. This patch adds the missing check.
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

Agreed :-)

Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

> ---
>   net/can/raw.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/net/can/raw.c b/net/can/raw.c
> index 80a1545bf51a..014874b11def 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -399,6 +399,8 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
>   
>   	if (len < sizeof(*addr))
>   		return -EINVAL;
> +	if (addr->can_family != AF_CAN)
> +		return -EINVAL;
>   
>   	lock_sock(sk);
>   
> 

Re: [PATCH 10/14] can: raw: raw_bind: bail out if can_family is not AF_CAN

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 687 bytes --]

On 08/24/2017 03:40 PM, Oliver Hartkopp wrote:
> 
> 
> On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
>> Until now CAN raw's bind() doesn't check if the can_familiy in the
>> struct sockaddr_can is set to AF_CAN. This patch adds the missing check.
>>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> Agreed :-)
> 
> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

tnx,

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 11/14] can: af_can: can_pernet_exit(): no need to iterate over and cleanup registered CAN devices

[Marc Kleine-Budde]

The networking core takes care and unregisters every network device in
a namespace before calling the can_pernet_exit() hook. This patch
removes the unneeded cleanup.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/af_can.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index e3df12cd2cae..d781b9330f2c 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -906,27 +906,12 @@ static int can_pernet_init(struct net *net)
 
 static void can_pernet_exit(struct net *net)
 {
-	struct net_device *dev;
-
 	if (IS_ENABLED(CONFIG_PROC_FS)) {
 		can_remove_proc(net);
 		if (stats_timer)
 			del_timer_sync(&net->can.can_stattimer);
 	}
 
-	/* remove created dev_rcv_lists from still registered CAN devices */
-	rcu_read_lock();
-	for_each_netdev_rcu(net, dev) {
-		if (dev->type == ARPHRD_CAN && dev->ml_priv) {
-			struct can_dev_rcv_lists *dev_rcv_lists = dev->ml_priv;
-
-			BUG_ON(dev_rcv_lists->entries);
-			kfree(dev_rcv_lists);
-			dev->ml_priv = NULL;
-		}
-	}
-	rcu_read_unlock();
-
 	kfree(net->can.can_rx_alldev_list);
 	kfree(net->can.can_stats);
 	kfree(net->can.can_pstats);
-- 
2.13.2

Re: [PATCH 11/14] can: af_can: can_pernet_exit(): no need to iterate over and cleanup registered CAN devices

[Oliver Hartkopp]

On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
> The networking core takes care and unregisters every network device in
> a namespace before calling the can_pernet_exit() hook. This patch
> removes the unneeded cleanup.
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

Huh.

IIRC this was introduced to make sure that the data structures are 
removed in the case one the af_can users (e.g. can-raw, can-bcm) crashes 
and does not remove its subscriptions for that reason.

So when unloading can.ko under these circumstances this code would just 
clean up the rubbish.

Not sure whether we would need to take care after your midlayer rework.

Regards,
Oliver

> ---
>   net/can/af_can.c | 15 ---------------
>   1 file changed, 15 deletions(-)
> 
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index e3df12cd2cae..d781b9330f2c 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -906,27 +906,12 @@ static int can_pernet_init(struct net *net)
>   
>   static void can_pernet_exit(struct net *net)
>   {
> -	struct net_device *dev;
> -
>   	if (IS_ENABLED(CONFIG_PROC_FS)) {
>   		can_remove_proc(net);
>   		if (stats_timer)
>   			del_timer_sync(&net->can.can_stattimer);
>   	}
>   
> -	/* remove created dev_rcv_lists from still registered CAN devices */
> -	rcu_read_lock();
> -	for_each_netdev_rcu(net, dev) {
> -		if (dev->type == ARPHRD_CAN && dev->ml_priv) {
> -			struct can_dev_rcv_lists *dev_rcv_lists = dev->ml_priv;
> -
> -			BUG_ON(dev_rcv_lists->entries);
> -			kfree(dev_rcv_lists);
> -			dev->ml_priv = NULL;
> -		}
> -	}
> -	rcu_read_unlock();
> -
>   	kfree(net->can.can_rx_alldev_list);
>   	kfree(net->can.can_stats);
>   	kfree(net->can.can_pstats);
> 

[PATCH 12/14] can: introduce CAN midlayer private and allocate it automatically

[Marc Kleine-Budde]

This patch introduces the CAN midlayer private structure ("struct
can_ml_priv") which should be used to hold protocol specific per device
data structures. For now it's only member is "struct can_dev_rcv_lists".

The CAN midlayer private is allocated via alloc_netdev()'s private and
assigned to "struct net_device::ml_priv" during device creation. This is
done transparently for CAN drivers using alloc_candev(). The slcan, vcan
and vxcan drivers which are not using alloc_candev() have been adopted
manually. The memory layout of the netdev_priv allocated via
alloc_candev() will looke like this:

  +-------------------------+
  | driver's priv           |
  +-------------------------+
  | struct can_ml_priv      |
  +-------------------------+
  | array of struct sk_buff |
  +-------------------------+

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev.c      | 22 ++++++++++++++++++----
 drivers/net/can/slcan.c    |  4 +++-
 drivers/net/can/vcan.c     |  6 ++++--
 drivers/net/can/vxcan.c    |  2 +-
 include/linux/can/can-ml.h | 23 +++++++++++++++++++++++
 5 files changed, 49 insertions(+), 8 deletions(-)
 create mode 100644 include/linux/can/can-ml.h

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 365a8cc62405..85a636760b98 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -23,6 +23,7 @@
 #include <linux/if_arp.h>
 #include <linux/workqueue.h>
 #include <linux/can.h>
+#include <linux/can/can-ml.h>
 #include <linux/can/dev.h>
 #include <linux/can/skb.h>
 #include <linux/can/netlink.h>
@@ -708,11 +709,24 @@ struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max)
 	struct can_priv *priv;
 	int size;
 
+	/* We put the driver's priv, the CAN mid layer priv and the
+	 * echo skb into the netdevice's priv. The memory layout for
+	 * the netdev_priv is like this:
+	 *
+	 * +-------------------------+
+	 * | driver's priv           |
+	 * +-------------------------+
+	 * | struct can_ml_priv      |
+	 * +-------------------------+
+	 * | array of struct sk_buff |
+	 * +-------------------------+
+	 */
+
+	size = ALIGN(sizeof_priv, NETDEV_ALIGN) + sizeof(struct can_ml_priv);
+
 	if (echo_skb_max)
-		size = ALIGN(sizeof_priv, sizeof(struct sk_buff *)) +
+		size = ALIGN(size, sizeof(struct sk_buff *)) +
 			echo_skb_max * sizeof(struct sk_buff *);
-	else
-		size = sizeof_priv;
 
 	dev = alloc_netdev(size, "can%d", NET_NAME_UNKNOWN, can_setup);
 	if (!dev)
@@ -724,7 +738,7 @@ struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max)
 	if (echo_skb_max) {
 		priv->echo_skb_max = echo_skb_max;
 		priv->echo_skb = (void *)priv +
-			ALIGN(sizeof_priv, sizeof(struct sk_buff *));
+			(size - echo_skb_max * sizeof(struct sk_buff *));
 	}
 
 	priv->state = CAN_STATE_STOPPED;
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 5d067c1b987f..f1647b4fd1e6 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -514,6 +514,7 @@ static struct slcan *slc_alloc(dev_t line)
 	char name[IFNAMSIZ];
 	struct net_device *dev = NULL;
 	struct slcan       *sl;
+	int size;
 
 	for (i = 0; i < maxdev; i++) {
 		dev = slcan_devs[i];
@@ -527,7 +528,8 @@ static struct slcan *slc_alloc(dev_t line)
 		return NULL;
 
 	sprintf(name, "slcan%d", i);
-	dev = alloc_netdev(sizeof(*sl), name, NET_NAME_UNKNOWN, slc_setup);
+	size = ALIGN(sizeof(*sl), NETDEV_ALIGN) + sizeof(struct can_ml_priv);
+	dev = alloc_netdev(size, name, NET_NAME_UNKNOWN, slc_setup);
 	if (!dev)
 		return NULL;
 
diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
index a8cb33264ff1..96e42eab2426 100644
--- a/drivers/net/can/vcan.c
+++ b/drivers/net/can/vcan.c
@@ -45,6 +45,7 @@
 #include <linux/if_arp.h>
 #include <linux/if_ether.h>
 #include <linux/can.h>
+#include <linux/can/can-ml.h>
 #include <linux/can/dev.h>
 #include <linux/can/skb.h>
 #include <linux/slab.h>
@@ -167,8 +168,9 @@ static void vcan_setup(struct net_device *dev)
 }
 
 static struct rtnl_link_ops vcan_link_ops __read_mostly = {
-	.kind	= DRV_NAME,
-	.setup	= vcan_setup,
+	.kind = DRV_NAME,
+	.priv_size = sizeof(struct can_ml_priv),
+	.setup = vcan_setup,
 };
 
 static __init int vcan_init_module(void)
diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index 8404e8852a0f..8925d797e8ac 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c
@@ -292,7 +292,7 @@ static struct net *vxcan_get_link_net(const struct net_device *dev)
 
 static struct rtnl_link_ops vxcan_link_ops = {
 	.kind		= DRV_NAME,
-	.priv_size	= sizeof(struct vxcan_priv),
+	.priv_size	= ALIGN(sizeof(struct vxcan_priv), NETDEV_ALIGN) + sizeof(struct can_ml_priv);
 	.setup		= vxcan_setup,
 	.newlink	= vxcan_newlink,
 	.dellink	= vxcan_dellink,
diff --git a/include/linux/can/can-ml.h b/include/linux/can/can-ml.h
new file mode 100644
index 000000000000..2786b04251ea
--- /dev/null
+++ b/include/linux/can/can-ml.h
@@ -0,0 +1,23 @@
+/*
+ * Copyright (C) 2017 Pengutronix, Marc Kleine-Budde <kernel@pengutronix.de>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * 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.
+ *
+ */
+#ifndef CAN_ML_H
+#define CAN_ML_H
+
+#include "../../net/can/af_can.h"
+
+struct can_ml_priv {
+	struct can_dev_rcv_lists dev_rcv_lists;
+};
+
+#endif /* CAN_ML_H */
-- 
2.13.2

Re: [PATCH 12/14] can: introduce CAN midlayer private and allocate it automatically

[Oliver Hartkopp]

Already commented this:

On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:

>   include/linux/can/can-ml.h | 23 +++++++++++++++++++++++

Need another name.


> diff --git a/include/linux/can/can-ml.h b/include/linux/can/can-ml.h
> new file mode 100644
> index 000000000000..2786b04251ea
> --- /dev/null
> +++ b/include/linux/can/can-ml.h
> @@ -0,0 +1,23 @@
> +/*
> + * Copyright (C) 2017 Pengutronix, Marc Kleine-Budde <kernel@pengutronix.de>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * 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.
> + *
> + */
> +#ifndef CAN_ML_H
> +#define CAN_ML_H
> +

Need some documentation about this weird include.

> +#include "../../net/can/af_can.h"
> +
> +struct can_ml_priv {
> +	struct can_dev_rcv_lists dev_rcv_lists;
> +};
> +
> +#endif /* CAN_ML_H */
> 

Regards,
Oliver

[PATCH 13/14] can: make use of preallocated can_ml_priv for per device struct can_dev_rcv_lists

[Marc Kleine-Budde]

This patch removes the old method of allocating the per device protocol
specific memory via a netdevice_notifier. This had the drawback, that
the allocation can fail, leading to a lot of null pointer checks in the
code. This also makes the live cycle management of this memory quite
complicated.

This patch switches from the allocating the struct can_dev_rcv_lists in
a NETDEV_REGISTER call to using the dev->ml_priv, which is allocated by
the driver since the previous patch.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev.c   |  2 ++
 drivers/net/can/slcan.c |  1 +
 drivers/net/can/vcan.c  |  1 +
 drivers/net/can/vxcan.c |  1 +
 net/can/af_can.c        | 45 ++++++++-------------------------------------
 net/can/af_can.h        |  1 -
 6 files changed, 13 insertions(+), 38 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 85a636760b98..89f9a05e97f0 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -735,6 +735,8 @@ struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max)
 	priv = netdev_priv(dev);
 	priv->dev = dev;
 
+	dev->ml_priv = (void *)priv + ALIGN(sizeof_priv, NETDEV_ALIGN);
+
 	if (echo_skb_max) {
 		priv->echo_skb_max = echo_skb_max;
 		priv->echo_skb = (void *)priv +
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index f1647b4fd1e6..38e82710848c 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -535,6 +535,7 @@ static struct slcan *slc_alloc(dev_t line)
 
 	dev->base_addr  = i;
 	sl = netdev_priv(dev);
+	dev->ml_priv = (void *)sl + ALIGN(sizeof(*sl), NETDEV_ALIGN);
 
 	/* Initialize channel control data */
 	sl->magic = SLCAN_MAGIC;
diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
index 96e42eab2426..ff388f036124 100644
--- a/drivers/net/can/vcan.c
+++ b/drivers/net/can/vcan.c
@@ -158,6 +158,7 @@ static void vcan_setup(struct net_device *dev)
 	dev->addr_len		= 0;
 	dev->tx_queue_len	= 0;
 	dev->flags		= IFF_NOARP;
+	dev->ml_priv		= netdev_priv(dev);
 
 	/* set flags according to driver capabilities */
 	if (echo)
diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index 8925d797e8ac..ed9293209ca2 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c
@@ -157,6 +157,7 @@ static void vxcan_setup(struct net_device *dev)
 	dev->flags		= (IFF_NOARP|IFF_ECHO);
 	dev->netdev_ops		= &vxcan_netdev_ops;
 	dev->needs_free_netdev	= true;
+	dev->ml_priv		= netdev_priv(dev) + ALIGN(sizeof(struct vxcan_priv), NETDEV_ALIGN);
 }
 
 /* forward declaration for rtnl_create_link() */
diff --git a/net/can/af_can.c b/net/can/af_can.c
index d781b9330f2c..c3eac4ab74e9 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -58,6 +58,7 @@
 #include <linux/can.h>
 #include <linux/can/core.h>
 #include <linux/can/skb.h>
+#include <linux/can/can-ml.h>
 #include <linux/ratelimit.h>
 #include <net/net_namespace.h>
 #include <net/sock.h>
@@ -324,10 +325,12 @@ EXPORT_SYMBOL(can_send);
 static struct can_dev_rcv_lists *can_dev_rcv_lists_find(struct net *net,
 							struct net_device *dev)
 {
-	if (!dev)
+	if (dev) {
+		struct can_ml_priv *ml_priv = dev->ml_priv;
+		return &ml_priv->dev_rcv_lists;
+	} else {
 		return net->can.can_rx_alldev_list;
-	else
-		return (struct can_dev_rcv_lists *)dev->ml_priv;
+	}
 }
 
 /**
@@ -589,12 +592,6 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	if (can_pstats->rcv_entries > 0)
 		can_pstats->rcv_entries--;
 
-	/* remove device structure requested by NETDEV_UNREGISTER */
-	if (dev_rcv_lists->remove_on_zero_entries && !dev_rcv_lists->entries) {
-		kfree(dev_rcv_lists);
-		dev->ml_priv = NULL;
-	}
-
  out:
 	spin_unlock(&net->can.can_rcvlists_lock);
 
@@ -827,7 +824,6 @@ static int can_notifier(struct notifier_block *nb, unsigned long msg,
 			void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
-	struct can_dev_rcv_lists *dev_rcv_lists;
 
 	if (dev->type != ARPHRD_CAN)
 		return NOTIFY_DONE;
@@ -835,33 +831,8 @@ static int can_notifier(struct notifier_block *nb, unsigned long msg,
 	switch (msg) {
 
 	case NETDEV_REGISTER:
-
-		/* create new dev_rcv_lists for this device */
-		dev_rcv_lists = kzalloc(sizeof(*dev_rcv_lists), GFP_KERNEL);
-		if (!dev_rcv_lists)
-			return NOTIFY_DONE;
-		BUG_ON(dev->ml_priv);
-		dev->ml_priv = dev_rcv_lists;
-
-		break;
-
-	case NETDEV_UNREGISTER:
-		spin_lock(&dev_net(dev)->can.can_rcvlists_lock);
-
-		dev_rcv_lists = dev->ml_priv;
-		if (dev_rcv_lists) {
-			if (dev_rcv_lists->entries)
-				dev_rcv_lists->remove_on_zero_entries = 1;
-			else {
-				kfree(dev_rcv_lists);
-				dev->ml_priv = NULL;
-			}
-		} else
-			pr_err("can: notifier: receive list not found for dev "
-			       "%s\n", dev->name);
-
-		spin_unlock(&dev_net(dev)->can.can_rcvlists_lock);
-
+		WARN(!dev->ml_priv,
+		     "No CAN mid layer private allocated, please fix your driver and use alloc_candev()!\n");
 		break;
 	}
 
diff --git a/net/can/af_can.h b/net/can/af_can.h
index 4b77c7951f17..dc198a07a8d1 100644
--- a/net/can/af_can.h
+++ b/net/can/af_can.h
@@ -71,7 +71,6 @@ struct can_dev_rcv_lists {
 	struct hlist_head rx[RX_MAX];
 	struct hlist_head rx_sff[CAN_SFF_RCV_ARRAY_SZ];
 	struct hlist_head rx_eff[CAN_EFF_RCV_ARRAY_SZ];
-	int remove_on_zero_entries;
 	int entries;
 };
 
-- 
2.13.2

Re: [PATCH 13/14] can: make use of preallocated can_ml_priv for per device struct can_dev_rcv_lists

[Oliver Hartkopp]

On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:

>   static struct can_dev_rcv_lists *can_dev_rcv_lists_find(struct net *net,
>   							struct net_device *dev)
>   {
> -	if (!dev)
> +	if (dev) {
> +		struct can_ml_priv *ml_priv = dev->ml_priv;
> +		return &ml_priv->dev_rcv_lists;
> +	} else {
>   		return net->can.can_rx_alldev_list;
> -	else
> -		return (struct can_dev_rcv_lists *)dev->ml_priv;
> +	}
>   }

I would not change the condition in the if-statement and the code order.

>   
>   /**
> @@ -589,12 +592,6 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
>   	if (can_pstats->rcv_entries > 0)
>   		can_pstats->rcv_entries--;
>   
> -	/* remove device structure requested by NETDEV_UNREGISTER */
> -	if (dev_rcv_lists->remove_on_zero_entries && !dev_rcv_lists->entries) {
> -		kfree(dev_rcv_lists);
> -		dev->ml_priv = NULL;
> -	}
> -
>    out:
>   	spin_unlock(&net->can.can_rcvlists_lock);
>   
> @@ -827,7 +824,6 @@ static int can_notifier(struct notifier_block *nb, unsigned long msg,
>   			void *ptr)
>   {
>   	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> -	struct can_dev_rcv_lists *dev_rcv_lists;
>   
>   	if (dev->type != ARPHRD_CAN)
>   		return NOTIFY_DONE;
> @@ -835,33 +831,8 @@ static int can_notifier(struct notifier_block *nb, unsigned long msg,
>   	switch (msg) {
>   
>   	case NETDEV_REGISTER:
> -
> -		/* create new dev_rcv_lists for this device */
> -		dev_rcv_lists = kzalloc(sizeof(*dev_rcv_lists), GFP_KERNEL);
> -		if (!dev_rcv_lists)
> -			return NOTIFY_DONE;
> -		BUG_ON(dev->ml_priv);
> -		dev->ml_priv = dev_rcv_lists;
> -
> -		break;
> -
> -	case NETDEV_UNREGISTER:
> -		spin_lock(&dev_net(dev)->can.can_rcvlists_lock);
> -
> -		dev_rcv_lists = dev->ml_priv;
> -		if (dev_rcv_lists) {
> -			if (dev_rcv_lists->entries)
> -				dev_rcv_lists->remove_on_zero_entries = 1;
> -			else {
> -				kfree(dev_rcv_lists);
> -				dev->ml_priv = NULL;
> -			}
> -		} else
> -			pr_err("can: notifier: receive list not found for dev "
> -			       "%s\n", dev->name);
> -
> -		spin_unlock(&dev_net(dev)->can.can_rcvlists_lock);
> -
> +		WARN(!dev->ml_priv,
> +		     "No CAN mid layer private allocated, please fix your driver and use alloc_candev()!\n");

Nice :-)

>   		break;
>   	}
>   
> diff --git a/net/can/af_can.h b/net/can/af_can.h
> index 4b77c7951f17..dc198a07a8d1 100644
> --- a/net/can/af_can.h
> +++ b/net/can/af_can.h
> @@ -71,7 +71,6 @@ struct can_dev_rcv_lists {
>   	struct hlist_head rx[RX_MAX];
>   	struct hlist_head rx_sff[CAN_SFF_RCV_ARRAY_SZ];
>   	struct hlist_head rx_eff[CAN_EFF_RCV_ARRAY_SZ];
> -	int remove_on_zero_entries;
>   	int entries;
>   };
>   
> 

[PATCH 14/14] can: af_can: remove NULL-ptr checks from users of can_dev_rcv_lists_find()

[Marc Kleine-Budde]

Since using the "struct can_ml_priv" for the per device "struct
dev_rcv_lists" the call can_dev_rcv_lists_find() cannot fail anymore.
This patch simplifies af_can by removing the NULL pointer checks from
the dev_rcv_lists returned by can_dev_rcv_lists_find().

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/af_can.c | 46 ++++++++++++++++------------------------------
 1 file changed, 16 insertions(+), 30 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index c3eac4ab74e9..f940030f797f 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -486,28 +486,22 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 	spin_lock(&net->can.can_rcvlists_lock);
 
 	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
-	if (dev_rcv_lists) {
-		rcv_list = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);
-
-		rcv->can_id = can_id;
-		rcv->mask = mask;
-		rcv->matches = 0;
-		rcv->func = func;
-		rcv->data = data;
-		rcv->ident = ident;
-		rcv->sk = sk;
-
-		hlist_add_head_rcu(&rcv->list, rcv_list);
-		dev_rcv_lists->entries++;
-
-		can_pstats->rcv_entries++;
-		can_pstats->rcv_entries_max = max(can_pstats->rcv_entries_max,
-						  can_pstats->rcv_entries);
-	} else {
-		kmem_cache_free(rcv_cache, rcv);
-		err = -ENODEV;
-	}
+	rcv_list = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);
+
+	rcv->can_id = can_id;
+	rcv->mask = mask;
+	rcv->matches = 0;
+	rcv->func = func;
+	rcv->data = data;
+	rcv->ident = ident;
+	rcv->sk = sk;
 
+	hlist_add_head_rcu(&rcv->list, rcv_list);
+	dev_rcv_lists->entries++;
+
+	can_pstats->rcv_entries++;
+	can_pstats->rcv_entries_max = max(can_pstats->rcv_entries_max,
+					  can_pstats->rcv_entries);
 	spin_unlock(&net->can.can_rcvlists_lock);
 
 	return err;
@@ -556,13 +550,6 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	spin_lock(&net->can.can_rcvlists_lock);
 
 	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
-	if (!dev_rcv_lists) {
-		pr_err("BUG: receive list not found for "
-		       "dev %s, id %03X, mask %03X\n",
-		       DNAME(dev), can_id, mask);
-		goto out;
-	}
-
 	rcv_list = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);
 
 	/*
@@ -697,8 +684,7 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
 
 	/* find receive list for this device */
 	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
-	if (dev_rcv_lists)
-		matches += can_rcv_filter(dev_rcv_lists, skb);
+	matches += can_rcv_filter(dev_rcv_lists, skb);
 
 	rcu_read_unlock();
 
-- 
2.13.2

Re: [PATCH 14/14] can: af_can: remove NULL-ptr checks from users of can_dev_rcv_lists_find()

[Oliver Hartkopp]

On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
> Since using the "struct can_ml_priv" for the per device "struct
> dev_rcv_lists" the call can_dev_rcv_lists_find() cannot fail anymore.
> This patch simplifies af_can by removing the NULL pointer checks from
> the dev_rcv_lists returned by can_dev_rcv_lists_find().
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

> ---
>   net/can/af_can.c | 46 ++++++++++++++++------------------------------
>   1 file changed, 16 insertions(+), 30 deletions(-)
> 
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index c3eac4ab74e9..f940030f797f 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -486,28 +486,22 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
>   	spin_lock(&net->can.can_rcvlists_lock);
>   
>   	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
> -	if (dev_rcv_lists) {
> -		rcv_list = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);
> -
> -		rcv->can_id = can_id;
> -		rcv->mask = mask;
> -		rcv->matches = 0;
> -		rcv->func = func;
> -		rcv->data = data;
> -		rcv->ident = ident;
> -		rcv->sk = sk;
> -
> -		hlist_add_head_rcu(&rcv->list, rcv_list);
> -		dev_rcv_lists->entries++;
> -
> -		can_pstats->rcv_entries++;
> -		can_pstats->rcv_entries_max = max(can_pstats->rcv_entries_max,
> -						  can_pstats->rcv_entries);
> -	} else {
> -		kmem_cache_free(rcv_cache, rcv);
> -		err = -ENODEV;
> -	}
> +	rcv_list = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);
> +
> +	rcv->can_id = can_id;
> +	rcv->mask = mask;
> +	rcv->matches = 0;
> +	rcv->func = func;
> +	rcv->data = data;
> +	rcv->ident = ident;
> +	rcv->sk = sk;
>   
> +	hlist_add_head_rcu(&rcv->list, rcv_list);
> +	dev_rcv_lists->entries++;
> +
> +	can_pstats->rcv_entries++;
> +	can_pstats->rcv_entries_max = max(can_pstats->rcv_entries_max,
> +					  can_pstats->rcv_entries);
>   	spin_unlock(&net->can.can_rcvlists_lock);
>   
>   	return err;
> @@ -556,13 +550,6 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
>   	spin_lock(&net->can.can_rcvlists_lock);
>   
>   	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
> -	if (!dev_rcv_lists) {
> -		pr_err("BUG: receive list not found for "
> -		       "dev %s, id %03X, mask %03X\n",
> -		       DNAME(dev), can_id, mask);
> -		goto out;
> -	}
> -
>   	rcv_list = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);
>   
>   	/*
> @@ -697,8 +684,7 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
>   
>   	/* find receive list for this device */
>   	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
> -	if (dev_rcv_lists)
> -		matches += can_rcv_filter(dev_rcv_lists, skb);
> +	matches += can_rcv_filter(dev_rcv_lists, skb);
>   
>   	rcu_read_unlock();
>   
> 

Re: [00/14] can: cleanup of af_can/raw + simplifying of ndev->ml_priv handling

[Oliver Hartkopp]

Hello Marc,

On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
> Hello,
> 
> while reviwing and cleaning up the j1939 stack I digged a bit deeper into
> af_can and raw implementation.
> 
> The first patch adds a missing error check and will probably go into -stable.

ack.
I'm currently working on an additional missing check for register of 
netdev notifier ...

> Patches 2-8 change some struct and variable names, making the code more
> readble, IMHO.

I'm not convinced on this part.
Where have you seen, that naming variables identical to the structs is 
common or better readable?

> Patch 9 removed the need for struct raw_sock::ifindex from the raw sock, by
> using struct sock::sk_bound_dev_if from the generic socket structure.

Have a bad feeling on that - will comment later.

> Patch 10: Checks if can_family is AF_CAN in raw's bind function.
fine.

> Patch 11: Cleans up the newly integrated CAN net namespace support.

need to review

> Patches 13-14: Where to put the per device protocol specific memory? af_can
> allocated it's memory during a netdev_notifier call, life cycle proves to be
> rather complicated (see remove_on_zero_entries, etc...), adding the j1939
> memory makes it even more compilcated. So I decided to allocate the memory
> during the allocation if net_device. And this seems to work. More details in
> the individual patches.

'Seems to work' sounds frightening. There was a racy reason to have that 
implementation as-is. Although this approach sounds interesting. Need to 
review too.

I'm currently pretty busy at work.
Please to not push these things without my ACK (as we had it with the 
namespace support where I crashed my easter holiday to fix/implement all 
the missing stuff to fit the merge window).

Thanks,
Oliver

Re: [00/14] can: cleanup of af_can/raw + simplifying of ndev->ml_priv handling

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 6489 bytes --]

On 08/03/2017 07:03 AM, Oliver Hartkopp wrote:
> Hello Marc,
> 
> On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
>> Hello,
>>
>> while reviwing and cleaning up the j1939 stack I digged a bit deeper into
>> af_can and raw implementation.
>>
>> The first patch adds a missing error check and will probably go into -stable.
> 
> ack.
> I'm currently working on an additional missing check for register of 
> netdev notifier ...
> 
>> Patches 2-8 change some struct and variable names, making the code more
>> readble, IMHO.
> 
> I'm not convinced on this part.
> Where have you seen, that naming variables identical to the structs is 
> common or better readable?

More readable than 1 and 2 character variable names at least :)

>> Patch 9 removed the need for struct raw_sock::ifindex from the raw sock, by
>> using struct sock::sk_bound_dev_if from the generic socket structure.
> 
> Have a bad feeling on that - will comment later.

Ok

>> Patch 10: Checks if can_family is AF_CAN in raw's bind function.
> fine.
> 
>> Patch 11: Cleans up the newly integrated CAN net namespace support.
> 
> need to review
> 
>> Patches 13-14: Where to put the per device protocol specific memory? af_can
>> allocated it's memory during a netdev_notifier call, life cycle proves to be
>> rather complicated (see remove_on_zero_entries, etc...), adding the j1939
>> memory makes it even more compilcated. So I decided to allocate the memory
>> during the allocation if net_device. And this seems to work. More details in
>> the individual patches.
> 
> 'Seems to work' sounds frightening. There was a racy reason to have that 
> implementation as-is. Although this approach sounds interesting. Need to 
> review too.

This is the callstack, when unplugging a USB device:

> [   53.144726] [<c0016cfc>] (unwind_backtrace) from [<c0013d04>] (show_stack+0x18/0x1c)
> [   53.152692] [<c0013d04>] (show_stack) from [<bf016510>] (can_rx_unregister+0x68/0x1c0 [can])
> [   53.161366] [<bf016510>] (can_rx_unregister [can]) from [<bf01f0c0>] (raw_disable_filters+0x44/0x60 [can_raw])
> [   53.171586] [<bf01f0c0>] (raw_disable_filters [can_raw]) from [<bf01f16c>] (raw_notifier+0x90/0x11c [can_raw])
> [   53.181799] [<bf01f16c>] (raw_notifier [can_raw]) from [<c0042000>] (notifier_call_chain+0x64/0xa4)
> [   53.191038] [<c0042000>] (notifier_call_chain) from [<c0042068>] (raw_notifier_call_chain+0x1c/0x24)
> [   53.200389] [<c0042068>] (raw_notifier_call_chain) from [<c03bc3c0>] (call_netdevice_notifiers+0x14/0x1c)
> [   53.210153] [<c03bc3c0>] (call_netdevice_notifiers) from [<c03bf634>] (rollback_registered_many+0x1a8/0x358)
> [   53.220180] [<c03bf634>] (rollback_registered_many) from [<c03bf82c>] (rollback_registered+0x48/0x68)
> [   53.229587] [<c03bf82c>] (rollback_registered) from [<c03bf8e0>] (unregister_netdevice_queue+0x94/0xb0)
> [   53.239280] [<c03bf8e0>] (unregister_netdevice_queue) from [<c03bf91c>] (unregister_netdev+0x20/0x28)
> [   53.248693] [<c03bf91c>] (unregister_netdev) from [<bf011268>] (gs_usb_disconnect+0x4c/0x7c [gs_usb])
> [   53.258112] [<bf011268>] (gs_usb_disconnect [gs_usb]) from [<c03515b0>] (usb_unbind_interface+0x80/0x1b4)
> [   53.267872] [<c03515b0>] (usb_unbind_interface) from [<c0324e00>] (device_release_driver_internal+0x124/0x1b4)
> [   53.278062] [<c0324e00>] (device_release_driver_internal) from [<c0323bb8>] (bus_remove_device+0xf0/0x124)
> [   53.287902] [<c0323bb8>] (bus_remove_device) from [<c03209b0>] (device_del+0x210/0x2c8)
> [   53.296089] [<c03209b0>] (device_del) from [<c035023c>] (usb_disable_device+0xa4/0x224)
> [   53.304275] [<c035023c>] (usb_disable_device) from [<c034826c>] (usb_disconnect+0x90/0x17c)
> [   53.312801] [<c034826c>] (usb_disconnect) from [<c0349000>] (hub_event+0x524/0xf74)
> [   53.320632] [<c0349000>] (hub_event) from [<c0039548>] (process_one_work+0x344/0x684)
> [   53.328653] [<c0039548>] (process_one_work) from [<c003ac7c>] (worker_thread+0x2b4/0x410)
> [   53.337013] [<c003ac7c>] (worker_thread) from [<c00407c8>] (kthread+0x134/0x154)
> [   53.344589] [<c00407c8>] (kthread) from [<c000fa6c>] (ret_from_fork+0x14/0x28)

The raw protocol is teared down within the unregister_netdev() via a
call_netdevice_notifiers()...

> [   53.423328] [<c0016cfc>] (unwind_backtrace) from [<c0013d04>] (show_stack+0x18/0x1c)
> [   53.434197] [<c0013d04>] (show_stack) from [<c03e4a5c>] (netdev_release+0x18/0x3c)
> [   53.445085] [<c03e4a5c>] (netdev_release) from [<c031fac8>] (device_release+0x64/0x9c)
> [   53.456348] [<c031fac8>] (device_release) from [<c048a904>] (kobject_put+0xd8/0x1d8)
> [   53.467330] [<c048a904>] (kobject_put) from [<bf011278>] (gs_usb_disconnect+0x5c/0x7c [gs_usb])
> [   53.479362] [<bf011278>] (gs_usb_disconnect [gs_usb]) from [<c03515b0>] (usb_unbind_interface+0x80/0x1b4)
> [   53.492137] [<c03515b0>] (usb_unbind_interface) from [<c0324e00>] (device_release_driver_internal+0x124/0x1b4)
> [   53.505308] [<c0324e00>] (device_release_driver_internal) from [<c0323bb8>] (bus_remove_device+0xf0/0x124)
> [   53.518221] [<c0323bb8>] (bus_remove_device) from [<c03209b0>] (device_del+0x210/0x2c8)
> [   53.529664] [<c03209b0>] (device_del) from [<c035023c>] (usb_disable_device+0xa4/0x224)
> [   53.540867] [<c035023c>] (usb_disable_device) from [<c034826c>] (usb_disconnect+0x90/0x17c)
> [   53.560981] [<c0349000>] (hub_event) from [<c0039548>] (process_one_work+0x344/0x684)
> [   53.569453] [<c0039548>] (process_one_work) from [<c003ac7c>] (worker_thread+0x2b4/0x410)
> [   53.578303] [<c003ac7c>] (worker_thread) from [<c00407c8>] (kthread+0x134/0x154)
> [   53.586337] [<c00407c8>] (kthread) from [<c000fa6c>] (ret_from_fork+0x14/0x28)

...while the netdev is finally discarded via the kobject_put later on.

> I'm currently pretty busy at work.

That's my world, too :)

> Please to not push these things without my ACK (as we had it with the 
> namespace support where I crashed my easter holiday to fix/implement all 
> the missing stuff to fit the merge window).

I'm on holidays too, SHA2017. If someone wants so meet me in person and
drink a mate or beer contact me.

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [00/14] can: cleanup of af_can/raw + simplifying of ndev->ml_priv handling

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1544 bytes --]

On 08/03/2017 07:03 AM, Oliver Hartkopp wrote:
>> Patch 9 removed the need for struct raw_sock::ifindex from the raw sock, by
>> using struct sock::sk_bound_dev_if from the generic socket structure.
> 
> Have a bad feeling on that - will comment later.

>> Patch 11: Cleans up the newly integrated CAN net namespace support.
> 
> need to review

>> Patches 13-14: Where to put the per device protocol specific memory? af_can
>> allocated it's memory during a netdev_notifier call, life cycle proves to be
>> rather complicated (see remove_on_zero_entries, etc...), adding the j1939
>> memory makes it even more compilcated. So I decided to allocate the memory
>> during the allocation if net_device. And this seems to work. More details in
>> the individual patches.
> 
> 'Seems to work' sounds frightening. There was a racy reason to have that 
> implementation as-is. Although this approach sounds interesting. Need to 
> review too.
> 
> I'm currently pretty busy at work.
> Please to not push these things without my ACK (as we had it with the 
> namespace support where I crashed my easter holiday to fix/implement all 
> the missing stuff to fit the merge window).

Have you found some time to look at these issues?

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [00/14] can: cleanup of af_can/raw + simplifying of ndev->ml_priv handling

[Oliver Hartkopp]

Hi Marc,

On 08/17/2017 01:57 PM, Marc Kleine-Budde wrote:

> 
> Have you found some time to look at these issues?
> 

I'm going through the code just now ;-)

One question:

All the midlayer private stuff was also done to cleanly separate the 
network layer stuff in net/can from the driver layer stuff in 
drivers/net/can.

Now you introduce a network layer data structure into the driver layer.
I'm not against this - and in fact I can see a bunch of advantages and 
simplifications as you intended.

But do you think it's a proper approach from the perspective of layer 
separation?

Best,
Oliver

Re: [00/14] can: cleanup of af_can/raw + simplifying of ndev->ml_priv handling

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1164 bytes --]

On 08/17/2017 02:57 PM, Oliver Hartkopp wrote:
> Hi Marc,
> 
> On 08/17/2017 01:57 PM, Marc Kleine-Budde wrote:
> 
>>
>> Have you found some time to look at these issues?
>>
> 
> I'm going through the code just now ;-)
> 
> One question:
> 
> All the midlayer private stuff was also done to cleanly separate the 
> network layer stuff in net/can from the driver layer stuff in 
> drivers/net/can.
> 
> Now you introduce a network layer data structure into the driver layer.
> I'm not against this - and in fact I can see a bunch of advantages and 
> simplifications as you intended.

> But do you think it's a proper approach from the perspective of layer 
> separation?

No. Another approach is keep the memory allocation in the
NETDEV_REGISTER hook, but refuse to start the netdev with NOTIFY_DONE.
Then we can skip all the NULL pointer checks, too.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [00/14] can: cleanup of af_can/raw + simplifying of ndev->ml_priv handling

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1235 bytes --]

On 08/17/2017 03:02 PM, Marc Kleine-Budde wrote:
> On 08/17/2017 02:57 PM, Oliver Hartkopp wrote:
>> Hi Marc,
>>
>> On 08/17/2017 01:57 PM, Marc Kleine-Budde wrote:
>>
>>>
>>> Have you found some time to look at these issues?
>>>
>>
>> I'm going through the code just now ;-)
>>
>> One question:
>>
>> All the midlayer private stuff was also done to cleanly separate the 
>> network layer stuff in net/can from the driver layer stuff in 
>> drivers/net/can.
>>
>> Now you introduce a network layer data structure into the driver layer.
>> I'm not against this - and in fact I can see a bunch of advantages and 
>> simplifications as you intended.
> 
>> But do you think it's a proper approach from the perspective of layer 
>> separation?
> 
> No. Another approach is keep the memory allocation in the
> NETDEV_REGISTER hook, but refuse to start the netdev with NOTIFY_DONE.

copy/paste confusion:
NOTIFY_STOP of course

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [00/14] can: cleanup of af_can/raw + simplifying of ndev->ml_priv handling

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1350 bytes --]

On 08/17/2017 03:03 PM, Marc Kleine-Budde wrote:
> On 08/17/2017 03:02 PM, Marc Kleine-Budde wrote:
>> On 08/17/2017 02:57 PM, Oliver Hartkopp wrote:
>>> Hi Marc,
>>>
>>> On 08/17/2017 01:57 PM, Marc Kleine-Budde wrote:
>>>
>>>>
>>>> Have you found some time to look at these issues?
>>>>
>>>
>>> I'm going through the code just now ;-)
>>>
>>> One question:
>>>
>>> All the midlayer private stuff was also done to cleanly separate the 
>>> network layer stuff in net/can from the driver layer stuff in 
>>> drivers/net/can.
>>>
>>> Now you introduce a network layer data structure into the driver layer.
>>> I'm not against this - and in fact I can see a bunch of advantages and 
>>> simplifications as you intended.
>>
>>> But do you think it's a proper approach from the perspective of layer 
>>> separation?
>>
>> No. Another approach is keep the memory allocation in the
>> NETDEV_REGISTER hook, but refuse to start the netdev with NOTIFY_DONE.
> 
> copy/paste confusion:
> NOTIFY_STOP of course

Or even use notifier_from_errno

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [00/14] can: cleanup of af_can/raw + simplifying of ndev->ml_priv handling

[Oliver Hartkopp]

On 08/17/2017 03:06 PM, Marc Kleine-Budde wrote:

>>> No. Another approach is keep the memory allocation in the
>>> NETDEV_REGISTER hook, but refuse to start the netdev with NOTIFY_DONE.
>>
>> copy/paste confusion:
>> NOTIFY_STOP of course
> 
> Or even use notifier_from_errno

I still have no idea what you mean :-(

With NETDEV_REGISTER the memory would be allocated. Ok.

An how would the memory be free'd then?

Regards,
Oliver

Re: [00/14] can: cleanup of af_can/raw + simplifying of ndev->ml_priv handling

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1126 bytes --]

On 08/17/2017 03:13 PM, Oliver Hartkopp wrote:
> 
> 
> On 08/17/2017 03:06 PM, Marc Kleine-Budde wrote:
> 
>>>> No. Another approach is keep the memory allocation in the
>>>> NETDEV_REGISTER hook, but refuse to start the netdev with NOTIFY_DONE.
>>>
>>> copy/paste confusion:
>>> NOTIFY_STOP of course
>>
>> Or even use notifier_from_errno
> 
> I still have no idea what you mean :-(

The code gets complicated, as during NETDEV_REGISTER the malloc may
fail. But we can cancel the register with return
notifier_from_errno(-ENOMEM);

> With NETDEV_REGISTER the memory would be allocated. Ok.
> 
> An how would the memory be free'd then?

NETDEV_UNREGISTER... But we have a problem, as j1939 needs per device
memory, too. Which is kfree()ed in a NETDEV_UNREGISTER hook, too. The
order of the hooks in not guaranteed.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 484 bytes --]

Re: [00/14] can: cleanup of af_can/raw + simplifying of ndev->ml_priv handling

[Oliver Hartkopp]

On 08/17/2017 03:17 PM, Marc Kleine-Budde wrote:

> The code gets complicated, as during NETDEV_REGISTER the malloc may
> fail. But we can cancel the register with return
> notifier_from_errno(-ENOMEM);

So this would lead to a netdevice completely failing to register then?
Nice!

>> With NETDEV_REGISTER the memory would be allocated. Ok.
>>
>> An how would the memory be free'd then?
> 
> NETDEV_UNREGISTER... But we have a problem, as j1939 needs per device
> memory, too. Which is kfree()ed in a NETDEV_UNREGISTER hook, too. The
> order of the hooks in not guaranteed.

Yes. That's the same problem I was working around with 
'd->remove_on_zero_entries' %-/

Just to get an impression about the j1939 data structures:

Would the per-device memory increase significantly when CAN_J1939 is 
enabled? Or is it some array of pointers which would be added?

Regards,
Oliver

Re: [00/14] can: cleanup of af_can/raw + simplifying of ndev->ml_priv handling

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1584 bytes --]

On 08/17/2017 03:54 PM, Oliver Hartkopp wrote:
> On 08/17/2017 03:17 PM, Marc Kleine-Budde wrote:
> 
>> The code gets complicated, as during NETDEV_REGISTER the malloc may
>> fail. But we can cancel the register with return
>> notifier_from_errno(-ENOMEM);
> 
> So this would lead to a netdevice completely failing to register then?
> Nice!
> 
>>> With NETDEV_REGISTER the memory would be allocated. Ok.
>>>
>>> An how would the memory be free'd then?
>>
>> NETDEV_UNREGISTER... But we have a problem, as j1939 needs per device
>> memory, too. Which is kfree()ed in a NETDEV_UNREGISTER hook, too. The
>> order of the hooks in not guaranteed.
> 
> Yes. That's the same problem I was working around with 
> 'd->remove_on_zero_entries' %-/

Using proper refcounting ...but it gets quite complicatd with j1939.

> Just to get an impression about the j1939 data structures:
> 
> Would the per-device memory increase significantly when CAN_J1939 is 
> enabled? Or is it some array of pointers which would be added?

An array with 256 members

> 	struct addr_ent {
> 		ktime_t rxtime;
> 		struct j1939_ecu *ecu;
> 		/* count users, to help transport protocol */
> 		int nusers;
> 	} ents[256];

and some more stuff. Sums up to sizeof(priv)=4184 Byte on armv5 (32bit).

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [00/14] can: cleanup of af_can/raw + simplifying of ndev->ml_priv handling

[Oliver Hartkopp]

Hi Marc,

On 08/17/2017 04:08 PM, Marc Kleine-Budde wrote:
> On 08/17/2017 03:54 PM, Oliver Hartkopp wrote:

> 
>> Just to get an impression about the j1939 data structures:
>>
>> Would the per-device memory increase significantly when CAN_J1939 is
>> enabled? Or is it some array of pointers which would be added?
> 
> An array with 256 members
> 
>> 	struct addr_ent {
>> 		ktime_t rxtime;
>> 		struct j1939_ecu *ecu;
>> 		/* count users, to help transport protocol */
>> 		int nusers;
>> 	} ents[256];
> 
> and some more stuff. Sums up to sizeof(priv)=4184 Byte on armv5 (32bit).

Ok - convinced :-)

Moving the filter and j1939 specific data structures into the CAN netdev 
private data space makes sense from the code simplicity and robustness 
perspective.

Although I'm not convinced about the renaming of data structures and 
variable names as it doesn't always increase the readability IMO.

Also include/linux/can/can-ml.h should be named differently, e.g.

  include/linux/can/mlpriv.h
  include/linux/can/midlayer.h
  include/linux/can/ml.h

And there should be some more documentation inside this include file 
that makes clear that this weird include path

#include "../../net/can/af_can.h"

is intended to store midlayer content in netdev data strctures.

I'll go through your data structure rename orgy and comment these 
changes patch by patch ;-)

Best regards,
Oliver

Re: [00/14] can: cleanup of af_can/raw + simplifying of ndev->ml_priv handling

[Oliver Hartkopp]

On 08/17/2017 03:02 PM, Marc Kleine-Budde wrote:
> On 08/17/2017 02:57 PM, Oliver Hartkopp wrote:


>> Now you introduce a network layer data structure into the driver layer.
>> I'm not against this - and in fact I can see a bunch of advantages and
>> simplifications as you intended.
> 
>> But do you think it's a proper approach from the perspective of layer
>> separation?
> 
> No. Another approach is keep the memory allocation in the
> NETDEV_REGISTER hook,

ok.

> but refuse to start the netdev with NOTIFY_DONE.

?? What does this mean ??

> Then we can skip all the NULL pointer checks, too.

Regards,
Oliver