Message-ID: <8641f978-76d5-464f-a312-414bd913c918.illusion.wang@nebula-matrix.com>
Date: Fri, 06 Feb 2026 17:26:35 +0800
From: "Illusion Wang" <illusion.wang@...ula-matrix.com>
To: "Andrew Lunn" <andrew@...n.ch>
Cc: "Dimon" <dimon.zhao@...ula-matrix.com>,
  "Alvin" <alvin.wang@...ula-matrix.com>,
  "Sam" <sam.chen@...ula-matrix.com>,
  "netdev" <netdev@...r.kernel.org>,
  "andrew+netdev" <andrew+netdev@...n.ch>,
  "corbet" <corbet@....net>,
  "kuba" <kuba@...nel.org>,
  "linux-doc" <linux-doc@...r.kernel.org>,
  "lorenzo" <lorenzo@...nel.org>,
  "pabeni" <pabeni@...hat.com>,
  "horms" <horms@...nel.org>,
  "vadim.fedorenko" <vadim.fedorenko@...ux.dev>,
  "lukas.bulwahn" <lukas.bulwahn@...hat.com>,
  "edumazet" <edumazet@...gle.com>,
  "open list" <linux-kernel@...r.kernel.org>
Subject: 回复：[PATCH v4 net-next 02/11] net/nebula-matrix: add our driver architecture

Last time sam had a question
"
Thank you for your feedback. You might have misunderstood me.
Our difficulties lie in the following:
1. Assuming only the mainline version changes the name (Assume name "nbl"),
   and our regularly released driver doesn't change its name, then when
   customers upgrade to a new kernel (containing the "nbl" driver),
   and then want to update our regularly released driver (named "nbl_core"),
   the module (ko) conflict will occur.
2. If both our mainline and regularly released drivers change their names,
   then customers who are already using the "nbl_core" driver will also
   encounter conflict issues when updating to the new driver "nbl".

Is it possible to do this: our net driver is also modified to be a driver based
on the auxiliary bus, while the PCIe driver only handles PCIe-related processing,
and these two drivers share a single kernel module (ko), namely "nbl_core"?"

There's no conclusion to this issue yet, so I haven't modified the 'core' parts for now
(as mentioned in patch0)
--illusion.wang


> +#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.



------------------------------------------------------------------
发件人：Andrew Lunn <andrew@...n.ch>
发送时间：2026年2月6日(周五) 11:39
收件人：Illusion Wang<illusion.wang@...ula-matrix.com>
抄　送：Dimon<dimon.zhao@...ula-matrix.com>; Alvin<alvin.wang@...ula-matrix.com>; Sam<sam.chen@...ula-matrix.com>; netdev<netdev@...r.kernel.org>; "andrew+netdev"<andrew+netdev@...n.ch>; corbet<corbet@....net>; kuba<kuba@...nel.org>; "linux-doc"<linux-doc@...r.kernel.org>; lorenzo<lorenzo@...nel.org>; pabeni<pabeni@...hat.com>; horms<horms@...nel.org>; "vadim.fedorenko"<vadim.fedorenko@...ux.dev>; "lukas.bulwahn"<lukas.bulwahn@...hat.com>; edumazet<edumazet@...gle.com>; open list<linux-kernel@...r.kernel.org>
主　题：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

Powered by blists - more mailing lists