Re: [tipc-discussion] [PATCH net-next v2 03/10] tipc: get rid of af_packet_priv usage when register TIPC protocol handler

[Erik H. <eri...@er...>]

On Thu, Aug 08, 2013 at 04:26:17PM +0800, Ying Xue wrote:
> [...]  
> -static int recv_notification(struct notifier_block *nb, unsigned long evt,
> -			     void *dv);
> -/*
> - * Network device notifier info
> - */
> -static struct notifier_block notifier = {
> -	.notifier_call	= recv_notification,
> -	.priority	= 0
> -};
> +static inline struct hlist_head *media_hashfn(int ifindex)
> +{
> +	return &media_hlist[ifindex & (NETDEV_HASHENTRIES - 1)];
> +}
> +
> +static struct eth_media *media_get(int ifindex)
> +{
> +	struct eth_media *eb_ptr;
> +	struct hlist_head *head = media_hashfn(ifindex);
> +
> +	hlist_for_each_entry(eb_ptr, head, hlist)
> +		if (eb_ptr->dev->ifindex == ifindex)
> +			return eb_ptr;
> +	return NULL;
> +}

The patch diff size can be reduced by moving these functions
before recv_notification.

> [...]
> @@ -155,10 +161,7 @@ static int recv_msg(struct sk_buff *buf, struct net_device *dev,
>   */
>  static void setup_media(struct work_struct *work)
>  {
> -	struct eth_media *eb_ptr =
> -		container_of(work, struct eth_media, setup);
> -
> -	dev_add_pack(&eb_ptr->tipc_packet_type);
> +	dev_add_pack(&tipc_packet_type);
>  }

I think this is wrong, we want to register tipc_packet_type handler once, not 
for each ethernet bearer enabled. I suggest this function is dropped, and that 
we call dev_add_pack immediately when the tipc module is loaded in 
eth_media_start instead.

After writing the comments for this, i see that it's actually done in the 
next patch. Not good to add code in one patch only to move/remove it in the
subsequent one. I suggest that this patch is merged with:
tipc: move registration of TIPC packet handler to media start

> [...] 
> @@ -217,9 +210,9 @@ static void cleanup_media(struct work_struct *work)
>  	struct eth_media *eb_ptr =
>  		container_of(work, struct eth_media, cleanup);
>  
> -	dev_remove_pack(&eb_ptr->tipc_packet_type);
> +	dev_remove_pack(&tipc_packet_type);
>  	dev_put(eb_ptr->dev);
> -	eb_ptr->dev = NULL;
> +	kfree(eb_ptr);
>  }

Same here, drop this function, free the bearer immediately in disable_media()
and call dev_remove_pack when we do eth_media_stop instead.

> [...]
> @@ -148,10 +164,7 @@ static int recv_msg(struct sk_buff *buf, struct net_device *dev,
>   */
>  static void setup_media(struct work_struct *work)
>  {
> -	struct ib_media *ib_ptr =
> -		container_of(work, struct ib_media, setup);
> -
> -	dev_add_pack(&ib_ptr->tipc_packet_type);
> +	dev_add_pack(&tipc_packet_type);
>  }

Same comment as for ethernet media/dev_add_pack.

> [...]
> @@ -210,9 +213,9 @@ static void cleanup_bearer(struct work_struct *work)
>  	struct ib_media *ib_ptr =
>  		container_of(work, struct ib_media, cleanup);
>  
> -	dev_remove_pack(&ib_ptr->tipc_packet_type);
> +	dev_remove_pack(&tipc_packet_type);
>  	dev_put(ib_ptr->dev);
> -	ib_ptr->dev = NULL;
> +	kfree(ib_ptr);
>  }

Same comment as for ethernet media/dev_remove_pack