On Fri, Mar 17, 2023 at 08:13:37AM +0100, 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.
>
> 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.
> 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...)
>
Okay Great! Thank you Greg!
Regards,
Khadija
> thanks,
>
> greg k-h
