netdev - Re: [PATCH net-next v4 00/15][pull request] Introduce Intel IDPF driver

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]

Date: Tue, 8 Aug 2023 19:08:53 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: "Linga, Pavan Kumar" <pavan.kumar.linga@...el.com>
Cc: Tony Nguyen <anthony.l.nguyen@...el.com>, <davem@...emloft.net>,
 <pabeni@...hat.com>, <edumazet@...gle.com>, <netdev@...r.kernel.org>,
 <emil.s.tantilov@...el.com>, <jesse.brandeburg@...el.com>,
 <sridhar.samudrala@...el.com>, <shiraz.saleem@...el.com>,
 <sindhu.devale@...el.com>, <willemb@...gle.com>, <decot@...gle.com>,
 <andrew@...n.ch>, <leon@...nel.org>, <mst@...hat.com>,
 <simon.horman@...igine.com>, <shannon.nelson@....com>,
 <stephen@...workplumber.org>
Subject: Re: [PATCH net-next v4 00/15][pull request] Introduce Intel IDPF
 driver

On Tue, 8 Aug 2023 17:35:27 -0700 Linga, Pavan Kumar wrote:
> On 8/8/2023 1:32 PM, Jakub Kicinski wrote:
> > On Mon,  7 Aug 2023 17:34:01 -0700 Tony Nguyen wrote:  
> >> This patch series introduces the Intel Infrastructure Data Path Function
> >> (IDPF) driver. It is used for both physical and virtual functions. Except
> >> for some of the device operations the rest of the functionality is the
> >> same for both PF and VF. IDPF uses virtchnl version2 opcodes and
> >> structures defined in the virtchnl2 header file which helps the driver
> >> to learn the capabilities and register offsets from the device
> >> Control Plane (CP) instead of assuming the default values.  
> > 
> > Patches 4 and 10 add kdoc warnings, please fix those.
> > And double check all the checkpatch warning about lines > 80 chars.  
> 
> Thanks for the feedback.
> 
> Will review the warnings regarding 80char limit. Are you wanting them 
> all removed or is it okay to leave the ones that help readability?

For some definition of helps readability, yes.
I glanced at the warnings on one of the first patches and most of them
were just comments.

>  > drivers/net/ethernet/intel/idpf/idpf.h:123: warning: Enum value   
> 'csum_caps' not described in enum 'idpf_cap_field'
>  > drivers/net/ethernet/intel/idpf/idpf.h:123: warning: Enum value   
> 'seg_caps' not described in enum 'idpf_cap_field'
>  > drivers/net/ethernet/intel/idpf/idpf.h:123: warning: Enum value   
> 'rss_caps' not described in enum 'idpf_cap_field'
>  > drivers/net/ethernet/intel/idpf/idpf.h:123: warning: Enum value   
> 'hsplit_caps' not described in enum 'idpf_cap_field'
>  > drivers/net/ethernet/intel/idpf/idpf.h:123: warning: Enum value   
> 'rsc_caps' not described in enum 'idpf_cap_field'
>  > drivers/net/ethernet/intel/idpf/idpf.h:123: warning: Enum value   
> 'other_caps' not described in enum 'idpf_cap_field'
>  > drivers/net/ethernet/intel/idpf/idpf_txrx.h:153: warning: Function   
> parameter or member 'DEFINE_DMA_UNMAP_ADDR(dma' not described in 
> 'idpf_tx_buf'
>  > drivers/net/ethernet/intel/idpf/idpf_txrx.h:153: warning: Function   
> parameter or member 'DEFINE_DMA_UNMAP_LEN(len' not described in 
> 'idpf_tx_buf'
> 
> /**
>   * enum idpf_cap_field - Offsets into capabilities struct for specific caps
>   * @IDPF_BASE_CAPS: generic base capabilities
>   * @IDPF_CSUM_CAPS: checksum offload capabilities
> ...
>   */
> enum idpf_cap_field {
>           IDPF_BASE_CAPS          = -1,
>           IDPF_CSUM_CAPS          = offsetof(struct virtchnl2_get_capabilities,
>                                              csum_caps),
>           IDPF_SEG_CAPS           = offsetof(struct
> ...
> }
> 
> 
> /**
>   * struct idpf_tx_buf
>   * @next_to_watch: Next descriptor to clean
>   * @skb: Pointer to the skb
>   * @dma: DMA address
>   * @len: DMA length
> ...
>   */
> struct idpf_tx_buf {
>          void *next_to_watch;
>          struct sk_buff *skb;
>          DEFINE_DMA_UNMAP_ADDR(dma);
>          DEFINE_DMA_UNMAP_LEN(len);
> ...
> }
> 
> The script is parsing the offsetof() argument as part of the enum, which 
> is not true. I believe it to be a false positive. Same for the second 
> one where it parses 'DEFINE_DMA_UNMAP_ADDR(dma'. Is it okay to use 'dma' 
> and 'len' in the kdoc header as-is or please suggest if you prefer 
> something?

The parser is in scripts/kernel-doc, it's not very complicated.
You can teach it new tricks.