Message-ID: <1821635.atdPhlSkOF@suse>
Date:   Thu, 16 Mar 2023 19:17:11 +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 17:17:47 CET Fabio M. De Francesco wrote:
> 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

I put my signature here, but it was a mistake.
Did you see the rest of the message below?

> > > > 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.

This unnecessary objection was due to that split I was talking about in the v6 
of your patch. At first glance it looked to me like a remnant of some mistake 
with copy-pasting. Sorry, I should have looked at it more carefully. 

> > > 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

Fabio

Powered by blists - more mailing lists