netdev - Re: [PATCH V4 3/8] wifi: mac80211: Add support for ACPI WBRF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 21 Jun 2023 12:22:21 +0200
From: Johannes Berg <johannes@...solutions.net>
To: Evan Quan <evan.quan@....com>, rafael@...nel.org, lenb@...nel.org, 
 alexander.deucher@....com, christian.koenig@....com, Xinhui.Pan@....com, 
 airlied@...il.com, daniel@...ll.ch, davem@...emloft.net,
 edumazet@...gle.com,  kuba@...nel.org, pabeni@...hat.com,
 mario.limonciello@....com, mdaenzer@...hat.com, 
 maarten.lankhorst@...ux.intel.com, tzimmermann@...e.de,
 hdegoede@...hat.com,  jingyuwang_vip@....com, lijo.lazar@....com,
 jim.cromie@...il.com,  bellosilicio@...il.com, andrealmeid@...lia.com,
 trix@...hat.com, jsg@....id.au,  arnd@...db.de
Cc: linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org, 
	amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org, 
	linux-wireless@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH V4 3/8] wifi: mac80211: Add support for ACPI WBRF

On Wed, 2023-06-21 at 13:45 +0800, Evan Quan wrote:
> To support AMD's WBRF interference mitigation mechanism, Wifi adapters
> utilized in the system must register the frequencies in use(or unregister
> those frequencies no longer used) via the dedicated APCI calls. So that,
> other drivers responding to the frequencies can take proper actions to
> mitigate possible interference.
> 
> To make WBRF feature functional, the kernel needs to be configured with
> CONFIG_ACPI_WBRF and the platform is equipped with WBRF support(from
> BIOS and drivers).
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
> Co-developed-by: Evan Quan <evan.quan@....com>
> Signed-off-by: Evan Quan <evan.quan@....com>

I was going to say this looks good ... but still have a few nits, sorry.

But then the next question anyway is how we merge this? The wifi parts
sort of depend on the first patch, although technically I guess I could
merge them since it's all hidden behind the CONFIG_ symbol, assuming you
get that in via some other tree it can combine upstream.

I'd also say you can merge those parts elsewhere but I'm planning to
also land some locking rework that I've been working on, so it will
probably conflict somewhere.

> +++ b/net/mac80211/chan.c
> @@ -506,11 +506,16 @@ static void _ieee80211_change_chanctx(struct ieee80211_local *local,
>  
>  	WARN_ON(!cfg80211_chandef_compatible(&ctx->conf.def, chandef));
>  
> +	ieee80211_remove_wbrf(local, &ctx->conf.def);
> +
>  	ctx->conf.def = *chandef;
>  
>  	/* check if min chanctx also changed */
>  	changed = IEEE80211_CHANCTX_CHANGE_WIDTH |
>  		  _ieee80211_recalc_chanctx_min_def(local, ctx, rsvd_for);
> +
> +	ieee80211_add_wbrf(local, &ctx->conf.def);

You ignore the return value here.


> @@ -668,6 +673,10 @@ static int ieee80211_add_chanctx(struct ieee80211_local *local,
>  	lockdep_assert_held(&local->mtx);
>  	lockdep_assert_held(&local->chanctx_mtx);
>  
> +	err = ieee80211_add_wbrf(local, &ctx->conf.def);
> +	if (err)
> +		return err;

But not here.

In the code, there are basically two error paths:

> +int ieee80211_add_wbrf(struct ieee80211_local *local,
> +		       struct cfg80211_chan_def *chandef)
> +{
> +	struct device *dev = local->hw.wiphy->dev.parent;
> +	struct wbrf_ranges_in ranges_in = {0};
> +	int ret;
> +
> +	if (!local->wbrf_supported)
> +		return 0;
> +
> +	ret = wbrf_get_ranges_from_chandef(chandef, &ranges_in);
> +	if (ret)
> +		return ret;

This really won't fail, just if the bandwidth calculation was bad, but
that's an internal error that WARNs anyway and we can ignore it.

> +	return wbrf_add_exclusion(ACPI_COMPANION(dev), &ranges_in);

This I find a bit confusing, why do we even propagate the error? If the
platform has some issue with it, should we really fail the connection?


I think it seems better to me to just make this void, and have it be
only a notification interface?

johannes