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
