[PATCH v1] net: fjes: Drop fjes_acpi_driver and rework initialization

[PATCH v1] net: fjes: Drop fjes_acpi_driver and rework initialization

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

The ACPI driver interface used by the Fujitsu Extended Socket (fjes)
Network Device driver is redundant because its only role is to create
a platform device the fjes platform driver can bind to, which can be
done already at the module initialization time.

Namely, acpi_find_extended_socket_device() looks for the requisite ACPI
device object anyway and it may as well check its resources, and the
platform device can be created when the ACPI object in question
has been found (and it can be freed when the module is unloaded).

Moreover, as a rule, it is better to avoid binding drivers directly to
ACPI device objects [1].

Accordingly, drop fjes_acpi_driver, adjust the module initialization
and exit code as per the above and set the fwnode for the fjes platform
device to point to the corresponding ACPI device object as its ACPI
companion.

While this is not expected to alter functionality, it changes sysfs
layout and so it will be visible to user space.

Link: https://lore.kernel.org/all/2396510.ElGaqSPkdT@rafael.j.wysocki/ [1]
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/net/fjes/fjes_main.c | 119 +++++++++++++++--------------------
 1 file changed, 49 insertions(+), 70 deletions(-)

diff --git a/drivers/net/fjes/fjes_main.c b/drivers/net/fjes/fjes_main.c
index b63965d9a1ba..efb399eb15e0 100644
--- a/drivers/net/fjes/fjes_main.c
+++ b/drivers/net/fjes/fjes_main.c
@@ -111,56 +111,6 @@ fjes_get_acpi_resource(struct acpi_resource *acpi_res, void *data)
 	return AE_OK;
 }
 
-static struct resource fjes_resource[] = {
-	DEFINE_RES_MEM(0, 1),
-	DEFINE_RES_IRQ(0)
-};
-
-static int fjes_acpi_add(struct acpi_device *device)
-{
-	struct platform_device *plat_dev;
-	acpi_status status;
-
-	if (!is_extended_socket_device(device))
-		return -ENODEV;
-
-	if (acpi_check_extended_socket_status(device))
-		return -ENODEV;
-
-	status = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
-				     fjes_get_acpi_resource, fjes_resource);
-	if (ACPI_FAILURE(status))
-		return -ENODEV;
-
-	/* create platform_device */
-	plat_dev = platform_device_register_simple(DRV_NAME, 0, fjes_resource,
-						   ARRAY_SIZE(fjes_resource));
-	if (IS_ERR(plat_dev))
-		return PTR_ERR(plat_dev);
-
-	device->driver_data = plat_dev;
-
-	return 0;
-}
-
-static void fjes_acpi_remove(struct acpi_device *device)
-{
-	struct platform_device *plat_dev;
-
-	plat_dev = (struct platform_device *)acpi_driver_data(device);
-	platform_device_unregister(plat_dev);
-}
-
-static struct acpi_driver fjes_acpi_driver = {
-	.name = DRV_NAME,
-	.class = DRV_NAME,
-	.ids = fjes_acpi_ids,
-	.ops = {
-		.add = fjes_acpi_add,
-		.remove = fjes_acpi_remove,
-	},
-};
-
 static int fjes_setup_resources(struct fjes_adapter *adapter)
 {
 	struct net_device *netdev = adapter->netdev;
@@ -1470,43 +1420,81 @@ static struct platform_driver fjes_driver = {
 	.remove = fjes_remove,
 };
 
+struct fjes_acpi_walk_context {
+	struct acpi_device *adev;
+	struct resource resources[2];
+};
+
 static acpi_status
 acpi_find_extended_socket_device(acpi_handle obj_handle, u32 level,
 				 void *context, void **return_value)
 {
+	struct fjes_acpi_walk_context *fjes_context = context;
 	struct acpi_device *device;
-	bool *found = context;
+	acpi_status status;
 
-	device = acpi_fetch_acpi_dev(obj_handle);
+	device = acpi_get_acpi_dev(obj_handle);
 	if (!device)
 		return AE_OK;
 
 	if (strcmp(acpi_device_hid(device), ACPI_MOTHERBOARD_RESOURCE_HID))
-		return AE_OK;
+		goto skip;
 
 	if (!is_extended_socket_device(device))
-		return AE_OK;
+		goto skip;
 
 	if (acpi_check_extended_socket_status(device))
-		return AE_OK;
+		goto skip;
+
+	status = acpi_walk_resources(obj_handle, METHOD_NAME__CRS,
+				     fjes_get_acpi_resource, fjes_context->resources);
+	if (ACPI_FAILURE(status))
+		goto skip;
+
+	fjes_context->adev = device;
 
-	*found = true;
 	return AE_CTRL_TERMINATE;
+
+skip:
+	acpi_dev_put(device);
+	return AE_OK;
 }
 
+static struct platform_device *fjes_plat_dev;
+
 /* fjes_init_module - Driver Registration Routine */
 static int __init fjes_init_module(void)
 {
-	bool found = false;
+	struct fjes_acpi_walk_context fjes_context = {
+		.adev = NULL,
+		.resources = {
+			DEFINE_RES_MEM(0, 1),
+			DEFINE_RES_IRQ(0)
+		}
+	};
+	struct platform_device_info pdevinfo;
 	int result;
 
 	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX,
-			    acpi_find_extended_socket_device, NULL, &found,
+			    acpi_find_extended_socket_device, NULL, &fjes_context,
 			    NULL);
-
-	if (!found)
+	if (!fjes_context.adev)
 		return -ENODEV;
 
+	memset(&pdevinfo, 0, sizeof(pdevinfo));
+
+	pdevinfo.name = DRV_NAME;
+	pdevinfo.res = fjes_context.resources;
+	pdevinfo.num_res = ARRAY_SIZE(fjes_context.resources);
+	pdevinfo.fwnode = acpi_fwnode_handle(fjes_context.adev);
+
+	fjes_plat_dev = platform_device_register_full(&pdevinfo);
+
+	acpi_dev_put(fjes_context.adev);
+
+	if (IS_ERR(fjes_plat_dev))
+		return PTR_ERR(fjes_plat_dev);
+
 	pr_info("%s - version %s - %s\n",
 		fjes_driver_string, fjes_driver_version, fjes_copyright);
 
@@ -1518,16 +1506,7 @@ static int __init fjes_init_module(void)
 		return result;
 	}
 
-	result = acpi_bus_register_driver(&fjes_acpi_driver);
-	if (result < 0)
-		goto fail_acpi_driver;
-
 	return 0;
-
-fail_acpi_driver:
-	platform_driver_unregister(&fjes_driver);
-	fjes_dbg_exit();
-	return result;
 }
 
 module_init(fjes_init_module);
@@ -1535,9 +1514,9 @@ module_init(fjes_init_module);
 /* fjes_exit_module - Driver Exit Cleanup Routine */
 static void __exit fjes_exit_module(void)
 {
-	acpi_bus_unregister_driver(&fjes_acpi_driver);
 	platform_driver_unregister(&fjes_driver);
 	fjes_dbg_exit();
+	platform_device_unregister(fjes_plat_dev);
 }
 
 module_exit(fjes_exit_module);
-- 
2.51.0

Re: [v1] net: fjes: Drop fjes_acpi_driver and rework initialization

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: fjes: Drop fjes_acpi_driver and rework initialization

This commit reworks the Fujitsu Extended Socket (fjes) Network Device
driver initialization by removing the ACPI driver interface and
creating the platform device directly during module initialization.

Link: https://lore.kernel.org/all/2396510.ElGaqSPkdT@rafael.j.wysocki/ [1]

> diff --git a/drivers/net/fjes/fjes_main.c b/drivers/net/fjes/fjes_main.c
> index b63965d9a1ba..efb399eb15e0 100644
> --- a/drivers/net/fjes/fjes_main.c
> +++ b/drivers/net/fjes/fjes_main.c

[ ... ]

> @@ -1518,16 +1506,7 @@ static int __init fjes_init_module(void)
>  		return result;
>  	}
>
> -	result = acpi_bus_register_driver(&fjes_acpi_driver);
> -	if (result < 0)
> -		goto fail_acpi_driver;
> -
>  	return 0;
> -
> -fail_acpi_driver:
> -	platform_driver_unregister(&fjes_driver);
> -	fjes_dbg_exit();
> -	return result;
>  }

In fjes_init_module(), if platform_driver_register() fails, does this
leak the platform device?

Looking at the new code, the platform device is successfully registered
earlier in the function:

	fjes_plat_dev = platform_device_register_full(&pdevinfo);
	...
	if (IS_ERR(fjes_plat_dev))
		return PTR_ERR(fjes_plat_dev);

	...
	result = platform_driver_register(&fjes_driver);

But when platform_driver_register() fails, the error path only calls
fjes_dbg_exit() and returns:

	result = platform_driver_register(&fjes_driver);
	if (result < 0) {
		fjes_dbg_exit();
		return result;
	}

Since the module init fails, fjes_exit_module() will never run, so
the platform_device_unregister() call there won't happen either. Should
the error path call platform_device_unregister(fjes_plat_dev) before
returning?

Re: [v1] net: fjes: Drop fjes_acpi_driver and rework initialization

[Rafael J. Wysocki]

On Tue, Mar 17, 2026 at 5:43 PM Simon Horman <horms@kernel.org> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
>
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> net: fjes: Drop fjes_acpi_driver and rework initialization
>
> This commit reworks the Fujitsu Extended Socket (fjes) Network Device
> driver initialization by removing the ACPI driver interface and
> creating the platform device directly during module initialization.
>
> Link: https://lore.kernel.org/all/2396510.ElGaqSPkdT@rafael.j.wysocki/ [1]
>
> > diff --git a/drivers/net/fjes/fjes_main.c b/drivers/net/fjes/fjes_main.c
> > index b63965d9a1ba..efb399eb15e0 100644
> > --- a/drivers/net/fjes/fjes_main.c
> > +++ b/drivers/net/fjes/fjes_main.c
>
> [ ... ]
>
> > @@ -1518,16 +1506,7 @@ static int __init fjes_init_module(void)
> >               return result;
> >       }
> >
> > -     result = acpi_bus_register_driver(&fjes_acpi_driver);
> > -     if (result < 0)
> > -             goto fail_acpi_driver;
> > -
> >       return 0;
> > -
> > -fail_acpi_driver:
> > -     platform_driver_unregister(&fjes_driver);
> > -     fjes_dbg_exit();
> > -     return result;
> >  }
>
> In fjes_init_module(), if platform_driver_register() fails, does this
> leak the platform device?
>
> Looking at the new code, the platform device is successfully registered
> earlier in the function:
>
>         fjes_plat_dev = platform_device_register_full(&pdevinfo);
>         ...
>         if (IS_ERR(fjes_plat_dev))
>                 return PTR_ERR(fjes_plat_dev);
>
>         ...
>         result = platform_driver_register(&fjes_driver);
>
> But when platform_driver_register() fails, the error path only calls
> fjes_dbg_exit() and returns:
>
>         result = platform_driver_register(&fjes_driver);
>         if (result < 0) {
>                 fjes_dbg_exit();
>                 return result;
>         }
>
> Since the module init fails, fjes_exit_module() will never run, so
> the platform_device_unregister() call there won't happen either. Should
> the error path call platform_device_unregister(fjes_plat_dev) before
> returning?

Yes, it should.

I'll send a v2, thanks!