On Tue, 2010-05-25 at 11:13 +0200, ext Johannes Berg wrote:
> On Tue, 2010-05-25 at 10:48 +0300, Juuso Oikarinen wrote:
>
> > +struct in_ifaddr;
> > +
>
> I think you should include a header file for that, or was there a reason
> you didn't?
Yeah, I'll add the header.
> > + * @configure_ip_filter: Configuration function for IP address based filters,
> > + * such as an ARP query filter. This function is called with all the IP
> > + * addresses configured to the interface as argument - all frames targeted
> > + * to any of these addresses should pass through.
>
> Huh ok I thought you wanted ARP filtering, not IP filtering. You need to
> be more explicit about what this should do. IP filtering is not useful
> since those frames will be unicast on the ethernet layer. I think this
> should be more focused on ARP filtering.
True, this is about ARP filtering. I was pondering naming it arp or IP,
then opted to go with IP just because I was thinking that there may be
some other frames we might want to filter based on IP in the future.
Maybe I'm thinking too far ahead ;)
I'll update the naming to be ARP all around.
> Additionally, it needs to be very explicit that it must filter ONLY ARP
> packets that do not match any of the given IP addresses. If, for
> example, the hardware can only handle 2 addresses, but you have 3 in the
> list, the filter must be turned off. Any packet that the device cannot
> parse properly must also be passed through. All such things should be
> documented.
This is a valid point. That's actually what the wl1271 driver does - as
the HW only supports one address for its filter. I'll add some text for
this.
> > +struct in_ifaddr;
>
> You should get the right include too.
>
> > +static inline int drv_configure_ip_filter(struct ieee80211_hw *hw,
> > + struct in_ifaddr *ifa_list)
> > +{
> > + struct ieee80211_local *local = hw_to_local(hw);
> > + int ret = 0;
> > +
> > + if (local->ops->configure_ip_filter)
> > + ret = local->ops->configure_ip_filter(hw, ifa_list);
> > + return ret;
> > +}
>
> Tracing would be nice, you should even able able to trace all addresses
> in a variable-length array.
Yeah, I was lazy. I will look into this :P
>
> > @@ -330,6 +330,7 @@ static int ieee80211_open(struct net_device *dev)
> > if (sdata->vif.type == NL80211_IFTYPE_STATION)
> > ieee80211_queue_work(&local->hw, &sdata->u.mgd.work);
> >
> > + ieee80211_set_arp_filter(dev);
>
> That seems unnecessary if drivers assume that there are no addresses to
> start with?
>
The drivers assume there is no address to start with - this is for the
scenario when there is, so that the driver enables the filtering from
scratch. You can do this, for instance:
ifconfig wlan0 up 192.168.1.1
ifconfig wlan0 down
ifconfig wlan0 up
The address 192.168.1.1 will be there already on ifconfig wlan0 up.
> BTW, how will drivers deal with getting this while unassociated? If, for
> example, I set an address before associating, you'll get it while
> unassociated and not again when associated. Another thing to document --
> driver needs to handle that, DHCP is not everything :)
This is actually a good point. The wl1271 driver configures the ARP
filter immediately - associated or not - once the chipset is booted up
(as above.) This is indeed not very generic.
I will move the set_arp_filter -thingy from _open() to when the we are
associated. That way we get the initial IP address (if there happen to
be any) from start when the address is actually needed.
>
> > +static int ieee80211_ifa_changed(struct notifier_block *nb,
> > + unsigned long data, void *arg)
> > +{
> > + struct in_ifaddr *ifa = arg;
> > + struct ieee80211_local *local =
> > + container_of(nb, struct ieee80211_local,
> > + ifa_notifier);
> > + struct net_device *ndev = ifa->ifa_dev->dev;
> > + struct wireless_dev *wdev = ndev->ieee80211_ptr;
> > +
> > + if (!wdev)
> > + return NOTIFY_DONE;
> > +
> > + if (wdev->wiphy != local->hw.wiphy)
> > + return NOTIFY_DONE;
> > +
> > + ieee80211_set_arp_filter(ndev);
> > + return NOTIFY_DONE;
> > +}
>
> This is obviously broken when you have multiple virtual interfaces, so
> you either need to build a common list of IP addresses, or punt the
> problem to the driver and give the callback an ieee80211_vif argument
> and clearly document that the driver will have to keep track of it for
> each interface.
Good point. I will "punt" (never heard this expression before!) the
problem to the driver with a vif pointer.
Thanks for your review. :)
-Juuso
> johannes
>
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
