Re: [PATCH net-next v8] net: mana: Expose hardware diagnostic info via debugfs

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: mana: Expose hardware diagnostic info via debugfs

Add debugfs entries to expose hardware configuration and diagnostic
information that aids in debugging driver initialization and runtime
operations without adding noise to dmesg.

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
[ ... ]
> @@ -2231,13 +2239,18 @@ int mana_gd_resume(struct pci_dev *pdev)
>  
>  	err = mana_probe(&gc->mana, true);
>  	if (err)
> -		return err;
> +		goto cleanup_gd;
>  
>  	err = mana_rdma_probe(&gc->mana_ib);
>  	if (err)
> -		return err;
> +		goto cleanup_mana;
>  
>  	return 0;
> +cleanup_mana:
> +	mana_remove(&gc->mana, true);
> +cleanup_gd:
> +	mana_gd_cleanup_device(pdev);
> +	return err;
>  }

Since mana_remove() called with true for suspending skips
unregister_netdevice(), the network devices remain registered in the system.

If mana_gd_cleanup_device() is then called, which destroys the hardware
channel and clears gc->hwc.driver_data, could this lead to a NULL pointer
dereference if userspace later interacts with the still-registered devices?

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
[ ... ]
> @@ -3161,6 +3169,23 @@ static int mana_init_port(struct net_device *ndev)
>  	eth_hw_addr_set(ndev, apc->mac_addr);
>  	sprintf(vport, "vport%d", port_idx);
>  	apc->mana_port_debugfs = debugfs_create_dir(vport, gc->mana_pci_debugfs);
> +
> +	debugfs_create_u64("port_handle", 0400, apc->mana_port_debugfs,
> +			   &apc->port_handle);

When the device suspends, mana_gd_suspend() calls mana_gd_cleanup_device(),
which recursively removes the entire debugfs directory tree.

During resume, the parent debugfs directory is recreated, but mana_probe()
skips calling mana_init_port() for existing ports.

Does this mean the per-vPort debugfs entries are permanently lost after a
suspend and resume cycle?

[ ... ]
> @@ -3800,6 +3830,9 @@ void mana_remove(struct gdma_dev *gd, bool suspending)
>  
>  	mana_gd_deregister_device(gd);
>  
> +	debugfs_lookup_and_remove("bm_hostmode", gc->mana_pci_debugfs);
> +	debugfs_lookup_and_remove("num_vports", gc->mana_pci_debugfs);
> +
>  	if (suspending)
>  		return;

Earlier in mana_remove(), free_netdev() is called, which frees the port
context structure (apc) allocated via netdev_priv(ndev).

Since the per-vPort debugfs files created in mana_init_port() directly
reference memory within apc, and apc->mana_port_debugfs is not explicitly
removed here, does this leave the debugfs files accessible but pointing to
freed memory until the parent directory is finally removed in
mana_gd_cleanup_device()?