[PATCH v4] staging: axis-fifo: initialize read_timeout and write_timeout once in probe function

[PATCH v4] staging: axis-fifo: initialize read_timeout and write_timeout once in probe function

[Khadija Kamran]

Module parameter, read_timeout, can only be set at loading time. As it
can only be modified once, initialize read_timeout once in the probe
function.
As a result, only use read_timeout as the last argument in
wait_event_interruptible_timeout() call.

Same goes for write_timeout.

Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
---
 drivers/staging/axis-fifo/axis-fifo.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
index b119cec25a60..7ec8722cef7d 100644
--- a/drivers/staging/axis-fifo/axis-fifo.c
+++ b/drivers/staging/axis-fifo/axis-fifo.c
@@ -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) {
@@ -814,6 +810,16 @@ static int axis_fifo_probe(struct platform_device *pdev)
 	struct axis_fifo *fifo = NULL;
 	char *device_name;
 	int rc = 0; /* error return value */
+	
+	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;
 
 	/* ----------------------------
 	 *     init wrapper device
-- 
2.34.1

Re: [PATCH v4] staging: axis-fifo: initialize read_timeout and write_timeout once in probe function

[Greg Kroah-Hartman]

On Sun, Mar 12, 2023 at 10:33:19PM +0500, Khadija Kamran wrote:
> Module parameter, read_timeout, can only be set at loading time. As it
> can only be modified once, initialize read_timeout once in the probe
> function.
> As a result, only use read_timeout as the last argument in
> wait_event_interruptible_timeout() call.
> 
> Same goes for write_timeout.
> 
> Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> ---
>  drivers/staging/axis-fifo/axis-fifo.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
> index b119cec25a60..7ec8722cef7d 100644
> --- a/drivers/staging/axis-fifo/axis-fifo.c
> +++ b/drivers/staging/axis-fifo/axis-fifo.c
> @@ -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) {
> @@ -814,6 +810,16 @@ static int axis_fifo_probe(struct platform_device *pdev)
>  	struct axis_fifo *fifo = NULL;
>  	char *device_name;
>  	int rc = 0; /* error return value */
> +	
> +	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;
>  
>  	/* ----------------------------
>  	 *     init wrapper device
> -- 
> 2.34.1
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- Your patch contains warnings and/or errors noticed by the
  scripts/checkpatch.pl tool.

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/process/submitting-patches.rst for what
  needs to be done here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Re: [PATCH v4] staging: axis-fifo: initialize read_timeout and write_timeout once in probe function

[Fabio M. De Francesco]

On domenica 12 marzo 2023 18:33:19 CET Khadija Kamran wrote:
> Module parameter, read_timeout, can only be set at loading time. As it
> can only be modified once, initialize read_timeout once in the probe
> function.
> As a result, only use read_timeout as the last argument in
> wait_event_interruptible_timeout() call.
> 
> Same goes for write_timeout.
> 

Nice idea... But it's not yours :-)

Therefore, you should give credit to Greg with the following tag:

Suggested-by: Greg Kroah-Hartman <...> 

Place the above-mentioned tag a line before the "Signed-off-by:" (which is 
always the last line, whatever other tags you might need to add).

> Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> ---

If this patch was a v4 you should have put a log right here, after the three 
dashes, explaining what changed from one release to another, release after 
release. Please read some other well formatted and accepted patches for real 
world examples of how to write version logs.

However, this patch is _not_ a v4 (so no version log is needed after the three 
dashes). This is your _first_ patch that addresses Greg's suggested 
refactoring. Therefore, just put [PATCH] in the subject line.

That inappropriate "v4" seems to explain the second error showed by the patch-
bot. Thus, read carefully its message and ask for further explanations if 
something is still unclear.

Thanks,

Fabio

P.S.: The code looks good but I could not apply it in mainline tree. I don't 
know whether this patch is somehow broken or the driver's files differ between 
the most recent staging tree and mainline.

However, does it work for you on the most recent staging tree? Did you run 
checkpatch on your own patch? (I'm also asking this question because of the 
first error showed by the patch-bot). Can you git-reset to a previous state 
and reapply your own patches to your local work branch?

>  drivers/staging/axis-fifo/axis-fifo.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)

Re: [PATCH v4] staging: axis-fifo: initialize read_timeout and write_timeout once in probe function

[Khadija Kamran]

On Mon, Mar 13, 2023 at 03:13:01PM +0100, Fabio M. De Francesco wrote:
> On domenica 12 marzo 2023 18:33:19 CET Khadija Kamran wrote:
> > Module parameter, read_timeout, can only be set at loading time. As it
> > can only be modified once, initialize read_timeout once in the probe
> > function.
> > As a result, only use read_timeout as the last argument in
> > wait_event_interruptible_timeout() call.
> > 
> > Same goes for write_timeout.
> > 
> 
> Nice idea... But it's not yours :-)
> 
> Therefore, you should give credit to Greg with the following tag:
> 
> Suggested-by: Greg Kroah-Hartman <...> 
> 
> Place the above-mentioned tag a line before the "Signed-off-by:" (which is 
> always the last line, whatever other tags you might need to add).
>

Hey Fabio!
Thank you for letting me know. I was confused as to where should I
mention that this change was recommended by Greg.

> > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> > ---
> 
> If this patch was a v4 you should have put a log right here, after the three 
> dashes, explaining what changed from one release to another, release after 
> release. Please read some other well formatted and accepted patches for real 
> world examples of how to write version logs.
> 

Okay, got it! I shouldn't have missed it.

> However, this patch is _not_ a v4 (so no version log is needed after the three 
> dashes). This is your _first_ patch that addresses Greg's suggested 
> refactoring. Therefore, just put [PATCH] in the subject line.
> 
> That inappropriate "v4" seems to explain the second error showed by the patch-
> bot. Thus, read carefully its message and ask for further explanations if 
> something is still unclear.
> 

Thank you! It is clear. I will send this again as first_patch. 

> Thanks,
> 
> Fabio
> 
> P.S.: The code looks good but I could not apply it in mainline tree. I don't 
> know whether this patch is somehow broken or the driver's files differ between 
> the most recent staging tree and mainline.
> 
> However, does it work for you on the most recent staging tree? Did you run 
> checkpatch on your own patch? (I'm also asking this question because of the 
> first error showed by the patch-bot). Can you git-reset to a previous state 
> and reapply your own patches to your local work branch?
> 

Yes,  I did run checkpatch on my patch as suggested by Dan before. It
showed errors regarding trailing white spaces. Sorry, I ignored them
thinking that they were present before in the code. I will correct them
in the next patch submission.

Also, I had one question. Is it okay to write a long subject as I have
used in this patch? 

Regards,
Khadija

> >  drivers/staging/axis-fifo/axis-fifo.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> 
> 

Re: [PATCH v4] staging: axis-fifo: initialize read_timeout and write_timeout once in probe function

[Dan Carpenter]

On Mon, Mar 13, 2023 at 07:48:43PM +0500, Khadija Kamran wrote:
> Also, I had one question. Is it okay to write a long subject as I have
> used in this patch?

People say subject should be 50 chars (I forget the exact limit) but
there is a little bit of flexibility.  This subject is too long though.
Perhaps I would use something like:

[PATCH v5] staging: axis-fifo: initialize timeouts in probe only

regards,
dan carpenter

Re: [PATCH v4] staging: axis-fifo: initialize read_timeout and write_timeout once in probe function

[Khadija Kamran]

On Mon, Mar 13, 2023 at 06:04:45PM +0300, Dan Carpenter wrote:
> On Mon, Mar 13, 2023 at 07:48:43PM +0500, Khadija Kamran wrote:
> > Also, I had one question. Is it okay to write a long subject as I have
> > used in this patch?
> 
> People say subject should be 50 chars (I forget the exact limit) but
> there is a little bit of flexibility.  This subject is too long though.
> Perhaps I would use something like:
> 
> [PATCH v5] staging: axis-fifo: initialize timeouts in probe only
>

Thanks Dan,
I am going to use this for the next patch  O:)

> regards,
> dan carpenter
> 

Re: [PATCH v4] staging: axis-fifo: initialize read_timeout and write_timeout once in probe function

[Alison Schofield]

On Mon, Mar 13, 2023 at 07:48:43PM +0500, Khadija Kamran wrote:
> On Mon, Mar 13, 2023 at 03:13:01PM +0100, Fabio M. De Francesco wrote:
> > On domenica 12 marzo 2023 18:33:19 CET Khadija Kamran wrote:
> > > Module parameter, read_timeout, can only be set at loading time. As it
> > > can only be modified once, initialize read_timeout once in the probe
> > > function.
> > > As a result, only use read_timeout as the last argument in
> > > wait_event_interruptible_timeout() call.
> > > 
> > > Same goes for write_timeout.
> > > 
> > 
> > Nice idea... But it's not yours :-)
> > 
> > Therefore, you should give credit to Greg with the following tag:
> > 
> > Suggested-by: Greg Kroah-Hartman <...> 
> > 
> > Place the above-mentioned tag a line before the "Signed-off-by:" (which is 
> > always the last line, whatever other tags you might need to add).
> >
> 
> Hey Fabio!
> Thank you for letting me know. I was confused as to where should I
> mention that this change was recommended by Greg.
> 
> > > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> > > ---
> > 
> > If this patch was a v4 you should have put a log right here, after the three 
> > dashes, explaining what changed from one release to another, release after 
> > release. Please read some other well formatted and accepted patches for real 
> > world examples of how to write version logs.
> > 
> 
> Okay, got it! I shouldn't have missed it.
> 
> > However, this patch is _not_ a v4 (so no version log is needed after the three 
> > dashes). This is your _first_ patch that addresses Greg's suggested 
> > refactoring. Therefore, just put [PATCH] in the subject line.
> > 
> > That inappropriate "v4" seems to explain the second error showed by the patch-
> > bot. Thus, read carefully its message and ask for further explanations if 
> > something is still unclear.
> > 
> 
> Thank you! It is clear. I will send this again as first_patch. 
> 
> > Thanks,
> > 
> > Fabio
> > 
> > P.S.: The code looks good but I could not apply it in mainline tree. I don't 
> > know whether this patch is somehow broken or the driver's files differ between 
> > the most recent staging tree and mainline.
> > 
> > However, does it work for you on the most recent staging tree? Did you run 
> > checkpatch on your own patch? (I'm also asking this question because of the 
> > first error showed by the patch-bot). Can you git-reset to a previous state 
> > and reapply your own patches to your local work branch?
> > 
> 
> Yes,  I did run checkpatch on my patch as suggested by Dan before. It
> showed errors regarding trailing white spaces. Sorry, I ignored them
> thinking that they were present before in the code. I will correct them
> in the next patch submission.

It sounds like the git commit hook for checkpatch, suggested in
the tutorial, would have saved you here. Please look that up.

Also see the section "Following the Driver commit style"
And, there are sections on revising patchsets too.

Much of the info in the tutorial doesn't make sense until you
need it. So, keep reviewing it to catch more useful info.

More on running checkpatch:
- Add that git commmit hook.
- The final checkpatch run can be on the formatted patches.
  After you've done git format-patch and have a .patch file you
  are thinking of sending, run it again. 

Something like this:
scripts/checkpatch.pl --no-tree --strict --codespellfile=/usr/bin/codespell ~/my_patches/*.patch

> 
> Also, I had one question. Is it okay to write a long subject as I have
> used in this patch? 
This is in the tutorial. section "Following the Driver commit style"

> 
> Regards,
> Khadija
> 
> > >  drivers/staging/axis-fifo/axis-fifo.c | 18 ++++++++++++------
> > >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > 
> > 
> 

Re: [PATCH v4] staging: axis-fifo: initialize read_timeout and write_timeout once in probe function

[Khadija Kamran]

On Mon, Mar 13, 2023 at 10:12:03AM -0700, Alison Schofield wrote:
> 
> It sounds like the git commit hook for checkpatch, suggested in
> the tutorial, would have saved you here. Please look that up.
>

Hey Alison!
I had set up git post-commit hook following the tutorial. The script
showed the errors, but I ignored them before. I have submitted another
patch. I had 'trailing white spaces' errors in my code and removed them
in the new patch after consultation from 'coding-style.rst' file.

> Also see the section "Following the Driver commit style"
> And, there are sections on revising patchsets too.
> 
> Much of the info in the tutorial doesn't make sense until you
> need it. So, keep reviewing it to catch more useful info.
>

Yes!! No matter how many times I read the tutorial, I still end up
getting back to it! :)
Thank you!

Regards,
Khadija

> More on running checkpatch:
> - Add that git commmit hook.
> - The final checkpatch run can be on the formatted patches.
>   After you've done git format-patch and have a .patch file you
>   are thinking of sending, run it again. 
> 
> Something like this:
> scripts/checkpatch.pl --no-tree --strict --codespellfile=/usr/bin/codespell ~/my_patches/*.patch
> 
> > 
> > Also, I had one question. Is it okay to write a long subject as I have
> > used in this patch? 
> This is in the tutorial. section "Following the Driver commit style"
>
> > 
> > Regards,
> > Khadija
> > 
> > > >  drivers/staging/axis-fifo/axis-fifo.c | 18 ++++++++++++------
> > > >  1 file changed, 12 insertions(+), 6 deletions(-)
> > > 
> > > 
> > > 
> >