Re: [GIT PULL] mmutable branch between pdx86 amd wbrf branch and wifi / amdgpu due for the v6.8 merge window
Hi, > Here is a pull-request for the platform-drivers-x86 parts of: > > https://lore.kernel.org/platform-driver-x86/20231211100630.2170152-1-jun@amd.com/ > > From my pov the pdx86 bits are ready and the > platform-drivers-x86-amd-wbrf-v6.8-1 tag can be merged by you to merge the > wifi-subsys resp. the amdgpu driver changes on top. > > This only adds kernel internal API, so if in the future the API needs work > that can be done. I've been fine with the wifi bits since around v3 of the patchset ;-) So the idea is that I'll pull this into wireless-next and then apply the two wireless patches on top, right? AFAICT, since the other patches don't depend on wireless for compilation, this is the only thing I need to do, i.e. no need to have another separate branch to send it further on, right? johannes
Re: [GIT PULL] mmutable branch between pdx86 amd wbrf branch and wifi / amdgpu due for the v6.8 merge window
On Mon, 2023-12-11 at 12:02 +0100, Hans de Goede wrote: > Hi Wifi and AMDGPU maintainers, > > Here is a pull-request for the platform-drivers-x86 parts of: > > https://lore.kernel.org/platform-driver-x86/20231211100630.2170152-1-jun@amd.com/ > > From my pov the pdx86 bits are ready and the > platform-drivers-x86-amd-wbrf-v6.8-1 tag can be merged by you to merge the > wifi-subsys resp. the amdgpu driver changes on top. > > This only adds kernel internal API, so if in the future the API needs work > that can be done. OK, thanks! I've pulled this into wireless-next, and applied the two wireless related patches on top. I guess if AMDGPU does the same, it will combine nicely in 6.8. johannes
Re: [Patch v13 4/9] wifi: mac80211: Add support for WBRF features
On Thu, 2023-11-02 at 13:55 +0200, Ilpo Järvinen wrote:
[please trim your quotes]
> > +static void get_chan_freq_boundary(u32 center_freq, u32 bandwidth, u64
> > *start, u64 *end)
> > +{
> > + bandwidth = MHZ_TO_KHZ(bandwidth);
> > + center_freq = MHZ_TO_KHZ(center_freq);
>
> Please use include/linux/units.h ones for these too.
Now we're feature creeping though - this has existed for *years* in the
wireless stack with many instances? We can convert them over, I guess,
but not sure that makes much sense here - we'd want to add such macros
to units.h, but ... moving them can be independent of this patch?
johannes
Re: [Patch v13 4/9] wifi: mac80211: Add support for WBRF features
On Thu, 2023-11-02 at 14:24 +0200, Ilpo Järvinen wrote:
> On Thu, 2 Nov 2023, Johannes Berg wrote:
> > On Thu, 2023-11-02 at 13:55 +0200, Ilpo Järvinen wrote:
> >
> > > > +static void get_chan_freq_boundary(u32 center_freq, u32 bandwidth, u64
> > > > *start, u64 *end)
> > > > +{
> > > > + bandwidth = MHZ_TO_KHZ(bandwidth);
> > > > + center_freq = MHZ_TO_KHZ(center_freq);
> > >
> > > Please use include/linux/units.h ones for these too.
> >
> > Now we're feature creeping though - this has existed for *years* in the
> > wireless stack with many instances? We can convert them over, I guess,
> > but not sure that makes much sense here - we'd want to add such macros
> > to units.h, but ... moving them can be independent of this patch?
>
> What new macros you're talking about?
Sorry, I got confused - for some reason I was pretty sure something here
was already being added to units.h in this patchset.
> Nothing new needs to be added
> as there's already KHZ_PER_MHZ so these would just be:
>
> bandwidth *= KHZ_PER_MHZ;
> center_freq *= KHZ_PER_MHZ;
Sure, and in this case that's probably pretty much equivalent. But
having a MHZ_TO_KHZ() macro isn't inherently *bad*, and I'm not sure
you're objection to it on anything other than "it's not defined in
units.h".
> Everything can of course be postponed by the argument that some
> subsystem specific mechanism has been there before the generic one
> but the end of that road won't be pretty... What I was trying to do
> here was to point out the new stuff introduced by this series into the
> direction of the generic thing.
I just think that the better course of action would be to eventually
move MHZ_TO_KHZ() to units.h ...
johannes
Re: [PATCH 4/4] kunit: tool: Disable broken options for --alltests
On Fri, 2022-02-18 at 15:57 +0800, David Gow wrote:
>
> Note that, while this does build again, it still segfaults on startup,
> so more work remains to be done.
That's probably just a lot more stuff getting included somehow?
> They are:
> - CONFIG_VFIO_PCI: Needs ioport_map/ioport_unmap.
> - CONFIG_INFINIBAND_RDMAVT: Needs cpuinfo_x86 and __copy_user_nocache
> - CONFIG_BNXT: Failing under UML with -Werror
> ERROR:root:../drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c: In function
> ‘bnxt_ptp_enable’:
> ../drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:400:43: error: array
> subscript 255 is above array bounds of ‘struct pps_pin[4]’
> [-Werror=array-bounds]
> 400 | ptp->pps_info.pins[pin_id].event =
> BNXT_PPS_EVENT_EXTERNAL;
> | ~~^~~~
> - CONFIG_PATA_CS5535: Needs MSR access (__tracepoint_{read,write}_msr)
> - CONFIG_VDPA: Enables CONFIG_DMA_OPS, which is unimplemented. ('dma_ops' is
> not defined)
>
> These are all issues which should be investigated properly and the
> corresponding options either fixed or disabled under UML. Having this
> list of broken options should act as a good to-do list here, and will
> allow these issues to be worked on independently, and other tests to
> work in the meantime.
>
I'm not really sure it makes sense to even do anything other than
disabling these.
It looks like all of them are just exposed by now being able to build
PCI drivers on UML. Surely the people writing the driver didn't expect
their drivers to run over simulated PCI (which is what the UML PCI
support is all about).
Now from a PCI driver point of view you can't really tell the difference
(and anyway the driver won't be probed), but the issues (at least the
build time ones) come from having
UML && PCI && X86_64
or
UML && PCI && X86_32
because drivers typically depend on X86_64 or X86_32, rather than on
"X86 && X86_64" or "X86 && X86_32". In a sense thus, the issue is those
drivers don't know that "!X86 && (X86_32 || X86_64)" can happen (with
UML).
Now you could say that's the driver bug, or you could say that they
should just add "depends on !UML" (though that's basically equivalent to
adding "depends on X86" and the latter may be preferable in some cases).
Or actually in the three patches you have (1-3) it's in the code, but
same thing, you can either add && !UML (like you did) or add && X86.
Arguably, however, building PCI drivers by default is somewhat
questionable in the first place?
So maybe you should just add
# CONFIG_UML_PCI_OVER_VIRTIO is not set
to the broken_on_uml.config since it exposes all these issues, and
really is not very useful since you're not going to actually run with
any simulated PCI devices anyway, so drivers will not be probed.
johannes
Re: [PATCH 4/4] kunit: tool: Disable broken options for --alltests
On Sat, 2022-02-19 at 16:00 +0800, David Gow wrote:
> On Fri, Feb 18, 2022 at 8:26 PM Johannes Berg
> wrote:
> >
> > On Fri, 2022-02-18 at 15:57 +0800, David Gow wrote:
> > >
> > > Note that, while this does build again, it still segfaults on startup,
> > > so more work remains to be done.
> >
> > That's probably just a lot more stuff getting included somehow?
> >
>
> Yeah: it used to work (a couple of years ago), but something has
> broken it in the meantime. It's just a shame that bisecting things
> with allyesconfig takes so long...
Heh, right.
But I guess you could "Kconfig bisect" first, i.e. see what option
breaks it? It might not even help to bisect, if it's just some option
getting enabled over time. Or perhaps the kernel is just too big for the
address space layout if you have allyesconfig? Though that shouldn't be
an issue, I think.
> I didn't realise X86 wasn't defined in UML:
X86 is the architecture, X86_{32,64} is kind of a selection for how you
want things to be built, and it's thus required for UML on x86, because
UML imports stuff from X86.
> that's definitely a bit
> cleaner than !UML in a number of these cases.
It looks like some (most?) of them don't really work that way though
since they're not really platform specific, they just know only about a
handful of platforms that they're compatible with.
> Not all of those issues are fundamentally solved by "depends on X86",
> though: there are a few which might be other missing things in UML
> (maybe the 'dma_ops' issues?), and/or might be the result of -Werror
> being enabled.
Right.
> We do want the ability to build PCI drivers under UML, as it makes
> running KUnit tests for PCI drivers much simpler and more pleasant.
OK, fair point. I'm thinking about this area in general also right now
for iwlwifi, and obviously we're probably the only user of the virtual
PCI code that lets us connect the driver to a simulated device on UML
(but the driver doesn't really know) :-)
> And indeed, it does work for KUnit in general, it's just that some
> drivers do have the issues mentioned above, so allyesconfig picks up
> every broken driver.
Right.
> We don't actually build the PCI drivers by default, only if the
> "--alltests" option is passed, which does include them, as we do have
> tests which depend on PCI we'd like to run (like the thunderbolt
> test).
Makes sense.
>
> I did try this as well, and it just got us a different set of issues
> (there are a bunch of drivers which depend on IOMEM but don't state it
> -- I'll try to send fixes for those out next week).
>
Fun.
> Ultimately, the 'broken_on_uml.config' file is just there to pare back
> allyesconfig a bit for KUnit's purposes, but we still definitely want
> as many options (and hence tests) enabled as possible long-term. So I
> think actual fixes to either the code or Kconfig do make sense.
Makes sense.
> Is 'make ARCH=um allyesconfig' something we actually want to be able
> to build? If so, no amount of adding things to KUnit's
> broken_on_uml.config will solve the underlying issues, and we'll need
> to at least update the Kconfig entries.
>
That's a good point, as long as people are doing allyes/randconfig
builds on UML, we probably need to have these fixes anyway rather than
disabling something for KUnit specifically.
johannes
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, 2022-02-28 at 20:16 +, Matthew Wilcox wrote: > On Mon, Feb 28, 2022 at 12:10:24PM -0800, Linus Torvalds wrote: > > We can do > > > > typeof(pos) pos > > > > in the 'for ()' loop, and never use __iter at all. > > > > That means that inside the for-loop, we use a _different_ 'pos' than > > outside. > > Then we can never use -Wshadow ;-( I'd love to be able to turn it on; > it catches real bugs. > I was just going to say the same thing... If we're willing to change the API for the macro, we could do list_for_each_entry(type, pos, head, member) and then actually take advantage of -Wshadow? johannes
Re: [PATCH V2 2/7] wifi: mac80211: Add support for ACPI WBRF
On Fri, 2023-06-09 at 15:28 +0800, Evan Quan wrote:
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -5551,6 +5551,10 @@ struct wiphy {
>
> u16 hw_timestamp_max_peers;
>
> +#ifdef CONFIG_ACPI_WBRF
> + bool wbrf_supported;
> +#endif
This should be in some private struct in mac80211, ieee80211_local I
think.
> char priv[] __aligned(NETDEV_ALIGN);
> };
>
> @@ -9067,4 +9071,18 @@ static inline int cfg80211_color_change_notify(struct
> net_device *dev)
> bool cfg80211_valid_disable_subchannel_bitmap(u16 *bitmap,
> const struct cfg80211_chan_def
> *chandef);
>
> +#ifdef CONFIG_ACPI_WBRF
> +void ieee80211_check_wbrf_support(struct wiphy *wiphy);
> +int ieee80211_add_wbrf(struct wiphy *wiphy,
> +struct cfg80211_chan_def *chandef);
> +void ieee80211_remove_wbrf(struct wiphy *wiphy,
> +struct cfg80211_chan_def *chandef);
> +#else
> +static inline void ieee80211_check_wbrf_support(struct wiphy *wiphy) { }
> +static inline int ieee80211_add_wbrf(struct wiphy *wiphy,
> + struct cfg80211_chan_def *chandef) {
> return 0; }
> +static inline void ieee80211_remove_wbrf(struct wiphy *wiphy,
> + struct cfg80211_chan_def *chandef) { }
> +#endif /* CONFIG_ACPI_WBRF */
Same here, not the right place. This should even be in an internal
mac80211 header (such as net/mac80211/ieee80211_i.h or create a new
net/mac80211/wrbf.h or so if you prefer.)
> --- a/net/mac80211/chan.c
> +++ b/net/mac80211/chan.c
> @@ -668,6 +668,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->hw.wiphy, &ctx->conf.def);
> + if (err)
> + return err;
> +
> if (!local->use_chanctx)
> local->hw.conf.radar_enabled = ctx->conf.radar_enabled;
>
> @@ -748,6 +752,8 @@ static void ieee80211_del_chanctx(struct ieee80211_local
> *local,
> }
>
> ieee80211_recalc_idle(local);
> +
> + ieee80211_remove_wbrf(local->hw.wiphy, &ctx->conf.def);
> }
>
> static void ieee80211_free_chanctx(struct ieee80211_local *local,
>
This is tricky, and quite likely incorrect.
First of all, chandefs can actually _change_, see
_ieee80211_change_chanctx(). You'd probably have to call this add/remove
(or have modify) whenever we call drv_change_chanctx() to change the
width (not if radar/rx chains change).
Secondly, you don't know if the driver will actually use ctx->conf.def,
or ctx->conf.mindef. For client mode that doesn't matter, but for AP
mode if the AP is configured to say 160 MHz, it might actually configure
down to 20 MHz when no stations are connected (or only 20 MHz stations
are). I don't know if you really care about taking that into account, I
also don't know how dynamic this really should be. Stations can connect
and disconnect quickly, so perhaps the WBRF should actually take the
full potential bandwidth into account all the time, in which case taking
ctx->conf.def would be correct.
I'll note that your previous in-driver approach had all the same
problems the way you had implemented it, though I don't know if that
driver ever can use mindef or not.
> +void ieee80211_check_wbrf_support(struct wiphy *wiphy)
> +{
> + struct device *dev = wiphy->dev.parent;
> + struct acpi_device *acpi_dev;
> +
> + acpi_dev = ACPI_COMPANION(dev);
Can this cope with 'dev' being NULL? Just not sure nothing like hwsim or
so always even has a parent. I guess it should, but ...
> +static int chan_width_to_mhz(enum nl80211_chan_width chan_width)
> +{
> + int mhz;
> +
> + switch (chan_width) {
> + case NL80211_CHAN_WIDTH_1:
> + mhz = 1;
> + break;
> + case NL80211_CHAN_WIDTH_2:
> + mhz = 2;
> + break;
> + case NL80211_CHAN_WIDTH_4:
> + mhz = 4;
> + break;
> + case NL80211_CHAN_WIDTH_8:
> + mhz = 8;
> + break;
> + case NL80211_CHAN_WIDTH_16:
> + mhz = 16;
> + break;
> + case NL80211_CHAN_WIDTH_5:
> + mhz = 5;
> + break;
> + case NL80211_CHAN_WIDTH_10:
> + mhz = 10;
> + break;
> + case NL80211_CHAN_WIDTH_20:
> + case NL80211_CHAN_WIDTH_20_NOHT:
> + mhz = 20;
> + break;
> + case NL80211_CHAN_WIDTH_40:
> + mhz = 40;
> + break;
> + case NL80211_CHAN_WIDTH_80P80:
> + case NL80211_CHAN_WIDTH_80:
> + mhz = 80;
> + break;
> + case NL80211_CHAN_WIDTH_160:
> + mhz = 160;
> + break;
> + case NL80211_CHAN_WIDTH_320:
> + mhz = 320;
> + break;
> + default:
> + WARN_ON_ONCE(1);
> + return -1;
> + }
> +
Re: [PATCH V3 2/7] wifi: mac80211: Add support for ACPI WBRF
On Sun, 2023-06-18 at 21:17 -0500, Mario Limonciello wrote: > > > --- a/include/net/cfg80211.h > > +++ b/include/net/cfg80211.h > > @@ -920,6 +920,14 @@ const struct cfg80211_chan_def * > > cfg80211_chandef_compatible(const struct cfg80211_chan_def *chandef1, > > const struct cfg80211_chan_def *chandef2); > > > > +/** > > + * nl80211_chan_width_to_mhz - get the channel width in Mhz > > + * @chan_width: the channel width from &enum nl80211_chan_width > > + * Return: channel width in Mhz if the chan_width from &enum > > nl80211_chan_width > > + * is valid. -1 otherwise. > > + */ > > +int nl80211_chan_width_to_mhz(enum nl80211_chan_width chan_width); > > + > > It's up to mac80211 maintainers, but I would think that the changes to > change nl80211_chan_width_to_mhz from static to exported should be > separate from the patch to introduced WBRF support in the series. Yeah, that'd be nice :) > > +#define KHZ_TO_HZ(freq)((freq) * 1000ULL) Together with MHZ_TO_KHZ() for example :) johannes
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
> Co-developed-by: Evan Quan
> Signed-off-by: Evan Quan
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
Re: [PATCH V4 3/8] wifi: mac80211: Add support for ACPI WBRF
On Wed, 2023-06-21 at 09:12 -0500, Limonciello, Mario wrote: > > > > 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. > Since it's all gated by CONFIG_ACPI_WBRF for each subsystem that it touches, > my take is that we should merge like this: > > 1) Get A-b/R-b on patch 1 (ACPI patch) from Rafael. > 2) Merge mac80211 bits through WLAN trees > 3) Merge AMDGPU bits *and* ACPI bits through amd-staging-drm-next > followed by drm tree > > Since WLAN and AMDGPU bits are using the exported ACPI functions from > patch 1, we need to make sure that it is accepted and won't change > interface before merging other bits. Right. > Everything can come together in the upstream tree and the bots > will be able to test linux-next as well this way. Yeah, that's what I thought. Sounds good to me! Oh, also, since it's pretty late in the cycle I'm assuming for now that you're not aiming this for 6.5 anymore. If you still are, it all would probably need to happen very quickly. johannes
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
On Wed, 2023-06-21 at 17:36 +0200, Andrew Lunn wrote: > On Wed, Jun 21, 2023 at 01:45:56PM +0800, Evan Quan wrote: > > From: Mario Limonciello > > > > Due to electrical and mechanical constraints in certain platform designs > > there may be likely interference of relatively high-powered harmonics of > > the (G-)DDR memory clocks with local radio module frequency bands used > > by Wifi 6/6e/7. > > > > To mitigate this, AMD has introduced an ACPI based mechanism that > > devices can use to notify active use of particular frequencies so > > that devices can make relative internal adjustments as necessary > > to avoid this resonance. > > Do only ACPI based systems have: > >interference of relatively high-powered harmonics of the (G-)DDR >memory clocks with local radio module frequency bands used by >Wifi 6/6e/7." > > Could Device Tree based systems not experience this problem? They could, of course, but they'd need some other driver to change _something_ in the system? I don't even know what this is doing precisely under the hood in the ACPI BIOS, perhaps it adjusts the DDR memory clock frequency in response to WiFi using a frequency that will cause interference with harmonics. > > +/** > > + * APIs needed by drivers/subsystems for contributing frequencies: > > + * During probe, check `wbrf_supported_producer` to see if WBRF is > > supported. > > + * If adding frequencies, then call `wbrf_add_exclusion` with the > > + * start and end points specified for the frequency ranges added. > > + * If removing frequencies, then call `wbrf_remove_exclusion` with > > + * start and end points specified for the frequency ranges added. > > + */ > > +bool wbrf_supported_producer(struct acpi_device *adev); > > +int wbrf_add_exclusion(struct acpi_device *adev, > > + struct wbrf_ranges_in *in); > > +int wbrf_remove_exclusion(struct acpi_device *adev, > > + struct wbrf_ranges_in *in); > > Could struct device be used here, to make the API agnostic to where > the information is coming from? That would then allow somebody in the > future to implement a device tree based information provider. That does make sense, and it wouldn't even be that much harder if we assume in a given platform there's only one provider - but once you go beyond that these would need to call function pointers I guess? Though that could be left for "future improvement" too. johannes
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
On Wed, 2023-06-21 at 21:25 +0200, Andrew Lunn wrote: > > ACPI core does has notifiers that are used, but they don't work the same. > > If you look at patch 4, you'll see amdgpu registers and unregisters using > > both > > > > acpi_install_notify_handler() > > and > > acpi_remove_notify_handler() > > > > If we supported both ACPI notifications and non-ACPI notifications > > all consumers would have to have support to register and use both types. > > Why would you want to support ACPI notifications and non-ACPI > notifications? All you need is wbrf notification. > > The new wbrf.c should implement wbrf_install_notify_handler() and > wbrf_remove_notify_handler(). > > As to where to put wbrf.c? I guess either drivers/base/ or > drivers/wbrf/. Maybe ask GregKH? Not sure it should even be called WBRF at that point, but hey :) Honestly I'm not sure though we need this complexity right now? I mean, it'd be really easy to replace the calls in mac80211 with some other more generalised calls in the future? You need some really deep platform/hardware level knowledge and involvement to do this, so I don't think it's something that someone will come up with very easily for a DT-based platform... If we do something with a notifier chain in the future, we can just install one in the ACPI code too, and react indirectly rather than calling from wifi to the ACPI directly. johannes
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
On Wed, 2023-06-21 at 18:14 +0200, Andrew Lunn wrote: > > > Do only ACPI based systems have: > > > > > > interference of relatively high-powered harmonics of the (G-)DDR > > > memory clocks with local radio module frequency bands used by > > > Wifi 6/6e/7." > > > > > > Could Device Tree based systems not experience this problem? > > > > They could, of course, but they'd need some other driver to change > > _something_ in the system? I don't even know what this is doing > > precisely under the hood in the ACPI BIOS > > If you don't know what it is actually doing, it suggests the API is > not very well defined. I wouldn't say that. At the level it's defined now, the API is very clear: the wifi subsystem tells the other side what channels it's operating on right now. > Is there even enough details that ARM64 ACPI > BIOS could implement this? This, in itself? No. You'd have to know about the physical characteristics of the system, what is actually causing interference and at what frequencies and of course what you can actually do to mitigate (such as adjusting clock frequencies.) But as an API? I'd think yes, since WiFi can't really move off a frequency, other than disconnect, anyway. > > > > +bool wbrf_supported_producer(struct acpi_device *adev); > > > > +int wbrf_add_exclusion(struct acpi_device *adev, > > > > + struct wbrf_ranges_in *in); > > > > +int wbrf_remove_exclusion(struct acpi_device *adev, > > > > + struct wbrf_ranges_in *in); > > > > > > Could struct device be used here, to make the API agnostic to where > > > the information is coming from? That would then allow somebody in the > > > future to implement a device tree based information provider. > > > > That does make sense, and it wouldn't even be that much harder if we > > assume in a given platform there's only one provider > > That seems like a very reasonable assumption. It is theoretically > possible to build an ACPI + DT hybrid, but i've never seen it actually > done. OK. > If an ARM64 ACPI BIOS could implement this, then i would guess the low > level bits would be solved, i guess jumping into the EL1 > firmware. Putting DT on top instead should not be too hard. Right. Maybe then this really shouldn't be called "wbrf", but maybe naming doesn't matter that much :) johannes
Re: Build regressions/improvements in v6.1-rc6
On Tue, 2022-11-22 at 08:55 -0500, Alex Deucher wrote:
> >
> >+ /kisskb/src/arch/um/include/asm/processor-generic.h: error: called
> > object is not a function or function pointer: => 94:18
> >+ /kisskb/src/drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_topology.c:
> > error: control reaches end of non-void function [-Werror=return-type]: =>
> > 1934:1
> >
> > um-x86_64/um-all{mod,yes}config (in kfd_cpumask_to_apic_id())
>
> Presumably cpu_data is not defined on um-x86_64? Does it even make
> sense to build drivers on um-x86_64?
Drivers in general yes ;-)
This driver, probably not.
But the issue is that a lot of drivers "depends on X86_64" or such,
where only "X86" is the arch symbol. You could add "X86 && X86_64" to
really build on x86 64-bit only.
I didn't check this driver, but this has mostly popped up since UM got
PCI support some time ago (which I added.)
johannes
Re: [PATCH V7 4/9] wifi: mac80211: Add support for ACPI WBRF
On Tue, 2023-07-25 at 22:09 +0200, Andrew Lunn wrote: > > > It could well be that AMD based machine has a different ACPI extension > to indicate this policy to what Intel machine has. As far as i > understand it, you have not submitted this yet for formal approval, > this is all vendor specific, so Intel could do it completely > differently. Already do, without the host software being involved in the same way. There, I believe the ACPI tables just indicate what's needed and the WiFi firmware sorts out the rest. johannes
Re: [V9 4/9] wifi: mac80211: Add support for WBRF features
On Fri, 2023-08-18 at 11:26 +0800, Evan Quan wrote:
> To support the WBRF mechanism, Wifi adapters utilized in the system must
> register the frequencies in use(or unregister those frequencies no longer
> used) via the dedicated calls. So that, other drivers responding to the
> frequencies can take proper actions to mitigate possible interference.
>
> Co-developed-by: Mario Limonciello
> Signed-off-by: Mario Limonciello
> Co-developed-by: Evan Quan
> Signed-off-by: Evan Quan
>From WiFi POV, this looks _almost_ fine to me.
> +static void wbrf_get_ranges_from_chandef(struct cfg80211_chan_def *chandef,
> + struct wbrf_ranges_in *ranges_in)
> +{
> + u64 start_freq1, end_freq1;
> + u64 start_freq2, end_freq2;
> + int bandwidth;
> +
> + bandwidth = nl80211_chan_width_to_mhz(chandef->width);
> +
> + get_chan_freq_boundary(chandef->center_freq1,
> +bandwidth,
> +&start_freq1,
> +&end_freq1);
> +
> + ranges_in->band_list[0].start = start_freq1;
> + ranges_in->band_list[0].end = end_freq1;
> +
> + if (chandef->width == NL80211_CHAN_WIDTH_80P80) {
> + get_chan_freq_boundary(chandef->center_freq2,
> +bandwidth,
> +&start_freq2,
> +&end_freq2);
> +
> + ranges_in->band_list[1].start = start_freq2;
> + ranges_in->band_list[1].end = end_freq2;
> + }
> +}
This has to setup ranges_in->num_of_ranges, no?
(Also no real good reason for num_of_ranges to be a u64, btw, since it
can only go up to 11)
With that fixed, you can add
Reviewed-by: Johannes Berg
johannes
Re: [PATCH v3 12/26] compat_ioctl: move more drivers to compat_ptr_ioctl
On Thu, 2019-04-25 at 17:55 +0200, Arnd Bergmann wrote:
> On Thu, Apr 25, 2019 at 5:35 PM Al Viro wrote:
> >
> > On Thu, Apr 25, 2019 at 12:21:53PM -0300, Mauro Carvalho Chehab wrote:
> >
> > > If I understand your patch description well, using compat_ptr_ioctl
> > > only works if the driver is not for s390, right?
> >
> > No; s390 is where "oh, just set ->compat_ioctl same as ->unlocked_ioctl
> > and be done with that; compat_ptr() is a no-op anyway" breaks. IOW,
> > s390 is the reason for having compat_ptr_ioctl() in the first place;
> > that thing works on all biarch architectures, as long as all stuff
> > handled by ->ioctl() takes pointer to arch-independent object as
> > argument. IOW,
> > argument ignored => OK
> > any arithmetical type => no go, compat_ptr() would bugger it
> > pointer to int => OK
> > pointer to string => OK
> > pointer to u64 => OK
> > pointer to struct {u64 addr; char s[11];} => OK
>
> To be extra pedantic, the 'struct {u64 addr; char s[11];} '
> case is also broken on x86, because sizeof (obj) is smaller
> on i386, even though the location of the members are
> the same. i.e. you can copy_from_user() this
Actually, you can't even do that because the struct might sit at the end
of a page and then you'd erroneously fault in this case.
We had this a while ago with struct ifreq, see commit 98406133dd and its
parents.
johannes
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
