On Fri, May 08, 2026 at 03:01:14PM -0700, Jakub Kicinski wrote:
> 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?
Sorry for delayed response.
This is a valid concern. I will update it to mana_remove(&gc->mana, false) in
the next version.
>
> > 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?
>
This is false positive.
The per-vPort debugfs entries are recreated during resume. The flow is:
Suspend: mana_gd_cleanup_device() removes the entire debugfs tree.
Resume: mana_gd_setup() recreates the top-level mana_pci_debugfs
directory. Then mana_probe(gd, true) takes the resuming=true path,
which calls mana_attach() for each port. mana_attach() calls
mana_init_port(), which recreates the vportN directory and all
per-vPort debugfs files (port_handle, max_sq, max_rq, steer_*,
current_speed).
So the per-vPort entries are not permanently lost, they survive the
suspend/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()?
This is a false positive.
The per-vPort debugfs files are removed before free_netdev() frees apc.
In mana_remove(gd, false), the sequence for each port is:
mana_detach(ndev, false) -> mana_cleanup_port_context(apc)
-> debugfs_remove(apc->mana_port_debugfs).
This removes the entire vportN directory and all its child files, since
debugfs_remove() is recursive (simple_recursive_removal()).
free_netdev(ndev) which frees apc.
Because step 1 removes all debugfs files referencing apc fields before
step 2 frees the memory, there is no window where the files point to
freed memory.
