Date:   Mon, 13 Mar 2023 19:48:43 +0500
From:   Khadija Kamran <kamrankhadijadj@gmail.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Cc:     outreachy@lists.linux.dev,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4] staging: axis-fifo: initialize read_timeout and
 write_timeout once in probe function
Message-ID: <ZA83yzPTq+nQF9Dd@khadija-virtual-machine>
References: <ZA4M3+ZeB1Rl2fbs@khadija-virtual-machine>
 <9938109.ZD9C4EEAuf@suse>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <9938109.ZD9C4EEAuf@suse>
Precedence: bulk

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