From:   "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To:     Khadija Kamran <kamrankhadijadj@gmail.com>
Cc:     outreachy@lists.linux.dev,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v8] staging: axis-fifo: initialize timeouts in init only
Date:   Mon, 20 Mar 2023 14:38:24 +0100
Message-ID: <3180435.l52yBJDM9G@suse>
In-Reply-To: <ZBf+XDN8GM0xDDCq@khadija-virtual-machine>
References: <ZBN3XAsItCiTk7CV@khadija-virtual-machine> <2222576.3ZeAukHxDK@suse>
 <ZBf+XDN8GM0xDDCq@khadija-virtual-machine>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
Precedence: bulk

On luned=EC 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,
> >=20
> > Congratulations for having your first patch in Linux, via Greg's staging
> > tree.
> >=20
> > It will take some time before it reaches mainline, although it is alrea=
dy=20
on
> > its way to get upstream.
>=20
> Thank you! :)
>=20
> > On gioved=EC 16 marzo 2023 21:09:00 CET Khadija Kamran wrote:
> > > Initialize the module parameters, read_timeout and write_timeout once=
 in
> > > init().
> > >=20
> > > Module parameters can only be set once and cannot be modified later, =
so=20
we
> > > don't need to evaluate them again when passing the parameters to
> > > wait_event_interruptible_timeout().
> >=20
> > Greg made you (and indirectly me notice) that the statement above is a=
=20
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=20
parameters.
> >=20
> > Please take a look at the permissions associated with those parameters:
> >=20
> > module_param(write_timeout, long, 0444);
> > module_param(read_timeout, long, 0444);
> >=20
> > Can you understand what '0444' stands for? What if their permissions we=
re
> > instead something like '0666' or '0664'?
>=20
> Hey Fabio!
>=20
> 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=20
the arguments. The point is that from that moment onward nobody is allowed =
to=20
change the initial values associated with this variables, but they can stil=
l=20
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 ru=
ns.=20

> 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.
=20
> Thank you!
>=20
> Regards,
> Khadija

You're welcome!

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

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

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

=46abio