| Message ID | 20191010035239.890311169@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | infiniband kernel-doc fixes & driver-api/ chapter | expand |
On Wed, Oct 09, 2019 at 08:52:44PM -0700, rd.dunlab@gmail.com wrote: > Make reserved struct fields "private:" so that they don't need to > be added to the kernel-doc notation. This removes 24 warnings. > +++ linux-next-20191009/drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h > @@ -129,21 +129,31 @@ struct opa_vesw_info { > __be16 fabric_id; > __be16 vesw_id; > > + /* private: */ > u8 rsvd0[6]; > + /* public: */ > __be16 def_port_mask; This seems overly ugly, is there some other way to handle these reserved fields? Maybe wire protocol structures shouldn't be kdoc? Jason
On 10/22/19 10:52 AM, Jason Gunthorpe wrote: > On Wed, Oct 09, 2019 at 08:52:44PM -0700, rd.dunlab@gmail.com wrote: >> Make reserved struct fields "private:" so that they don't need to >> be added to the kernel-doc notation. This removes 24 warnings. > >> +++ linux-next-20191009/drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h >> @@ -129,21 +129,31 @@ struct opa_vesw_info { >> __be16 fabric_id; >> __be16 vesw_id; >> >> + /* private: */ >> u8 rsvd0[6]; >> + /* public: */ >> __be16 def_port_mask; > > This seems overly ugly, is there some other way to handle these > reserved fields? Maybe wire protocol structures shouldn't be kdoc? I don't know of any other way to handle them with kernel-doc. Sure, changing the /** to just /* would be one way to hide the warnings. Either this patch or not having them be kernel-doc is needed just to "fix" 24 warnings.
On Tue, 22 Oct 2019, Randy Dunlap <rdunlap@infradead.org> wrote: > On 10/22/19 10:52 AM, Jason Gunthorpe wrote: >> On Wed, Oct 09, 2019 at 08:52:44PM -0700, rd.dunlab@gmail.com wrote: >>> Make reserved struct fields "private:" so that they don't need to >>> be added to the kernel-doc notation. This removes 24 warnings. >> >>> +++ linux-next-20191009/drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h >>> @@ -129,21 +129,31 @@ struct opa_vesw_info { >>> __be16 fabric_id; >>> __be16 vesw_id; >>> >>> + /* private: */ >>> u8 rsvd0[6]; >>> + /* public: */ >>> __be16 def_port_mask; >> >> This seems overly ugly, is there some other way to handle these >> reserved fields? Maybe wire protocol structures shouldn't be kdoc? > > I don't know of any other way to handle them with kernel-doc. > Sure, changing the /** to just /* would be one way to hide the > warnings. Either this patch or not having them be kernel-doc > is needed just to "fix" 24 warnings. The currently available options are: - The patch at hand (private/public comments). Ugly and verbose. - Document the structs using regular comments instead of kernel-doc. Might be suitable here, but not a generally useful approach. Loses all format checking and generated documentation. - Also document the reserved fields. Ugly and verbose, also in the generated documentation. Some options that I think might be relatively easy to implement: - Add struct documentation comment indicator to not complain about missing member documentation. Some special tag in the struct comment. This would also ignore members that actually need to be documented. - Add support for designating private members in the member documentation, i.e. require the documentation, but omit the members from the generated document. Something like this, with PRIVATE replaced with your favorite bikeshed colors: /** * @rsvd0: PRIVATE */ This could be used either in the struct documentation comment or in the inline member documentation comment. Less ugly than the patch at hand, and arguably a better notation, but still requires documenting the members. - Add support for a catch-all member documentation comment, for example: /** * struct foo - bar * @*: This member is private. */ Would generate the documentation for the member with the catch-all documentation, which might be a generally useful feature, and would be easy on the source code side. This could be combined with the PRIVATE designation above, practically leading to the same result as the first option but with more flexibility. BR, Jani.
On 10/22/2019 3:51 PM, Randy Dunlap wrote: > On 10/22/19 10:52 AM, Jason Gunthorpe wrote: >> On Wed, Oct 09, 2019 at 08:52:44PM -0700, rd.dunlab@gmail.com wrote: >>> Make reserved struct fields "private:" so that they don't need to >>> be added to the kernel-doc notation. This removes 24 warnings. >> >>> +++ linux-next-20191009/drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h >>> @@ -129,21 +129,31 @@ struct opa_vesw_info { >>> __be16 fabric_id; >>> __be16 vesw_id; >>> >>> + /* private: */ >>> u8 rsvd0[6]; >>> + /* public: */ >>> __be16 def_port_mask; >> >> This seems overly ugly, is there some other way to handle these >> reserved fields? Maybe wire protocol structures shouldn't be kdoc? > > I don't know of any other way to handle them with kernel-doc. > Sure, changing the /** to just /* would be one way to hide the > warnings. Either this patch or not having them be kernel-doc > is needed just to "fix" 24 warnings. > I would be in favor of just not including this in kernel-doc at this time, but the mess of private/public tags while hard on the eyes doesn't really bother me either. Also wouldn't take this as a statement that wire protocol structures not be in kdoc, but just that this one doesn't need to be there. -Denny
--- linux-next-20191009.orig/drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h +++ linux-next-20191009/drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h @@ -129,21 +129,31 @@ struct opa_vesw_info { __be16 fabric_id; __be16 vesw_id; + /* private: */ u8 rsvd0[6]; + /* public: */ __be16 def_port_mask; + /* private: */ u8 rsvd1[2]; + /* public: */ __be16 pkey; + /* private: */ u8 rsvd2[4]; + /* public: */ __be32 u_mcast_dlid; __be32 u_ucast_dlid[OPA_VESW_MAX_NUM_DEF_PORT]; __be32 rc; + /* private: */ u8 rsvd3[56]; + /* public: */ __be16 eth_mtu; + /* private: */ u8 rsvd4[2]; + /* public: */ } __packed; /** @@ -172,7 +182,9 @@ struct opa_per_veswport_info { __be32 port_num; u8 eth_link_status; + /* private: */ u8 rsvd0[3]; + /* public: */ u8 base_mac_addr[ETH_ALEN]; u8 config_state; @@ -181,7 +193,9 @@ struct opa_per_veswport_info { __be16 max_mac_tbl_ent; __be16 max_smac_ent; __be32 mac_tbl_digest; + /* private: */ u8 rsvd1[4]; + /* public: */ __be32 encap_slid; @@ -195,12 +209,16 @@ struct opa_per_veswport_info { u8 non_vlan_sc_mc; u8 non_vlan_vl_mc; + /* private: */ u8 rsvd2[48]; + /* public: */ __be16 uc_macs_gen_count; __be16 mc_macs_gen_count; + /* private: */ u8 rsvd3[8]; + /* public: */ } __packed; /** @@ -239,7 +257,7 @@ struct opa_veswport_mactable_entry { * @offset: mac table starting offset * @num_entries: Number of entries to get or set * @mac_tbl_digest: mac table digest - * @tbl_entries[]: Array of table entries + * @tbl_entries: Array of table entries * * The EM sends down this structure in a MAD indicating * the starting offset in the forwarding table that this @@ -337,7 +355,9 @@ struct opa_veswport_summary_counters { __be64 rx_1024_1518; __be64 rx_1519_max; + /* private: */ __be64 reserved[16]; + /* public: */ } __packed; /** @@ -368,28 +388,42 @@ struct opa_veswport_error_counters { __be64 tx_errors; __be64 rx_errors; + /* private: */ __be64 rsvd0; + /* public: */ __be64 tx_smac_filt; + /* private: */ __be64 rsvd1; __be64 rsvd2; __be64 rsvd3; + /* public: */ __be64 tx_dlid_zero; + /* private: */ __be64 rsvd4; + /* public: */ __be64 tx_logic; + /* private: */ __be64 rsvd5; + /* public: */ __be64 tx_drop_state; __be64 rx_bad_veswid; + /* private: */ __be64 rsvd6; + /* public: */ __be64 rx_runt; __be64 rx_oversize; + /* private: */ __be64 rsvd7; + /* public: */ __be64 rx_eth_down; __be64 rx_drop_state; __be64 rx_logic; + /* private: */ __be64 rsvd8; __be64 rsvd9[16]; + /* public: */ } __packed; /** @@ -453,7 +487,9 @@ struct opa_veswport_iface_macs { struct opa_vnic_vema_mad { struct ib_mad_hdr mad_hdr; struct ib_rmpp_hdr rmpp_hdr; + /* private: */ u8 reserved; + /* public: */ u8 oui[3]; u8 data[OPA_VNIC_EMA_DATA]; }; @@ -478,7 +514,9 @@ struct opa_vnic_notice_attr { __be16 trap_num; __be16 toggle_count; __be32 issuer_lid; + /* private: */ __be32 reserved; + /* public: */ u8 issuer_gid[16]; u8 raw_data[64]; } __packed; @@ -493,7 +531,9 @@ struct opa_vnic_notice_attr { struct opa_vnic_vema_mad_trap { struct ib_mad_hdr mad_hdr; struct ib_rmpp_hdr rmpp_hdr; + /* private: */ u8 reserved; + /* public: */ u8 oui[3]; struct opa_vnic_notice_attr notice; };
Make reserved struct fields "private:" so that they don't need to be added to the kernel-doc notation. This removes 24 warnings. Remove "[]" in one struct field description to that kernel-doc won't be confused. ../drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:148: warning: Function parameter or member 'rsvd0' not described in 'opa_vesw_info' [5 rsvd members] ../drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:205: warning: Function parameter or member 'rsvd0' not described in 'opa_per_veswport_info' [4 rsvd members] ../drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:342: warning: Function parameter or member 'reserved' not described in 'opa_veswport_summary_counters' ../drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:394: warning: Function parameter or member 'rsvd0' not described in 'opa_veswport_error_counters' [10 rsvd members] ../drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:460: warning: Function parameter or member 'reserved' not described in 'opa_vnic_vema_mad' ../drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:485: warning: Function parameter or member 'reserved' not described in 'opa_vnic_notice_attr' ../drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:500: warning: Function parameter or member 'reserved' not described in 'opa_vnic_vema_mad_trap' ../drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:263: warning: Function parameter or member 'tbl_entries' not described in 'opa_veswport_mactable' Signed-off-by: Randy Dunlap <rd.dunlab@gmail.com> Cc: Randy Dunlap <rdunlap@infradead.org> Cc: Doug Ledford <dledford@redhat.com> Cc: Jason Gunthorpe <jgg@mellanox.com> Cc: linux-doc@vger.kernel.org --- drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h | 42 ++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-)