Re: [PATCH net-next v7 1/2] dinghai: add ZTE network driver support

[Paolo Abeni]

From: AI Reviewer <ai@xxxxxxxxxxx>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/
---
dinghai: add ZTE network driver support

Add basic framework for ZTE DingHai ethernet PF driver, including
Kconfig/Makefile build support and PCIe device probe/remove skeleton.

> diff --git a/drivers/net/ethernet/zte/dinghai/en_pf.c b/drivers/net/ethernet/zte/dinghai/en_pf.c
> new file mode 100644
> index 0000000000000..b492fd2c261b8
> --- /dev/null
> +++ b/drivers/net/ethernet/zte/dinghai/en_pf.c
[ ... ]
> +static int dh_pf_pci_init(struct dh_core_dev *dev)
> +{
> +	struct zxdh_pf_device *pf_dev = dev->priv;
> +	int ret;
[ ... ]
> +	pf_dev->pci_ioremap_addr[0] =
> +		ioremap(pci_resource_start(dev->pdev, 0),
> +			pci_resource_len(dev->pdev, 0));

[Severity: Medium]
Should this check if BAR 0 is actually an IORESOURCE_MEM before mapping it?

Calling ioremap() on an I/O port address or an unpopulated BAR could map
arbitrary physical memory.

Would it be safer to use pci_ioremap_bar() or pcim_iomap() which enforce
resource type and length checks?

> +	if (!pf_dev->pci_ioremap_addr[0]) {
> +		ret = -ENOMEM;
> +		dev_err(dev->device, "dh pf pci ioremap failed\n");
> +		goto err_pci_save_state;
> +	}
[ ... ]
> +static void dh_pf_remove(struct pci_dev *pdev)
> +{
> +	struct dh_core_dev *dh_dev = pci_get_drvdata(pdev);
> +	struct devlink *devlink = priv_to_devlink(dh_dev);
> +	struct zxdh_pf_device *pf_dev = dh_dev->priv;
> +
> +	devlink_unregister(devlink);
> +	dh_pf_pci_close(dh_dev);
> +	mutex_destroy(&pf_dev->irq_lock);
> +	mutex_destroy(&dh_dev->lock);
> +	dh_core_free_priv(dh_dev);
> +	devlink_free(devlink);
> +	pci_set_drvdata(pdev, NULL);
> +}
> +
> +static void dh_pf_shutdown(struct pci_dev *pdev)
> +{
> +	struct dh_core_dev *dh_dev = pci_get_drvdata(pdev);
> +	struct devlink *devlink = priv_to_devlink(dh_dev);
> +	struct zxdh_pf_device *pf_dev = dh_dev->priv;
> +
> +	devlink_unregister(devlink);
> +	dh_pf_pci_close(dh_dev);
> +	mutex_destroy(&pf_dev->irq_lock);
> +	mutex_destroy(&dh_dev->lock);
> +	dh_core_free_priv(dh_dev);
> +	devlink_free(devlink);
> +	pci_set_drvdata(pdev, NULL);
> +}

[Severity: High]
Does this destructive teardown in the .shutdown callback create a race if
.remove is called afterwards?

If .remove is triggered (for example, during a hot-remove event during
shutdown), pci_get_drvdata() in dh_pf_remove() will return NULL.

The macro priv_to_devlink() will then convert NULL into a bad pointer,
causing a panic in devlink_unregister().

Should .shutdown only quiesce the hardware without destroying software
structures?
-- 
This is an AI-generated review.