diff mbox series

firewire: fix warnings to generate UAPI documentation

Message ID 20230601144937.121179-1-o-takashi@sakamocchi.jp
State New
Headers
Series firewire: fix warnings to generate UAPI documentation |

Commit Message

Takashi Sakamoto June 1, 2023, 2:49 p.m. UTC 
  Any target to generate UAPI documentation reports warnings to missing
annotation for padding member in structures added recently.

This commit suppresses the warnings.

Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Closes: https://lore.kernel.org/lkml/20230531135306.43613a59@canb.auug.org.au/
Fixes: 7c22d4a92bb2 ("firewire: cdev: add new event to notify request subaction with time stamp")
Fixes: fc2b52cf2e0e ("firewire: cdev: add new event to notify response subaction with time stamp")
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 include/uapi/linux/firewire-cdev.h | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Comments

Randy Dunlap June 1, 2023, 8:23 p.m. UTC | #1 
Hi,

On 6/1/23 07:49, Takashi Sakamoto wrote:
> Any target to generate UAPI documentation reports warnings to missing
> annotation for padding member in structures added recently.
> 
> This commit suppresses the warnings.
> 
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Closes: https://lore.kernel.org/lkml/20230531135306.43613a59@canb.auug.org.au/
> Fixes: 7c22d4a92bb2 ("firewire: cdev: add new event to notify request subaction with time stamp")
> Fixes: fc2b52cf2e0e ("firewire: cdev: add new event to notify response subaction with time stamp")
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  include/uapi/linux/firewire-cdev.h | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 

You can do it as this patch shows, or you can hide those padding fields as
described in Documentation/doc-guide/kernel-doc.rst:

Inside a struct or union description, you can use the ``private:`` and
``public:`` comment tags. Structure fields that are inside a ``private:``
area are not listed in the generated output documentation.

The ``private:`` and ``public:`` tags must begin immediately following a
``/*`` comment marker. They may optionally include comments between the
``:`` and the ending ``*/`` marker.

See below.

> diff --git a/include/uapi/linux/firewire-cdev.h b/include/uapi/linux/firewire-cdev.h
> index 99e823935427..1f2c9469f921 100644
> --- a/include/uapi/linux/firewire-cdev.h
> +++ b/include/uapi/linux/firewire-cdev.h
> @@ -130,6 +130,9 @@ struct fw_cdev_event_response {
>   * @length:	Data length, i.e. the response's payload size in bytes
>   * @request_tstamp:	The time stamp of isochronous cycle at which the request was sent.
>   * @response_tstamp:	The time stamp of isochronous cycle at which the response was sent.
> + * @padding:	Padding to keep the size of structure as multiples of 8 in various architectures
> + *		since 4 byte alignment is used for 8 byte of object type in System V ABI for i386
> + *		architecture.

You could drop that change.

>   * @data:	Payload data, if any
>   *
>   * This event is sent when the stack receives a response to an outgoing request
> @@ -155,10 +158,6 @@ struct fw_cdev_event_response2 {
>  	__u32 length;
>  	__u32 request_tstamp;
>  	__u32 response_tstamp;

	/* private: */
> -	/*
> -	 * Padding to keep the size of structure as multiples of 8 in various architectures since
> -	 * 4 byte alignment is used for 8 byte of object type in System V ABI for i386 architecture.
> -	 */
>  	__u32 padding;

	/* public: */

>  	__u32 data[];
>  };
> @@ -231,6 +230,9 @@ struct fw_cdev_event_request2 {
>   * @handle:	Reference to the kernel-side pending request
>   * @length:	Data length, i.e. the request's payload size in bytes
>   * @tstamp:	The time stamp of isochronous cycle at which the request arrived.
> + * @padding:	Padding to keep the size of structure as multiples of 8 in various architectures
> + *		since 4 byte alignment is used for 8 byte of object type in System V ABI for i386
> + *		architecture.

drop that.

>   * @data:	Incoming data, if any
>   *
>   * This event is sent when the stack receives an incoming request to an address
> @@ -284,10 +286,6 @@ struct fw_cdev_event_request3 {
>  	__u32 handle;
>  	__u32 length;
>  	__u32 tstamp;

	/* private: */

> -	/*
> -	 * Padding to keep the size of structure as multiples of 8 in various architectures since
> -	 * 4 byte alignment is used for 8 byte of object type in System V ABI for i386 architecture.
> -	 */
>  	__u32 padding;

	/* public: */

>  	__u32 data[];
>  };
Takashi Sakamoto June 3, 2023, 4:25 a.m. UTC | #2 
Hi Randy,

On Thu, Jun 01, 2023 at 01:23:27PM -0700, Randy Dunlap wrote:
> Hi,
> 
> On 6/1/23 07:49, Takashi Sakamoto wrote:
> > Any target to generate UAPI documentation reports warnings to missing
> > annotation for padding member in structures added recently.
> > 
> > This commit suppresses the warnings.
> > 
> > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > Closes: https://lore.kernel.org/lkml/20230531135306.43613a59@canb.auug.org.au/
> > Fixes: 7c22d4a92bb2 ("firewire: cdev: add new event to notify request subaction with time stamp")
> > Fixes: fc2b52cf2e0e ("firewire: cdev: add new event to notify response subaction with time stamp")
> > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > ---
> >  include/uapi/linux/firewire-cdev.h | 14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> > 
> 
> You can do it as this patch shows, or you can hide those padding fields as
> described in Documentation/doc-guide/kernel-doc.rst:
> 
> Inside a struct or union description, you can use the ``private:`` and
> ``public:`` comment tags. Structure fields that are inside a ``private:``
> area are not listed in the generated output documentation.
> 
> The ``private:`` and ``public:`` tags must begin immediately following a
> ``/*`` comment marker. They may optionally include comments between the
> ``:`` and the ending ``*/`` marker.
> 
> See below.
> (snip) 

Thanks for your review and suggestion. Indeed, the private/public
annotation is one of our options and I have realized it.

I think it my preference to reduce load when reading structure layout.
The usage of private/public requires readers' brain to switch context
of access modifier. I guess that I would like to avoid such load if I were
the reader.

If the structure definition includes many annotations, it also increases
such load. In my experience, it is likely that annotation in structure
definition does not necessarily include enough information about the
member itself, especially when writing applications. I think it my
preference as well that member annotation of structure is in document,
(but it would be case-by-case.) It seems to be explicit way, vaguely.

Anyway thanks for your comment. It is fun to me;)


Takashi Sakamoto
Takashi Sakamoto June 5, 2023, 10:55 p.m. UTC | #3 
On Thu, Jun 01, 2023 at 11:49:37PM +0900, Takashi Sakamoto wrote:
> Any target to generate UAPI documentation reports warnings to missing
> annotation for padding member in structures added recently.
> 
> This commit suppresses the warnings.
> 
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Closes: https://lore.kernel.org/lkml/20230531135306.43613a59@canb.auug.org.au/
> Fixes: 7c22d4a92bb2 ("firewire: cdev: add new event to notify request subaction with time stamp")
> Fixes: fc2b52cf2e0e ("firewire: cdev: add new event to notify response subaction with time stamp")
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  include/uapi/linux/firewire-cdev.h | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)

Applied to for-next branch.


Regards

Takashi Sakamoto
diff mbox series

Patch

diff --git a/include/uapi/linux/firewire-cdev.h b/include/uapi/linux/firewire-cdev.h
index 99e823935427..1f2c9469f921 100644
--- a/include/uapi/linux/firewire-cdev.h
+++ b/include/uapi/linux/firewire-cdev.h
@@ -130,6 +130,9 @@  struct fw_cdev_event_response {
  * @length:	Data length, i.e. the response's payload size in bytes
  * @request_tstamp:	The time stamp of isochronous cycle at which the request was sent.
  * @response_tstamp:	The time stamp of isochronous cycle at which the response was sent.
+ * @padding:	Padding to keep the size of structure as multiples of 8 in various architectures
+ *		since 4 byte alignment is used for 8 byte of object type in System V ABI for i386
+ *		architecture.
  * @data:	Payload data, if any
  *
  * This event is sent when the stack receives a response to an outgoing request
@@ -155,10 +158,6 @@  struct fw_cdev_event_response2 {
 	__u32 length;
 	__u32 request_tstamp;
 	__u32 response_tstamp;
-	/*
-	 * Padding to keep the size of structure as multiples of 8 in various architectures since
-	 * 4 byte alignment is used for 8 byte of object type in System V ABI for i386 architecture.
-	 */
 	__u32 padding;
 	__u32 data[];
 };
@@ -231,6 +230,9 @@  struct fw_cdev_event_request2 {
  * @handle:	Reference to the kernel-side pending request
  * @length:	Data length, i.e. the request's payload size in bytes
  * @tstamp:	The time stamp of isochronous cycle at which the request arrived.
+ * @padding:	Padding to keep the size of structure as multiples of 8 in various architectures
+ *		since 4 byte alignment is used for 8 byte of object type in System V ABI for i386
+ *		architecture.
  * @data:	Incoming data, if any
  *
  * This event is sent when the stack receives an incoming request to an address
@@ -284,10 +286,6 @@  struct fw_cdev_event_request3 {
 	__u32 handle;
 	__u32 length;
 	__u32 tstamp;
-	/*
-	 * Padding to keep the size of structure as multiples of 8 in various architectures since
-	 * 4 byte alignment is used for 8 byte of object type in System V ABI for i386 architecture.
-	 */
 	__u32 padding;
 	__u32 data[];
 };