Re: [PATCH v1 net-next 02/15] net/nebula-matrix: add simple probe/remove.

[Andrew Lunn <andrew@lunn.ch>]

> +/* debug masks - set these bits in adapter->debug_mask to control output */
> +enum nbl_debug_mask {
> +	/* BIT0~BIT30 use to define adapter debug_mask */
> +	NBL_DEBUG_MAIN			= 0x00000001,
> +	NBL_DEBUG_COMMON		= 0x00000002,
> +	NBL_DEBUG_DEBUGFS		= 0x00000004,
> +	NBL_DEBUG_HW			= 0x00000008,
> +	NBL_DEBUG_FLOW			= 0x00000010,
> +	NBL_DEBUG_RESOURCE		= 0x00000020,
> +	NBL_DEBUG_QUEUE			= 0x00000040,
> +	NBL_DEBUG_INTR			= 0x00000080,
> +	NBL_DEBUG_ADMINQ		= 0x00000100,
> +	NBL_DEBUG_DEVLINK		= 0x00000200,
> +	NBL_DEBUG_ACCEL			= 0x00000400,
> +	NBL_DEBUG_MBX			= 0x00000800,
> +	NBL_DEBUG_ST			= 0x00001000,
> +	NBL_DEBUG_VSI			= 0x00002000,
> +	NBL_DEBUG_CUSTOMIZED_P4		= 0x00004000,
> +
> +	/* BIT31 use to distinguish netif debug level or adapter debug_mask */
> +	NBL_DEBUG_USER			= 0x80000000,
> +
> +	/* Means turn on all adapter debug_mask */
> +	NBL_DEBUG_ALL			= 0xFFFFFFFF
> +};
> +
> +#define nbl_err(common, lvl, fmt, ...)						\
> +do {										\
> +	typeof(common) _common = (common);					\
> +	if (((lvl) & NBL_COMMON_TO_DEBUG_LVL(_common)))				\
> +		dev_err(NBL_COMMON_TO_DEV(_common), fmt, ##__VA_ARGS__);	\
> +} while (0)

Please try to make use of msg_level, netif_msg_init() etc.

> +#define NBL_OK 0
> +#define NBL_CONTINUE 1
> +#define NBL_FAIL -1

You don't use these in this patch, so i cannot see how they are
actually used. But generally, you should use error codes, not -1.

Also, please only add things in a patch which are used by the
patch. Otherwise it makes it hard to review.

> +struct nbl_adapter *nbl_core_init(struct pci_dev *pdev, struct nbl_init_param *param)
> +{
> +	struct nbl_adapter *adapter;
> +	struct nbl_common_info *common;
> +	struct nbl_product_base_ops *product_base_ops;
> +
> +	if (!pdev)
> +		return NULL;
> +
> +	adapter = devm_kzalloc(&pdev->dev, sizeof(struct nbl_adapter), GFP_KERNEL);
> +	if (!adapter)
> +		return NULL;
> +
> +	adapter->pdev = pdev;
> +	common = NBL_ADAPTER_TO_COMMON(adapter);
> +
> +	NBL_COMMON_TO_PDEV(common) = pdev;
> +	NBL_COMMON_TO_DEV(common) = &pdev->dev;
> +	NBL_COMMON_TO_DMA_DEV(common) = &pdev->dev;
> +	NBL_COMMON_TO_DEBUG_LVL(common) |= NBL_DEBUG_ALL;
> +	NBL_COMMON_TO_VF_CAP(common) = param->caps.is_vf;
> +	NBL_COMMON_TO_OCP_CAP(common) = param->caps.is_ocp;
> +	NBL_COMMON_TO_PCI_USING_DAC(common) = param->pci_using_dac;
> +	NBL_COMMON_TO_PCI_FUNC_ID(common) = PCI_FUNC(pdev->devfn);

Macros like this are generally not used on the left side.

> +void nbl_core_remove(struct nbl_adapter *adapter)
> +{
> +	struct device *dev;
> +
> +	struct nbl_product_base_ops *product_base_ops;
> +
> +	if (!adapter)
> +		return;

How can that happen? If you are writing defensive code, it suggests
you don't actually understand how the driver and the kernel works.

>  static int nbl_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *id)
>  {
>  	struct device *dev = &pdev->dev;
> +	struct nbl_adapter *adapter = NULL;
> +	struct nbl_init_param param = {{0}};
> +	int err;
> +
> +	dev_info(dev, "nbl probe\n");

deb_debug(), or not at all.

>  
> +	err = pci_enable_device(pdev);
> +	if (err)
> +		return err;
> +
> +	param.pci_using_dac = true;
> +	nbl_get_func_param(pdev, id->driver_data, &param);
> +
> +	err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> +	if (err) {
> +		dev_info(dev, "Configure DMA 64 bit mask failed, err = %d\n", err);
> +		param.pci_using_dac = false;
> +		err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> +		if (err) {
> +			dev_err(dev, "Configure DMA 32 bit mask failed, err = %d\n", err);
> +			goto configure_dma_err;
> +		}
> +	}
> +
> +	pci_set_master(pdev);
> +
> +	pci_save_state(pdev);
> +
> +	adapter = nbl_core_init(pdev, &param);
> +	if (!adapter) {
> +		dev_err(dev, "Nbl adapter init fail\n");
> +		err = -EAGAIN;

EAGAIN is an odd code for a probe failure.

>  static void nbl_remove(struct pci_dev *pdev)
>  {
> +	struct nbl_adapter *adapter = pci_get_drvdata(pdev);
> +
> +	dev_info(&pdev->dev, "nbl remove\n");

All these dev_info() messages suggests you have not fully debugged
your driver, even the basics of probe and remove! Production quality
code should not need these.

	Andrew