All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
@ 2021-03-05  9:48 Gustavo A. R. Silva
  2021-03-05 13:40 ` Kalle Valo
                   ` (3 more replies)
  0 siblings, 4 replies; 18+ messages in thread
From: Gustavo A. R. Silva @ 2021-03-05  9:48 UTC (permalink / raw)
  To: Jes Sorensen, Kalle Valo, David S. Miller, Jakub Kicinski
  Cc: linux-wireless, netdev, linux-kernel, Gustavo A. R. Silva,
	linux-hardening

In preparation to enable -Wimplicit-fallthrough for Clang, fix
multiple warnings by replacing /* fall through */ comments with
the new pseudo-keyword macro fallthrough; instead of letting the
code fall through to the next case.

Notice that Clang doesn't recognize /* fall through */ comments as
implicit fall-through markings.

Link: https://github.com/KSPP/linux/issues/115
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index 5cd7ef3625c5..afc97958fa4d 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -1145,7 +1145,7 @@ void rtl8xxxu_gen1_config_channel(struct ieee80211_hw *hw)
 	switch (hw->conf.chandef.width) {
 	case NL80211_CHAN_WIDTH_20_NOHT:
 		ht = false;
-		/* fall through */
+		fallthrough;
 	case NL80211_CHAN_WIDTH_20:
 		opmode |= BW_OPMODE_20MHZ;
 		rtl8xxxu_write8(priv, REG_BW_OPMODE, opmode);
@@ -1272,7 +1272,7 @@ void rtl8xxxu_gen2_config_channel(struct ieee80211_hw *hw)
 	switch (hw->conf.chandef.width) {
 	case NL80211_CHAN_WIDTH_20_NOHT:
 		ht = false;
-		/* fall through */
+		fallthrough;
 	case NL80211_CHAN_WIDTH_20:
 		rf_mode_bw |= WMAC_TRXPTCL_CTL_BW_20;
 		subchannel = 0;
@@ -1741,11 +1741,11 @@ static int rtl8xxxu_identify_chip(struct rtl8xxxu_priv *priv)
 		case 3:
 			priv->ep_tx_low_queue = 1;
 			priv->ep_tx_count++;
-			/* fall through */
+			fallthrough;
 		case 2:
 			priv->ep_tx_normal_queue = 1;
 			priv->ep_tx_count++;
-			/* fall through */
+			fallthrough;
 		case 1:
 			priv->ep_tx_high_queue = 1;
 			priv->ep_tx_count++;
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
  2021-03-05  9:48 [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang Gustavo A. R. Silva
@ 2021-03-05 13:40 ` Kalle Valo
  2021-03-05 16:49   ` Gustavo A. R. Silva
  2021-03-10 19:14   ` Kees Cook
  2021-04-17 17:52 ` Kalle Valo
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 18+ messages in thread
From: Kalle Valo @ 2021-03-05 13:40 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Jes Sorensen, David S. Miller, Jakub Kicinski, linux-wireless,
	netdev, linux-kernel, linux-hardening

"Gustavo A. R. Silva" <gustavoars@kernel.org> writes:

> In preparation to enable -Wimplicit-fallthrough for Clang, fix
> multiple warnings by replacing /* fall through */ comments with
> the new pseudo-keyword macro fallthrough; instead of letting the
> code fall through to the next case.
>
> Notice that Clang doesn't recognize /* fall through */ comments as
> implicit fall-through markings.
>
> Link: https://github.com/KSPP/linux/issues/115
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>

It's not cool that you ignore the comments you got in [1], then after a
while mark the patch as "RESEND" and not even include a changelog why it
was resent.

[1] https://patchwork.kernel.org/project/linux-wireless/patch/d522f387b2d0dde774785c7169c1f25aa529989d.1605896060.git.gustavoars@kernel.org/

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
  2021-03-05 13:40 ` Kalle Valo
@ 2021-03-05 16:49   ` Gustavo A. R. Silva
  2021-03-10 19:14   ` Kees Cook
  1 sibling, 0 replies; 18+ messages in thread
From: Gustavo A. R. Silva @ 2021-03-05 16:49 UTC (permalink / raw)
  To: Kalle Valo, Gustavo A. R. Silva
  Cc: Jes Sorensen, David S. Miller, Jakub Kicinski, linux-wireless,
	netdev, linux-kernel, linux-hardening



On 3/5/21 07:40, Kalle Valo wrote:
> "Gustavo A. R. Silva" <gustavoars@kernel.org> writes:
> 
>> In preparation to enable -Wimplicit-fallthrough for Clang, fix
>> multiple warnings by replacing /* fall through */ comments with
>> the new pseudo-keyword macro fallthrough; instead of letting the
>> code fall through to the next case.
>>
>> Notice that Clang doesn't recognize /* fall through */ comments as
>> implicit fall-through markings.
>>
>> Link: https://github.com/KSPP/linux/issues/115
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> 
> It's not cool that you ignore the comments you got in [1], then after a
> while mark the patch as "RESEND" and not even include a changelog why it
> was resent.

I'm a bit confused. I replied to the same thread at the time. You can see
my response there. No one replied back. :/

--
Gustavo

> 
> [1] https://patchwork.kernel.org/project/linux-wireless/patch/d522f387b2d0dde774785c7169c1f25aa529989d.1605896060.git.gustavoars@kernel.org/
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
  2021-03-05 13:40 ` Kalle Valo
  2021-03-05 16:49   ` Gustavo A. R. Silva
@ 2021-03-10 19:14   ` Kees Cook
  2021-03-10 19:31     ` Jes Sorensen
  2021-03-11  7:00     ` Kalle Valo
  1 sibling, 2 replies; 18+ messages in thread
From: Kees Cook @ 2021-03-10 19:14 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Gustavo A. R. Silva, Jes Sorensen, David S. Miller,
	Jakub Kicinski, linux-wireless, netdev, linux-kernel,
	linux-hardening

On Fri, Mar 05, 2021 at 03:40:33PM +0200, Kalle Valo wrote:
> "Gustavo A. R. Silva" <gustavoars@kernel.org> writes:
> 
> > In preparation to enable -Wimplicit-fallthrough for Clang, fix
> > multiple warnings by replacing /* fall through */ comments with
> > the new pseudo-keyword macro fallthrough; instead of letting the
> > code fall through to the next case.
> >
> > Notice that Clang doesn't recognize /* fall through */ comments as
> > implicit fall-through markings.
> >
> > Link: https://github.com/KSPP/linux/issues/115
> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> 
> It's not cool that you ignore the comments you got in [1], then after a
> while mark the patch as "RESEND" and not even include a changelog why it
> was resent.
> 
> [1] https://patchwork.kernel.org/project/linux-wireless/patch/d522f387b2d0dde774785c7169c1f25aa529989d.1605896060.git.gustavoars@kernel.org/

Hm, this conversation looks like a miscommunication, mainly? I see
Gustavo, as requested by many others[1], replacing the fallthrough
comments with the "fallthrough" statement. (This is more than just a
"Clang doesn't parse comments" issue.)

This could be a tree-wide patch and not bother you, but Greg KH has
generally advised us to send these changes broken out. Anyway, this
change still needs to land, so what would be the preferred path? I think
Gustavo could just carry it for Linus to merge without bothering you if
that'd be preferred?

-Kees

[1] https://git.kernel.org/linus/294f69e662d1570703e9b56e95be37a9fd3afba5

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
  2021-03-10 19:14   ` Kees Cook
@ 2021-03-10 19:31     ` Jes Sorensen
  2021-03-10 19:45       ` Kees Cook
  2021-03-11  7:00     ` Kalle Valo
  1 sibling, 1 reply; 18+ messages in thread
From: Jes Sorensen @ 2021-03-10 19:31 UTC (permalink / raw)
  To: Kees Cook, Kalle Valo
  Cc: Gustavo A. R. Silva, David S. Miller, Jakub Kicinski,
	linux-wireless, netdev, linux-kernel, linux-hardening

On 3/10/21 2:14 PM, Kees Cook wrote:
> On Fri, Mar 05, 2021 at 03:40:33PM +0200, Kalle Valo wrote:
>> "Gustavo A. R. Silva" <gustavoars@kernel.org> writes:
>>
>>> In preparation to enable -Wimplicit-fallthrough for Clang, fix
>>> multiple warnings by replacing /* fall through */ comments with
>>> the new pseudo-keyword macro fallthrough; instead of letting the
>>> code fall through to the next case.
>>>
>>> Notice that Clang doesn't recognize /* fall through */ comments as
>>> implicit fall-through markings.
>>>
>>> Link: https://github.com/KSPP/linux/issues/115
>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>>
>> It's not cool that you ignore the comments you got in [1], then after a
>> while mark the patch as "RESEND" and not even include a changelog why it
>> was resent.
>>
>> [1] https://patchwork.kernel.org/project/linux-wireless/patch/d522f387b2d0dde774785c7169c1f25aa529989d.1605896060.git.gustavoars@kernel.org/
> 
> Hm, this conversation looks like a miscommunication, mainly? I see
> Gustavo, as requested by many others[1], replacing the fallthrough
> comments with the "fallthrough" statement. (This is more than just a
> "Clang doesn't parse comments" issue.)
> 
> This could be a tree-wide patch and not bother you, but Greg KH has
> generally advised us to send these changes broken out. Anyway, this
> change still needs to land, so what would be the preferred path? I think
> Gustavo could just carry it for Linus to merge without bothering you if
> that'd be preferred?

I'll respond with the same I did last time, fallthrough is not C and
it's ugly.

Instead of mutilating the kernel, Gustavo should invest in fixing the
broken clang compiler.

Thanks,
Jes


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
  2021-03-10 19:31     ` Jes Sorensen
@ 2021-03-10 19:45       ` Kees Cook
  2021-03-10 19:51         ` Jes Sorensen
  0 siblings, 1 reply; 18+ messages in thread
From: Kees Cook @ 2021-03-10 19:45 UTC (permalink / raw)
  To: Jes Sorensen
  Cc: Kalle Valo, Gustavo A. R. Silva, David S. Miller, Jakub Kicinski,
	linux-wireless, netdev, linux-kernel, linux-hardening

On Wed, Mar 10, 2021 at 02:31:57PM -0500, Jes Sorensen wrote:
> On 3/10/21 2:14 PM, Kees Cook wrote:
> > On Fri, Mar 05, 2021 at 03:40:33PM +0200, Kalle Valo wrote:
> >> "Gustavo A. R. Silva" <gustavoars@kernel.org> writes:
> >>
> >>> In preparation to enable -Wimplicit-fallthrough for Clang, fix
> >>> multiple warnings by replacing /* fall through */ comments with
> >>> the new pseudo-keyword macro fallthrough; instead of letting the
> >>> code fall through to the next case.
> >>>
> >>> Notice that Clang doesn't recognize /* fall through */ comments as
> >>> implicit fall-through markings.
> >>>
> >>> Link: https://github.com/KSPP/linux/issues/115
> >>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> >>
> >> It's not cool that you ignore the comments you got in [1], then after a
> >> while mark the patch as "RESEND" and not even include a changelog why it
> >> was resent.
> >>
> >> [1] https://patchwork.kernel.org/project/linux-wireless/patch/d522f387b2d0dde774785c7169c1f25aa529989d.1605896060.git.gustavoars@kernel.org/
> > 
> > Hm, this conversation looks like a miscommunication, mainly? I see
> > Gustavo, as requested by many others[1], replacing the fallthrough
> > comments with the "fallthrough" statement. (This is more than just a
> > "Clang doesn't parse comments" issue.)
> > 
> > This could be a tree-wide patch and not bother you, but Greg KH has
> > generally advised us to send these changes broken out. Anyway, this
> > change still needs to land, so what would be the preferred path? I think
> > Gustavo could just carry it for Linus to merge without bothering you if
> > that'd be preferred?
> 
> I'll respond with the same I did last time, fallthrough is not C and
> it's ugly.

I understand your point of view, but this is not the consensus[1] of
the community. "fallthrough" is a macro, using the GCC fallthrough
attribute, with the expectation that we can move to the C17/C18
"[[fallthrough]]" statement once it is finalized by the C standards
body.

-Kees

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#implicit-switch-case-fall-through

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
  2021-03-10 19:45       ` Kees Cook
@ 2021-03-10 19:51         ` Jes Sorensen
  2021-03-10 20:59           ` Kees Cook
  0 siblings, 1 reply; 18+ messages in thread
From: Jes Sorensen @ 2021-03-10 19:51 UTC (permalink / raw)
  To: Kees Cook
  Cc: Kalle Valo, Gustavo A. R. Silva, David S. Miller, Jakub Kicinski,
	linux-wireless, netdev, linux-kernel, linux-hardening

On 3/10/21 2:45 PM, Kees Cook wrote:
> On Wed, Mar 10, 2021 at 02:31:57PM -0500, Jes Sorensen wrote:
>> On 3/10/21 2:14 PM, Kees Cook wrote:
>>> Hm, this conversation looks like a miscommunication, mainly? I see
>>> Gustavo, as requested by many others[1], replacing the fallthrough
>>> comments with the "fallthrough" statement. (This is more than just a
>>> "Clang doesn't parse comments" issue.)
>>>
>>> This could be a tree-wide patch and not bother you, but Greg KH has
>>> generally advised us to send these changes broken out. Anyway, this
>>> change still needs to land, so what would be the preferred path? I think
>>> Gustavo could just carry it for Linus to merge without bothering you if
>>> that'd be preferred?
>>
>> I'll respond with the same I did last time, fallthrough is not C and
>> it's ugly.
> 
> I understand your point of view, but this is not the consensus[1] of
> the community. "fallthrough" is a macro, using the GCC fallthrough
> attribute, with the expectation that we can move to the C17/C18
> "[[fallthrough]]" statement once it is finalized by the C standards
> body.

I don't know who decided on that, but I still disagree. It's an ugly and
pointless change that serves little purpose. We shouldn't have allowed
the ugly /* fall-through */ comments in either, but at least they didn't
mess with the code. I guess when you give someone an inch, they take a mile.

Last time this came up, the discussion was that clang refused to fix
their brokenness and therefore this nonsense was being pushed into the
kernel. It's still a pointless argument, if clang can't fix it's crap,
then stop using it.

As Kalle correctly pointed out, none of the previous comments to this
were addressed, the patches were just reposted as fact. Not exactly a
nice way to go about it either.

Jes

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
  2021-03-10 19:51         ` Jes Sorensen
@ 2021-03-10 20:59           ` Kees Cook
  2021-04-17 18:29             ` Jes Sorensen
  0 siblings, 1 reply; 18+ messages in thread
From: Kees Cook @ 2021-03-10 20:59 UTC (permalink / raw)
  To: Jes Sorensen
  Cc: Kalle Valo, Gustavo A. R. Silva, David S. Miller, Jakub Kicinski,
	linux-wireless, netdev, linux-kernel, linux-hardening

On Wed, Mar 10, 2021 at 02:51:24PM -0500, Jes Sorensen wrote:
> On 3/10/21 2:45 PM, Kees Cook wrote:
> > On Wed, Mar 10, 2021 at 02:31:57PM -0500, Jes Sorensen wrote:
> >> On 3/10/21 2:14 PM, Kees Cook wrote:
> >>> Hm, this conversation looks like a miscommunication, mainly? I see
> >>> Gustavo, as requested by many others[1], replacing the fallthrough
> >>> comments with the "fallthrough" statement. (This is more than just a
> >>> "Clang doesn't parse comments" issue.)
> >>>
> >>> This could be a tree-wide patch and not bother you, but Greg KH has
> >>> generally advised us to send these changes broken out. Anyway, this
> >>> change still needs to land, so what would be the preferred path? I think
> >>> Gustavo could just carry it for Linus to merge without bothering you if
> >>> that'd be preferred?
> >>
> >> I'll respond with the same I did last time, fallthrough is not C and
> >> it's ugly.
> > 
> > I understand your point of view, but this is not the consensus[1] of
> > the community. "fallthrough" is a macro, using the GCC fallthrough
> > attribute, with the expectation that we can move to the C17/C18
> > "[[fallthrough]]" statement once it is finalized by the C standards
> > body.
> 
> I don't know who decided on that, but I still disagree. It's an ugly and
> pointless change that serves little purpose. We shouldn't have allowed
> the ugly /* fall-through */ comments in either, but at least they didn't
> mess with the code. I guess when you give someone an inch, they take a mile.
> 
> Last time this came up, the discussion was that clang refused to fix
> their brokenness and therefore this nonsense was being pushed into the
> kernel. It's still a pointless argument, if clang can't fix it's crap,
> then stop using it.
> 
> As Kalle correctly pointed out, none of the previous comments to this
> were addressed, the patches were just reposted as fact. Not exactly a
> nice way to go about it either.

Do you mean changing the commit log to re-justify these changes? I
guess that could be done, but based on the thread, it didn't seem to
be needed. The change is happening to match the coding style consensus
reached to give the kernel the flexibility to move from a gcc extension
to the final C standards committee results without having to do treewide
commits again (i.e. via the macro).

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
  2021-03-10 19:14   ` Kees Cook
  2021-03-10 19:31     ` Jes Sorensen
@ 2021-03-11  7:00     ` Kalle Valo
  2021-03-11  7:16       ` Gustavo A. R. Silva
  1 sibling, 1 reply; 18+ messages in thread
From: Kalle Valo @ 2021-03-11  7:00 UTC (permalink / raw)
  To: Kees Cook
  Cc: Gustavo A. R. Silva, Jes Sorensen, David S. Miller,
	Jakub Kicinski, linux-wireless, netdev, linux-kernel,
	linux-hardening

Kees Cook <keescook@chromium.org> writes:

> On Fri, Mar 05, 2021 at 03:40:33PM +0200, Kalle Valo wrote:
>> "Gustavo A. R. Silva" <gustavoars@kernel.org> writes:
>> 
>> > In preparation to enable -Wimplicit-fallthrough for Clang, fix
>> > multiple warnings by replacing /* fall through */ comments with
>> > the new pseudo-keyword macro fallthrough; instead of letting the
>> > code fall through to the next case.
>> >
>> > Notice that Clang doesn't recognize /* fall through */ comments as
>> > implicit fall-through markings.
>> >
>> > Link: https://github.com/KSPP/linux/issues/115
>> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> 
>> It's not cool that you ignore the comments you got in [1], then after a
>> while mark the patch as "RESEND" and not even include a changelog why it
>> was resent.
>> 
>> [1] https://patchwork.kernel.org/project/linux-wireless/patch/d522f387b2d0dde774785c7169c1f25aa529989d.1605896060.git.gustavoars@kernel.org/
>
> Hm, this conversation looks like a miscommunication, mainly? I see
> Gustavo, as requested by many others[1], replacing the fallthrough
> comments with the "fallthrough" statement. (This is more than just a
> "Clang doesn't parse comments" issue.)

v1 was clearly rejected by Jes, so sending a new version without any
changelog or comments is not the way to go. The changelog shoud at least
have had "v1 was rejected but I'm resending this again because <insert
reason here>" or something like that to make it clear what's happening.

> This could be a tree-wide patch and not bother you, but Greg KH has
> generally advised us to send these changes broken out. Anyway, this
> change still needs to land, so what would be the preferred path? I think
> Gustavo could just carry it for Linus to merge without bothering you if
> that'd be preferred?

I agree with Greg. Please don't do cleanups like this via another tree
as that just creates more work due to conflicts between the trees, which
is a lot more annoying to deal with than applying few patches. But when
submitting patches please follow the rules, don't cut corners.

Jes, I don't like 'fallthrough' either and prefer the original comment,
but the ship has sailed on this one. Maybe we should just take it?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
  2021-03-11  7:00     ` Kalle Valo
@ 2021-03-11  7:16       ` Gustavo A. R. Silva
  0 siblings, 0 replies; 18+ messages in thread
From: Gustavo A. R. Silva @ 2021-03-11  7:16 UTC (permalink / raw)
  To: Kalle Valo, Kees Cook
  Cc: Gustavo A. R. Silva, Jes Sorensen, David S. Miller,
	Jakub Kicinski, linux-wireless, netdev, linux-kernel,
	linux-hardening



On 3/11/21 01:00, Kalle Valo wrote:
> Kees Cook <keescook@chromium.org> writes:
> 
>> On Fri, Mar 05, 2021 at 03:40:33PM +0200, Kalle Valo wrote:
>>> "Gustavo A. R. Silva" <gustavoars@kernel.org> writes:
>>>
>>>> In preparation to enable -Wimplicit-fallthrough for Clang, fix
>>>> multiple warnings by replacing /* fall through */ comments with
>>>> the new pseudo-keyword macro fallthrough; instead of letting the
>>>> code fall through to the next case.
>>>>
>>>> Notice that Clang doesn't recognize /* fall through */ comments as
>>>> implicit fall-through markings.
>>>>
>>>> Link: https://github.com/KSPP/linux/issues/115
>>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>>>
>>> It's not cool that you ignore the comments you got in [1], then after a
>>> while mark the patch as "RESEND" and not even include a changelog why it
>>> was resent.
>>>
>>> [1] https://patchwork.kernel.org/project/linux-wireless/patch/d522f387b2d0dde774785c7169c1f25aa529989d.1605896060.git.gustavoars@kernel.org/
>>
>> Hm, this conversation looks like a miscommunication, mainly? I see
>> Gustavo, as requested by many others[1], replacing the fallthrough
>> comments with the "fallthrough" statement. (This is more than just a
>> "Clang doesn't parse comments" issue.)
> 
> v1 was clearly rejected by Jes, so sending a new version without any
> changelog or comments is not the way to go. The changelog shoud at least
> have had "v1 was rejected but I'm resending this again because <insert
> reason here>" or something like that to make it clear what's happening.

Why the fact that I replied to that original thread with the message
below is being ignored?

"Just notice that the idea behind this and the following patch is exactly
the same:

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git/commit/?id=3f95e92c8a8516b745594049dfccc8c5f8895eea

I could resend this same patch with a different changelog text, but I
don't think such a thing is necessary. However, if people prefer that
approach, just let me know and I can do it.

Thanks
--
Gustavo"

Why no one replied to what I was proposing at the time?

It seems to me that the person that was ignored was actually me, and not the
other way around. :/

--
Gustavo

> 
>> This could be a tree-wide patch and not bother you, but Greg KH has
>> generally advised us to send these changes broken out. Anyway, this
>> change still needs to land, so what would be the preferred path? I think
>> Gustavo could just carry it for Linus to merge without bothering you if
>> that'd be preferred?
> 
> I agree with Greg. Please don't do cleanups like this via another tree
> as that just creates more work due to conflicts between the trees, which
> is a lot more annoying to deal with than applying few patches. But when
> submitting patches please follow the rules, don't cut corners.
> 
> Jes, I don't like 'fallthrough' either and prefer the original comment,
> but the ship has sailed on this one. Maybe we should just take it?
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
  2021-03-05  9:48 [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang Gustavo A. R. Silva
  2021-03-05 13:40 ` Kalle Valo
@ 2021-04-17 17:52 ` Kalle Valo
       [not found] ` <20210417175201.2D5A7C433F1@smtp.codeaurora.org>
       [not found] ` <20210417175201.280F9C4338A@smtp.codeaurora.org>
  3 siblings, 0 replies; 18+ messages in thread
From: Kalle Valo @ 2021-04-17 17:52 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Jes Sorensen, David S. Miller, Jakub Kicinski, linux-wireless,
	netdev, linux-kernel, Gustavo A. R. Silva, linux-hardening

"Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:

> In preparation to enable -Wimplicit-fallthrough for Clang, fix
> multiple warnings by replacing /* fall through */ comments with
> the new pseudo-keyword macro fallthrough; instead of letting the
> code fall through to the next case.
> 
> Notice that Clang doesn't recognize /* fall through */ comments as
> implicit fall-through markings.
> 
> Link: https://github.com/KSPP/linux/issues/115
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Patch applied to wireless-drivers-next.git, thanks.

bf3365a856a1 rtl8xxxu: Fix fall-through warnings for Clang

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20210305094850.GA141221@embeddedor/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
  2021-03-10 20:59           ` Kees Cook
@ 2021-04-17 18:29             ` Jes Sorensen
  2021-04-17 19:24               ` Gustavo A. R. Silva
  0 siblings, 1 reply; 18+ messages in thread
From: Jes Sorensen @ 2021-04-17 18:29 UTC (permalink / raw)
  To: Kees Cook
  Cc: Kalle Valo, Gustavo A. R. Silva, David S. Miller, Jakub Kicinski,
	linux-wireless, netdev, linux-kernel, linux-hardening

On 3/10/21 3:59 PM, Kees Cook wrote:
> On Wed, Mar 10, 2021 at 02:51:24PM -0500, Jes Sorensen wrote:
>> On 3/10/21 2:45 PM, Kees Cook wrote:
>>> On Wed, Mar 10, 2021 at 02:31:57PM -0500, Jes Sorensen wrote:
>>>> On 3/10/21 2:14 PM, Kees Cook wrote:
>>>>> Hm, this conversation looks like a miscommunication, mainly? I see
>>>>> Gustavo, as requested by many others[1], replacing the fallthrough
>>>>> comments with the "fallthrough" statement. (This is more than just a
>>>>> "Clang doesn't parse comments" issue.)
>>>>>
>>>>> This could be a tree-wide patch and not bother you, but Greg KH has
>>>>> generally advised us to send these changes broken out. Anyway, this
>>>>> change still needs to land, so what would be the preferred path? I think
>>>>> Gustavo could just carry it for Linus to merge without bothering you if
>>>>> that'd be preferred?
>>>>
>>>> I'll respond with the same I did last time, fallthrough is not C and
>>>> it's ugly.
>>>
>>> I understand your point of view, but this is not the consensus[1] of
>>> the community. "fallthrough" is a macro, using the GCC fallthrough
>>> attribute, with the expectation that we can move to the C17/C18
>>> "[[fallthrough]]" statement once it is finalized by the C standards
>>> body.
>>
>> I don't know who decided on that, but I still disagree. It's an ugly and
>> pointless change that serves little purpose. We shouldn't have allowed
>> the ugly /* fall-through */ comments in either, but at least they didn't
>> mess with the code. I guess when you give someone an inch, they take a mile.
>>
>> Last time this came up, the discussion was that clang refused to fix
>> their brokenness and therefore this nonsense was being pushed into the
>> kernel. It's still a pointless argument, if clang can't fix it's crap,
>> then stop using it.
>>
>> As Kalle correctly pointed out, none of the previous comments to this
>> were addressed, the patches were just reposted as fact. Not exactly a
>> nice way to go about it either.
> 
> Do you mean changing the commit log to re-justify these changes? I
> guess that could be done, but based on the thread, it didn't seem to
> be needed. The change is happening to match the coding style consensus
> reached to give the kernel the flexibility to move from a gcc extension
> to the final C standards committee results without having to do treewide
> commits again (i.e. via the macro).

No, I am questioning why Gustavo continues to push this nonsense that
serves no purpose whatsoever. In addition he has consistently ignored
comments and just keep reposting it. But I guess that is how it works,
ignore feedback, repost junk, repeat.

Jes


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
       [not found] ` <20210417175201.2D5A7C433F1@smtp.codeaurora.org>
@ 2021-04-17 18:30   ` Jes Sorensen
  2021-04-18  0:09     ` Joe Perches
  0 siblings, 1 reply; 18+ messages in thread
From: Jes Sorensen @ 2021-04-17 18:30 UTC (permalink / raw)
  To: Kalle Valo, Gustavo A. R. Silva
  Cc: David S. Miller, Jakub Kicinski, linux-wireless, netdev,
	linux-kernel, linux-hardening

On 4/17/21 1:52 PM, Kalle Valo wrote:
> "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
> 
>> In preparation to enable -Wimplicit-fallthrough for Clang, fix
>> multiple warnings by replacing /* fall through */ comments with
>> the new pseudo-keyword macro fallthrough; instead of letting the
>> code fall through to the next case.
>>
>> Notice that Clang doesn't recognize /* fall through */ comments as
>> implicit fall-through markings.
>>
>> Link: https://github.com/KSPP/linux/issues/115
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> 
> Patch applied to wireless-drivers-next.git, thanks.
> 
> bf3365a856a1 rtl8xxxu: Fix fall-through warnings for Clang
> 

Sorry this junk patch should not have been applied.

Jes

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
  2021-04-17 18:29             ` Jes Sorensen
@ 2021-04-17 19:24               ` Gustavo A. R. Silva
  2021-04-19 11:58                 ` Jes Sorensen
  0 siblings, 1 reply; 18+ messages in thread
From: Gustavo A. R. Silva @ 2021-04-17 19:24 UTC (permalink / raw)
  To: Jes Sorensen, Kees Cook
  Cc: Kalle Valo, Gustavo A. R. Silva, David S. Miller, Jakub Kicinski,
	linux-wireless, netdev, linux-kernel, linux-hardening



On 4/17/21 13:29, Jes Sorensen wrote:
> On 3/10/21 3:59 PM, Kees Cook wrote:
>> On Wed, Mar 10, 2021 at 02:51:24PM -0500, Jes Sorensen wrote:
>>> On 3/10/21 2:45 PM, Kees Cook wrote:
>>>> On Wed, Mar 10, 2021 at 02:31:57PM -0500, Jes Sorensen wrote:
>>>>> On 3/10/21 2:14 PM, Kees Cook wrote:
>>>>>> Hm, this conversation looks like a miscommunication, mainly? I see
>>>>>> Gustavo, as requested by many others[1], replacing the fallthrough
>>>>>> comments with the "fallthrough" statement. (This is more than just a
>>>>>> "Clang doesn't parse comments" issue.)
>>>>>>
>>>>>> This could be a tree-wide patch and not bother you, but Greg KH has
>>>>>> generally advised us to send these changes broken out. Anyway, this
>>>>>> change still needs to land, so what would be the preferred path? I think
>>>>>> Gustavo could just carry it for Linus to merge without bothering you if
>>>>>> that'd be preferred?
>>>>>
>>>>> I'll respond with the same I did last time, fallthrough is not C and
>>>>> it's ugly.
>>>>
>>>> I understand your point of view, but this is not the consensus[1] of
>>>> the community. "fallthrough" is a macro, using the GCC fallthrough
>>>> attribute, with the expectation that we can move to the C17/C18
>>>> "[[fallthrough]]" statement once it is finalized by the C standards
>>>> body.
>>>
>>> I don't know who decided on that, but I still disagree. It's an ugly and
>>> pointless change that serves little purpose. We shouldn't have allowed
>>> the ugly /* fall-through */ comments in either, but at least they didn't
>>> mess with the code. I guess when you give someone an inch, they take a mile.
>>>
>>> Last time this came up, the discussion was that clang refused to fix
>>> their brokenness and therefore this nonsense was being pushed into the
>>> kernel. It's still a pointless argument, if clang can't fix it's crap,
>>> then stop using it.
>>>
>>> As Kalle correctly pointed out, none of the previous comments to this
>>> were addressed, the patches were just reposted as fact. Not exactly a
>>> nice way to go about it either.
>>
>> Do you mean changing the commit log to re-justify these changes? I
>> guess that could be done, but based on the thread, it didn't seem to
>> be needed. The change is happening to match the coding style consensus
>> reached to give the kernel the flexibility to move from a gcc extension
>> to the final C standards committee results without having to do treewide
>> commits again (i.e. via the macro).
> 
> No, I am questioning why Gustavo continues to push this nonsense that
> serves no purpose whatsoever. In addition he has consistently ignored
> comments and just keep reposting it. But I guess that is how it works,
> ignore feedback, repost junk, repeat.

I was asking for feedback here[1] and here[2] after people (you and Kalle)
commented on this patch. How is that ignoring people? And -again- why
people ignored my requests for feedback in this conversation? It's a mystery
to me, honestly.

Thanks
--
Gustavo

[1] https://lore.kernel.org/lkml/20201124160906.GB17735@embeddedor/
[2] https://lore.kernel.org/lkml/e10b2a6a-d91a-9783-ddbe-ea2c10a1539a@embeddedor.com/

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
  2021-04-17 18:30   ` Jes Sorensen
@ 2021-04-18  0:09     ` Joe Perches
  2021-04-19 11:56       ` Jes Sorensen
  0 siblings, 1 reply; 18+ messages in thread
From: Joe Perches @ 2021-04-18  0:09 UTC (permalink / raw)
  To: Jes Sorensen, Kalle Valo, Gustavo A. R. Silva
  Cc: David S. Miller, Jakub Kicinski, linux-wireless, netdev,
	linux-kernel, linux-hardening

On Sat, 2021-04-17 at 14:30 -0400, Jes Sorensen wrote:
> On 4/17/21 1:52 PM, Kalle Valo wrote:
> > "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
> > 
> > > In preparation to enable -Wimplicit-fallthrough for Clang, fix
> > > multiple warnings by replacing /* fall through */ comments with
> > > the new pseudo-keyword macro fallthrough; instead of letting the
> > > code fall through to the next case.
> > > 
> > > Notice that Clang doesn't recognize /* fall through */ comments as
> > > implicit fall-through markings.
> > > 
> > > Link: https://github.com/KSPP/linux/issues/115
> > > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> > 
> > Patch applied to wireless-drivers-next.git, thanks.
> > 
> > bf3365a856a1 rtl8xxxu: Fix fall-through warnings for Clang
> > 
> 
> Sorry this junk patch should not have been applied.

I don't believe it's a junk patch.
I believe your characterization of it as such is flawed.

You don't like the style, that's fine, but:

Any code in the kernel should not be a unique style of your own choosing
when it could cause various compilers to emit unnecessary warnings.

Please remember the kernel code base is a formed by a community with a
nominally generally accepted style.  There is a real desire in that
community to both enable compiler warnings that might show defects and
simultaneously avoid unnecessary compiler warnings.

This particular change just avoids a possible compiler warning.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
  2021-04-18  0:09     ` Joe Perches
@ 2021-04-19 11:56       ` Jes Sorensen
  0 siblings, 0 replies; 18+ messages in thread
From: Jes Sorensen @ 2021-04-19 11:56 UTC (permalink / raw)
  To: Joe Perches, Kalle Valo, Gustavo A. R. Silva
  Cc: David S. Miller, Jakub Kicinski, linux-wireless, netdev,
	linux-kernel, linux-hardening

On 4/17/21 8:09 PM, Joe Perches wrote:
> On Sat, 2021-04-17 at 14:30 -0400, Jes Sorensen wrote:
>> On 4/17/21 1:52 PM, Kalle Valo wrote:
>>> "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
>>>
>>>> In preparation to enable -Wimplicit-fallthrough for Clang, fix
>>>> multiple warnings by replacing /* fall through */ comments with
>>>> the new pseudo-keyword macro fallthrough; instead of letting the
>>>> code fall through to the next case.
>>>>
>>>> Notice that Clang doesn't recognize /* fall through */ comments as
>>>> implicit fall-through markings.
>>>>
>>>> Link: https://github.com/KSPP/linux/issues/115
>>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>>>
>>> Patch applied to wireless-drivers-next.git, thanks.
>>>
>>> bf3365a856a1 rtl8xxxu: Fix fall-through warnings for Clang
>>>
>>
>> Sorry this junk patch should not have been applied.
> 
> I don't believe it's a junk patch.
> I believe your characterization of it as such is flawed.
> 
> You don't like the style, that's fine, but:
> 
> Any code in the kernel should not be a unique style of your own choosing
> when it could cause various compilers to emit unnecessary warnings.
> 
> Please remember the kernel code base is a formed by a community with a
> nominally generally accepted style.  There is a real desire in that
> community to both enable compiler warnings that might show defects and
> simultaneously avoid unnecessary compiler warnings.
> 
> This particular change just avoids a possible compiler warning.

Please go back and look at the thread. This patch fixes nothing, it
mutilates the code by introducing non C for the sole purpose of avoiding
to work with the Clang community to fix their broken compiler. The
author of this patch ignored previous feedback and just reposted the
same patch unaltered and when it was called out, the answer was other
people was fine with it. None of the issues raised have been addressed,
so yes, that makes the patch junk.

Jes

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
  2021-04-17 19:24               ` Gustavo A. R. Silva
@ 2021-04-19 11:58                 ` Jes Sorensen
  0 siblings, 0 replies; 18+ messages in thread
From: Jes Sorensen @ 2021-04-19 11:58 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Kees Cook
  Cc: Kalle Valo, Gustavo A. R. Silva, David S. Miller, Jakub Kicinski,
	linux-wireless, netdev, linux-kernel, linux-hardening

On 4/17/21 3:24 PM, Gustavo A. R. Silva wrote:
> 
> 
> On 4/17/21 13:29, Jes Sorensen wrote:
>> On 3/10/21 3:59 PM, Kees Cook wrote:
>>> On Wed, Mar 10, 2021 at 02:51:24PM -0500, Jes Sorensen wrote:
>>>> On 3/10/21 2:45 PM, Kees Cook wrote:
>>>>> On Wed, Mar 10, 2021 at 02:31:57PM -0500, Jes Sorensen wrote:
>>>>>> On 3/10/21 2:14 PM, Kees Cook wrote:
>>>>>>> Hm, this conversation looks like a miscommunication, mainly? I see
>>>>>>> Gustavo, as requested by many others[1], replacing the fallthrough
>>>>>>> comments with the "fallthrough" statement. (This is more than just a
>>>>>>> "Clang doesn't parse comments" issue.)
>>>>>>>
>>>>>>> This could be a tree-wide patch and not bother you, but Greg KH has
>>>>>>> generally advised us to send these changes broken out. Anyway, this
>>>>>>> change still needs to land, so what would be the preferred path? I think
>>>>>>> Gustavo could just carry it for Linus to merge without bothering you if
>>>>>>> that'd be preferred?
>>>>>>
>>>>>> I'll respond with the same I did last time, fallthrough is not C and
>>>>>> it's ugly.
>>>>>
>>>>> I understand your point of view, but this is not the consensus[1] of
>>>>> the community. "fallthrough" is a macro, using the GCC fallthrough
>>>>> attribute, with the expectation that we can move to the C17/C18
>>>>> "[[fallthrough]]" statement once it is finalized by the C standards
>>>>> body.
>>>>
>>>> I don't know who decided on that, but I still disagree. It's an ugly and
>>>> pointless change that serves little purpose. We shouldn't have allowed
>>>> the ugly /* fall-through */ comments in either, but at least they didn't
>>>> mess with the code. I guess when you give someone an inch, they take a mile.
>>>>
>>>> Last time this came up, the discussion was that clang refused to fix
>>>> their brokenness and therefore this nonsense was being pushed into the
>>>> kernel. It's still a pointless argument, if clang can't fix it's crap,
>>>> then stop using it.
>>>>
>>>> As Kalle correctly pointed out, none of the previous comments to this
>>>> were addressed, the patches were just reposted as fact. Not exactly a
>>>> nice way to go about it either.
>>>
>>> Do you mean changing the commit log to re-justify these changes? I
>>> guess that could be done, but based on the thread, it didn't seem to
>>> be needed. The change is happening to match the coding style consensus
>>> reached to give the kernel the flexibility to move from a gcc extension
>>> to the final C standards committee results without having to do treewide
>>> commits again (i.e. via the macro).
>>
>> No, I am questioning why Gustavo continues to push this nonsense that
>> serves no purpose whatsoever. In addition he has consistently ignored
>> comments and just keep reposting it. But I guess that is how it works,
>> ignore feedback, repost junk, repeat.
> 
> I was asking for feedback here[1] and here[2] after people (you and Kalle)
> commented on this patch. How is that ignoring people? And -again- why
> people ignored my requests for feedback in this conversation? It's a mystery
> to me, honestly.

All you did was post a pointer to the fact that some other people
couldn't be bothered speaking out against the patch, and let it go in.
You haven't addressed any of the original concerns raised.

The big mistake here was of course to allow the pointless /* fallthrough
*/ changes to go in in the first place.

Jes

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
       [not found] ` <20210417175201.280F9C4338A@smtp.codeaurora.org>
@ 2021-04-19 22:58   ` Gustavo A. R. Silva
  0 siblings, 0 replies; 18+ messages in thread
From: Gustavo A. R. Silva @ 2021-04-19 22:58 UTC (permalink / raw)
  To: Kalle Valo, Gustavo A. R. Silva
  Cc: Jes Sorensen, David S. Miller, Jakub Kicinski, linux-wireless,
	netdev, linux-kernel, linux-hardening, Kees Cook



On 4/17/21 12:52, Kalle Valo wrote:
> "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
> 
>> In preparation to enable -Wimplicit-fallthrough for Clang, fix
>> multiple warnings by replacing /* fall through */ comments with
>> the new pseudo-keyword macro fallthrough; instead of letting the
>> code fall through to the next case.
>>
>> Notice that Clang doesn't recognize /* fall through */ comments as
>> implicit fall-through markings.
>>
>> Link: https://github.com/KSPP/linux/issues/115
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> 
> Patch applied to wireless-drivers-next.git, thanks.
> 
> bf3365a856a1 rtl8xxxu: Fix fall-through warnings for Clang

Thanks for this, Kalle.

Could you take this series too, please?

https://lore.kernel.org/lkml/cover.1618442265.git.gustavoars@kernel.org/

Thanks
--
Gustavo


^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2021-04-19 23:47 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-05  9:48 [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang Gustavo A. R. Silva
2021-03-05 13:40 ` Kalle Valo
2021-03-05 16:49   ` Gustavo A. R. Silva
2021-03-10 19:14   ` Kees Cook
2021-03-10 19:31     ` Jes Sorensen
2021-03-10 19:45       ` Kees Cook
2021-03-10 19:51         ` Jes Sorensen
2021-03-10 20:59           ` Kees Cook
2021-04-17 18:29             ` Jes Sorensen
2021-04-17 19:24               ` Gustavo A. R. Silva
2021-04-19 11:58                 ` Jes Sorensen
2021-03-11  7:00     ` Kalle Valo
2021-03-11  7:16       ` Gustavo A. R. Silva
2021-04-17 17:52 ` Kalle Valo
     [not found] ` <20210417175201.2D5A7C433F1@smtp.codeaurora.org>
2021-04-17 18:30   ` Jes Sorensen
2021-04-18  0:09     ` Joe Perches
2021-04-19 11:56       ` Jes Sorensen
     [not found] ` <20210417175201.280F9C4338A@smtp.codeaurora.org>
2021-04-19 22:58   ` Gustavo A. R. Silva

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.