Re: [net-next] 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.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: mana: Expose hardware diagnostic info via debugfs

This commit adds debugfs entries to expose hardware configuration and
diagnostic information for the MANA driver. It creates device-level and
port-level debugfs entries to aid in debugging initialization and runtime
operations.

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index aef8612b73cb..fb513d7802cb 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -152,6 +152,11 @@ static int mana_gd_query_max_resources(struct pci_dev *pdev)
>  	if (gc->max_num_queues > gc->num_msix_usable - 1)
>  		gc->max_num_queues = gc->num_msix_usable - 1;
>
> +	debugfs_create_u32("num_msix_usable", 0400, gc->mana_pci_debugfs,
> +			   &gc->num_msix_usable);
> +	debugfs_create_u32("max_num_queues", 0400, gc->mana_pci_debugfs,
> +			   &gc->max_num_queues);
> +
>  	return 0;
>  }
>
> @@ -1222,6 +1227,13 @@ int mana_gd_verify_vf_version(struct pci_dev *pdev)
>  		return err ? err : -EPROTO;
>  	}
>  	gc->pf_cap_flags1 = resp.pf_cap_flags1;
> +	gc->gdma_protocol_ver = resp.gdma_protocol_ver;
> +
> +	debugfs_create_x64("gdma_protocol_ver", 0400, gc->mana_pci_debugfs,
> +			   &gc->gdma_protocol_ver);
> +	debugfs_create_x64("pf_cap_flags1", 0400, gc->mana_pci_debugfs,
> +			   &gc->pf_cap_flags1);
> +

Do these debugfs entries get recreated on every suspend/resume cycle?

Both mana_gd_query_max_resources() and mana_gd_verify_vf_version() are
called from mana_gd_setup() during device initialization. On suspend,
mana_gd_suspend() calls mana_gd_cleanup() which doesn't remove these
debugfs entries. When mana_gd_resume() calls mana_gd_setup() again, it
attempts to create the same debugfs entries a second time.

This means debugfs_start_creating() in fs/debugfs/inode.c will print an
error for each duplicate file:

    "'num_msix_usable' already exists in 'mana/<slot>/'"
    "'max_num_queues' already exists in 'mana/<slot>/'"
    "'gdma_protocol_ver' already exists in 'mana/<slot>/'"
    "'pf_cap_flags1' already exists in 'mana/<slot>/'"

This produces 4 error messages in dmesg on every suspend/resume cycle,
contradicting the commit message's goal of operating "without adding
noise to dmesg."

Should these device-level entries be removed during suspend (in
mana_gd_cleanup or mana_gd_suspend), or should their creation be moved
to mana_gd_probe where they're only created once?

[ ... ]
-- 
pw-bot: cr