Re: [PATCH v3 net-next 02/15] net/nebula-matrix: add our driver architecture

[Andrew Lunn <andrew@lunn.ch>]

> +	NBL_CHAN_MGT_TO_COMMON(&(*chan_mgt_leonis)->chan_mgt) = common;

Macros on the left hand side is pretty unusual. Why is it needed?

The cast suggests your types are messed up. You should not need casts.

> +#define nbl_err(common, fmt, ...)					\
> +do {									\
> +	typeof(common) _common = (common);				\
> +		dev_err(NBL_COMMON_TO_DEV(_common), fmt, ##__VA_ARGS__);\
> +} while (0)

You should probably remove these and just use dev_err() or
netdev_err() directly.

> +#define NBL_COMMON_TO_PDEV(common)		((common)->pdev)
> +#define NBL_COMMON_TO_DEV(common)		((common)->dev)
> +#define NBL_COMMON_TO_DMA_DEV(common)		((common)->dma_dev)
> +#define NBL_COMMON_TO_VSI_ID(common)		((common)->vsi_id)
> +#define NBL_COMMON_TO_ETH_ID(common)		((common)->eth_id)
> +#define NBL_COMMON_TO_ETH_MODE(common)		((common)->eth_mode)
> +#define NBL_COMMON_TO_DEBUG_LVL(common)		((common)->debug_lvl)
> +
> +#define NBL_COMMON_TO_OCP_CAP(common)		((common)->is_ocp)
> +#define NBL_COMMON_TO_PCI_USING_DAC(common)	((common)->pci_using_dac)
> +#define NBL_COMMON_TO_MGT_PF(common)		((common)->mgt_pf)
> +#define NBL_COMMON_TO_PCI_FUNC_ID(common)	((common)->function)
> +#define NBL_COMMON_TO_LOGIC_ETH_ID(common)	((common)->logic_eth_id)

These are all just obfuscation. Please remove them. And not just
these. All of them. Such macros don't help.

> +int nbl_dev_init(void *p, struct nbl_init_param *param);
> +void nbl_dev_remove(void *p);
> +int nbl_dev_start(void *p, struct nbl_init_param *param);
> +void nbl_dev_stop(void *p);

Using a void * as a parameter for something like nbl_dev_start() and
nbl_dev_stop() is a red flag. You should know they type you are
passing to functions like this. In general, you want the compiler to
be doing type checking for you, so you need real types.

> +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;
> +	int ret = 0;

Reverse Christmas tree. That applies to all functions.

> +
> +	if (!pdev)
> +		return NULL;

Can that happen? Don't have defensive code. If there is a real reason
this could be NULL then fine, but you might want to add a comment why
it can happen. But if this is not actually expected let pdev be
dereferenced, get the Opps, so you can debug your driver. And i don't
mean this one instance. I mean all similar tests in the driver. No
defensive code, it just hides bugs.

> +void nbl_core_remove(struct nbl_adapter *adapter)
> +{
> +	struct nbl_product_base_ops *product_base_ops;
> +	struct device *dev;
> +
> +	dev = NBL_ADAP_TO_DEV(adapter);
> +	product_base_ops = NBL_ADAP_TO_RPDUCT_BASE_OPS(adapter);
> +	nbl_dev_remove(adapter);
> +	nbl_serv_remove(adapter);
> +	nbl_disp_remove(adapter);
> +	product_base_ops->res_remove(adapter);
> +	product_base_ops->chan_remove(adapter);
> +	product_base_ops->hw_remove(adapter);
> +	devm_kfree(dev, adapter);

Calling devm_kfree() is unusual. Why do you do this? I suggests you
don't understand what devm_ actually does. Which is pretty scary if
you have such a basic thing wrong for a driver this size.

>  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;
>  
> +	if (pci_enable_device(pdev)) {
> +		dev_err(&pdev->dev, "Failed to enable PCI device\n");
> +		return -ENODEV;
> +	}
> +
> +	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_err(dev, "Configure DMA 64 bit mask failed, err = %d\n",
> +			err);

dev_err() is usually for a fatal error. Here you just keep going, so
it is not really an error. dev_dbg()?

    Andrew

---
pw-bot: cr