* Re: [net-next-2.6 PATCH] wireless: convert to use netdev_for_each_mc_addr [not found] ` <20100303200510.17531s9x98zyvwm8@hayate.sektori.org> @ 2010-03-03 18:09 ` Jiri Pirko 2010-03-04 8:41 ` David Miller 0 siblings, 1 reply; 5+ messages in thread From: Jiri Pirko @ 2010-03-03 18:09 UTC (permalink / raw) To: davem; +Cc: netdev, jussi.kivilinna, linux-wireless Wed, Mar 03, 2010 at 07:05:10PM CET, jussi.kivilinna@mbnet.fi wrote: >Quoting "Jiri Pirko" <jpirko@redhat.com>: > >>Wed, Mar 03, 2010 at 05:42:56PM CET, jussi.kivilinna@mbnet.fi wrote: >>>Hello! >>> >>>Quoting "Jiri Pirko" <jpirko@redhat.com>: >>> >>>>+ } else if (mc_count) { >>>>+ int size = min(priv->multicast_size, mc_count); >>>>+ int i = 0; >>>>+ >>>>+ mc_addrs = kmalloc(size * ETH_ALEN, GFP_ATOMIC); >>>... >>>>+ if (filter != basefilter) >>>>+ goto set_filter; >>>>+ >>>>+ if (mc_count) { >>>>+ ret = rndis_set_oid(usbdev, OID_802_3_MULTICAST_LIST, mc_addrs, >>>>+ mc_count * ETH_ALEN); >>>>+ kfree(mc_addrs); >>> >>>mc_addrs was alloced by with 'size * ETH_ALEN', which might be less >>>than mc_count * ETH_ALEN. >> >>Actually it cannot. That's covered by: >> >>if (mc_count > priv->multicast_size) { >> >>This was also in the original code. In that case "size" can be eliminated and >>"mc_addrs" can be allocated with "mc_count * ETH_ALEN". >> >>Jussi are you ok with this? >> >>Jirka > >Ah, you're right. Yes, 'size' can go away, it isn't needed after all. >I'm ok with this patch, I can fix 'size' to 'mc_count' myself later. Here's corrected patch: Subject: [net-2.6 PATCH] rndis_wlan: correct multicast_list handling V2 My previous patch (655ffee284dfcf9a24ac0343f3e5ee6db85b85c5) added locking in a bad way. Because rndis_set_oid can sleep, there is need to prepare multicast addresses into local buffer under netif_addr_lock first, then call rndis_set_oid outside. This caused reorganizing of the whole function. Signed-off-by: Jiri Pirko <jpirko@redhat.com> Reported-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi> diff --git a/drivers/net/wireless/rndis_wlan.c b/drivers/net/wireless/rndis_wlan.c index 9f6d6bf..07e6fdd 100644 --- a/drivers/net/wireless/rndis_wlan.c +++ b/drivers/net/wireless/rndis_wlan.c @@ -1496,23 +1496,37 @@ static void set_multicast_list(struct usbnet *usbdev) { struct rndis_wlan_private *priv = get_rndis_wlan_priv(usbdev); struct dev_mc_list *mclist; - __le32 filter; - int ret, i, size; - char *buf; + __le32 filter, basefilter; + int ret; + char *mc_addrs = NULL; + int mc_count; - filter = RNDIS_PACKET_TYPE_DIRECTED | RNDIS_PACKET_TYPE_BROADCAST; + basefilter = filter = RNDIS_PACKET_TYPE_DIRECTED | + RNDIS_PACKET_TYPE_BROADCAST; - netif_addr_lock_bh(usbdev->net); if (usbdev->net->flags & IFF_PROMISC) { filter |= RNDIS_PACKET_TYPE_PROMISCUOUS | RNDIS_PACKET_TYPE_ALL_LOCAL; - } else if (usbdev->net->flags & IFF_ALLMULTI || - netdev_mc_count(usbdev->net) > priv->multicast_size) { + } else if (usbdev->net->flags & IFF_ALLMULTI) { + filter |= RNDIS_PACKET_TYPE_ALL_MULTICAST; + } + + if (filter != basefilter) + goto set_filter; + + /* + * mc_list should be accessed holding the lock, so copy addresses to + * local buffer first. + */ + netif_addr_lock_bh(usbdev->net); + mc_count = netdev_mc_count(usbdev->net); + if (mc_count > priv->multicast_size) { filter |= RNDIS_PACKET_TYPE_ALL_MULTICAST; - } else if (!netdev_mc_empty(usbdev->net)) { - size = min(priv->multicast_size, netdev_mc_count(usbdev->net)); - buf = kmalloc(size * ETH_ALEN, GFP_KERNEL); - if (!buf) { + } else if (mc_count) { + int i = 0; + + mc_addrs = kmalloc(mc_count * ETH_ALEN, GFP_ATOMIC); + if (!mc_addrs) { netdev_warn(usbdev->net, "couldn't alloc %d bytes of memory\n", size * ETH_ALEN); @@ -1520,27 +1534,29 @@ static void set_multicast_list(struct usbnet *usbdev) return; } - i = 0; - netdev_for_each_mc_addr(mclist, usbdev->net) { - if (i == size) - break; - memcpy(buf + i++ * ETH_ALEN, mclist->dmi_addr, ETH_ALEN); - } + netdev_for_each_mc_addr(mclist, usbdev->net) + memcpy(mc_addrs + i++ * ETH_ALEN, + mclist->dmi_addr, ETH_ALEN); + } + netif_addr_unlock_bh(usbdev->net); - ret = rndis_set_oid(usbdev, OID_802_3_MULTICAST_LIST, buf, - i * ETH_ALEN); - if (ret == 0 && i > 0) + if (filter != basefilter) + goto set_filter; + + if (mc_count) { + ret = rndis_set_oid(usbdev, OID_802_3_MULTICAST_LIST, mc_addrs, + mc_count * ETH_ALEN); + kfree(mc_addrs); + if (ret == 0) filter |= RNDIS_PACKET_TYPE_MULTICAST; else filter |= RNDIS_PACKET_TYPE_ALL_MULTICAST; netdev_dbg(usbdev->net, "OID_802_3_MULTICAST_LIST(%d, max: %d) -> %d\n", - i, priv->multicast_size, ret); - - kfree(buf); + mc_count, priv->multicast_size, ret); } - netif_addr_unlock_bh(usbdev->net); +set_filter: ret = rndis_set_oid(usbdev, OID_GEN_CURRENT_PACKET_FILTER, &filter, sizeof(filter)); if (ret < 0) { ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [net-next-2.6 PATCH] wireless: convert to use netdev_for_each_mc_addr 2010-03-03 18:09 ` [net-next-2.6 PATCH] wireless: convert to use netdev_for_each_mc_addr Jiri Pirko @ 2010-03-04 8:41 ` David Miller 2010-03-04 8:52 ` David Miller 0 siblings, 1 reply; 5+ messages in thread From: David Miller @ 2010-03-04 8:41 UTC (permalink / raw) To: jpirko; +Cc: netdev, jussi.kivilinna, linux-wireless From: Jiri Pirko <jpirko@redhat.com> Date: Wed, 3 Mar 2010 19:09:56 +0100 > Here's corrected patch: > > Subject: [net-2.6 PATCH] rndis_wlan: correct multicast_list handling V2 > > My previous patch (655ffee284dfcf9a24ac0343f3e5ee6db85b85c5) added locking in > a bad way. Because rndis_set_oid can sleep, there is need to prepare multicast > addresses into local buffer under netif_addr_lock first, then call > rndis_set_oid outside. This caused reorganizing of the whole function. > > Signed-off-by: Jiri Pirko <jpirko@redhat.com> > Reported-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi> Applied. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [net-next-2.6 PATCH] wireless: convert to use netdev_for_each_mc_addr 2010-03-04 8:41 ` David Miller @ 2010-03-04 8:52 ` David Miller 2010-03-04 9:20 ` Jiri Pirko 0 siblings, 1 reply; 5+ messages in thread From: David Miller @ 2010-03-04 8:52 UTC (permalink / raw) To: jpirko; +Cc: netdev, jussi.kivilinna, linux-wireless From: David Miller <davem@davemloft.net> Date: Thu, 04 Mar 2010 00:41:50 -0800 (PST) > From: Jiri Pirko <jpirko@redhat.com> > Date: Wed, 3 Mar 2010 19:09:56 +0100 > >> Here's corrected patch: >> >> Subject: [net-2.6 PATCH] rndis_wlan: correct multicast_list handling V2 >> >> My previous patch (655ffee284dfcf9a24ac0343f3e5ee6db85b85c5) added locking in >> a bad way. Because rndis_set_oid can sleep, there is need to prepare multicast >> addresses into local buffer under netif_addr_lock first, then call >> rndis_set_oid outside. This caused reorganizing of the whole function. >> >> Signed-off-by: Jiri Pirko <jpirko@redhat.com> >> Reported-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi> > > Applied. Jiri did ou even type make on this patch? The 'size' is still referenced: drivers/net/wireless/rndis_wlan.c: In function 'set_multicast_list': drivers/net/wireless/rndis_wlan.c:1530: error: 'size' undeclared (first use in this function) drivers/net/wireless/rndis_wlan.c:1530: error: (Each undeclared identifier is reported only once drivers/net/wireless/rndis_wlan.c:1530: error: for each function it appears in.) I reverted, and I'm pissed off. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [net-next-2.6 PATCH] wireless: convert to use netdev_for_each_mc_addr 2010-03-04 8:52 ` David Miller @ 2010-03-04 9:20 ` Jiri Pirko 2010-03-04 11:38 ` David Miller 0 siblings, 1 reply; 5+ messages in thread From: Jiri Pirko @ 2010-03-04 9:20 UTC (permalink / raw) To: David Miller; +Cc: netdev, jussi.kivilinna, linux-wireless Thu, Mar 04, 2010 at 09:52:46AM CET, davem@davemloft.net wrote: >From: David Miller <davem@davemloft.net> >Date: Thu, 04 Mar 2010 00:41:50 -0800 (PST) > >> From: Jiri Pirko <jpirko@redhat.com> >> Date: Wed, 3 Mar 2010 19:09:56 +0100 >> >>> Here's corrected patch: >>> >>> Subject: [net-2.6 PATCH] rndis_wlan: correct multicast_list handling V2 >>> >>> My previous patch (655ffee284dfcf9a24ac0343f3e5ee6db85b85c5) added locking in >>> a bad way. Because rndis_set_oid can sleep, there is need to prepare multicast >>> addresses into local buffer under netif_addr_lock first, then call >>> rndis_set_oid outside. This caused reorganizing of the whole function. >>> >>> Signed-off-by: Jiri Pirko <jpirko@redhat.com> >>> Reported-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi> >> >> Applied. > >Jiri did ou even type make on this patch? The 'size' is still >referenced: > >drivers/net/wireless/rndis_wlan.c: In function 'set_multicast_list': >drivers/net/wireless/rndis_wlan.c:1530: error: 'size' undeclared (first use in this function) >drivers/net/wireless/rndis_wlan.c:1530: error: (Each undeclared identifier is reported only once >drivers/net/wireless/rndis_wlan.c:1530: error: for each function it appears in.) > >I reverted, and I'm pissed off. Slapping myself twice on each cheek. Subject: [net-2.6 PATCH] rndis_wlan: correct multicast_list handling V3 My previous patch (655ffee284dfcf9a24ac0343f3e5ee6db85b85c5) added locking in a bad way. Because rndis_set_oid can sleep, there is need to prepare multicast addresses into local buffer under netif_addr_lock first, then call rndis_set_oid outside. This caused reorganizing of the whole function. Signed-off-by: Jiri Pirko <jpirko@redhat.com> Reported-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi> diff --git a/drivers/net/wireless/rndis_wlan.c b/drivers/net/wireless/rndis_wlan.c index 9f6d6bf..2887047 100644 --- a/drivers/net/wireless/rndis_wlan.c +++ b/drivers/net/wireless/rndis_wlan.c @@ -1496,51 +1496,67 @@ static void set_multicast_list(struct usbnet *usbdev) { struct rndis_wlan_private *priv = get_rndis_wlan_priv(usbdev); struct dev_mc_list *mclist; - __le32 filter; - int ret, i, size; - char *buf; + __le32 filter, basefilter; + int ret; + char *mc_addrs = NULL; + int mc_count; - filter = RNDIS_PACKET_TYPE_DIRECTED | RNDIS_PACKET_TYPE_BROADCAST; + basefilter = filter = RNDIS_PACKET_TYPE_DIRECTED | + RNDIS_PACKET_TYPE_BROADCAST; - netif_addr_lock_bh(usbdev->net); if (usbdev->net->flags & IFF_PROMISC) { filter |= RNDIS_PACKET_TYPE_PROMISCUOUS | RNDIS_PACKET_TYPE_ALL_LOCAL; - } else if (usbdev->net->flags & IFF_ALLMULTI || - netdev_mc_count(usbdev->net) > priv->multicast_size) { + } else if (usbdev->net->flags & IFF_ALLMULTI) { + filter |= RNDIS_PACKET_TYPE_ALL_MULTICAST; + } + + if (filter != basefilter) + goto set_filter; + + /* + * mc_list should be accessed holding the lock, so copy addresses to + * local buffer first. + */ + netif_addr_lock_bh(usbdev->net); + mc_count = netdev_mc_count(usbdev->net); + if (mc_count > priv->multicast_size) { filter |= RNDIS_PACKET_TYPE_ALL_MULTICAST; - } else if (!netdev_mc_empty(usbdev->net)) { - size = min(priv->multicast_size, netdev_mc_count(usbdev->net)); - buf = kmalloc(size * ETH_ALEN, GFP_KERNEL); - if (!buf) { + } else if (mc_count) { + int i = 0; + + mc_addrs = kmalloc(mc_count * ETH_ALEN, GFP_ATOMIC); + if (!mc_addrs) { netdev_warn(usbdev->net, "couldn't alloc %d bytes of memory\n", - size * ETH_ALEN); + mc_count * ETH_ALEN); netif_addr_unlock_bh(usbdev->net); return; } - i = 0; - netdev_for_each_mc_addr(mclist, usbdev->net) { - if (i == size) - break; - memcpy(buf + i++ * ETH_ALEN, mclist->dmi_addr, ETH_ALEN); - } + netdev_for_each_mc_addr(mclist, usbdev->net) + memcpy(mc_addrs + i++ * ETH_ALEN, + mclist->dmi_addr, ETH_ALEN); + } + netif_addr_unlock_bh(usbdev->net); - ret = rndis_set_oid(usbdev, OID_802_3_MULTICAST_LIST, buf, - i * ETH_ALEN); - if (ret == 0 && i > 0) + if (filter != basefilter) + goto set_filter; + + if (mc_count) { + ret = rndis_set_oid(usbdev, OID_802_3_MULTICAST_LIST, mc_addrs, + mc_count * ETH_ALEN); + kfree(mc_addrs); + if (ret == 0) filter |= RNDIS_PACKET_TYPE_MULTICAST; else filter |= RNDIS_PACKET_TYPE_ALL_MULTICAST; netdev_dbg(usbdev->net, "OID_802_3_MULTICAST_LIST(%d, max: %d) -> %d\n", - i, priv->multicast_size, ret); - - kfree(buf); + mc_count, priv->multicast_size, ret); } - netif_addr_unlock_bh(usbdev->net); +set_filter: ret = rndis_set_oid(usbdev, OID_GEN_CURRENT_PACKET_FILTER, &filter, sizeof(filter)); if (ret < 0) { ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [net-next-2.6 PATCH] wireless: convert to use netdev_for_each_mc_addr 2010-03-04 9:20 ` Jiri Pirko @ 2010-03-04 11:38 ` David Miller 0 siblings, 0 replies; 5+ messages in thread From: David Miller @ 2010-03-04 11:38 UTC (permalink / raw) To: jpirko; +Cc: netdev, jussi.kivilinna, linux-wireless From: Jiri Pirko <jpirko@redhat.com> Date: Thu, 4 Mar 2010 10:20:14 +0100 > Slapping myself twice on each cheek. > > Subject: [net-2.6 PATCH] rndis_wlan: correct multicast_list handling V3 > > My previous patch (655ffee284dfcf9a24ac0343f3e5ee6db85b85c5) added locking in > a bad way. Because rndis_set_oid can sleep, there is need to prepare multicast > addresses into local buffer under netif_addr_lock first, then call > rndis_set_oid outside. This caused reorganizing of the whole function. > > Signed-off-by: Jiri Pirko <jpirko@redhat.com> > Reported-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi> Applied. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-03-04 11:37 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20100303114506.19575pbsu8yz2kjk@hayate.sektori.org>
[not found] ` <20100303124249.GD2961@psychotron.lab.eng.brq.redhat.com>
[not found] ` <20100303184256.19122kg5hcqolfgg@hayate.sektori.org>
[not found] ` <20100303175229.GB2760@psychotron.redhat.com>
[not found] ` <20100303200510.17531s9x98zyvwm8@hayate.sektori.org>
2010-03-03 18:09 ` [net-next-2.6 PATCH] wireless: convert to use netdev_for_each_mc_addr Jiri Pirko
2010-03-04 8:41 ` David Miller
2010-03-04 8:52 ` David Miller
2010-03-04 9:20 ` Jiri Pirko
2010-03-04 11:38 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox