Message-ID: <ed1e07c5-0765-4868-9d39-2078c7c51e1f@infradead.org>
Date: Fri, 5 Dec 2025 12:02:17 -0800
From: Randy Dunlap <rdunlap@...radead.org>
To: Edward Cree <ecree.xilinx@...il.com>, netdev@...r.kernel.org
Cc: Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Simon Horman <horms@...nel.org>, linux-net-drivers@....com
Subject: Re: [PATCH net-next] sfc: correct kernel-doc complaints

Hi Edward,

On 12/5/25 9:19 AM, Edward Cree wrote:
> On 29/11/2025 22:03, Randy Dunlap wrote:
>> Fix kernel-doc warnings by adding 3 missing struct member descriptions
>> in struct efx_ef10_nic_data and removing preprocessor directives (which
>> are not handled by kernel-doc).
>>
>> Fixes these 5 warnings:
>> Warning: drivers/net/ethernet/sfc/nic.h:158 bad line: #ifdef CONFIG_SFC_SRIOV
>> Warning: drivers/net/ethernet/sfc/nic.h:160 bad line: #endif
> 
> Does kernel-doc not complain if a member is documented but the actual
>  declaration is ifdefed out?  Normal practice seems to be to move the
>  doc into another comment adjacent to the declaration so it's under
>  the same ifdef; is that unnecessary?

kernel-doc knows nothing about the kernel config (.config).
Inside a struct/union, it strips away (ignores) preprocessor lines.
So no, it does not complain.

>> Warning: drivers/net/ethernet/sfc/nic.h:204 struct member 'port_id'
>>  not described in 'efx_ef10_nic_data'
>> Warning: drivers/net/ethernet/sfc/nic.h:204 struct member 'vf_index'
>>  not described in 'efx_ef10_nic_data'
>> Warning: drivers/net/ethernet/sfc/nic.h:204 struct member 'licensed_features'
>>  not described in 'efx_ef10_nic_data'
>>
>> Signed-off-by: Randy Dunlap <rdunlap@...radead.org>
>> ---
>> Cc: Edward Cree <ecree.xilinx@...il.com>
>> Cc: Andrew Lunn <andrew+netdev@...n.ch>
>> Cc: "David S. Miller" <davem@...emloft.net>
>> Cc: Eric Dumazet <edumazet@...gle.com>
>> Cc: Jakub Kicinski <kuba@...nel.org>
>> Cc: Paolo Abeni <pabeni@...hat.com>
>> Cc: Simon Horman <horms@...nel.org>
>> Cc: linux-net-drivers@....com
>> ---
>>  drivers/net/ethernet/sfc/nic.h |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> --- linux-next-20251128.orig/drivers/net/ethernet/sfc/nic.h
>> +++ linux-next-20251128/drivers/net/ethernet/sfc/nic.h
>> @@ -156,9 +156,10 @@ enum {
>>   * @tx_dpcpu_fw_id: Firmware ID of the TxDPCPU
>>   * @must_probe_vswitching: Flag: vswitching has yet to be setup after MC reboot
>>   * @pf_index: The number for this PF, or the parent PF if this is a VF
>> -#ifdef CONFIG_SFC_SRIOV
>> + * @port_id: port id (Ethernet address) if !CONFIG_SFC_SRIOV;
>> + *   for CONFIG_SFC_SRIOV, the VF port id
> 
> I think this is always the PF's MAC address, and is used by
>  ndo_get_phys_port_id.  On EF100 that method only exists for PFs
>  (the vfrep's ndo_get_port_parent_id also shows the PF's port_id),
>  whereas on EF10 VFs also have a phys_port_id with the PF's MAC
>  address.
> I guess the best way to summarise this for the kerneldoc comment
>  would be:
>  * @port_id: Ethernet address of owning PF, used for phys_port_id
> In our local tree we just have "@port_id: Physical port identity".
> 
>> + * @vf_index: Index of particular VF in the VF data structure
> 
> This isn't quite right; this field is the index of this VF more
>  generally within the PF's set of VFs; it's provided by firmware,
>  and passed back to firmware in various requests.
> And when it's used as an index into the VF data structure array,
>  it's the _parent PF's_ nic_data->vf that is indexed by the VF's
>  nic_data->vf_index.  (The VF's nic_data->vf is %NULL afaik.)
> Not really sure how to summarise this, other than just following
>  the pattern of @pf_index above:
>  * @vf_index: The number for this VF, or 0xFFFF if this is a VF
>  which isn't greatly informative, but we could add more to @vf:
> 
>>   * @vf: Pointer to VF data structure
> 
>  * @vf: for a PF, array of VF data structures indexed by VF's
> 	@vf_index
> 
>> -#endif
>>   * @vport_mac: The MAC address on the vport, only for PFs; VFs will be zero
>>   * @vlan_list: List of VLANs added over the interface. Serialised by vlan_lock.
>>   * @vlan_lock: Lock to serialize access to vlan_list.
>> @@ -166,6 +167,7 @@ enum {
>>   * @udp_tunnels_dirty: flag indicating a reboot occurred while pushing
>>   *	@udp_tunnels to hardware and thus the push must be re-done.
>>   * @udp_tunnels_lock: Serialises writes to @udp_tunnels and @udp_tunnels_dirty.
>> + * @licensed_features: used to enable features if the adapter is licensed for it
> 
> In our local tree we have:
>  * @licensed_features: Flags for licensed firmware features.
>  which might be better as it doesn't give the impression that the
>  driver can change this ('enable' things) — it's a bitmask that
>  comes directly from firmware.
> 
> -ed

I can just use what's in your local tree (comments above), or
would you prefer to send the patch?

Thanks for the info.
-- 
~Randy

Powered by blists - more mailing lists