Re: Code cleanups are not always simple.

[Khadija Kamran <kamrankhadijadj@gmail.com>]

On Mon, Mar 20, 2023 at 05:08:58PM -0700, Ira Weiny wrote:
> Cleaning up code is not always easy.  Yes there are various rules, coding
> styles and tools which can automate the process.  But we should all remember to
> think about the problem being reported and determine if a simple search/replace
> is the proper thing to do.  I think this is becoming more apparent as the
> number of things checkpatch reports on is dwindling to these more complicated
> issues.
> 
> So I'd like to call attention to a couple of problems which have required a bit
> more attention.  Hopefully by highlighting these problems folks will be more
> aware when looking at future issues.
> 
> The first problem was a week or so back when Khadija was trying to fix line
> length formatting.[1]  The repeated attempts were causing circular problems
> with checkpatch failing on different checks.  The core issue was that
> checkpatch was only checking the symptoms of the issue, not the issue itself.
> Checkpatch and the kernel coding style are in place not to enforce random rules
> on how folks write code.  They are there to make the code cleaner, less buggy,
> and easier to maintain.
> 
> Today I saw a similar situation.  Julia called out the need to make macro
> definitions safer by turning them into inline functions.[2]
> 
> But Sumitra took that a bit too literally.[3]  The container_of() macro is
> useful on it's own.  The only reason to create additional helper functions is
> when the conversion is either more complicated or used so much that the bulk of
> the code becomes much much cleaner.
> 
> I'll end this with a recent example which is part of the kmap() project.
> 
> I was looking at the call to kmap_atomic() in memcpy_page_flushcache() inside
> the x86 code.  At first it looks like a simple substitute and replace from
> kmap_atmoic() to kmap_local_page().  But I took the time to understand the
> context of where memcpy_page_flushcache() was called.  Doing so made me realize
> it was not called at all!!!  The proper fix was to simply remove the call
> entirely from the kernel.
> 
> I know you are all excited to get your first patches into the kernel.  But even
> simple patches take time.  After removing these calls I needed to ensure that
> x86, powerpc, and arm all compiled without issues.  I used both 'git grep' and
> cscope to ensure I removed the header file declarations.  Even then there was
> discussion because I accidentally submitted as a series which made a bit more
> work for the maintainers.[5]
> 
> So in summary take your time.  Try to understand why checkpatch or other tools
> are flagging errors.
> 
> Most importantly, don't be afraid to take your time when submitting patches.
> 
> Ira
> 
> [1] https://lore.kernel.org/all/640e75cfd8fc_229a89294a3@iweiny-mobl.notmuch/
> [2] https://lore.kernel.org/all/124d2ef9-6f1d-2652-c88d-161f1bc06443@inria.fr/
> [3] https://lore.kernel.org/all/ZBh%2Fs5lyONaF37gs@kroah.com/
> [4] https://lore.kernel.org/all/20221230-kmap-x86-v1-0-15f1ecccab50@intel.com/
> [5] https://lore.kernel.org/all/3523ddf9-03f5-3179-9f39-cec09f79aa97@intel.com/
>

Hey Ira,

Thank you for the mail. I will make sure to keep your instructions in
mind while working on the upcoming patches.

Thank you,
Regards,
Khadija

Thank you