Re: [RFC] cfg80211: Android P2P-Device workaround

["Arend van Spriel" <arend@broadcom.com>]

On 04/23/2013 06:42 PM, Johannes Berg wrote:
> On Tue, 2013-04-23 at 07:50 -0700, Marcel Holtmann wrote:
>
>>> +config CFG80211_ANDROID_P2P_HACK
>>> +	bool "Android P2P netdevice hack"
>>> +	depends on CFG80211
>>> +	depends on !CFG80211_WEXT
>>> +	help
>>> +	  Enable this option for Android P2P w/ P2P Device.
>>> +
>>
>> do you really think this is a good idea? Wouldn't it be better if
>> Android just gets its userspace fixed. This way, they never will and
>> you carry this workaround forever.
>
> Well, I'm about 50/50 on whether to merge this. On the one hand, it's a
> very simple patch to carry out of tree, on the other hand more than one
> person will need it.
>

Here is one who needs it. I consider this patch as a temporary solution, 
but it would be good to have it upstream. We would be using 
compat-drivers to backport the upstream brcmfmac to the android kernel, 
which also takes care of cfg80211.

>> Has anybody actually looked into what it would take to just fix
>> Android userspace to use nl80211 to bring up P2P. I just see tons of
>> #ifdef added to the kernel.
>
> Yes, I did :-)
>
> However, for any system running Jelly Bean that's not really an option,
> and I have no idea if/how K-whatever is coming, so it might not be
> possible there either. Additionally, fairly large changes are needed in
> wpa_supplicant to actually support the P2P-Device with only nl82011
> APIs.

If android is moving to 3.8 kernel with KLP (key lime pie), you could 
say that it requires middleware/wifi-framework to deal with the new 
kernel API. However, it mostly involves wpa_supplicant regardless of 
using it on android or not. So this is more a wpa_supplicant P2P hack.

> This gives us a transition path while making changes to the other parts
> of the stack.

For 'selfish' reasons I am not opposed to this path ;-)

>>> +			if (!dev
>>> +#ifdef CONFIG_CFG80211_ANDROID_P2P_HACK
>>> +			    || (wdev->iftype == NL80211_IFTYPE_P2P_DEVICE &&
>>> +				info->genlhdr->cmd != NL80211_CMD_SET_INTERFACE)
>>> +#endif
>>> +			) {
>>> 				mutex_unlock(&cfg80211_mutex);
>>> 				if (rtnl)
>>> 					rtnl_unlock();
>>
>> And doing an #ifdef in the middle of if-clause doesn't ring any alarm
>> bells that this might be a really bad idea ;)
>
> Well, I could put an #ifdef into each and every command instead which I
> didn't want to, which incidentally is the reason for not allowing this
> with WEXT, WEXT ioctls on the P2P-Device netdev just cause crashes :)

I dislike these kind of constructs as well so I would like this to be 
made somewhat cleaner (conditional helper function?).

> Basically this way also reduces the surface of this patch to make it
> easier to use out of tree.
>
>> When I am looking at the USB gadget handling, I see a lot of progress
>> in establishing a standard kernel interface and changing Android
>> userspace to work with that instead.
>
> "progress" doesn't really help, at least in this case it's pretty much
> an either-or situation and changing the userspace will take a long time.

As long as changing the user-space is done eventually, right.

Gr. AvS