Re: [PATCH v8] staging: axis-fifo: initialize timeouts in init only

[Fabio M. De Francesco]

On lunedì 20 marzo 2023 07:34:04 CET Khadija Kamran wrote:
> On Fri, Mar 17, 2023 at 11:29:25AM +0100, Fabio M. De Francesco wrote:
> > Khadija,
> > 
> > Congratulations for having your first patch in Linux, via Greg's staging
> > tree.
> > 
> > It will take some time before it reaches mainline, although it is already 
on
> > its way to get upstream.
> 
> Thank you! :)
> 
> > On giovedì 16 marzo 2023 21:09:00 CET 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().
> > 
> > Greg made you (and indirectly me notice) that the statement above is a 
kind
> > of short-circuit because it misses to make the readers notice that you are
> > dealing with specific permissions granted to these two module's 
parameters.
> > 
> > Please take a look at the permissions associated with those parameters:
> > 
> > module_param(write_timeout, long, 0444);
> > module_param(read_timeout, long, 0444);
> > 
> > Can you understand what '0444' stands for? What if their permissions were
> > instead something like '0666' or '0664'?
> 
> Hey Fabio!
> 
> I understand that 0444 shows read permissions only.

Yes, correct.

Only "read" permissions for owner, group, others.
Obviously, when the module is initialized, "insmod" can pass actual values to 
the arguments. The point is that from that moment onward nobody is allowed to 
change the initial values associated with this variables, but they can still 
be read at will.

> I am trying to make sense of this. As the permissions do not allow
> write, so the value cannot be configured afterwards.

Yes, if with "afterwards" you are intending after they are set at insmod runs. 

> Instead of saying 'cannot be modified later', we should talk more about
> permissions here too.

I'm confused by this statement. Can you please rephrase?

> Am I getting it right?

Not sure, it depends on what you meant with the previous phrase.
 
> Thank you!
> 
> Regards,
> Khadija

You're welcome!

So, thanks for working on this patch as long as it takes to get it done. 

I think the lesson to be learned is that in our community there are barriers 
to the entry of substandard products and therefore people have to do their 
best if they really want to see their work applied.

These tasks are not for the "faints of heart" :-)

Fabio