回复：[PATCH v13 net-next 03/11] net/nebula-matrix: add chip related definitions

回复：[PATCH v13 net-next 03/11] net/nebula-matrix: add chip related definitions

[Illusion Wang]

[...]
>> +void nbl_write_all_regs(struct nbl_hw_mgt *hw_mgt)
>> +{
>> + struct nbl_common_info *common = hw_mgt->>common;
>> + u8 eth_mode = common->>eth_mode;
>> + const u32 *nbl_sec046_data;
>> + const u32 *nbl_sec071_data;
>> + u32 i;
>> +
>> + switch (eth_mode) {
>> + case 1:
>> +  nbl_sec046_data = nbl_sec046_1p_data;
>> +  nbl_sec071_data = nbl_sec071_1p_data;
>> +  break;
>> + case 2:
>> +  nbl_sec046_data = nbl_sec046_2p_data;
>> +  nbl_sec071_data = nbl_sec071_2p_data;
>> +  break;
>> + case 4:
>> +  nbl_sec046_data = nbl_sec046_4p_data;
>> +  nbl_sec071_data = nbl_sec071_4p_data;
>> +  break;
>> + default:
>> +  nbl_sec046_data = nbl_sec046_2p_data;
>> +  nbl_sec071_data = nbl_sec071_2p_data;
>> + }
>> +
>> + nbl_flush_writes(hw_mgt);
>> + for (i = 0; i < NBL_SEC006_SIZE; i++) {
>> +  if ((i + 1) % NBL_SEC_BLOCK_SIZE == 0)
>> +   nbl_hw_rd32(hw_mgt, NBL_HW_DUMMY_REG);
>> +
>> +  nbl_hw_wr32(hw_mgt, NBL_SEC006_REGI(i), nbl_sec006_data[i]);
>> + }
>> +
>> + nbl_flush_writes(hw_mgt);
>> + for (i = 0; i < NBL_SEC007_SIZE; i++)
>> +  nbl_hw_wr32(hw_mgt, NBL_SEC007_REGI(i), nbl_sec007_data[i]);
>> +
>> + nbl_flush_writes(hw_mgt);
>> + for (i = 0; i < NBL_SEC008_SIZE; i++) {
>> +  if ((i + 1) % NBL_SEC_BLOCK_SIZE == 0)
>> +   nbl_hw_rd32(hw_mgt, NBL_HW_DUMMY_REG);
>> +
>> +  nbl_hw_wr32(hw_mgt, NBL_SEC008_REGI(i), nbl_sec008_data[i]);
>> + }
>> +
>> + nbl_flush_writes(hw_mgt);
>> + for (i = 0; i < NBL_SEC009_SIZE; i++) {
>> +  if ((i + 1) % NBL_SEC_BLOCK_SIZE == 0)
>> +   nbl_hw_rd32(hw_mgt, NBL_HW_DUMMY_REG);
>> +
>> +  nbl_hw_wr32(hw_mgt, NBL_SEC009_REGI(i), nbl_sec009_data[i]);
>> + }
>> +
>> + nbl_flush_writes(hw_mgt);
>> + for (i = 0; i < NBL_SEC010_SIZE; i++)
>> +  nbl_hw_wr32(hw_mgt, NBL_SEC010_REGI(i), nbl_sec010_data[i]);
>> +
>> + nbl_flush_writes(hw_mgt);
>> + for (i = 0; i < NBL_SEC011_SIZE; i++) {
>> +  if ((i + 1) % NBL_SEC_BLOCK_SIZE == 0)
>> +   nbl_hw_rd32(hw_mgt, NBL_HW_DUMMY_REG);
>> +
>> +  nbl_hw_wr32(hw_mgt, NBL_SEC011_REGI(i), nbl_sec011_data[i]);
>> + }
>> +
>> + nbl_flush_writes(hw_mgt);
>> + for (i = 0; i < NBL_SEC012_SIZE; i++)
>> +  nbl_hw_wr32(hw_mgt, NBL_SEC012_REGI(i), nbl_sec012_data[i]);
>> +
>> + nbl_flush_writes(hw_mgt);
>> + for (i = 0; i < NBL_SEC013_SIZE; i++)
>> +  nbl_hw_wr32(hw_mgt, NBL_SEC013_REGI(i), nbl_sec013_data[i]);
>> +
>> + nbl_flush_writes(hw_mgt);
>> + for (i = 0; i < NBL_SEC014_SIZE; i++)
>> +  nbl_hw_wr32(hw_mgt, NBL_SEC014_REGI(i), nbl_sec014_data[i]);
>> +
>> + nbl_flush_writes(hw_mgt);
>> + for (i = 0; i < NBL_SEC022_SIZE; i++)
>> +  nbl_hw_wr32(hw_mgt, NBL_SEC022_REGI(i), nbl_sec022_data[i]);
>> +
>> + nbl_flush_writes(hw_mgt);
>> + for (i = 0; i < NBL_SEC023_SIZE; i++)
>> +  nbl_hw_wr32(hw_mgt, NBL_SEC023_REGI(i), nbl_sec023_data[i]);
>> +
>> + nbl_flush_writes(hw_mgt);
>> + for (i = 0; i < NBL_SEC024_SIZE; i++) {
>> +  if ((i + 1) % NBL_SEC_BLOCK_SIZE == 0)
>> +   nbl_hw_rd32(hw_mgt, NBL_HW_DUMMY_REG);
>> +
>> +  nbl_hw_wr32(hw_mgt, NBL_SEC024_REGI(i), nbl_sec024_data[i]);
>> + }
>> +
>> + nbl_flush_writes(hw_mgt);
>> + for (i = 0; i < NBL_SEC025_SIZE; i++) {
>> +  if ((i + 1) % NBL_SEC_BLOCK_SIZE == 0)
>> +   nbl_hw_rd32(hw_mgt, NBL_HW_DUMMY_REG);
>> +
>> +  nbl_hw_wr32(hw_mgt, NBL_SEC025_REGI(i), nbl_sec025_data[i]);
>> + }
>> +
>> + nbl_flush_writes(hw_mgt);
>> + for (i = 0; i < NBL_SEC026_SIZE; i++)
>> +  nbl_hw_wr32(hw_mgt, NBL_SEC026_REGI(i), nbl_sec026_data[i]);
>> +
>> + nbl_flush_writes(hw_mgt);
>> + for (i = 0; i < NBL_SEC027_SIZE; i++) {
>> +  if ((i + 1) % NBL_SEC_BLOCK_SIZE == 0)
>> +   nbl_hw_rd32(hw_mgt, NBL_HW_DUMMY_REG);
>> +
>> +  nbl_hw_wr32(hw_mgt, NBL_SEC027_REGI(i), nbl_sec027_data[i]);

>Sashiko says:

>Could this loop read past the end of the nbl_sec009_data array?
>The macro NBL_SEC009_SIZE is defined as 2048, but the nbl_sec009_data array
>contains significantly fewer elements (around 754). This appears to cause
>sequential out-of-bounds reads into the .rodata section, writing unrelated
>memory to the device registers.
>Similar size mismatches exist for nbl_sec025_data (262 elements vs size
>1024)
>and nbl_sec022_data (506 elements vs size 256).
>Would it be safer to use ARRAY_SIZE() to bound these iterations?

But I printed out the results: 
ARRAY_SIZE(nbl_sec009_data) equals NBL_SEC009_SIZE,
ARRAY_SIZE(nbl_sec025_data) equals NBL_SEC025_SIZE,
and ARRAY_SIZE(nbl_sec022_data) equals NBL_SEC022_SIZE.

Is the AI making a mistake here?

Re: 回复：[PATCH v13 net-next 03/11] net/nebula-matrix: add chip related definitions

[Andrew Lunn]

> But I printed out the results: 
> ARRAY_SIZE(nbl_sec009_data) equals NBL_SEC009_SIZE,
> ARRAY_SIZE(nbl_sec025_data) equals NBL_SEC025_SIZE,
> and ARRAY_SIZE(nbl_sec022_data) equals NBL_SEC022_SIZE.
> 
> Is the AI making a mistake here?

Just a guess, i've not looked at this patch at all.

Are you doing this on a 32 bit build? Maybe the AI is?

    Andrew

回复：回复：[PATCH v13 net-next 03/11] net/nebula-matrix: add chip related definitions

[Illusion Wang]

>> But I printed out the results: 
>> ARRAY_SIZE(nbl_sec009_data) equals NBL_SEC009_SIZE,
>> ARRAY_SIZE(nbl_sec025_data) equals NBL_SEC025_SIZE,
>> and ARRAY_SIZE(nbl_sec022_data) equals NBL_SEC022_SIZE.
>> 
>> Is the AI making a mistake here?

>Just a guess, i've not looked at this patch at all.

>Are you doing this on a 32 bit build? Maybe the AI is?

 >   Andrew

 We are using this on a 64 bit build.

回复：[PATCH v13 net-next 05/11] net/nebula-matrix: add channel layer

[Illusion Wang]

>> +static struct nbl_common_wq_mgt *wq_mgt;
>> +
>> +void nbl_common_queue_work(struct work_struct *task)
>> +{
>> +	queue_work(wq_mgt->ctrl_dev_wq, task);
>> +}
>> +
>> +void nbl_common_destroy_wq(void)
>> +{
>> +	destroy_workqueue(wq_mgt->ctrl_dev_wq);
>> +	kfree(wq_mgt);
>> +	wq_mgt = NULL;
>> +}
>> +
>> +int nbl_common_create_wq(void)
>> +{
>> +	wq_mgt = kzalloc_obj(*wq_mgt);
>> +	if (!wq_mgt)
>> +		return -ENOMEM;
>> +
>> +	wq_mgt->ctrl_dev_wq = alloc_workqueue("%s", WQ_MEM_RECLAIM | WQ_UNBOUND,
>> +					      0, "nbl_ctrldev_wq");
>> +	if (!wq_mgt->ctrl_dev_wq) {
>> +		pr_err("Failed to create workqueue nbl_ctrldev_wq\n");
>> +		goto alloc_ctrl_dev_wq_failed;
>> +	}
>> +
>> +	return 0;
>> +alloc_ctrl_dev_wq_failed:
>> +	kfree(wq_mgt);
>> +	wq_mgt = NULL;
>> +	return -ENOMEM;
>> +}
>Does this global singleton survive multiple adapter probes?
>wq_mgt is a file-scope static pointer. If nbl_common_create_wq() is
>called from a second probe, the prior wq_mgt is overwritten and the
>previous workqueue is leaked. On remove of the second device,
>nbl_common_destroy_wq() unconditionally calls destroy_workqueue() and
>kfree() and sets wq_mgt to NULL — so a later remove of the first
>device will dereference NULL in destroy_workqueue(wq_mgt->ctrl_dev_wq).
>nbl_common_queue_work() also dereferences wq_mgt with no NULL check.
>This patch does not wire up the callers, but the later series patch
>"net/nebula-matrix: add common dev start/stop operation"
>(1ff0ad206da3) does. Would refcounting create/destroy, or switching to
>per-adapter workqueues, address this?

In the last patch:
static int __init nbl_module_init(void)
{
	int status;

	status = nbl_common_create_wq();
	if (status) {
		pr_err("Failed to create wq, err = %d\n", status);
		goto wq_create_failed;
	}
	status = pci_register_driver(&nbl_driver);
	if (status) {
		pr_err("Failed to register PCI driver, err = %d\n", status);
		goto pci_register_driver_failed;
	}

	return 0;

pci_register_driver_failed:
	nbl_common_destroy_wq();
wq_create_failed:
	return status;
}

static void __exit nbl_module_exit(void)
{
	pci_unregister_driver(&nbl_driver);
	nbl_common_destroy_wq();
}

module_init(nbl_module_init);
module_exit(nbl_module_exit);

The function nbl_common_create_wq is only called within nbl_module_init, and
nbl_common_destroy_wq is only called once within nbl_module_exit. So, is the
AI's suggestion also incorrect in this case?
---illusion

Re: 回复：回复：[PATCH v13 net-next 03/11] net/nebula-matrix: add chip related definitions

[Andrew Lunn]

On Sat, May 09, 2026 at 11:38:27AM +0800, Illusion Wang wrote:
> 
> >> But I printed out the results: 
> >> ARRAY_SIZE(nbl_sec009_data) equals NBL_SEC009_SIZE,
> >> ARRAY_SIZE(nbl_sec025_data) equals NBL_SEC025_SIZE,
> >> and ARRAY_SIZE(nbl_sec022_data) equals NBL_SEC022_SIZE.
> >> 
> >> Is the AI making a mistake here?
> 
> >Just a guess, i've not looked at this patch at all.
> 
> >Are you doing this on a 32 bit build? Maybe the AI is?
> 
>  >   Andrew
> 
>  We are using this on a 64 bit build.

And what happens with a 32 bit build? Try think of all the ways the AI
could be correct., and disprove them.

      Andrew