Message-ID: <2162728.C4sosBPzcN@suse>
Date:   Thu, 16 Mar 2023 17:17:47 +0100
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Khadija Kamran <kamrankhadijadj@...il.com>
Cc:     outreachy@...ts.linux.dev,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5] staging: axis-fifo: initialize timeouts in probe only

On giovedì 16 marzo 2023 16:09:08 CET Khadija Kamran wrote:
> On Thu, Mar 16, 2023 at 03:38:03PM +0100, Fabio M. De Francesco wrote:
> > Khadija,
> > 
> > Just saw your v5 patch and Greg's two replies.
> > 
> > For v6 you will need to change the subject to "[PATCH v6] staging:
> > axis-fifo:
> > initialize timeouts in init only" to indicate that you are doing 
assignments
> > in axis_fifo_init().
> > 
> > Don't forget to extend the version log with "Changes in v6:" and clarify
> > that
> > v5 had a different "Object" (you should probably also add a link to the v5
> > patch in lore: https://lore.kernel.org/lkml
> > /ZBMR4s8xyHGqMm72@...dija-virtual- machine/). When the "Subject" changes,
> > readers may not find the previous versions easily.
> > 
> > On giovedì 16 marzo 2023 13:56:02 CET Khadija Kamran wrote:
> > > Module parameter, read_timeout, can only be set at the loading time. As
> > > it can only be modified once, initialize read_timeout once in the probe
> > 
> > Substitute "probe" with "init".
> > 
> > > function.
> > > 
> > > As a result, only use read_timeout as the last argument in
> > > wait_event_interruptible_timeout() call.
> > 
> > This two sentences are not much clear. I'd merge and rework:
> > 
> > "Initialize the module parameters read_timeout and write_timeout once in
> > init().
> > 
> > Module parameters can only be set once and cannot be modified later, so we
> > don't need to evaluate them again when passing the parameters to
> > wait_event_interruptible_timeout()."
> > 
> > > Convert datatpe
> > 
> > s/datatpe/type/
> > 
> > > of read_timeout
> > 
> > of {read,write}_timeout
> > 
> > > from 'int' to 'long int' because
> > > implicit conversion of 'long int' to 'int' in statement 'read_timeout =
> > > MAX_SCHEDULE_TIMEOUT' results in an overflow warning.
> > 
> > We don't care too much about the warning themselves: I mean, it overflows
> > and
> > you must avoid it to happen (as you are doing with the changes of types),
> > not
> > merely be interested in avoiding the warning. "[] results in an overflow."
> > is
> > all we care about.
> 
> Hey Fabio!
> Thank you for your feedback. I have understood it and will make sure to
> send them in the next PATCH v6.

Great to hear it!

> > Add also the previous paragraph in the last part of the commit message.
> > 
> > > Perform same steps formodule parameter, write_timeout.
> > 
> > And instead delete the this last phrase.
> 
> Can you please explain the above feedback. I am confused. What should I
> use instead of this last phrase?

Sorry, I made a typo in the sentence above and that may confuse you :-(

I just intended to suggest to delete "Perform same steps formodule parameter, 
write_timeout.".

In the previous lines I suggested you to merge and rework your entire commit 
message. If you like it you are left with the following text (that I put for 
you between two '"'):

"Initialize the module parameters read_timeout and write_timeout once in
init().

Module parameters can only be set once and cannot be modified later, so we
don't need to evaluate them again when passing the parameters to
wait_event_interruptible_timeout().

Convert the type of {read,write}_timeout from 'int' to 'long int' because 
implicit conversion of 'long int' to 'int' in statement 'read_timeout = 
MAX_SCHEDULE_TIMEOUT' results in an overflow.".

Just three small sentences are all you need (and don't forget to change the 
Subject - "probe()" -> "init()".

I hope I have been clearer this time.
If not, please ask for further clarification.

Thanks,

Fabio 

> > > Suggested-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > > Signed-off-by: Khadija Kamran <kamrankhadijadj@...il.com>
> > > ---
> > > 
> > > Changes in v5:
> > >  - Convert timeout's datatype from int to long.
> > > 
> > > Changes in v4:
> > >  - Initialize timeouts once as suggested by Greg; this automatically
> > >  
> > >    fixes the indentation problems.
> > >  
> > >  - Change the subject and description.
> > > 
> > > Changes in v3:
> > >  - Fix grammatical mistakes
> > >  - Do not change the second argument's indentation in split lines
> > > 
> > > Changes in v2:
> > >  - Instead of matching alignment to open parenthesis, align second and
> > >  
> > >    the last argument instead.
> > >  
> > >  - Change the subject to 'remove tabs to align arguments'.
> > >  - Use imperative language in subject and description
> > >  
> > >  drivers/staging/axis-fifo/axis-fifo.c | 26 ++++++++++++++++----------
> > >  1 file changed, 16 insertions(+), 10 deletions(-)

[snip]

> > >  			
> > >  				 >= words_to_write,
> > 
> > What is this? You haven't yet compiled your patch.
> > Any further problems with enabling axis-fifo as a module?
> 
> Sorry, my bad.  Instead of fixing the menuconfig I used this command to
> remove the warnings:
> make -j"$(nproc)" ARCH=arm64 LLVM=1 drivers/staging/axis-fifo/
> I thought it is compiling my module correctly.
> But I am working on your feedback. And before sending my next patch I
> will make sure to compile it properly.

When you are done with build, install, and final reboot to test if your module 
can "modprobe" or "insmod" (i.e. link with the running custom kernel you 
built, installed and boot), try to compare the output of the following 
commands:

# uname -a
Linux suse 6.2.2-1-default #1 SMP PREEMPT_DYNAMIC Thu Mar  9 06:06:13 UTC 2023 
(44ca817) x86_64 x86_64 x86_64 GNU/Linux

AND

# modinfo <name of the module you are testing here>

I'm running "modinfo kvm" (but showing only two of many lines):

# modinfo kvm 
filename:       /lib/modules/6.2.2-1-default/kernel/arch/x86/kvm/kvm.ko.zst
vermagic:       6.2.2-1-default SMP preempt mod_unload modversions 

Can you see that the kernel in "uname -a" and the filename and vermagic have 
the same "6.2.2-1-default"? Well, so I'm sure I'm running the right Kernel and 
inserted the appropriate "kvm" module. 

Furthermore, before rebooting your custom kernel, you may also look at the 
directory in the Kernel where you compiled your module and search for "*.o" 
"*mod*" and "*.ko" files. If you have them, you built your module properly.

Thanks,

Fabio

Powered by blists - more mailing lists