Re: [PATCH v2 2/2] cfg80211/nl80211: Add packet coalesce support

[Johannes Berg <johannes@sipsolutions.net>]

Hi,

sorry for the late reply. I did see you sent another version, but wanted
to comment here anyway.

> > This is curious, you already discard those that you don't care about by
> > way of multicast filtering, no? What's the added advantage here? Would
> > you coalesce interrupts for those packets that pass the filter(s)?
> 
> Yes. we don't want to simply discard those packets, because few of
> them may be useful for the host.
> The advantage here is instead of getting random per packet receive
> interrupts, host receives multiple buffered packets with a single
> receive interrupt.

I guess that makes some sense.

> > Actually, you should probably make that a pointer, then it can be NULL
> > for drivers not supporting it, and static const for those that do. Means
> > you should make it a const pointer, of course.
> > 
> > Userspace can tell by checking if the support is advertised.
> 
> We will use following condition to check if driver is not supporting
> the feature. (wowlan code does the same thing)
> 
> "if (!dev->wiphy.coalesce.n_patterns || !dev->wiphy.coalesce.n_rules)"

Hmm. I think wowlan should probably be changed though, no need to make
the struct larger just for this, right? Not all drivers are going to
support it.

For WoWLAN I'm going to apply this:
http://p.sipsolutions.net/fa808e0722a7624a.txt

> > This is bad. You leave rdev->coalesce assigned, but it's completely
> > invalid data. IMHO you should use a temporary variable and only assign
> > it when it's fully parsed, freeing it if not. That way, you also don't
> > kill old values when new invalid values are parsed.
> 
> "iw coalesce add" command adds one coalesce rule at a time. "iw
> coalesce disable" command removes all configured rules.

Can you please change this to "iw coalesce set" and make it set all the
rules in one? Otherwise you're going to have very awkward races and need
to always clear etc. The code would also be easier, though obviously
you'd need to be able to specify multiple rules at the same time.

johannes