Message-ID: <2222576.3ZeAukHxDK@suse>
Date:   Fri, 17 Mar 2023 11:29:25 +0100
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     outreachy@...ts.linux.dev,
        Khadija Kamran <kamrankhadijadj@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8] staging: axis-fifo: initialize timeouts in init only

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.

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

(I'm not asking you to answer these questions, instead I am only asking you to 
learn how it works if you don't know it yet).

Fabio

> 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.
> 
> Reviewed-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
> Suggested-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Signed-off-by: Khadija Kamran <kamrankhadijadj@...il.com>
> ---
> 
> Changes in v8:
>  - Fixed a spelling mistake
> 
> Changes in v7:
>  - Fixed a grammatical error
> 
> Changes in v6:
>  - Initialize module parameters in init instead of probe function.
>  - Change the subject and description
>  - Change format specifiers of module parameters to "%li"
> 
> Changes in v5:
>  - Convert module parameters datatype from int to long.
>  - Link to patch:
>  https://lore.kernel.org/outreachy/ZBMR4s8xyHGqMm72@khadija-virtual-machine/
> 
> Changes in v4:
>  - Initialize timeouts once as suggested by Greg; this automatically
>    fixes the indentation problems.
>  - Change the subject and description.
>  - Link to patch:
>  https://lore.kernel.org/outreachy/ZA4M3+ZeB1Rl2fbs@khadija-virtual-machine/
> 
> Changes in v3:
>  - Correct grammatical mistakes
>  - Do not change the second argument's indentation in split lines
> 
> Changes in v2:
>  - Instead of matching alignment to open parenthesis, align second and
>    the last argument.
>  - Change the subject and use imperative language.
>  - Link to patch:
>  https://lore.kernel.org/outreachy/ZAxNYw2rFQkrdtKl@khadija-virtual-machine/
> 
> Link to first patch:
>  https://lore.kernel.org/outreachy/ZAZSmPpB6fcozGa4@khadija-virtual-machine/
> 
>  drivers/staging/axis-fifo/axis-fifo.c | 28 ++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/axis-fifo/axis-fifo.c
> b/drivers/staging/axis-fifo/axis-fifo.c index dfd2b357f484..0a85ea667a1b
> 100644
> --- a/drivers/staging/axis-fifo/axis-fifo.c
> +++ b/drivers/staging/axis-fifo/axis-fifo.c
> @@ -103,17 +103,17 @@
>   *           globals
>   * ----------------------------
>   */
> -static int read_timeout = 1000; /* ms to wait before read() times out */
> -static int write_timeout = 1000; /* ms to wait before write() times out */
> +static long read_timeout = 1000; /* ms to wait before read() times out */
> +static long write_timeout = 1000; /* ms to wait before write() times out */
> 
>  /* ----------------------------
>   * module command-line arguments
>   * ----------------------------
>   */
> 
> -module_param(read_timeout, int, 0444);
> +module_param(read_timeout, long, 0444);
>  MODULE_PARM_DESC(read_timeout, "ms to wait before blocking read() timing 
out;
> set to -1 for no timeout"); -module_param(write_timeout, int, 0444);
> +module_param(write_timeout, long, 0444);
>  MODULE_PARM_DESC(write_timeout, "ms to wait before blocking write() timing
> out; set to -1 for no timeout");
> 
>  /* ----------------------------
> @@ -384,9 +384,7 @@ static ssize_t axis_fifo_read(struct file *f, char 
__user
> *buf, mutex_lock(&fifo->read_lock);
>  		ret = wait_event_interruptible_timeout(fifo->read_queue,
>  			ioread32(fifo->base_addr + XLLF_RDFO_OFFSET),
> -				 (read_timeout >= 0) ?
> -				  msecs_to_jiffies(read_timeout) :
> -				  MAX_SCHEDULE_TIMEOUT);
> +			read_timeout);
> 
>  		if (ret <= 0) {
>  			if (ret == 0) {
> @@ -528,9 +526,7 @@ static ssize_t axis_fifo_write(struct file *f, const 
char
> __user *buf, ret = wait_event_interruptible_timeout(fifo->write_queue,
>  			ioread32(fifo->base_addr + XLLF_TDFV_OFFSET)
> 
>  				 >= words_to_write,
> 
> -				 (write_timeout >= 0) ?
> -				  msecs_to_jiffies(write_timeout) :
> -				  MAX_SCHEDULE_TIMEOUT);
> +			write_timeout);
> 
>  		if (ret <= 0) {
>  			if (ret == 0) {
> @@ -948,7 +944,17 @@ static struct platform_driver axis_fifo_driver = {
> 
>  static int __init axis_fifo_init(void)
>  {
> -	pr_info("axis-fifo driver loaded with parameters read_timeout = %i,
> write_timeout = %i\n", +	if (read_timeout >= 0)
> +		read_timeout = msecs_to_jiffies(read_timeout);
> +	else
> +		read_timeout = MAX_SCHEDULE_TIMEOUT;
> +
> +	if (write_timeout >= 0)
> +		write_timeout = msecs_to_jiffies(write_timeout);
> +	else
> +		write_timeout = MAX_SCHEDULE_TIMEOUT;
> +
> +	pr_info("axis-fifo driver loaded with parameters read_timeout = %li,
> write_timeout = %li\n", read_timeout, write_timeout);
>  	return platform_driver_register(&axis_fifo_driver);
>  }
> --
> 2.34.1

Powered by blists - more mailing lists