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