netdev - Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

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]

Message-ID: <e4168f82-f750-4917-b319-85bb7fe71ce4@lunn.ch>
Date: Thu, 22 Jun 2023 23:20:26 +0200
From: Andrew Lunn <andrew@...n.ch>
To: "Limonciello, Mario" <mario.limonciello@....com>
Cc: Johannes Berg <johannes@...solutions.net>,
	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, 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, 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 1/8] drivers/acpi: Add support for Wifi band RF
 mitigations

On Wed, Jun 21, 2023 at 01:50:34PM -0500, Limonciello, Mario wrote:
> So if we go down this path of CONFIG_WBRF and CONFIG_WBRF_ACPI, another
> question would be where should the new "wbrf.c" be stored?  The ACPI only
> version most certainly made sense in drivers/acpi/wbrf.c, but a generic
> version that only has an ACPI implementation right now not so much.
> 
> On 6/21/2023 1:30 PM, Andrew Lunn wrote:
> > > And consumer would need to call it, but only if CONFIG_WBRF_ACPI isn't set.
> > Why? How is ACPI special that it does not need notifiers?
> 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.

I took a quick look at this:

#define CPM_GPU_NOTIFY_COMMAND		0x55
+static void amdgpu_acpi_wbrf_event(acpi_handle handle, u32 event, void *data)
+{
+	struct amdgpu_device *adev = (struct amdgpu_device *)data;
+
+	if (event == CPM_GPU_NOTIFY_COMMAND &&
+	    adev->wbrf_event_handler)
+		adev->wbrf_event_handler(adev);
+}

handle is ignored, All you need is the void * data to link back to
your private data.

I find it interesting that CPM_GPU_NOTIFY_COMMAND is defined here. So
nothing else can use it. Should it maybe be called
CPM_AMDGPU_NOTIFY_COMMAND?

Overall, looking at this, i don't see anything which could not be made
abstract:

static void amdgpu_wbrf_event(u32 event, void *data)
{
       struct amdgpu_device *adev = (struct amdgpu_device *)data;

       if (event == WBRF_GPU_NOTIFY_COMMAND &&
           adev->wbrf_event_handler)
               adev->wbrf_event_handler(adev);
}

int amdgpu_register_wbrf_notify_handler(struct amdgpu_device *adev,
					wbrf_notify_handler handler)
{
	struct device *dev = adev->dev);

	adev->wbrf_event_handler = handler;

	return wbrf_install_notify_handler(dev, amdgpu_wbrf_event, adev);
}

	Andrew