[dpdk-kmods] linux/igb_uio: fix build for switch fall through

[dpdk-kmods] linux/igb_uio: fix build for switch fall through

[Ferruh Yigit]

Linux is using '-Wimplicit-fallthrough=5' compiler option, which doesn't
take any fall through comments into account but only uses compiler
'fallthrough' attribute to document fall through action is intended.

"falls through" comment was used in the code which is causing a build
error now, this patch converts comment to the 'fallthrough' macro
defined in the Linux.

To cover the case where Linux version doesn't have the macro, defined it
in the compatibility header too.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 linux/igb_uio/compat.h  | 4 ++++
 linux/igb_uio/igb_uio.c | 6 +++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/linux/igb_uio/compat.h b/linux/igb_uio/compat.h
index 8dbb896ae118..04ff7f60031f 100644
--- a/linux/igb_uio/compat.h
+++ b/linux/igb_uio/compat.h
@@ -152,3 +152,7 @@ static inline bool igbuio_kernel_is_locked_down(void)
 	return false;
 #endif
 }
+
+#ifndef fallthrough
+#define fallthrough	do {} while (0)  /* fallthrough */
+#endif
diff --git a/linux/igb_uio/igb_uio.c b/linux/igb_uio/igb_uio.c
index ea439d131de1..33e0e0286b69 100644
--- a/linux/igb_uio/igb_uio.c
+++ b/linux/igb_uio/igb_uio.c
@@ -250,7 +250,7 @@ igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev)
 		}
 #endif
 
-	/* falls through - to MSI */
+	fallthrough;
 	case RTE_INTR_MODE_MSI:
 #ifndef HAVE_ALLOC_IRQ_VECTORS
 		if (pci_enable_msi(udev->pdev) == 0) {
@@ -269,7 +269,7 @@ igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev)
 			break;
 		}
 #endif
-	/* falls through - to INTX */
+	fallthrough;
 	case RTE_INTR_MODE_LEGACY:
 		if (pci_intx_mask_supported(udev->pdev)) {
 			dev_dbg(&udev->pdev->dev, "using INTX");
@@ -279,7 +279,7 @@ igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev)
 			break;
 		}
 		dev_notice(&udev->pdev->dev, "PCI INTX mask not supported\n");
-	/* falls through - to no IRQ */
+	fallthrough;
 	case RTE_INTR_MODE_NONE:
 		udev->mode = RTE_INTR_MODE_NONE;
 		udev->info.irq = UIO_IRQ_NONE;
-- 
2.33.1

Re: [dpdk-kmods] linux/igb_uio: fix build for switch fall through

[Stephen Hemminger]

On Wed, 15 Dec 2021 18:48:59 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> Linux is using '-Wimplicit-fallthrough=5' compiler option, which doesn't
> take any fall through comments into account but only uses compiler
> 'fallthrough' attribute to document fall through action is intended.
> 
> "falls through" comment was used in the code which is causing a build
> error now, this patch converts comment to the 'fallthrough' macro
> defined in the Linux.
> 
> To cover the case where Linux version doesn't have the macro, defined it
> in the compatibility header too.
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Are you sure that fixes it? because the comment in the macro is typically
not visible in a macro expansion.
  Since in most case Linux uses gcc why not use the gcc attribute

 __attribute__ ((fallthrough))

Re: [dpdk-kmods] linux/igb_uio: fix build for switch fall through

[Ferruh Yigit]

On 12/15/2021 7:20 PM, Stephen Hemminger wrote:
> On Wed, 15 Dec 2021 18:48:59 +0000
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
>> Linux is using '-Wimplicit-fallthrough=5' compiler option, which doesn't
>> take any fall through comments into account but only uses compiler
>> 'fallthrough' attribute to document fall through action is intended.
>>
>> "falls through" comment was used in the code which is causing a build
>> error now, this patch converts comment to the 'fallthrough' macro
>> defined in the Linux.
>>
>> To cover the case where Linux version doesn't have the macro, defined it
>> in the compatibility header too.
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> Are you sure that fixes it? because the comment in the macro is typically
> not visible in a macro expansion.
>    Since in most case Linux uses gcc why not use the gcc attribute
> 
>   __attribute__ ((fallthrough))

Hi Stephen,

That is the intention already.

Patch is using the Linux kernel defined macro:
#if __has_attribute(__fallthrough__)
# define fallthrough                    __attribute__((__fallthrough__))
#else
# define fallthrough                    do {} while (0)  /* fallthrough */
#endif

And it builds fine without the macro in the 'compat.h'.


I added the define in the 'compat.h' for old kernels which doesn't
define the macro. For that case I expect default '-Wimplicit-fallthrough'
option is used which accepts 'fallthrough;' as a regex hit.

Re: [dpdk-kmods] linux/igb_uio: fix build for switch fall through

[Stephen Hemminger]

On Wed, 15 Dec 2021 21:04:30 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 12/15/2021 7:20 PM, Stephen Hemminger wrote:
> > On Wed, 15 Dec 2021 18:48:59 +0000
> > Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> >   
> >> Linux is using '-Wimplicit-fallthrough=5' compiler option, which doesn't
> >> take any fall through comments into account but only uses compiler
> >> 'fallthrough' attribute to document fall through action is intended.
> >>
> >> "falls through" comment was used in the code which is causing a build
> >> error now, this patch converts comment to the 'fallthrough' macro
> >> defined in the Linux.
> >>
> >> To cover the case where Linux version doesn't have the macro, defined it
> >> in the compatibility header too.
> >>
> >> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>  
> > 
> > Are you sure that fixes it? because the comment in the macro is typically
> > not visible in a macro expansion.
> >    Since in most case Linux uses gcc why not use the gcc attribute
> > 
> >   __attribute__ ((fallthrough))  
> 
> Hi Stephen,
> 
> That is the intention already.
> 
> Patch is using the Linux kernel defined macro:
> #if __has_attribute(__fallthrough__)
> # define fallthrough                    __attribute__((__fallthrough__))
> #else
> # define fallthrough                    do {} while (0)  /* fallthrough */
> #endif
> 
> And it builds fine without the macro in the 'compat.h'.
> 
> 
> I added the define in the 'compat.h' for old kernels which doesn't
> define the macro. For that case I expect default '-Wimplicit-fallthrough'
> option is used which accepts 'fallthrough;' as a regex hit.

Your right, on older kernels it really is just a dummy statement.
The regex can't work, you can take (or leave the comment) it has no effect
because implicit-fallthrough won't see it in a macro.

Re: [dpdk-kmods] linux/igb_uio: fix build for switch fall through

[Ferruh Yigit]

On 12/15/2021 11:15 PM, Stephen Hemminger wrote:
> On Wed, 15 Dec 2021 21:04:30 +0000
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
>> On 12/15/2021 7:20 PM, Stephen Hemminger wrote:
>>> On Wed, 15 Dec 2021 18:48:59 +0000
>>> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>>    
>>>> Linux is using '-Wimplicit-fallthrough=5' compiler option, which doesn't
>>>> take any fall through comments into account but only uses compiler
>>>> 'fallthrough' attribute to document fall through action is intended.
>>>>
>>>> "falls through" comment was used in the code which is causing a build
>>>> error now, this patch converts comment to the 'fallthrough' macro
>>>> defined in the Linux.
>>>>
>>>> To cover the case where Linux version doesn't have the macro, defined it
>>>> in the compatibility header too.
>>>>
>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>
>>> Are you sure that fixes it? because the comment in the macro is typically
>>> not visible in a macro expansion.
>>>     Since in most case Linux uses gcc why not use the gcc attribute
>>>
>>>    __attribute__ ((fallthrough))
>>
>> Hi Stephen,
>>
>> That is the intention already.
>>
>> Patch is using the Linux kernel defined macro:
>> #if __has_attribute(__fallthrough__)
>> # define fallthrough                    __attribute__((__fallthrough__))
>> #else
>> # define fallthrough                    do {} while (0)  /* fallthrough */
>> #endif
>>
>> And it builds fine without the macro in the 'compat.h'.
>>
>>
>> I added the define in the 'compat.h' for old kernels which doesn't
>> define the macro. For that case I expect default '-Wimplicit-fallthrough'
>> option is used which accepts 'fallthrough;' as a regex hit.
> 
> Your right, on older kernels it really is just a dummy statement.
> The regex can't work, you can take (or leave the comment) it has no effect
> because implicit-fallthrough won't see it in a macro.

True, it has no benefit for 'Wimplicit-fallthrough', and regex doesn't hit.

But for the old kernels (<= 5.3) still need to have this define to prevent
build error, because they are missing 'fallthrough' macro.
But for those kernels, '-Wimplicit-fallthrough' compiler option is not
set, so that should be OK **.
I have tested with '4.19.221' stable kernel, this change builds fine.


**: Only v5.3 has potential problem, which defines '-Wimplicit-fallthrough' and
doesn't define the 'fallthrough' macro. When igb_uio compiled with this version
'fallthrough' is dummy statement from 'compat.h' and '-Wimplicit-fallthrough'
can cause warning.
To fix this, I will move both dummy and attribute macro definition to compat.h

[dpdk-kmods v2] linux/igb_uio: fix build for switch fall through

[Ferruh Yigit]

Linux is using '-Wimplicit-fallthrough=5' compiler option, which doesn't
take any fall through comments into account but only uses compiler
'fallthrough' attribute to document fall through action is intended.

"falls through" comment was used in the code which is causing a build
error now, this patch converts comment to the 'fallthrough' macro
defined in the Linux.

To cover the case where an old Linux version doesn't have the macro,
defined it in the compatibility header too.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: Stephen Hemminger <stephen@networkplumber.org>

v2:
* Add both dummy and attribute fallthrough macro definition to
  compatibility header.
---
 linux/igb_uio/compat.h  | 14 ++++++++++++++
 linux/igb_uio/igb_uio.c |  6 +++---
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/linux/igb_uio/compat.h b/linux/igb_uio/compat.h
index 8dbb896ae118..71172f40cff1 100644
--- a/linux/igb_uio/compat.h
+++ b/linux/igb_uio/compat.h
@@ -152,3 +152,17 @@ static inline bool igbuio_kernel_is_locked_down(void)
 	return false;
 #endif
 }
+
+#ifndef fallthrough
+
+#ifndef __has_attribute
+#define __has_attribute(x) 0
+#endif
+
+#if __has_attribute(__fallthrough__)
+#define fallthrough	__attribute__((__fallthrough__))
+#else
+#define fallthrough	do {} while (0)  /* fallthrough */
+#endif
+
+#endif
diff --git a/linux/igb_uio/igb_uio.c b/linux/igb_uio/igb_uio.c
index ea439d131de1..33e0e0286b69 100644
--- a/linux/igb_uio/igb_uio.c
+++ b/linux/igb_uio/igb_uio.c
@@ -250,7 +250,7 @@ igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev)
 		}
 #endif
 
-	/* falls through - to MSI */
+	fallthrough;
 	case RTE_INTR_MODE_MSI:
 #ifndef HAVE_ALLOC_IRQ_VECTORS
 		if (pci_enable_msi(udev->pdev) == 0) {
@@ -269,7 +269,7 @@ igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev)
 			break;
 		}
 #endif
-	/* falls through - to INTX */
+	fallthrough;
 	case RTE_INTR_MODE_LEGACY:
 		if (pci_intx_mask_supported(udev->pdev)) {
 			dev_dbg(&udev->pdev->dev, "using INTX");
@@ -279,7 +279,7 @@ igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev)
 			break;
 		}
 		dev_notice(&udev->pdev->dev, "PCI INTX mask not supported\n");
-	/* falls through - to no IRQ */
+	fallthrough;
 	case RTE_INTR_MODE_NONE:
 		udev->mode = RTE_INTR_MODE_NONE;
 		udev->info.irq = UIO_IRQ_NONE;
-- 
2.33.1

Re: [dpdk-kmods v2] linux/igb_uio: fix build for switch fall through

[Thomas Monjalon]

16/12/2021 13:03, Ferruh Yigit:
> Linux is using '-Wimplicit-fallthrough=5' compiler option, which doesn't
> take any fall through comments into account but only uses compiler
> 'fallthrough' attribute to document fall through action is intended.
> 
> "falls through" comment was used in the code which is causing a build
> error now, this patch converts comment to the 'fallthrough' macro
> defined in the Linux.
> 
> To cover the case where an old Linux version doesn't have the macro,
> defined it in the compatibility header too.
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied, thanks.