Re: [PATCH net-next 2/3] net/ethernet/zte/dinghai: add logging infrastructure

[Andrew Lunn <andrew@lunn.ch>]

> +#define dh_dbg(__dev, format, ...)                    \
> +    dev_dbg((__dev)->device, "%s:%d:(pid %d): " format,        \
> +         __func__, __LINE__, current->pid,            \
> +         ##__VA_ARGS__)
> +
> +#define dh_dbg_once(__dev, format, ...)                \
> +    dev_dbg_once((__dev)->device,                \
> +             "%s:%d:(pid %d): " format,            \
> +             __func__, __LINE__, current->pid,        \
> +             ##__VA_ARGS__)

There is a dislike for adding wrappers around the existing kernel log
functions. What value does this add?

> +int debug_print;
> +module_param(debug_print, int, 0644);

Module parameters are not liked.

> +module_param_named(debug_mask, dh_debug_mask, uint, 0644);
> +MODULE_PARM_DESC(debug_mask, "debug mask: 1 = dump cmd data, 2 =
>  dump cmd exec time, 3 = both. Default=0");
> +static bool probe_vf = 1;
> +module_param(probe_vf, bool, 0644);
> +MODULE_PARM_DESC(probe_vf, "probe_vf: 0 = N, 1 = Y");

I suggest you remove all these.

Debug code is reasonable to have during development. But once the
driver actually works, it should not be needed.

>  const struct pci_device_id dh_pf_pci_table[] = {
> @@ -39,26 +47,34 @@ static int dh_pf_pci_init(struct dh_core_dev *dev)
>      pci_set_drvdata(dev->pdev, dev);
>  
>      ret = pci_enable_device(dev->pdev);
> -    if (ret)
> +    if (ret) {
> +        LOG_ERR("pci_enable_device failed: %d\n", ret);

dev_err(dev, ..)

>          return -ENOMEM;
> +    }
>  
>      ret = dma_set_mask_and_coherent(dev->device, DMA_BIT_MASK(64));
>      if (ret) {
>          ret = dma_set_mask_and_coherent(dev->device, DMA_BIT_MASK(32));
> -        if (ret)
> +        if (ret) {
> +            LOG_ERR("dma_set_mask_and_coherent failed: %d\n", ret);

dev_err(dev, ...

And maybe it should actually be dev_err_probe() if this is a probe
function, and you need to handle -EPROBE_DEFER.

Debug logging should be limited to actual debug stuff. Please use the
standard logging primitives for anything which is not debug, so
dev_err(), dev_warn(), dev_info(). netdev_err(), netdev_warn() etc.

	Andrew