netdev - Re: [PATCH net-next 1/7] sfc: initial debugfs implementation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ab5b48cd-9c99-f1a0-45b7-d1182b9adaa8@gmail.com>
Date: Thu, 8 Feb 2024 21:25:26 +0000
From: Edward Cree <ecree.xilinx@...il.com>
To: "Nelson, Shannon" <shannon.nelson@....com>, edward.cree@....com,
 linux-net-drivers@....com, davem@...emloft.net, kuba@...nel.org,
 pabeni@...hat.com, edumazet@...gle.com
Cc: netdev@...r.kernel.org, habetsm.xilinx@...il.com,
 Jonathan Cooper <jonathan.s.cooper@....com>
Subject: Re: [PATCH net-next 1/7] sfc: initial debugfs implementation

On 15/12/2023 00:05, Nelson, Shannon wrote:
> It would be nice to have a couple of example paths listed here

Sure; added this to v2:
See included DOC: comment for directory structure; leaf nodes are
 rx_dma_len, rx_buffer_order, rx_buffer_truesize and interrupt_mode.
Is that what you had in mind?  Or more like
 "grep -H . /sys/kernel/debug/sfc" output on a running system?

>> +/* replace debugfs sym-links on net device rename */
>> +void efx_update_debugfs_netdev(struct efx_nic *efx)
>> +{
>> +       mutex_lock(&efx->debugfs_symlink_mutex);
>> +       if (efx->debug_symlink)
>> +               efx_fini_debugfs_netdev(efx->net_dev);
>> +       efx_init_debugfs_netdev(efx->net_dev);
>> +       mutex_unlock(&efx->debugfs_symlink_mutex);
>> +}
> 
> How necessary is this netdev symlink?  This seems like a bunch of extra maintenance.

AFAIK we've had it out-of-tree for a very long time and not found it
 to need any real maintenance effort.  And while it's not strictly
 necessary, it is fairly convenient.

>> +       /* Populate debugfs */
>> +#ifdef CONFIG_DEBUG_FS
>> +       rc = efx_init_debugfs_nic(efx);
>> +       if (rc)
>> +               pci_err(efx->pci_dev, "failed to init device debugfs\n");
>> +#endif
> 
> I don't think you need the ifdef here because you have the static version defined in debugfs.h

You're right; I'll fix these.

>> +#ifdef CONFIG_DEBUG_FS
>> +       mutex_lock(&efx->debugfs_symlink_mutex);
>> +       efx_fini_debugfs_netdev(efx->net_dev);
>> +       mutex_unlock(&efx->debugfs_symlink_mutex);
>> +#endif
> 
> Can you do the mutex dance inside of efx_fini_debugfs_netdev() and then not need the ifdef here?

Yes, although I needed to refactor slightly because it's also
 called by efx_update_debugfs_netdev() which is already holding
 the mutex.

>> diff --git a/drivers/net/ethernet/sfc/efx_common.c b/drivers/net/ethernet/sfc/efx_common.c
>> index 175bd9cdfdac..7a9d6b6b66e5 100644
>> --- a/drivers/net/ethernet/sfc/efx_common.c
>> +++ b/drivers/net/ethernet/sfc/efx_common.c
>> @@ -1022,6 +1022,9 @@ int efx_init_struct(struct efx_nic *efx, struct pci_dev *pci_dev)
>>          INIT_WORK(&efx->mac_work, efx_mac_work);
>>          init_waitqueue_head(&efx->flush_wq);
>>
>> +#ifdef CONFIG_DEBUG_FS
>> +       mutex_init(&efx->debugfs_symlink_mutex);
>> +#endif
> 
> Can we do this without the ifdefs in the mainline code?
> (okay, I'll stop grinding on that one for now)

Ifdefs for struct members that may not exist seems to be the
 existing pattern in efx_init_struct and efx_fini_struct, so
 I'd rather leave this here than wrap this single call in an
 efx_init_struct_debugfs function.

Thanks for the review!