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()?
