On Fri, May 01, 2026 at 05:54:09PM -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
>
> This patch adds debugfs entries under /sys/kernel/debug/mana/<BDF>/ to
> expose device-level and per-vPort hardware configuration and diagnostic
> information. Debugfs creation and removal are moved into mana_gd_setup()
> and a renamed mana_gd_cleanup_device() so probe, remove, suspend, resume
> and shutdown share a single path.
>
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > index a654b3699c4c..077d3a1ff6bf 100644
> > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
>
> [ ... ]
>
> > @@ -3659,6 +3684,11 @@ int mana_probe(struct gdma_dev *gd, bool resuming)
> >
> > ac->bm_hostmode = bm_hostmode;
> >
> > + debugfs_create_u16("num_vports", 0400, gc->mana_pci_debugfs,
> > + &ac->num_ports);
> > + debugfs_create_u8("bm_hostmode", 0400, gc->mana_pci_debugfs,
> > + &ac->bm_hostmode);
> > +
> > if (!resuming) {
> > ac->num_ports = num_ports;
> > } else {
>
> Is the num_vports debugfs file pointed at ac->num_ports before that
> field has been assigned from the queried num_ports value?
>
> On a fresh probe (!resuming), the sequence in mana_probe() is:
>
> err = mana_query_device_cfg(ac, ..., &num_ports, &bm_hostmode);
> ...
> ac->bm_hostmode = bm_hostmode;
>
> debugfs_create_u16("num_vports", 0400, gc->mana_pci_debugfs,
> &ac->num_ports);
> ...
>
> if (!resuming) {
> ac->num_ports = num_ports;
> } else {
> ...
> }
> ...
> if (ac->num_ports > MAX_PORTS_IN_MANA_DEV)
> ac->num_ports = MAX_PORTS_IN_MANA_DEV;
>
> During the window between debugfs_create_u16() and the assignment (and
> subsequent clamp to MAX_PORTS_IN_MANA_DEV), would a reader of the
> num_vports debugfs file observe 0 rather than the hardware-reported
> value?
>
> Would it be clearer to create this file after ac->num_ports has been
> assigned and clamped, so the exposed value matches what the commit
> message describes as "Device configuration"?
> --
> pw-bot: cr
Thankyou for pointing out, Jakub.
I will update the comment in the next version.
- Vennela
