Re: [PATCH 1/2] staging: gpib: change return type of t1_delay function to report errors

[Dan Carpenter <dan.carpenter@linaro.org>]

On Tue, Feb 11, 2025 at 11:35:36PM -0300, Rodrigo Gobbi wrote:
> Propagate t1 delay configuration error to userspace
> 

Better to spell out the problem a bit more.  What is the effect in terms
of what the user experiences and explain why what you are doing is safe.

  The current code returns "unsigned int" and it doesn't handle errors
  correctly.

  The ni_usb_t1_delay() is the only function which returned an error (-1).
  The caller, t1_delay_ioctl() doesn't check for errors and sets
  board->t1_nano_sec to -1 and returns success.  The board->t1_nano_sec
  value is used in ni_usb_setup_t1_delay() and a value of -1 is treated as
  being above 1100ns.  It may or may not have a noticeable effect, but
  it's obviously not right.

  Typical delays are in the 200-2000 range, but definitely not more
  than INT_MAX so we can fix this code by changing the return type to int
  and adding a check for errors.  While we're at it, lets change the error
  code in ni_usb_t1_delay() from -1 and instead propagate the error from
  ni_usb_write_registers().

You should add a Fixes tag.


> diff --git a/drivers/staging/gpib/ni_usb/ni_usb_gpib.c b/drivers/staging/gpib/ni_usb/ni_usb_gpib.c
> index d0656dc520f5..cb8840f2a461 100644
> --- a/drivers/staging/gpib/ni_usb/ni_usb_gpib.c
> +++ b/drivers/staging/gpib/ni_usb/ni_usb_gpib.c
> @@ -1591,7 +1591,7 @@ static int ni_usb_setup_t1_delay(struct ni_usb_register *reg, unsigned int nano_
>  	return i;
>  }
>  
> -static unsigned int ni_usb_t1_delay(gpib_board_t *board, unsigned int nano_sec)
> +static int ni_usb_t1_delay(gpib_board_t *board, unsigned int nano_sec)
>  {
>  	int retval;
>  	struct ni_usb_priv *ni_priv = board->private_data;
> @@ -1605,7 +1605,7 @@ static unsigned int ni_usb_t1_delay(gpib_board_t *board, unsigned int nano_sec)
>  	retval = ni_usb_write_registers(ni_priv, writes, i, &ibsta);
>  	if (retval < 0) {
>  		dev_err(&usb_dev->dev, "%s: register write failed, retval=%i\n", __func__, retval);
> -		return -1;	//FIXME should change return type to int for error reporting
> +		return -EIO;

Better to "return retval;"

>  	}
>  	board->t1_nano_sec = actual_ns;
>  	ni_usb_soft_update_status(board, ibsta, 0);

regards,
dan carpenter