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