linux-kernel - Re: [PATCH v4 net-next 02/11] net/nebula-matrix: add our driver architecture

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <769fbfa8-a881-403f-bc65-56c60c389185@lunn.ch>
Date: Fri, 6 Feb 2026 04:39:42 +0100
From: Andrew Lunn <andrew@...n.ch>
To: "illusion.wang" <illusion.wang@...ula-matrix.com>
Cc: dimon.zhao@...ula-matrix.com, alvin.wang@...ula-matrix.com,
	sam.chen@...ula-matrix.com, netdev@...r.kernel.org,
	andrew+netdev@...n.ch, corbet@....net, kuba@...nel.org,
	linux-doc@...r.kernel.org, lorenzo@...nel.org, pabeni@...hat.com,
	horms@...nel.org, vadim.fedorenko@...ux.dev,
	lukas.bulwahn@...hat.com, edumazet@...gle.com,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 net-next 02/11] net/nebula-matrix: add our driver
 architecture

> +static int
> +nbl_chan_setup_chan_mgt(struct nbl_adapter *adapter,
> +			struct nbl_init_param *param,
> +			struct nbl_channel_mgt_leonis **chan_mgt_leonis)

It is pretty unusual to pass ** pointers. More normal would be 


> +static struct nbl_channel_mgt_leonis *
> +nbl_chan_setup_chan_mgt(struct nbl_adapter *adapter,
> +			struct nbl_init_param *param)

> +{
> +	struct nbl_hw_ops_tbl *hw_ops_tbl = adapter->intf.hw_ops_tbl;
> +	struct nbl_common_info *common = &adapter->common;
	struct nbl_channel_mgt_leonis *chan_mgt_leonis;
> +	struct device *dev = &adapter->pdev->dev;
> +	struct nbl_chan_info *mailbox;
> +
> +	chan_mgt_leonis = devm_kzalloc(dev,
> +					sizeof(struct nbl_channel_mgt_leonis),
> +					GFP_KERNEL);
> +	if (!chan_mgt_leonis)
> +		goto alloc_channel_mgt_leonis_fail;
> +
> +	chan_mgt_leonis->chan_mgt->common = common;
> +	chan_mgt_leonis->chan_mgt.hw_ops_tbl = hw_ops_tbl;
> +
> +	mailbox = devm_kzalloc(dev, sizeof(struct nbl_chan_info), GFP_KERNEL);
> +	if (!mailbox)
> +		goto alloc_mailbox_fail;
> +	mailbox->chan_type = NBL_CHAN_TYPE_MAILBOX;
> +	chan_mgt_leonis->chan_mgt->chan_info[NBL_CHAN_TYPE_MAILBOX] =
> +		mailbox;
> +
	return chan_mgt_leonis;
	
> +alloc_mailbox_fail:
> +alloc_channel_mgt_leonis_fail:
> +	return ERR_PTR(-ENOMEM);

This is starting to look like Linux driver code now. Lots of () have
been removed. Your aim is to make the driver look like all other
drivers in linux, by copying the common patterns.

I would probably also remove the two labels, and just due the return
ERR_PTR() inline.

Please spend some time reading other drivers. Learn from them, and
make your driver look like them.

> +#define NBL_ADAP_TO_HW_MGT(adapter) ((adapter)->core.hw_mgt)
> +#define NBL_ADAP_TO_RES_MGT(adapter) ((adapter)->core.res_mgt)
> +#define NBL_ADAP_TO_DISP_MGT(adapter) ((adapter)->core.disp_mgt)
> +#define NBL_ADAP_TO_DEV_MGT(adapter) ((adapter)->core.dev_mgt)
> +#define NBL_ADAP_TO_CHAN_MGT(adapter) ((adapter)->core.chan_mgt)

I would suggest removing these. Just use adapter->core.chan_mgt
directly in the code. Using macros normally means there is some magic
involved, but this is just plain pointers, nothing magical.

> +int nbl_hw_init_leonis(struct nbl_adapter *adapter,
> +		       struct nbl_init_param *param)
> +{
> +	struct nbl_hw_ops_tbl **hw_ops_tbl = &adapter->intf.hw_ops_tbl;
> +	struct nbl_common_info *common = &adapter->common;
> +	struct nbl_hw_mgt_leonis **hw_mgt_leonis;
> +	struct pci_dev *pdev = common->pdev;
> +	struct nbl_hw_mgt *hw_mgt;
> +	int bar_mask;
> +	int ret = 0;
> +
> +	hw_mgt_leonis =
> +		(struct nbl_hw_mgt_leonis **)&NBL_ADAP_TO_HW_MGT(adapter);

Why the cast? Cast like this suggest your data structure design is not
correct.

> +static inline u32 rd32(u8 __iomem *addr, u64 reg)
> +{
> +	return readl(addr + (reg));
> +}
> +
> +static inline void wr32_barrier(u8 __iomem *addr, u64 reg, u32 value)
> +{
> +	writel((value), (addr + (reg)));
> +}

Why _barrier here for write, but not for rd32()? readl() has a barrier
same as writel()? Should rd32 actually be using readl_relaxed()?

> +static inline void nbl_hw_rd_regs(struct nbl_hw_mgt *hw_mgt, u64 reg, u8 *data,
> +				  u32 len)
> +{
> +	u32 size = len / 4;
> +	u32 i = 0;
> +
> +	if (len % 4)
> +		return;

It is actually a bug?

> +
> +	spin_lock(&hw_mgt->reg_lock);
> +
> +	for (i = 0; i < size; i++)
> +		*(u32 *)(data + i * sizeof(u32)) =
> +			rd32(hw_mgt->hw_addr, reg + i * sizeof(u32));
> +	spin_unlock(&hw_mgt->reg_lock);
> +}

This function is a bit big for inline. Have you used bloat-o-meter to
look at the size with this and the next as functions vs inline?

> +static inline void nbl_hw_wr_regs(struct nbl_hw_mgt *hw_mgt, u64 reg,
> +				  const u8 *data, u32 len)
> +{
> +	u32 size = len / 4;
> +	u32 i = 0;
> +
> +	if (len % 4)
> +		return;
> +	spin_lock(&hw_mgt->reg_lock);
> +	for (i = 0; i < size; i++)
> +		/* Used for emu, make sure that we won't write too frequently */
> +		wr32_barrier(hw_mgt->hw_addr, reg + i * sizeof(u32),
> +			     *(u32 *)(data + i * sizeof(u32)));
> +	spin_unlock(&hw_mgt->reg_lock);
> +}

	Andrew