diff mbox

[00/22] Replace comments with C99 initializers

Message ID 8f9271b6-0381-70a9-f0c2-595b2235866a@stuerz.xyz (mailing list archive)
State Not Applicable
Delegated to: Johannes Berg
Headers show

Commit Message

Benjamin Stürz March 27, 2022, 12:46 p.m. UTC
This patch series replaces comments with C99's designated initializers
in a few places. It also adds some enum initializers. This is my first
time contributing to the Linux kernel, therefore I'm probably doing a
lot of things the wrong way. I'm sorry for that.

I've gotten a few emails so far stating that this patch series is
unnecessary. Yes, in fact this patch series is not necessary by itself,
but it could help me understand how the whole process works and maybe I
could help somewhere, where help is actually needed.

This patch itself is a no-op.

Signed-off-by: Benjamin Stürz <benni@stuerz.xyz>
---
 .gitignore | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kalle Valo March 28, 2022, 9:33 a.m. UTC | #1
Benjamin Stürz <benni@stuerz.xyz> writes:

> This patch series replaces comments with C99's designated initializers
> in a few places. It also adds some enum initializers. This is my first
> time contributing to the Linux kernel, therefore I'm probably doing a
> lot of things the wrong way. I'm sorry for that.

Just a small tip: If you are new, start with something small and learn
from that. Don't do a controversial big patchset spanning multiple
subsystems, that's the hard way to learn things. First submit one patch
at a time to one subsystem and gain understanding of the process that
way.
Benjamin Stürz March 28, 2022, 11:51 a.m. UTC | #2
On 28.03.22 11:33, Kalle Valo wrote:
> Benjamin Stürz <benni@stuerz.xyz> writes:
> 
>> This patch series replaces comments with C99's designated initializers
>> in a few places. It also adds some enum initializers. This is my first
>> time contributing to the Linux kernel, therefore I'm probably doing a
>> lot of things the wrong way. I'm sorry for that.
> 
> Just a small tip: If you are new, start with something small and learn
> from that. Don't do a controversial big patchset spanning multiple
> subsystems, that's the hard way to learn things. First submit one patch
> at a time to one subsystem and gain understanding of the process that
> way.
> 

I actually thought this would be such simple thing. Do you know of any
good thing where to start? I already looked into drivers/staging/*/TODO
and didn't found something for me personally.

Should I drop this patchset and start with something different? If yes,
what would the proper way to drop it? Just announcing, that this is
going nowhere in a separate patch?
Kalle Valo March 28, 2022, 12:31 p.m. UTC | #3
Benjamin Stürz <benni@stuerz.xyz> writes:

> On 28.03.22 11:33, Kalle Valo wrote:
>> Benjamin Stürz <benni@stuerz.xyz> writes:
>> 
>>> This patch series replaces comments with C99's designated initializers
>>> in a few places. It also adds some enum initializers. This is my first
>>> time contributing to the Linux kernel, therefore I'm probably doing a
>>> lot of things the wrong way. I'm sorry for that.
>> 
>> Just a small tip: If you are new, start with something small and learn
>> from that. Don't do a controversial big patchset spanning multiple
>> subsystems, that's the hard way to learn things. First submit one patch
>> at a time to one subsystem and gain understanding of the process that
>> way.
>
> I actually thought this would be such simple thing.

If there are 22 patches and a dozen different subsystems it's far from
simple, as you noticed from your replies :)

> Do you know of any good thing where to start? I already looked into
> drivers/staging/*/TODO and didn't found something for me personally.

I work in wireless and one my annoyance is use of BUG_ON() in wireless
drivers. There just isn't a good reason to crash the whole system when
there's a bug in a wireless driver or firmware. You can get list like
this:

git grep BUG_ON drivers/net/wireless/ | grep -v BUILD_BUG_ON

It might not be always trivial to fix BUG_ON() usage, so it would be a
good challenge as well. See the wiki link below how to submit wireless
patches. But just send a one patch first, don't work for several hours
and then submit a big set of patches.

We also might have a todo list somewhere in the wiki, but don't know how
to up-to-date it is.

> Should I drop this patchset and start with something different? 

Like Mauro suggested, splitting the patchset per subsystem is a very
good idea. And first try out with one subsystem, and after seeing how it
goes (if they are accepted or rejected), decide if you send more patches
to other subsystems.

> If yes, what would the proper way to drop it? Just announcing, that
> this is going nowhere in a separate patch?

Replying to Mauro's email and telling your intentions is a good way to
inform everyone.
Daniel Thompson March 28, 2022, 1:47 p.m. UTC | #4
On Sun, Mar 27, 2022 at 02:46:00PM +0200, Benjamin Stürz wrote:
> This patch series replaces comments with C99's designated initializers
> in a few places. It also adds some enum initializers. This is my first
> time contributing to the Linux kernel, therefore I'm probably doing a
> lot of things the wrong way. I'm sorry for that.

Welcome!


> I've gotten a few emails so far stating that this patch series is
> unnecessary. Yes, in fact this patch series is not necessary by itself,
> but it could help me understand how the whole process works and maybe I
> could help somewhere, where help is actually needed.

Have you been told the series is unnecessary or too big?

Although all patches represent a variant of the same mechanical
transformation but they are mostly unrelated to each other and, if
accepted, they will be applied by many different people.

Taken as a whole presenting this to maintainers as a 22 patch set is too
big. I'd recommend starting with a smaller patch or patch series where
all the patches get picked up by the same maintainer.


> This patch itself is a no-op.

PATCH 0/XX is for the covering letter. You should generate a template for
it using the --cover-letter option of git format-patch. That way patch 0
will contain the diffstat for the whole series (which is often useful
to help understand what the series is for) and there is no need to
make no-op changes.


Daniel.

> 
> Signed-off-by: Benjamin Stürz <benni@stuerz.xyz>
> ---
>  .gitignore | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/.gitignore b/.gitignore
> index 7afd412dadd2..706f667261eb 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -20,7 +20,7 @@
>  *.dtb
>  *.dtbo
>  *.dtb.S
> -*.dwo
> +*.dwo
>  *.elf
>  *.gcno
>  *.gz
> -- 
> 2.35.1
Jakub Kicinski March 28, 2022, 8:20 p.m. UTC | #5
On Mon, 28 Mar 2022 13:51:42 +0200 Benjamin Stürz wrote:
> > Just a small tip: If you are new, start with something small and learn
> > from that. Don't do a controversial big patchset spanning multiple
> > subsystems, that's the hard way to learn things. First submit one patch
> > at a time to one subsystem and gain understanding of the process that
> > way.
> 
> I actually thought this would be such simple thing. Do you know of any
> good thing where to start? I already looked into drivers/staging/*/TODO
> and didn't found something for me personally.

FWIW on the netdev side there's work coming to convert a set of features
from unsigned long to a BITMAP which will require converting a lot of
drivers to an explicit helpers from direct access.

https://lore.kernel.org/all/20220324154932.17557-14-shenjian15@huawei.com/

If it seems interesting enough you can try reaching out to Jian Shen.
diff mbox

Patch

diff --git a/.gitignore b/.gitignore
index 7afd412dadd2..706f667261eb 100644
--- a/.gitignore
+++ b/.gitignore
@@ -20,7 +20,7 @@ 
 *.dtb
 *.dtbo
 *.dtb.S
-*.dwo
+*.dwo
 *.elf
 *.gcno
 *.gz