Re: [PATCH net-next 1/3] net/ethernet: add ZTE network driver support

[Junyang Han <han.junyang@zte.com.cn>]

[-- Attachment #1.1.1: Type: text/plain, Size: 4071 bytes --]

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

[-- Attachment #1.1.2: Type: text/html , Size: 7872 bytes --]