Re: [PATCH 4/10] d80211: reduce mdev usage, fix ieee80211_rx_mgmt

2006-11-19 Thread David Kimdon
Reply-To: 
In-Reply-To: <[EMAIL PROTECTED]>


> 
> --- wireless-dev.orig/net/d80211/ieee80211.c  2006-11-17 20:01:54.999703408 
> +0100
> +++ wireless-dev/net/d80211/ieee80211.c   2006-11-17 20:01:55.659703408 
> +0100
> @@ -210,9 +210,16 @@ static void ieee80211_key_threshold_noti
>  struct ieee80211_key *key,
>  struct sta_info *sta)
>  {
> + struct ieee80211_local *local = dev->ieee80211_ptr;
>   struct sk_buff *skb;
>   struct ieee80211_msg_key_notification *msg;
>  
> + /* if no one will get it anyway, don't even allocate it.
> +  * unlikely because this is only relevant for APs
> +  * where the device must be open... */
> + if (unlikely(!local->apdev))
> + return;

Why not just let it Oops and show the bug?  In what cases is it ok
for apdev to not be set?

>   skb = dev_alloc_skb(sizeof(struct ieee80211_frame_info) +
>   sizeof(struct ieee80211_msg_key_notification));
>   if (!skb)


Can we get whitespace changes like this in a separate patch?

>  
> -jiffies_to_timespec(status->hosttime, &ts);
> + jiffies_to_timespec(status->hosttime, &ts);
>   fi->hosttime = cpu_to_be64((u64) ts.tv_sec * 100 +
>  ts.tv_nsec / 1000);
>   fi->mactime = cpu_to_be64(status->mactime);
>   switch (status->phymode) {
> -case MODE_IEEE80211A:
> -fi->phytype = htonl(ieee80211_phytype_ofdm_dot11_a);
> -break;
> -case MODE_IEEE80211B:
> -fi->phytype = htonl(ieee80211_phytype_dsss_dot11_b);
> -break;
> -case MODE_IEEE80211G:
> -fi->phytype = htonl(ieee80211_phytype_pbcc_dot11_g);
> -break;
> -case MODE_ATHEROS_TURBO:
> + case MODE_IEEE80211A:
> + fi->phytype = htonl(ieee80211_phytype_ofdm_dot11_a);
> + break;
> + case MODE_IEEE80211B:
> + fi->phytype = htonl(ieee80211_phytype_dsss_dot11_b);
> + break;
> + case MODE_IEEE80211G:
> + fi->phytype = htonl(ieee80211_phytype_pbcc_dot11_g);
> + break;
> + case MODE_ATHEROS_TURBO:
>   fi->phytype =
>   htonl(ieee80211_phytype_dsss_dot11_turbo);
>  break;
>  default:
> -fi->phytype = 0x;
> + fi->phytype = 0x;
>   break;
> -}
> -fi->channel = htonl(status->channel);
> + }
> + fi->channel = htonl(status->channel);


. . . 


>  int ieee80211_set_aid_for_sta(struct net_device *dev, u8 *peer_address,
> u16 aid)
>  {
> + struct ieee80211_local *local = dev->ieee80211_ptr;
>   struct sk_buff *skb;
>   struct ieee80211_msg_set_aid_for_sta *msg;
>  
> + /* unlikely because if this event only happens for APs,
> +  * which require an open ap device. */
> + if (unlikely(!local->apdev))
> + return 0;

again, so should we just let the Oops happen?

-David
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 4/10] d80211: reduce mdev usage, fix ieee80211_rx_mgmt

2006-11-19 Thread Johannes Berg

> > +   /* if no one will get it anyway, don't even allocate it.
> > +* unlikely because this is only relevant for APs
> > +* where the device must be open... */
> > +   if (unlikely(!local->apdev))
> > +   return;
> 
> Why not just let it Oops and show the bug?  In what cases is it ok
> for apdev to not be set?

> Can we get whitespace changes like this in a separate patch?

Sure. I'll just remove it from this patch for now.

> again, so should we just let the Oops happen?

I'm thinking there might be race conditions where userspace does
something, then drops the ap device but after that some operations
complete and would lead to the oops too. Wasn't really sure.

johannes


signature.asc
Description: This is a digitally signed message part