Date:   Mon, 20 Mar 2023 11:17:18 +0500
From:   Khadija Kamran <kamrankhadijadj@gmail.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        outreachy@lists.linux.dev, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v8] staging: axis-fifo: initialize timeouts in init only
Message-ID: <ZBf6bhzRXRQJi6Bc@khadija-virtual-machine>
References: <ZBN3XAsItCiTk7CV@khadija-virtual-machine>
 <ZBQTIVk0zsgv1hMH@kroah.com>
 <2499410.4XsnlVU6TS@suse>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <2499410.4XsnlVU6TS@suse>
Precedence: bulk

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