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@khadija-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@xxxxxxxxxxxxxxxxxxx> > > > Signed-off-by: Khadija Kamran <kamrankhadijadj@xxxxxxxxx> > > > --- > > > > > > 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
