[PATCH v8] staging: axis-fifo: initialize timeouts in init only

[PATCH v8] staging: axis-fifo: initialize timeouts in init only

[Khadija Kamran]

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 datatype of {read,write}_timeout from 'int' to 'long int' because
implicit conversion of 'long int' to 'int' in statement
'{read,write}_timeout = MAX_SCHEDULE_TIMEOUT' results in an overflow.

Change format specifier for {read,write}_timeout from %i to %li.

Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
---

Changes in v8:
 - Fixed a spelling mistake

Changes in v7:
 - Fixed a grammatical error

Changes in v6:
 - Initialize module parameters in init instead of probe function.
 - Change the subject and description
 - Change format specifiers of module parameters to "%li"

Changes in v5:
 - Convert module parameters datatype from int to long.
 - Link to patch:
 https://lore.kernel.org/outreachy/ZBMR4s8xyHGqMm72@khadija-virtual-machine/

Changes in v4:
 - Initialize timeouts once as suggested by Greg; this automatically
   fixes the indentation problems.
 - Change the subject and description.
 - Link to patch:
 https://lore.kernel.org/outreachy/ZA4M3+ZeB1Rl2fbs@khadija-virtual-machine/

Changes in v3:
 - Correct 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.
 - Change the subject and use imperative language.
 - Link to patch:
 https://lore.kernel.org/outreachy/ZAxNYw2rFQkrdtKl@khadija-virtual-machine/

Link to first patch:
 https://lore.kernel.org/outreachy/ZAZSmPpB6fcozGa4@khadija-virtual-machine/

 drivers/staging/axis-fifo/axis-fifo.c | 28 ++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
index dfd2b357f484..0a85ea667a1b 100644
--- a/drivers/staging/axis-fifo/axis-fifo.c
+++ b/drivers/staging/axis-fifo/axis-fifo.c
@@ -103,17 +103,17 @@
  *           globals
  * ----------------------------
  */
-static int read_timeout = 1000; /* ms to wait before read() times out */
-static int write_timeout = 1000; /* ms to wait before write() times out */
+static long read_timeout = 1000; /* ms to wait before read() times out */
+static long write_timeout = 1000; /* ms to wait before write() times out */
 
 /* ----------------------------
  * module command-line arguments
  * ----------------------------
  */
 
-module_param(read_timeout, int, 0444);
+module_param(read_timeout, long, 0444);
 MODULE_PARM_DESC(read_timeout, "ms to wait before blocking read() timing out; set to -1 for no timeout");
-module_param(write_timeout, int, 0444);
+module_param(write_timeout, long, 0444);
 MODULE_PARM_DESC(write_timeout, "ms to wait before blocking write() timing out; set to -1 for no timeout");
 
 /* ----------------------------
@@ -384,9 +384,7 @@ static ssize_t axis_fifo_read(struct file *f, char __user *buf,
 		mutex_lock(&fifo->read_lock);
 		ret = wait_event_interruptible_timeout(fifo->read_queue,
 			ioread32(fifo->base_addr + XLLF_RDFO_OFFSET),
-				 (read_timeout >= 0) ?
-				  msecs_to_jiffies(read_timeout) :
-				  MAX_SCHEDULE_TIMEOUT);
+			read_timeout);
 
 		if (ret <= 0) {
 			if (ret == 0) {
@@ -528,9 +526,7 @@ static ssize_t axis_fifo_write(struct file *f, const char __user *buf,
 		ret = wait_event_interruptible_timeout(fifo->write_queue,
 			ioread32(fifo->base_addr + XLLF_TDFV_OFFSET)
 				 >= words_to_write,
-				 (write_timeout >= 0) ?
-				  msecs_to_jiffies(write_timeout) :
-				  MAX_SCHEDULE_TIMEOUT);
+			write_timeout);
 
 		if (ret <= 0) {
 			if (ret == 0) {
@@ -948,7 +944,17 @@ static struct platform_driver axis_fifo_driver = {
 
 static int __init axis_fifo_init(void)
 {
-	pr_info("axis-fifo driver loaded with parameters read_timeout = %i, write_timeout = %i\n",
+	if (read_timeout >= 0)
+		read_timeout = msecs_to_jiffies(read_timeout);
+	else
+		read_timeout = MAX_SCHEDULE_TIMEOUT;
+
+	if (write_timeout >= 0)
+		write_timeout = msecs_to_jiffies(write_timeout);
+	else
+		write_timeout = MAX_SCHEDULE_TIMEOUT;
+
+	pr_info("axis-fifo driver loaded with parameters read_timeout = %li, write_timeout = %li\n",
 		read_timeout, write_timeout);
 	return platform_driver_register(&axis_fifo_driver);
 }
-- 
2.34.1

Re: [PATCH v8] staging: axis-fifo: initialize timeouts in init only

[Greg Kroah-Hartman]

On Fri, Mar 17, 2023 at 01:09:00AM +0500, Khadija Kamran wrote:
> 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().

I feel like we are being too picky here, but this isn't the correct
wording.  It is possible for module parameters to be modified "later",
if the permissions on the parameter are set to allow this.  But that's
not what this driver does here, so this might be better phrased as:

  The module parameters in this driver can not be modified after
  loading, so they can be evaluated once at module load and set to
  default values if needed at that time.

> Convert datatype of {read,write}_timeout from 'int' to 'long int' because
> implicit conversion of 'long int' to 'int' in statement
> '{read,write}_timeout = MAX_SCHEDULE_TIMEOUT' results in an overflow.
> 
> Change format specifier for {read,write}_timeout from %i to %li.

You are listing all of _what_ you do here, not really _why_ you are
doing any of this.

Anyway, if I were writing this, here's what I would say as a changelog
text:

  The module parameters, read_timeout and write_timeout, are only ever
  evaluated at module load time, so set the default values then if
  needed so as to not recompute the timeout values every time the driver
  reads or writes data.


And that's it, short, concise, and it explains why you are doing this.

Writing changelog comments are almost always harder than actually
writing the patch (at least for me.)  So don't feel bad, it take a lot
of experience doing it.

All that being said, I think we are just polishing something that
doesn't need to really be polished anymore, so let me go just apply this
patch as-is to the tree now so you can move on to a different change.
You've put in the effort here, and I don't want you to get bogged down
in specifics that really do not matter at all overall (like the memory
size of your vm...)

thanks,

greg k-h

Re: [PATCH v8] staging: axis-fifo: initialize timeouts in init only

[Fabio M. De Francesco]

On venerdì 17 marzo 2023 08:13:37 CET Greg Kroah-Hartman wrote:
> On Fri, Mar 17, 2023 at 01:09:00AM +0500, Khadija Kamran wrote:
> > 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().
> 
> I feel like we are being too picky here, but this isn't the correct
> wording.  It is possible for module parameters to be modified "later",
> if the permissions on the parameter are set to allow this.  But that's
> not what this driver does here, so this might be better phrased as:
> 
>   The module parameters in this driver can not be modified after
>   loading, so they can be evaluated once at module load and set to
>   default values if needed at that time.
> 
> > Convert datatype of {read,write}_timeout from 'int' to 'long int' because
> > implicit conversion of 'long int' to 'int' in statement
> > '{read,write}_timeout = MAX_SCHEDULE_TIMEOUT' results in an overflow.
> > 
> > Change format specifier for {read,write}_timeout from %i to %li.
> 
> You are listing all of _what_ you do here, not really _why_ you are
> doing any of this.
> 
> Anyway, if I were writing this, here's what I would say as a changelog
> text:
> 
>   The module parameters, read_timeout and write_timeout, are only ever
>   evaluated at module load time, so set the default values then if
>   needed so as to not recompute the timeout values every time the driver
>   reads or writes data.
> 
> 
> And that's it, short, concise, and it explains why you are doing this.
> 
> Writing changelog comments are almost always harder than actually
> writing the patch (at least for me.)  So don't feel bad, it take a lot
> of experience doing it.

Thanks for helping Khadija (and also me, indirectly, because I helped them to 
make that commit message). After a little less than 200 commits already 
upstream, there is still to learn how to make a good changelog (which I think 
it's not less important than the code in the patch).  

> All that being said, I think we are just polishing something that
> doesn't need to really be polished anymore,
> so let me go just apply this
> patch as-is to the tree now so you can move on to a different change.

Yes, thanks. It's time to move on.

> You've put in the effort here, and I don't want you to get bogged down
> in specifics that really do not matter at all overall (like the memory
> size of your vm...)

You probably didn't pay attention to the problems Khadija has being 
experiencing with memory exhaustion and compilation times. 

First time they complained had several crashes while building and messages 
saying there was not enough available memory. Then several other problems with 
builds' time. Finally, yesterday they wrote that "make modules_install 
install" took "hours" (literally).

This is why I think they should also be helped with their VM configuration 
(despite it was unrelated to the patch). They won't be able to work on some 
projects if they cannot do quick builds and run hundreds of tests in no time 
(e.g., I'm especially talking about Ira's and my project about the conversions 
from kmap{,_atomic}() to kmap_local_page()).

I understand that this is only noise for you, as a maintainer. However I'm 
pretty sure that they need help with speeding up builds and installation. The 
projects cannot wait "hours" just for install of modules and kernel images.

Again thanks for your precious suggestions about how to improve commit 
messages.

Fabio 

> thanks,
> 
> greg k-h

Re: [PATCH v8] staging: axis-fifo: initialize timeouts in init only

[Khadija Kamran]

On Fri, Mar 17, 2023 at 09:58:17AM +0100, Fabio M. De Francesco wrote:
> On venerdì 17 marzo 2023 08:13:37 CET Greg Kroah-Hartman wrote:
> > On Fri, Mar 17, 2023 at 01:09:00AM +0500, Khadija Kamran wrote:
> > > 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().
> > 
> > I feel like we are being too picky here, but this isn't the correct
> > wording.  It is possible for module parameters to be modified "later",
> > if the permissions on the parameter are set to allow this.  But that's
> > not what this driver does here, so this might be better phrased as:
> > 
> >   The module parameters in this driver can not be modified after
> >   loading, so they can be evaluated once at module load and set to
> >   default values if needed at that time.
> > 
> > > Convert datatype of {read,write}_timeout from 'int' to 'long int' because
> > > implicit conversion of 'long int' to 'int' in statement
> > > '{read,write}_timeout = MAX_SCHEDULE_TIMEOUT' results in an overflow.
> > > 
> > > Change format specifier for {read,write}_timeout from %i to %li.
> > 
> > You are listing all of _what_ you do here, not really _why_ you are
> > doing any of this.
> > 
> > Anyway, if I were writing this, here's what I would say as a changelog
> > text:
> > 
> >   The module parameters, read_timeout and write_timeout, are only ever
> >   evaluated at module load time, so set the default values then if
> >   needed so as to not recompute the timeout values every time the driver
> >   reads or writes data.
> > 
> > 
> > And that's it, short, concise, and it explains why you are doing this.
> > 
> > Writing changelog comments are almost always harder than actually
> > writing the patch (at least for me.)  So don't feel bad, it take a lot
> > of experience doing it.
> 
> Thanks for helping Khadija (and also me, indirectly, because I helped them to 
> make that commit message). After a little less than 200 commits already 
> upstream, there is still to learn how to make a good changelog (which I think 
> it's not less important than the code in the patch).  
>

Hey Fabio!
I am glad to be a part of this process. Thank you for all the help
with the commit messages. :)

> > All that being said, I think we are just polishing something that
> > doesn't need to really be polished anymore,
> > so let me go just apply this
> > patch as-is to the tree now so you can move on to a different change.
> 
> Yes, thanks. It's time to move on.
>

yes, please.

> > You've put in the effort here, and I don't want you to get bogged down
> > in specifics that really do not matter at all overall (like the memory
> > size of your vm...)
> 
> You probably didn't pay attention to the problems Khadija has being 
> experiencing with memory exhaustion and compilation times. 
> 
> First time they complained had several crashes while building and messages 
> saying there was not enough available memory. Then several other problems with 
> builds' time. Finally, yesterday they wrote that "make modules_install 
> install" took "hours" (literally).
> 
> This is why I think they should also be helped with their VM configuration 
> (despite it was unrelated to the patch). They won't be able to work on some 
> projects if they cannot do quick builds and run hundreds of tests in no time 
> (e.g., I'm especially talking about Ira's and my project about the conversions 
> from kmap{,_atomic}() to kmap_local_page()).
>

Thank you Fabio!
I am waiting for your message on #kernel-outreachy. 

Regards,
Khadija

> I understand that this is only noise for you, as a maintainer. However I'm 
> pretty sure that they need help with speeding up builds and installation. The 
> projects cannot wait "hours" just for install of modules and kernel images.
> 
> Again thanks for your precious suggestions about how to improve commit 
> messages.
> 
> Fabio 
> 
> > thanks,
> > 
> > greg k-h
> 
> 
> 
> 

Re: [PATCH v8] staging: axis-fifo: initialize timeouts in init only

[Khadija Kamran]

On Fri, Mar 17, 2023 at 08:13:37AM +0100, Greg Kroah-Hartman wrote:
> On Fri, Mar 17, 2023 at 01:09:00AM +0500, Khadija Kamran wrote:
> > 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().
> 
> I feel like we are being too picky here, but this isn't the correct
> wording.  It is possible for module parameters to be modified "later",
> if the permissions on the parameter are set to allow this.  But that's
> not what this driver does here, so this might be better phrased as:
> 
>   The module parameters in this driver can not be modified after
>   loading, so they can be evaluated once at module load and set to
>   default values if needed at that time.
> 
> > Convert datatype of {read,write}_timeout from 'int' to 'long int' because
> > implicit conversion of 'long int' to 'int' in statement
> > '{read,write}_timeout = MAX_SCHEDULE_TIMEOUT' results in an overflow.
> > 
> > Change format specifier for {read,write}_timeout from %i to %li.
> 
> You are listing all of _what_ you do here, not really _why_ you are
> doing any of this.
> 
> Anyway, if I were writing this, here's what I would say as a changelog
> text:
> 
>   The module parameters, read_timeout and write_timeout, are only ever
>   evaluated at module load time, so set the default values then if
>   needed so as to not recompute the timeout values every time the driver
>   reads or writes data.
> 
> 
> And that's it, short, concise, and it explains why you are doing this.
> 
> Writing changelog comments are almost always harder than actually
> writing the patch (at least for me.)  So don't feel bad, it take a lot
> of experience doing it.
> 
> All that being said, I think we are just polishing something that
> doesn't need to really be polished anymore, so let me go just apply this
> patch as-is to the tree now so you can move on to a different change.
> You've put in the effort here, and I don't want you to get bogged down
> in specifics that really do not matter at all overall (like the memory
> size of your vm...)
>


Okay Great! Thank you Greg!

Regards,
Khadija


> thanks,
> 
> greg k-h

Re: [PATCH v8] staging: axis-fifo: initialize timeouts in init only

[Fabio M. De Francesco]

Khadija,

Congratulations for having your first patch in Linux, via Greg's staging tree.

It will take some time before it reaches mainline, although it is already on 
its way to get upstream.

On giovedì 16 marzo 2023 21:09:00 CET Khadija Kamran wrote:
> 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().

Greg made you (and indirectly me notice) that the statement above is a kind of 
short-circuit because it misses to make the readers notice that you are 
dealing with specific permissions granted to these two module's parameters.

Please take a look at the permissions associated with those parameters:

module_param(write_timeout, long, 0444);
module_param(read_timeout, long, 0444);

Can you understand what '0444' stands for? What if their permissions were  
instead something like '0666' or '0664'?

(I'm not asking you to answer these questions, instead I am only asking you to 
learn how it works if you don't know it yet).

Fabio

> Convert datatype of {read,write}_timeout from 'int' to 'long int' because
> implicit conversion of 'long int' to 'int' in statement
> '{read,write}_timeout = MAX_SCHEDULE_TIMEOUT' results in an overflow.
> 
> Change format specifier for {read,write}_timeout from %i to %li.
> 
> Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> ---
> 
> Changes in v8:
>  - Fixed a spelling mistake
> 
> Changes in v7:
>  - Fixed a grammatical error
> 
> Changes in v6:
>  - Initialize module parameters in init instead of probe function.
>  - Change the subject and description
>  - Change format specifiers of module parameters to "%li"
> 
> Changes in v5:
>  - Convert module parameters datatype from int to long.
>  - Link to patch:
>  https://lore.kernel.org/outreachy/ZBMR4s8xyHGqMm72@khadija-virtual-machine/
> 
> Changes in v4:
>  - Initialize timeouts once as suggested by Greg; this automatically
>    fixes the indentation problems.
>  - Change the subject and description.
>  - Link to patch:
>  https://lore.kernel.org/outreachy/ZA4M3+ZeB1Rl2fbs@khadija-virtual-machine/
> 
> Changes in v3:
>  - Correct 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.
>  - Change the subject and use imperative language.
>  - Link to patch:
>  https://lore.kernel.org/outreachy/ZAxNYw2rFQkrdtKl@khadija-virtual-machine/
> 
> Link to first patch:
>  https://lore.kernel.org/outreachy/ZAZSmPpB6fcozGa4@khadija-virtual-machine/
> 
>  drivers/staging/axis-fifo/axis-fifo.c | 28 ++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/axis-fifo/axis-fifo.c
> b/drivers/staging/axis-fifo/axis-fifo.c index dfd2b357f484..0a85ea667a1b
> 100644
> --- a/drivers/staging/axis-fifo/axis-fifo.c
> +++ b/drivers/staging/axis-fifo/axis-fifo.c
> @@ -103,17 +103,17 @@
>   *           globals
>   * ----------------------------
>   */
> -static int read_timeout = 1000; /* ms to wait before read() times out */
> -static int write_timeout = 1000; /* ms to wait before write() times out */
> +static long read_timeout = 1000; /* ms to wait before read() times out */
> +static long write_timeout = 1000; /* ms to wait before write() times out */
> 
>  /* ----------------------------
>   * module command-line arguments
>   * ----------------------------
>   */
> 
> -module_param(read_timeout, int, 0444);
> +module_param(read_timeout, long, 0444);
>  MODULE_PARM_DESC(read_timeout, "ms to wait before blocking read() timing 
out;
> set to -1 for no timeout"); -module_param(write_timeout, int, 0444);
> +module_param(write_timeout, long, 0444);
>  MODULE_PARM_DESC(write_timeout, "ms to wait before blocking write() timing
> out; set to -1 for no timeout");
> 
>  /* ----------------------------
> @@ -384,9 +384,7 @@ static ssize_t axis_fifo_read(struct file *f, char 
__user
> *buf, mutex_lock(&fifo->read_lock);
>  		ret = wait_event_interruptible_timeout(fifo->read_queue,
>  			ioread32(fifo->base_addr + XLLF_RDFO_OFFSET),
> -				 (read_timeout >= 0) ?
> -				  msecs_to_jiffies(read_timeout) :
> -				  MAX_SCHEDULE_TIMEOUT);
> +			read_timeout);
> 
>  		if (ret <= 0) {
>  			if (ret == 0) {
> @@ -528,9 +526,7 @@ static ssize_t axis_fifo_write(struct file *f, const 
char
> __user *buf, ret = wait_event_interruptible_timeout(fifo->write_queue,
>  			ioread32(fifo->base_addr + XLLF_TDFV_OFFSET)
> 
>  				 >= words_to_write,
> 
> -				 (write_timeout >= 0) ?
> -				  msecs_to_jiffies(write_timeout) :
> -				  MAX_SCHEDULE_TIMEOUT);
> +			write_timeout);
> 
>  		if (ret <= 0) {
>  			if (ret == 0) {
> @@ -948,7 +944,17 @@ static struct platform_driver axis_fifo_driver = {
> 
>  static int __init axis_fifo_init(void)
>  {
> -	pr_info("axis-fifo driver loaded with parameters read_timeout = %i,
> write_timeout = %i\n", +	if (read_timeout >= 0)
> +		read_timeout = msecs_to_jiffies(read_timeout);
> +	else
> +		read_timeout = MAX_SCHEDULE_TIMEOUT;
> +
> +	if (write_timeout >= 0)
> +		write_timeout = msecs_to_jiffies(write_timeout);
> +	else
> +		write_timeout = MAX_SCHEDULE_TIMEOUT;
> +
> +	pr_info("axis-fifo driver loaded with parameters read_timeout = %li,
> write_timeout = %li\n", read_timeout, write_timeout);
>  	return platform_driver_register(&axis_fifo_driver);
>  }
> --
> 2.34.1

Re: [PATCH v8] staging: axis-fifo: initialize timeouts in init only

[Khadija Kamran]

On Fri, Mar 17, 2023 at 11:29:25AM +0100, Fabio M. De Francesco wrote:
> Khadija,
> 
> Congratulations for having your first patch in Linux, via Greg's staging tree.
> 
> It will take some time before it reaches mainline, although it is already on 
> its way to get upstream.

Thank you! :)

> 
> On giovedì 16 marzo 2023 21:09:00 CET Khadija Kamran wrote:
> > 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().
> 
> Greg made you (and indirectly me notice) that the statement above is a kind of 
> short-circuit because it misses to make the readers notice that you are 
> dealing with specific permissions granted to these two module's parameters.
>
> Please take a look at the permissions associated with those parameters:
> 
> module_param(write_timeout, long, 0444);
> module_param(read_timeout, long, 0444);
> 
> Can you understand what '0444' stands for? What if their permissions were  
> instead something like '0666' or '0664'?
>

Hey Fabio!

I understand that 0444 shows read permissions only.
I am trying to make sense of this. As the permissions do not allow
write, so the value cannot be configured afterwards. 
Instead of saying 'cannot be modified later', we should talk more about
permissions here too.
Am I getting it right?

Thank you!

Regards,
Khadija


> (I'm not asking you to answer these questions, instead I am only asking you to 
> learn how it works if you don't know it yet).
> 
> Fabio
> 
> > Convert datatype of {read,write}_timeout from 'int' to 'long int' because
> > implicit conversion of 'long int' to 'int' in statement
> > '{read,write}_timeout = MAX_SCHEDULE_TIMEOUT' results in an overflow.
> > 
> > Change format specifier for {read,write}_timeout from %i to %li.
> > 
> > Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> > ---
> > 
> > Changes in v8:
> >  - Fixed a spelling mistake
> > 
> > Changes in v7:
> >  - Fixed a grammatical error
> > 
> > Changes in v6:
> >  - Initialize module parameters in init instead of probe function.
> >  - Change the subject and description
> >  - Change format specifiers of module parameters to "%li"
> > 
> > Changes in v5:
> >  - Convert module parameters datatype from int to long.
> >  - Link to patch:
> >  https://lore.kernel.org/outreachy/ZBMR4s8xyHGqMm72@khadija-virtual-machine/
> > 
> > Changes in v4:
> >  - Initialize timeouts once as suggested by Greg; this automatically
> >    fixes the indentation problems.
> >  - Change the subject and description.
> >  - Link to patch:
> >  https://lore.kernel.org/outreachy/ZA4M3+ZeB1Rl2fbs@khadija-virtual-machine/
> > 
> > Changes in v3:
> >  - Correct 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.
> >  - Change the subject and use imperative language.
> >  - Link to patch:
> >  https://lore.kernel.org/outreachy/ZAxNYw2rFQkrdtKl@khadija-virtual-machine/
> > 
> > Link to first patch:
> >  https://lore.kernel.org/outreachy/ZAZSmPpB6fcozGa4@khadija-virtual-machine/
> > 
> >  drivers/staging/axis-fifo/axis-fifo.c | 28 ++++++++++++++++-----------
> >  1 file changed, 17 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/staging/axis-fifo/axis-fifo.c
> > b/drivers/staging/axis-fifo/axis-fifo.c index dfd2b357f484..0a85ea667a1b
> > 100644
> > --- a/drivers/staging/axis-fifo/axis-fifo.c
> > +++ b/drivers/staging/axis-fifo/axis-fifo.c
> > @@ -103,17 +103,17 @@
> >   *           globals
> >   * ----------------------------
> >   */
> > -static int read_timeout = 1000; /* ms to wait before read() times out */
> > -static int write_timeout = 1000; /* ms to wait before write() times out */
> > +static long read_timeout = 1000; /* ms to wait before read() times out */
> > +static long write_timeout = 1000; /* ms to wait before write() times out */
> > 
> >  /* ----------------------------
> >   * module command-line arguments
> >   * ----------------------------
> >   */
> > 
> > -module_param(read_timeout, int, 0444);
> > +module_param(read_timeout, long, 0444);
> >  MODULE_PARM_DESC(read_timeout, "ms to wait before blocking read() timing 
> out;
> > set to -1 for no timeout"); -module_param(write_timeout, int, 0444);
> > +module_param(write_timeout, long, 0444);
> >  MODULE_PARM_DESC(write_timeout, "ms to wait before blocking write() timing
> > out; set to -1 for no timeout");
> > 
> >  /* ----------------------------
> > @@ -384,9 +384,7 @@ static ssize_t axis_fifo_read(struct file *f, char 
> __user
> > *buf, mutex_lock(&fifo->read_lock);
> >  		ret = wait_event_interruptible_timeout(fifo->read_queue,
> >  			ioread32(fifo->base_addr + XLLF_RDFO_OFFSET),
> > -				 (read_timeout >= 0) ?
> > -				  msecs_to_jiffies(read_timeout) :
> > -				  MAX_SCHEDULE_TIMEOUT);
> > +			read_timeout);
> > 
> >  		if (ret <= 0) {
> >  			if (ret == 0) {
> > @@ -528,9 +526,7 @@ static ssize_t axis_fifo_write(struct file *f, const 
> char
> > __user *buf, ret = wait_event_interruptible_timeout(fifo->write_queue,
> >  			ioread32(fifo->base_addr + XLLF_TDFV_OFFSET)
> > 
> >  				 >= words_to_write,
> > 
> > -				 (write_timeout >= 0) ?
> > -				  msecs_to_jiffies(write_timeout) :
> > -				  MAX_SCHEDULE_TIMEOUT);
> > +			write_timeout);
> > 
> >  		if (ret <= 0) {
> >  			if (ret == 0) {
> > @@ -948,7 +944,17 @@ static struct platform_driver axis_fifo_driver = {
> > 
> >  static int __init axis_fifo_init(void)
> >  {
> > -	pr_info("axis-fifo driver loaded with parameters read_timeout = %i,
> > write_timeout = %i\n", +	if (read_timeout >= 0)
> > +		read_timeout = msecs_to_jiffies(read_timeout);
> > +	else
> > +		read_timeout = MAX_SCHEDULE_TIMEOUT;
> > +
> > +	if (write_timeout >= 0)
> > +		write_timeout = msecs_to_jiffies(write_timeout);
> > +	else
> > +		write_timeout = MAX_SCHEDULE_TIMEOUT;
> > +
> > +	pr_info("axis-fifo driver loaded with parameters read_timeout = %li,
> > write_timeout = %li\n", read_timeout, write_timeout);
> >  	return platform_driver_register(&axis_fifo_driver);
> >  }
> > --
> > 2.34.1
> 
> 
> 
> 

Re: [PATCH v8] staging: axis-fifo: initialize timeouts in init only

[Fabio M. De Francesco]

On lunedì 20 marzo 2023 07:34:04 CET Khadija Kamran wrote:
> On Fri, Mar 17, 2023 at 11:29:25AM +0100, Fabio M. De Francesco wrote:
> > Khadija,
> > 
> > Congratulations for having your first patch in Linux, via Greg's staging
> > tree.
> > 
> > It will take some time before it reaches mainline, although it is already 
on
> > its way to get upstream.
> 
> Thank you! :)
> 
> > On giovedì 16 marzo 2023 21:09:00 CET Khadija Kamran wrote:
> > > 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().
> > 
> > Greg made you (and indirectly me notice) that the statement above is a 
kind
> > of short-circuit because it misses to make the readers notice that you are
> > dealing with specific permissions granted to these two module's 
parameters.
> > 
> > Please take a look at the permissions associated with those parameters:
> > 
> > module_param(write_timeout, long, 0444);
> > module_param(read_timeout, long, 0444);
> > 
> > Can you understand what '0444' stands for? What if their permissions were
> > instead something like '0666' or '0664'?
> 
> Hey Fabio!
> 
> I understand that 0444 shows read permissions only.

Yes, correct.

Only "read" permissions for owner, group, others.
Obviously, when the module is initialized, "insmod" can pass actual values to 
the arguments. The point is that from that moment onward nobody is allowed to 
change the initial values associated with this variables, but they can still 
be read at will.

> I am trying to make sense of this. As the permissions do not allow
> write, so the value cannot be configured afterwards.

Yes, if with "afterwards" you are intending after they are set at insmod runs. 

> Instead of saying 'cannot be modified later', we should talk more about
> permissions here too.

I'm confused by this statement. Can you please rephrase?

> Am I getting it right?

Not sure, it depends on what you meant with the previous phrase.
 
> Thank you!
> 
> Regards,
> Khadija

You're welcome!

So, thanks for working on this patch as long as it takes to get it done. 

I think the lesson to be learned is that in our community there are barriers 
to the entry of substandard products and therefore people have to do their 
best if they really want to see their work applied.

These tasks are not for the "faints of heart" :-)

Fabio

Re: [PATCH v8] staging: axis-fifo: initialize timeouts in init only

[Khadija Kamran]

On Mon, Mar 20, 2023 at 02:38:24PM +0100, Fabio M. De Francesco wrote:
> On lunedì 20 marzo 2023 07:34:04 CET Khadija Kamran wrote:
> > On Fri, Mar 17, 2023 at 11:29:25AM +0100, Fabio M. De Francesco wrote:
> > > Khadija,
> > > 
> > > Congratulations for having your first patch in Linux, via Greg's staging
> > > tree.
> > > 
> > > It will take some time before it reaches mainline, although it is already 
> on
> > > its way to get upstream.
> > 
> > Thank you! :)
> > 
> > > On giovedì 16 marzo 2023 21:09:00 CET Khadija Kamran wrote:
> > > > 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().
> > >


Hey Fabio,
I am talking about this below:


> > > Greg made you (and indirectly me notice) that the statement above is a 
> kind
> > > of short-circuit because it misses to make the readers notice that you are
> > > dealing with specific permissions granted to these two module's 
> parameters.
> 
> Only "read" permissions for owner, group, others.
> Obviously, when the module is initialized, "insmod" can pass actual values to 
> the arguments. The point is that from that moment onward nobody is allowed to 
> change the initial values associated with this variables, but they can still 
> be read at will.
> 
> > I am trying to make sense of this. As the permissions do not allow
> > write, so the value cannot be configured afterwards.
> 
> Yes, if with "afterwards" you are intending after they are set at insmod runs. 
> 
> > Instead of saying 'cannot be modified later', we should talk more about
> > permissions here too.
> 
> I'm confused by this statement. Can you please rephrase?
> 
> > Am I getting it right?
> 
> Not sure, it depends on what you meant with the previous phrase.

As you said above that the commit message makes the reader miss the
permission details, so should we write more about permissions in the
description?


>  
> > Thank you!
> > 
> > Regards,
> > Khadija
> 
> You're welcome!
> 
> So, thanks for working on this patch as long as it takes to get it done. 
> 
> I think the lesson to be learned is that in our community there are barriers 
> to the entry of substandard products and therefore people have to do their 
> best if they really want to see their work applied.


Yes, you are right. Due to this reason, the whole process is making me
learn a lot. I am really glad to be a part of it ^-^ 


> These tasks are not for the "faints of heart" :-)
> 
> Fabio
>

Regards,
Khadija :)


> 
> 

Re: [PATCH v8] staging: axis-fifo: initialize timeouts in init only

[Fabio M. De Francesco]

On lunedì 20 marzo 2023 15:36:03 CET Khadija Kamran wrote:
> On Mon, Mar 20, 2023 at 02:38:24PM +0100, Fabio M. De Francesco wrote:
> > On lunedì 20 marzo 2023 07:34:04 CET Khadija Kamran wrote:
> > > On Fri, Mar 17, 2023 at 11:29:25AM +0100, Fabio M. De Francesco wrote:
> > > > Khadija,
> > > > 
> > > > Congratulations for having your first patch in Linux, via Greg's 
staging
> > > > tree.
> > > > 
> > > > It will take some time before it reaches mainline, although it is
> > > > already on its way to get upstream.
> > > 
> > > [snip]
> > >
> > I'm confused by this statement. Can you please rephrase?
> > 
> > > Am I getting it right?
> > 
> > Not sure, it depends on what you meant with the previous phrase.
> 
> As you said above that the commit message makes the reader miss the
> permission details, so should we write more about permissions in the
> description?

No. Greg took your patch as is.
No new versions for patches that Maintainers (i.e., Greg Kroah-Hartman for 
drivers/staging and several other subsystems and drivers) already applied.

We've been discussing this topic after Greg had already taken your patch only 
for the purpose to clarify to you why he suggested a better commit message. 
However, he suggested that better message while stating that he prefers to 
take your patch as is to give you time to work on something new.

I suppose that you received the two automated messages, the first which 
communicates that your patch has been applied to his staging-test tree and the 
second which says that it has moved forward to the next step, which is the 
staging-next tree. I saw those messages in Cc because I put my "Reviewed-by" 
tag to your patch, so I know you had them.

Fabio

P.S.: Today I sent you an off-list message. Please locate it and reply.

> > > Thank you!
> > > 
> > > Regards,
> > > Khadija
> > 
> > You're welcome!
> > 
> > So, thanks for working on this patch as long as it takes to get it done.
> > 
> > I think the lesson to be learned is that in our community there are 
barriers
> > to the entry of substandard products and therefore people have to do their
> > best if they really want to see their work applied.
> 
> Yes, you are right. Due to this reason, the whole process is making me
> learn a lot. I am really glad to be a part of it ^-^
> 
> > These tasks are not for the "faints of heart" :-)
> > 
> > Fabio
> 
> Regards,
> Khadija :)

Re: [PATCH v8] staging: axis-fifo: initialize timeouts in init only

[Khadija Kamran]

On Mon, Mar 20, 2023 at 05:55:20PM +0100, Fabio M. De Francesco wrote:
> On lunedì 20 marzo 2023 15:36:03 CET Khadija Kamran wrote:
> > On Mon, Mar 20, 2023 at 02:38:24PM +0100, Fabio M. De Francesco wrote:
> > > On lunedì 20 marzo 2023 07:34:04 CET Khadija Kamran wrote:
> > > > On Fri, Mar 17, 2023 at 11:29:25AM +0100, Fabio M. De Francesco wrote:
> > > > > Khadija,
> > > > > 
> > > > > Congratulations for having your first patch in Linux, via Greg's 
> staging
> > > > > tree.
> > > > > 
> > > > > It will take some time before it reaches mainline, although it is
> > > > > already on its way to get upstream.
> > > > 
> > > > [snip]
> > > >
> > > I'm confused by this statement. Can you please rephrase?
> > > 
> > > > Am I getting it right?
> > > 
> > > Not sure, it depends on what you meant with the previous phrase.
> > 
> > As you said above that the commit message makes the reader miss the
> > permission details, so should we write more about permissions in the
> > description?
> 
> No. Greg took your patch as is.
> No new versions for patches that Maintainers (i.e., Greg Kroah-Hartman for 
> drivers/staging and several other subsystems and drivers) already applied.
> 
> We've been discussing this topic after Greg had already taken your patch only 
> for the purpose to clarify to you why he suggested a better commit message. 
> However, he suggested that better message while stating that he prefers to 
> take your patch as is to give you time to work on something new.
> 

Hey Fabio!
Okay understood. 

> I suppose that you received the two automated messages, the first which 
> communicates that your patch has been applied to his staging-test tree and the 
> second which says that it has moved forward to the next step, which is the 
> staging-next tree. I saw those messages in Cc because I put my "Reviewed-by" 
> tag to your patch, so I know you had them.
>

Yes, I did receive the messages. 

> Fabio
> 
> P.S.: Today I sent you an off-list message. Please locate it and reply.
>

Okay Thank you for your help.

Regards,
Khadija

> > > > Thank you!
> > > > 
> > > > Regards,
> > > > Khadija
> > > 
> > > You're welcome!
> > > 
> > > So, thanks for working on this patch as long as it takes to get it done.
> > > 
> > > I think the lesson to be learned is that in our community there are 
> barriers
> > > to the entry of substandard products and therefore people have to do their
> > > best if they really want to see their work applied.
> > 
> > Yes, you are right. Due to this reason, the whole process is making me
> > learn a lot. I am really glad to be a part of it ^-^
> > 
> > > These tasks are not for the "faints of heart" :-)
> > > 
> > > Fabio
> > 
> > Regards,
> > Khadija :)
> 
> 
> 
>