Linux-Wireless Archive on lore.kernel.org
 help / color / Atom feed
From: Aloka Dixit <alokad@codeaurora.org>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: linux-wireless@vger.kernel.org, linux-wireless-owner@vger.kernel.org
Subject: Re: [PATCH v4 2/2] mac80211: Add FILS discovery support
Date: Thu, 30 Jul 2020 15:08:11 -0700
Message-ID: <78c89ff151efbdff2e579733d6b1d98c@codeaurora.org> (raw)
In-Reply-To: <d1b5dd7d9f93d3fc87c58dcfc28c9a3c6b4c923e.camel@sipsolutions.net>

On 2020-07-30 14:26, Johannes Berg wrote:
> On Thu, 2020-07-30 at 14:00 -0700, Aloka Dixit wrote:
> 
>> > > +	fd = &sdata->vif.bss_conf.fils_discovery;
>> > > +	fd->min_interval = params->min_interval;
>> > > +	fd->max_interval = params->max_interval;
>> > > +
>> > > +	if (!params->tmpl || !params->tmpl_len) /* Optional template */
>> > > +		return 0;
>> >
>> > Now I'm even more confused. If the template is optional, then if it's
>> > not given it doesn't mean *everything* should be ignored, does it?
>> >
>> > What would be the point of that? OTOH, if the template isn't there,
>> > what
>> > would you do?
>> >
>> > But it still doesn't make sense - if no template means you shouldn't do
>> > anything then that doesn't mean the template should be optional, that
>> > just means userspace shouldn't even put the NL80211_ATTR_FILS_DISCOVERY
>> > attribute when it doesn't want anything to be done?
>> >
>> > So it seems to me that something doesn't match. Either the template is
>> > truly optional and then this shouldn't just return success, or the
>> > template isn't actually optional?
>> >
>> 
>> Everything is not ignored, I set the minimum and maximum interval 
>> values
>> before checking for the template so that those are accepted even if
>> template isn't present.
> 
> Right, oops, missed that.
> 
>> For 6GHz, template is required, at least for ath11k driver.
>> But for 2.4GHz and 5GHz FILS discovery transmission is not offloaded 
>> to
>> FW.
> 
> But ... now I'm still confused.
> 
> If you *don't* offload it, how will it work? Will it all bubble up to
> hostapd and that will send the response? Does that work without any
> other changes?
> 
> But then what would you need the min/max for? I guess I still don't
> understand it... I thought this was a periodic frame anyway like 
> beacon,
> so how could you _not_ offload it?
> 

Min and max intervals are used to decide if a FILS discovery frame 
should be sent at all when respective timers expires.
Depending on how close that time is to the next beacons, the device may 
just send the beacon instead.

In lower bands, for non-offloaded case, FW will send events asking for 
the frame until it gets one.
Whether that should go all the way to hostapd or should the driver 
itself handle it remains to be seen.
My current focus is only 6GHz, but didn't want to restrict kernel 
implementation so moved 6GHz related checks to the driver instead.

All in all, making the template mandatory will be safer so that the 
driver will always have one if required.

>> We can make the template mandatory instead and then the respective
>> drivers will choose the handling.
>> Please suggest.
> 
> I have no idea ... still trying to understand it.
> 
>> > > +	err = ieee80211_set_fils_discovery(sdata, &params->fils_discovery);
>> > > +	if (err < 0) {
>> > > +		ieee80211_vif_release_channel(sdata);
>> > > +		return err;
>> >
>> > Is there no goto label for this error case?
>> >
>> 
>> Existing function doesn't use goto labels for error cases, only 
>> return.
> 
> Maybe add one? Surely the release_channel() must alraedy exist there
> somewhere.
> 

Okay.

>> > > +	skb = dev_alloc_skb(tmpl->len);
>> > > +	if (skb)
>> > > +		skb_put_data(skb, tmpl->data, tmpl->len);
>> >
>> > You should consider the headroom that the driver may have requested.
>> >
>> 
>> I didn't understand this point, what would the driver request headroom
>> for?
> 
> Whatever it wants for ... drivers are allowed request extra headroom
> (hw->extra_tx_headroom) and we generally honour that for every skb we
> build for the driver.
> 

I will look into other instances to understand this requirement.

> johannes

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-18  5:04 [PATCH v4 0/2] " Aloka Dixit
2020-06-18  5:04 ` [PATCH v4 1/2] nl80211: " Aloka Dixit
2020-07-30 14:43   ` Johannes Berg
2020-07-30 21:17     ` Aloka Dixit
2020-07-30 21:22       ` Johannes Berg
2020-07-30 21:53         ` Aloka Dixit
2020-06-18  5:04 ` [PATCH v4 2/2] mac80211: " Aloka Dixit
2020-07-30 14:47   ` Johannes Berg
2020-07-30 21:00     ` Aloka Dixit
2020-07-30 21:26       ` Johannes Berg
2020-07-30 22:08         ` Aloka Dixit [this message]
2020-07-31  8:21           ` Johannes Berg

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=78c89ff151efbdff2e579733d6b1d98c@codeaurora.org \
    --to=alokad@codeaurora.org \
    --cc=johannes@sipsolutions.net \
    --cc=linux-wireless-owner@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Wireless Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-wireless/0 linux-wireless/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-wireless linux-wireless/ https://lore.kernel.org/linux-wireless \
		linux-wireless@vger.kernel.org
	public-inbox-index linux-wireless

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-wireless


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git