Re: [PATCH 1/3] compat-wireless: backport convert multicast list to list_head.

[Hauke Mehrtens <hauke@hauke-m.de>]

[-- Attachment #1: Type: text/plain, Size: 2265 bytes --]

Hi Pavel,

thank you for your comments on this.
Pavel Roskin wrote:
> Hello, Hauke!
> 
> Thank you for doing this effort!  I would prefer that we avoid patching
> as much as possible.  Patches tend to break as the code changes.  I
> think there are several cases where patching can be eliminated.
> 
>> ++#if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,35))
>> +     struct netdev_hw_addr *ha;
>> ++#else
>> ++    struct dev_mc_list *ha;
>> ++#endif
> 
> We could simply use this in some header:
> 
> #define dev_mc_list netdev_hw_addr
This will be possible, but as I see, only in some easy parts where the
patch looks like the quote above.
I will change that and resend this patch.
> 
>> +     /* comoute mc addresses' hash value ,and put it into hash table */
> 
> Someone had a bout of dyslexia, and it will be fixed, breaking the patch
> :-)
> 
>> +     netdev_for_each_mc_addr(ha, netdev) {
>> ++#if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,35))
>> +         hash_value = atl1c_hash_mc_addr(hw, ha->addr);
>> ++#else
>> ++        hash_value = atl1c_hash_mc_addr(hw, ha->dmi_addr);
>> ++#endif
> 
> #define addr dmi_addr
> 
> OK, this is likely to break if done in a header, but maybe it could be
> done in the C code away from the rest of the code.
I think the if defs are better than a conditional define in every file
that needs it. It will result in strange compile errors if someone uses
addr for an other addresses.
> 
>> ++#if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,35))
>> +     __hw_addr_unsync(&local->mc_list, &dev->mc, dev->addr_len);
>> ++#else
>> ++    __dev_addr_unsync(&local->mc_list, &local->mc_count,
>> ++              &dev->mc_list, &dev->mc_count);
>> ++#endif
> 
> Cannot we reimplement __hw_addr_unsync()?
This change depends on some changes structs in
net/mac80211/ieee80211_i.h I do not see a way to reimplement this.
> 
>> ++#if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,35))
>> +
>> +     __hw_addr_init(&local->mc_list);
>> +
>> ++#endif
> 
> That could be an empty function.
Yes that could be done, but I think it is much more clear to use this
patch here, because we do not need __hw_addr_init, because
local->mc_list is of a completely different type.

Hauke


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