Re: [PATCH v2] rtl_pci: add support for Realtek management device

["Arnd Bergmann" <arnd@arndb.de>]

On Tue, Mar 3, 2026, at 07:18, javen wrote:
> From: Javen Xu <javen_xu@realsil.com.cn>
>
> This is not the standard ethernet driver. It specially handles the
> management function.
>
> I tried to submitted to linux/net-next before, but they said these class
> didn't belong there. So I came here, and I'm wondering if these unknown
> classes used for management functions should be submitted here.

It depends on what the driver ends up doing. The driver you posted
here does not appear to actually do anything, so it's hard to tell.

> +/*
> + *  This module handles PCI endpoint functions exposed by Realtek
> + *  management controllers (e.g. RTL8111x series). It manages device
> + *  probing for virtual devices.
> + *
> + *  Copyright(c) 2026 Realtek Semiconductor Corp.
> + */

RTL8111x seems to refer to Ethernet controllers.

> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/init.h>
> +
> +#define PCI_DEVICE_ID_REALTEK_PTOU      0x8164
> +#define PCI_DEVICE_ID_REALTEK_COM1      0x816a
> +#define PCI_DEVICE_ID_REALTEK_COM2      0x816b
> +#define PCI_DEVICE_ID_REALTEK_IPMI      0x816c
> +#define PCI_DEVICE_ID_REALTEK_BMC       0x816e
> +#define PCI_DEVICE_ID_REALTEK_PCIBR     0x9151

The naming here suggests that these would be some type of
PC southbridge chip, not an ethernet controller, and that
they are likely devices that would each need an actual driver
to communicate with the device.

> +static int rtl_probe(struct pci_dev *pdev,
> +			 const struct pci_device_id *ent)
> +{
> +	int rc;
> +
> +	/* enable device (incl. PCI PM wakeup and hotplug setup) */
> +	rc = pcim_enable_device(pdev);
> +	if (rc < 0)
> +		return dev_err_probe(&pdev->dev, rc, "enable failure\n");
> +
> +	dev_info(&pdev->dev, "enable device\n");
> +
> +	return rc;
> +}
> +
> +static void rtl_remove(struct pci_dev *pdev) {}

This seems to be the only part of the driver that does anything:
you enable the device, but then ignore it afterwards, which seems
pointless when reading the driver.

Please make sure that there is enough context here that a reader
can understand why this is done. Are you planning to add actual
functionality later, are you just working around some unusual
requirements of the hardware, or are you trying to prevent
the devices from being bound to another driver?

> +static int rtl_pm_suspend(struct device *device)
> +{
> +	return 0;
> +}
> +
> +static int rtl_pm_resume(struct device *device)
> +{
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops rtl_pm_ops = {
> +	SYSTEM_SLEEP_PM_OPS(rtl_pm_suspend, rtl_pm_resume)
> +};

These also do nothing and can likely be left out.

     Arnd