DEV Community

moonlitpath
moonlitpath

Posted on

Submitting My Second Linux Kernel Patch for GSoC 2026 — Still Messy, Still Learning

So, after going through the entire drivers/staging directory, and running checkpatch script in each, plus checking on [[lore.kernel.org]] to see if anyone is already working the file, I found that almost all subsystems are free of warnings.
There are minor CHECK issues like camelcase, or blank space issues, but most of them have nothing.
The ones which have are already being actively fixed by another person.

So, after a long time of searching, I found this file drivers/staging/media/atomisp/pci/system_local.c

These were the warnings checkpatch showed me:

anu@laptop:~/kernel-dev/source/linux$ scripts/checkpatch.pl -f --terse --strict drivers/staging/media/atomisp/pci/system_local.c
drivers/staging/media/atomisp/pci/system_local.c:59: WARNING: Block comments use * on subsequent lines
drivers/staging/media/atomisp/pci/system_local.c:74: WARNING: Block comments use * on subsequent lines
drivers/staging/media/atomisp/pci/system_local.c:88: WARNING: Block comments use a trailing */ on a separate line
drivers/staging/media/atomisp/pci/system_local.c:90: WARNING: please, no spaces at the start of a line
total: 0 errors, 4 warnings, 0 checks, 170 lines checked
Enter fullscreen mode Exit fullscreen mode

And then, I checked lore
![[../images/Pasted image 20260323223244.png]]

It seems no one is actively working on this file. I can start with this one!


I decided to work with the whitespace issue, since the comments issue was related to commented out code, which would have been a logical issue, requiring thorough knowledge to fix.

The issue was that the coder had indented using spaces instead of tabs. I fixed the issue and sent it to the mentors to review.

snippet from git diff

-    (hrt_address)0x0000000000000600ULL;
+       (hrt_address)0x0000000000000600ULL;

Enter fullscreen mode Exit fullscreen mode

[24-03-26 01:42:35]
Okay. I have sent my new patch!!
[[https://lore.kernel.org/linux-media/20260324081007.23165-1-anushkabadhe@gmail.com/T/#u]]

I got a reply, from maintainer Andy Shevchenko!

My whitespace patch, as you can see, is very small, it doesn't add value to the main code. I was advised that I can improve readabilty of the code by merging the split variable declartion on 2 lines.

That's so nice of him!! He guided me on how I can make a useful contribution!

So, I made the changes, and i noticed that the comment block position seemed to be wrong.. I fixed that too, and then sent them both in v2.


[25-03-26]

Mistake: One patch should address just one type of a change. My file now had 2 types of changes: comment */ position modification as well as improving readability.

I can't believe I forgot about that, I will definitely remember to check my patch changes carefully in the future!

Thus, my patch was marked NAK.. and I was told to make just 1 change.

So, I removed the change and sent it as v3


[26-03-26]

This time, I got a "Nice" from Andy as a review! That made me really happy.
He told me that since this constant GP_TIMER_BASE is used in just one file -- gp_timer.c, I should remove that constant from this file along with it's comment to there and remove it's mentioning from the other header files.

And this will make my patch gain value!!

So, now, I am doing just that.

First, I searched for all the places where the constant was defined:

anu@laptop:~/kernel-dev/source/linux$ rg "GP_TIMER_BASE"
drivers/staging/media/atomisp/pci/system_local.h
59:extern const hrt_address GP_TIMER_BASE;

drivers/staging/media/atomisp/pci/system_local.c
89:const hrt_address GP_TIMER_BASE = (hrt_address)0x0000000000000600ULL;

drivers/staging/media/atomisp/pci/hive_isp_css_common/host/gp_timer.c
28:        GP_TIMER_BASE +
35: ia_css_device_store_uint32((GP_TIMER_BASE +

drivers/staging/media/atomisp/pci/hive_isp_css_include/gp_timer.h
24:#include "system_local.h"    /*GP_TIMER_BASE address */
Enter fullscreen mode Exit fullscreen mode

It seems that it's inside 1 header file (system_local.h) as an extern,which is imported in another header file (gp_timer.h) and then this header file is imported in gp_timer.c where it is actually used.

Wow, that's so unnecessarily complicated. He's right, moving that file directly to where it's used, will make it much more useful.

Understanding the subsystem and what the file does
I should have done this from the start, but ig, better late than never. I will remember to always to this in the future though.

media/atomisp/pci/system_local.c
Enter fullscreen mode Exit fullscreen mode

media/atomisp -> part of the media subsystem -- this handles camera drivers -- Intel’s Atom Image Signal Processor (ISP)

pci -> communicates over the PCI

system_local.c -> this file is the main file that basically handles the interaction between the hardware and the internal code for the device

drivers/staging/media/atomisp/pci/hive_isp_css_common/host/gp_timer.c
Enter fullscreen mode Exit fullscreen mode

hive_isp_css_common -> intel isp's camera support system

host -> code running on cpu (not isp firmware)

gp_timer.c -> general purpose timer -- it is used by the driver to measure time, delays, schedule commands, etc

after examining all the files once to confirm it's safe, i proceeded with moving the variable GP_TIMER_BASE to gp_timer.c

Here's what i did:
first, I copied the declaration of GP_TIMER_BASE along with it's comment from system_local.c to gp_timer.c and pasted it at the top, before function declaration

now the next question is whether or not I should remove system_local.h from gp_timer.h .....

I just noticed something really interesting here... system_local.h had been imported in both gp_timer.c as well as gp_timer.h ...
and gp_timer.h has been included in gp_timer.c .. obviously

so i conclude that system_local.h had been imported twice in gp_timer.c, and it is redundant

so here is the question: how to solve this redundancy?
I will get back here once i confirm whether or not removing system_local.h does not affect the code. Only if there are errors, is the redundancy question to be solved, else, it doesnt need to be solved.

I plan to comment out the
#include "system_local.h" line in both gp_timer.c and gp_timer.h
and then compile the code and check for errors.

I know that the code for gp_timer.h had a comment beside importing system_local.h for GP_TIMER_BASEaddress, still it's better to be safe than sorry.

okay, i have run the code.

make -j8 drivers/staging/media/atomisp/pci/system_local.o
Enter fullscreen mode Exit fullscreen mode

there was one error, but it isn't related to my code:

drivers/staging/media/atomisp/pci/system_global.h:47:10: fatal error: hive_isp_css_defs.h: No such file or directory
   47 | #include <hive_isp_css_defs.h>
      |          ^~~~~~~~~~~~~~~~~~~~~
compilation terminated.
Enter fullscreen mode Exit fullscreen mode

i ran the make command in the master branch and this error showed up there too.

here it seems that someone imported this header using < > instead of " "... since they are in the same file.. they should be imported as " "
maybe i can submit that as a patch, but that's for later.
it's not a part of my patch.

Each patch should fix just 1 issue

now, back to task, i can go ahead and delete those lines directly.

I ran make -j8 and let it process


[27-03-26]

I sent the patch, and the following night, I got a mail from the Media CI bot:


# Test static:test-sparse  
drivers/staging/media/atomisp/pci/hive_isp_css_common/host/gp_timer.c:19:19: warning: symbol 'GP_TIMER_BASE' was not declared. Should it be static?
Enter fullscreen mode Exit fullscreen mode

of course! how could i have forgotten this? I should have remembered this myself, I had used static for this very reason in my mini malloc.c series!

Functions or variables that are going to be used only within that file should be called static!

This is the link forthe official rules for contributing the patch for media subsystem:
https://docs.kernel.org/driver-api/media/maintainer-entry-profile.html

This is something that they had mentioned

Also, please notice that we build the Kernel with:

make CF=-D__CHECK_ENDIAN__ CONFIG_DEBUG_SECTION_MISMATCH=y C=1 W=1 CHECK=check_script
Enter fullscreen mode Exit fullscreen mode

Where the check script is:

#!/bin/bash
/devel/smatch/smatch -p=kernel $@ >&2
/devel/sparse/sparse $@ >&2
Enter fullscreen mode Exit fullscreen mode

Be sure to not introduce new warnings on your patches without a very good reason.

I had no idea about this checking method. I will remember to check out the docs for that subsystem before working on a patch.

Now, I plan to fix this issue and send a v6

i added static as a property for the variable, to modify it's scope to stay only in the file and sent the patch.

now i am waiting for the response..


I will keep posting updates as I receive them.. stay tuned for the updates..

Top comments (0)