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

[Ying X. <yin...@wi...>]

On 08/08/2013 05:16 PM, Erik Hugne wrote:
> 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. 

No, here is not wrong absolutely because we one tipc protocol handler is
registered when now each bearer/media is enabled. So when media/bearer
is disabled, protocol handler should be removed too.

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
> 

On one hand, the patch is already a bit big, but merging them will make
it more bigger; on another hand, most of patches in this series are not
final versions. When we involve RCU, we have to change many codes which
are added/modified by the series. In all, by my understanding this needs
more skills or even tricks about how to divide patch.

Additionally, there have no much differences between v1 and v2.
Beside changelogs being revised, what most of works are done in v2 is
that patches in v1 are reorganized again.

In conclusion, sometimes I am very confused about how to divide patches,
having reviewers more understandable for our made patches.

But, by my experience, smaller patches seem a little better :)

Regards,
Ying

>> [...] 
>> @@ -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
> 
> 
>