netdev - Re: [PATCH net-next 3/7] sfc: debugfs for (nic) RX queues

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <a73e6859-6a64-db5a-66ec-4fa884dd5b74@gmail.com>
Date: Fri, 14 Feb 2025 15:51:44 +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 3/7] sfc: debugfs for (nic) RX queues

On 15/12/2023—
wait, has it really been more than a year?  Yikes.

On 15/12/2023 00:05, Nelson, Shannon wrote:
> On 12/11/2023 9:18 AM, edward.cree@....com wrote:
>> +       if (snprintf(name, sizeof(name), "rx-%d", efx_rx_queue_index(rx_queue))
> 
> Adding leading 0's here can be helpful for directory entry sorting

True, but it's not clear how many to use — the hardware supports over
 1000 in principle, and in practice it's normal to have one per core
 (and more than that on TX) which can get over 100 on powerful systems.
Yet on something like an 8-core box having queues 000 to 007 just looks
 silly imho.  I don't plan to change this line in v2.

>> +           >= sizeof(name))
>> +               return -ENAMETOOLONG;
>> +       rx_queue->debug_dir = debugfs_create_dir(name,
>> +                                                rx_queue->efx->debug_queues_dir);
>> +       if (!rx_queue->debug_dir)
>> +               return -ENOMEM;
>> +
>> +       /* Create files */
>> +       efx_init_debugfs_rx_queue_files(rx_queue);
>> +
>> +       /* Create symlink to channel */
>> +       if (snprintf(target, sizeof(target), "../../channels/%d",
>> +                    channel->channel) >= sizeof(target))
>> +               return -ENAMETOOLONG;
>> +       if (!debugfs_create_symlink("channel", rx_queue->debug_dir, target))
>> +               return -ENOMEM;
> 
> If these fail, should you clean up the earlier create_dir()?

No; these errors mean "we didn't do everything we wanted to", not
 "it's all broken", and the files/dir previously created are still
 useful.  The caller treats errors as non-fatal.
See also the kdoc comment on efx_init_debugfs_rx_queue:
>> + * The directory must be cleaned up using efx_fini_debugfs_rx_queue(),
>> + * even if this function returns an error.

I can't think of a suitable comment on these return statements to
 clarify this, but suggestions are welcome.