Hi Andrew,
Thank you for your detailed review. Please see my responses below.
> Please always include a patch 0/X in a patch set, explaining the big
> picture.
Cover letter (patch 0/X) has been added in v2 explaining the big picture.
> + * ZTE DingHai Ethernet driver
> + * Copyright (c) 2022-2024, ZTE Corporation.
> And the last two years?
Copyright year updated to 2022-2026.
> +#define DRV_VERSION "1.0-1"
> Driver versions are generally useless. What does this actually mean
> for the given very limited driver? Are you going to change the version
> with each patchset?
Removed. Driver version is not needed for upstream submission.
> +#define DRV_SUMMARY "ZTE(R) zxdh-net driver"
> +
> +const char zxdh_pf_driver_version[] = DRV_VERSION;
> +static const char zxdh_pf_driver_string[] = DRV_SUMMARY;
> +static const char zxdh_pf_copyright[] = "Copyright (c)
> 2022-2024, ZTE Corporation.";
> You don't need this, you have the copyright above.
Removed all these. The copyright in the file header is sufficient.
> +MODULE_AUTHOR("ZTE");
> Author is a person, with an email address.
Fixed. Now using:
MODULE_AUTHOR("Junyang Han <han.junyang@zte.com.cn>");
> +MODULE_DESCRIPTION(DRV_SUMMARY);
> Please just put the string here, not #define.
Fixed. Now using:
MODULE_DESCRIPTION("ZTE Corporation network adapters (DingHai series) Ethernet driver");
> +MODULE_VERSION(DRV_VERSION);
> +MODULE_LICENSE("GPL");
> +static int dh_pf_pci_init(struct dh_core_dev *dev)
> +{
> + int ret = 0;
> + struct zxdh_pf_device *pf_dev = NULL;
> Reverse Christmas tree. This applies everywhere for a netdev driver.
Fixed. Function variable declarations now follow reverse Christmas tree.
> +static int dh_pf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +err_cfg_init:
> + mutex_destroy(&pf_dev->irq_lock);
> + mutex_destroy(&dh_dev->lock);
> + devlink_free(devlink);
> + pf_dev = NULL;
> Since this is a probe function, do you really need to set pf_dev to
> NULL? How is it going to keep a value over EPROBE_DEFER cycles?
Removed. pf_dev doesn't need to be set to NULL.
> +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_core_priv(dh_dev);
> +
> + if (!dh_dev)
> + return;
> How does that happen?
Removed the NULL check.
> + dh_pf_pci_close(dh_dev);
> + mutex_destroy(&pf_dev->irq_lock);
> + mutex_destroy(&dh_dev->lock);
> + devlink_free(devlink);
> + pci_set_drvdata(pdev, NULL);
> +}
> +static int dh_pf_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> + return 0;
> +}
> +
> +static int dh_pf_resume(struct pci_dev *pdev)
> +{
> + return 0;
> +}
> If they do nothing, don't provide them. You can add them when you add
> suspend/resume support.
Removed. Suspend/resume will be added when needed.
> +static int __init dh_pf_pci_init_module(void)
> +{
> + return pci_register_driver(&dh_pf_driver);
> +}
> +
> +static void __exit dh_pf_pci_exit_module(void)
> +{
> + pci_unregister_driver(&dh_pf_driver);
> +}
> +
> +module_init(dh_pf_pci_init_module);
> +module_exit(dh_pf_pci_exit_module);
> The PCI subsystem offers a wrapper to do this.
Fixed. Now using module_pci_driver(dh_pf_driver).
> +struct dh_core_dev {
> + struct device *device;
> + enum dh_coredev_type coredev_type;
> + struct pci_dev *pdev;
> + struct devlink *devlink;
> + struct mutex lock; /* Protects device configuration */
> + char priv[] __aligned(32);
> That is unusual. priv is usually a void * and allocated. If you want
> an actual array, you might want to have a second member indicate the
> size of the array, look at all the work done recently on flexible
> arrays.
Fixed. Changed to void *priv, allocated dynamically with kzalloc().
Best regards,
Junyang Han