Propagate t1 delay configuration error to userspace --- Noticed that at [1] there is a FIXME tag to propagate ni_usb_write_registers() errors but the t1_delay pointer definition is defined as unsigned int. Checking at userspace libs, it is possible to notice that the error could be returned/treated in the following sequence [1], [2] and [3] (I'm simplyfing the codepath here). The patch is big due the pointer signature change affecting all gpib drivers. I'm suggesting to return a int value. I've tested the compilation of every driver after that change. Tks and regards. [1] https://github.com/torvalds/linux/blob/master/drivers/staging/gpib/ni_usb/ni_usb_gpib.c#L1608 [2] https://sourceforge.net/p/linux-gpib/git/ci/master/tree/linux-gpib-user/lib/ibconfig.c#l70 [3] https://sourceforge.net/p/linux-gpib/git/ci/master/tree/linux-gpib-user/lib/ibutil.c#l469 [4] https://sourceforge.net/p/linux-gpib/git/ci/master/tree/linux-gpib-user/lib/ibutil.c#l433 --- Rodrigo Gobbi (2): staging: gpib: change return type of t1_delay function to report errors staging: gpib: fix style at nec7210_t1_delay definition drivers/staging/gpib/agilent_82350b/agilent_82350b.c | 2 +- drivers/staging/gpib/agilent_82357a/agilent_82357a.c | 2 +- drivers/staging/gpib/cb7210/cb7210.c | 2 +- drivers/staging/gpib/cec/cec_gpib.c | 2 +- drivers/staging/gpib/common/gpib_os.c | 5 ++++- drivers/staging/gpib/eastwood/fluke_gpib.c | 2 +- drivers/staging/gpib/fmh_gpib/fmh_gpib.c | 2 +- drivers/staging/gpib/gpio/gpib_bitbang.c | 2 +- drivers/staging/gpib/hp_82335/hp82335.c | 2 +- drivers/staging/gpib/hp_82341/hp_82341.c | 2 +- drivers/staging/gpib/include/gpib_types.h | 2 +- drivers/staging/gpib/include/nec7210.h | 4 ++-- drivers/staging/gpib/ines/ines.h | 2 +- drivers/staging/gpib/ines/ines_gpib.c | 2 +- drivers/staging/gpib/lpvo_usb_gpib/lpvo_usb_gpib.c | 3 +-- drivers/staging/gpib/nec7210/nec7210.c | 2 +- drivers/staging/gpib/ni_usb/ni_usb_gpib.c | 4 ++-- drivers/staging/gpib/pc2/pc2_gpib.c | 2 +- drivers/staging/gpib/tnt4882/tnt4882_gpib.c | 2 +- 19 files changed, 24 insertions(+), 22 deletions(-) -- 2.47.0
Copy & paste the following snippet into your terminal to import this patchset into git:
curl -s https://lists.sr.ht/~lkcamp/patches/patches/57429/mbox | git am -3Learn more about email & git
Propagate t1 delay configuration error to userspace
Dan Carpenter <dan.carpenter@linaro.org>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.
Signed-off-by: Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com> --- drivers/staging/gpib/agilent_82350b/agilent_82350b.c | 2 +- drivers/staging/gpib/agilent_82357a/agilent_82357a.c | 2 +- drivers/staging/gpib/cb7210/cb7210.c | 2 +- drivers/staging/gpib/cec/cec_gpib.c | 2 +- drivers/staging/gpib/common/gpib_os.c | 5 ++++- drivers/staging/gpib/eastwood/fluke_gpib.c | 2 +- drivers/staging/gpib/fmh_gpib/fmh_gpib.c | 2 +- drivers/staging/gpib/gpio/gpib_bitbang.c | 2 +- drivers/staging/gpib/hp_82335/hp82335.c | 2 +- drivers/staging/gpib/hp_82341/hp_82341.c | 2 +- drivers/staging/gpib/include/gpib_types.h | 2 +- drivers/staging/gpib/include/nec7210.h | 2 +- drivers/staging/gpib/ines/ines.h | 2 +- drivers/staging/gpib/ines/ines_gpib.c | 2 +- drivers/staging/gpib/lpvo_usb_gpib/lpvo_usb_gpib.c | 3 +-- drivers/staging/gpib/nec7210/nec7210.c | 2 +- drivers/staging/gpib/ni_usb/ni_usb_gpib.c | 4 ++-- drivers/staging/gpib/pc2/pc2_gpib.c | 2 +- drivers/staging/gpib/tnt4882/tnt4882_gpib.c | 2 +- 19 files changed, 23 insertions(+), 21 deletions(-) diff --git a/drivers/staging/gpib/agilent_82350b/agilent_82350b.c b/drivers/staging/gpib/agilent_82350b/agilent_82350b.c index 5c62ec24fced..d3635d283234 100644 --- a/drivers/staging/gpib/agilent_82350b/agilent_82350b.c +++ b/drivers/staging/gpib/agilent_82350b/agilent_82350b.c @@ -475,7 +475,7 @@ static int agilent_82350b_line_status(const gpib_board_t *board) return tms9914_line_status(board, &priv->tms9914_priv); } -static unsigned int agilent_82350b_t1_delay(gpib_board_t *board, unsigned int nanosec) +static int agilent_82350b_t1_delay(gpib_board_t *board, unsigned int nanosec) { struct agilent_82350b_priv *a_priv = board->private_data; static const int nanosec_per_clock = 30; diff --git a/drivers/staging/gpib/agilent_82357a/agilent_82357a.c b/drivers/staging/gpib/agilent_82357a/agilent_82357a.c index 69f0e490d401..f081bbbda54c 100644 --- a/drivers/staging/gpib/agilent_82357a/agilent_82357a.c +++ b/drivers/staging/gpib/agilent_82357a/agilent_82357a.c @@ -1044,7 +1044,7 @@ static unsigned short nanosec_to_fast_talker_bits(unsigned int *nanosec) return bits; } -static unsigned int agilent_82357a_t1_delay(gpib_board_t *board, unsigned int nanosec) +static int agilent_82357a_t1_delay(gpib_board_t *board, unsigned int nanosec) { struct agilent_82357a_priv *a_priv = board->private_data; struct usb_device *usb_dev = interface_to_usbdev(a_priv->bus_interface); diff --git a/drivers/staging/gpib/cb7210/cb7210.c b/drivers/staging/gpib/cb7210/cb7210.c index 381c508f62eb..e2e90dd11f0b 100644 --- a/drivers/staging/gpib/cb7210/cb7210.c +++ b/drivers/staging/gpib/cb7210/cb7210.c @@ -410,7 +410,7 @@ static int cb7210_line_status(const gpib_board_t *board) return status; } -static unsigned int cb7210_t1_delay(gpib_board_t *board, unsigned int nano_sec) +static int cb7210_t1_delay(gpib_board_t *board, unsigned int nano_sec) { struct cb7210_priv *cb_priv = board->private_data; struct nec7210_priv *nec_priv = &cb_priv->nec7210_priv; diff --git a/drivers/staging/gpib/cec/cec_gpib.c b/drivers/staging/gpib/cec/cec_gpib.c index 18933223711e..801f041def99 100644 --- a/drivers/staging/gpib/cec/cec_gpib.c +++ b/drivers/staging/gpib/cec/cec_gpib.c @@ -169,7 +169,7 @@ static uint8_t cec_serial_poll_status(gpib_board_t *board) return nec7210_serial_poll_status(board, &priv->nec7210_priv); } -static unsigned int cec_t1_delay(gpib_board_t *board, unsigned int nano_sec) +static int cec_t1_delay(gpib_board_t *board, unsigned int nano_sec) { struct cec_priv *priv = board->private_data; diff --git a/drivers/staging/gpib/common/gpib_os.c b/drivers/staging/gpib/common/gpib_os.c index 4901e660242e..321f93d926b6 100644 --- a/drivers/staging/gpib/common/gpib_os.c +++ b/drivers/staging/gpib/common/gpib_os.c @@ -2018,8 +2018,11 @@ static int t1_delay_ioctl(gpib_board_t *board, unsigned long arg) delay = cmd; - board->t1_nano_sec = board->interface->t1_delay(board, delay); + retval = board->interface->t1_delay(board, delay); + if (retval < 0) + return retval; + board->t1_nano_sec = retval; return 0; } diff --git a/drivers/staging/gpib/eastwood/fluke_gpib.c b/drivers/staging/gpib/eastwood/fluke_gpib.c index d5b1a03abf11..77b912326018 100644 --- a/drivers/staging/gpib/eastwood/fluke_gpib.c +++ b/drivers/staging/gpib/eastwood/fluke_gpib.c @@ -221,7 +221,7 @@ static int fluke_line_status(const gpib_board_t *board) return status; } -static unsigned int fluke_t1_delay(gpib_board_t *board, unsigned int nano_sec) +static int fluke_t1_delay(gpib_board_t *board, unsigned int nano_sec) { struct fluke_priv *e_priv = board->private_data; struct nec7210_priv *nec_priv = &e_priv->nec7210_priv; diff --git a/drivers/staging/gpib/fmh_gpib/fmh_gpib.c b/drivers/staging/gpib/fmh_gpib/fmh_gpib.c index f950e7cdd8f8..211807c25a6b 100644 --- a/drivers/staging/gpib/fmh_gpib/fmh_gpib.c +++ b/drivers/staging/gpib/fmh_gpib/fmh_gpib.c @@ -255,7 +255,7 @@ static int fmh_gpib_line_status(const gpib_board_t *board) return status; } -static unsigned int fmh_gpib_t1_delay(gpib_board_t *board, unsigned int nano_sec) +static int fmh_gpib_t1_delay(gpib_board_t *board, unsigned int nano_sec) { struct fmh_priv *e_priv = board->private_data; struct nec7210_priv *nec_priv = &e_priv->nec7210_priv; diff --git a/drivers/staging/gpib/gpio/gpib_bitbang.c b/drivers/staging/gpib/gpio/gpib_bitbang.c index 828c99ea613f..23af35b96354 100644 --- a/drivers/staging/gpib/gpio/gpib_bitbang.c +++ b/drivers/staging/gpib/gpio/gpib_bitbang.c @@ -1012,7 +1012,7 @@ static uint8_t bb_serial_poll_status(gpib_board_t *board) return 0; // -ENOSYS; } -static unsigned int bb_t1_delay(gpib_board_t *board, unsigned int nano_sec) +static int bb_t1_delay(gpib_board_t *board, unsigned int nano_sec) { struct bb_priv *priv = board->private_data; diff --git a/drivers/staging/gpib/hp_82335/hp82335.c b/drivers/staging/gpib/hp_82335/hp82335.c index 451d5dc6d340..a1151ee8f373 100644 --- a/drivers/staging/gpib/hp_82335/hp82335.c +++ b/drivers/staging/gpib/hp_82335/hp82335.c @@ -161,7 +161,7 @@ static int hp82335_line_status(const gpib_board_t *board) return tms9914_line_status(board, &priv->tms9914_priv); } -static unsigned int hp82335_t1_delay(gpib_board_t *board, unsigned int nano_sec) +static int hp82335_t1_delay(gpib_board_t *board, unsigned int nano_sec) { struct hp82335_priv *priv = board->private_data; diff --git a/drivers/staging/gpib/hp_82341/hp_82341.c b/drivers/staging/gpib/hp_82341/hp_82341.c index 800f99c05566..aeca0013328f 100644 --- a/drivers/staging/gpib/hp_82341/hp_82341.c +++ b/drivers/staging/gpib/hp_82341/hp_82341.c @@ -397,7 +397,7 @@ static int hp_82341_line_status(const gpib_board_t *board) return tms9914_line_status(board, &priv->tms9914_priv); } -static unsigned int hp_82341_t1_delay(gpib_board_t *board, unsigned int nano_sec) +static int hp_82341_t1_delay(gpib_board_t *board, unsigned int nano_sec) { struct hp_82341_priv *priv = board->private_data; diff --git a/drivers/staging/gpib/include/gpib_types.h b/drivers/staging/gpib/include/gpib_types.h index b41781a55a60..5ee72683b2e0 100644 --- a/drivers/staging/gpib/include/gpib_types.h +++ b/drivers/staging/gpib/include/gpib_types.h @@ -170,7 +170,7 @@ struct gpib_interface_struct { */ uint8_t (*serial_poll_status)(gpib_board_t *board); /* adjust T1 delay */ - unsigned int (*t1_delay)(gpib_board_t *board, unsigned int nano_sec); + int (*t1_delay)(gpib_board_t *board, unsigned int nano_sec); /* go to local mode */ void (*return_to_local)(gpib_board_t *board); /* board does not support 7 bit eos comparisons */ diff --git a/drivers/staging/gpib/include/nec7210.h b/drivers/staging/gpib/include/nec7210.h index ca998c4a84bf..aa03c234aa60 100644 --- a/drivers/staging/gpib/include/nec7210.h +++ b/drivers/staging/gpib/include/nec7210.h @@ -108,7 +108,7 @@ void nec7210_parallel_poll_response(gpib_board_t *board, struct nec7210_priv *priv, int ist); uint8_t nec7210_serial_poll_status(gpib_board_t *board, struct nec7210_priv *priv); -unsigned int nec7210_t1_delay(gpib_board_t *board, +int nec7210_t1_delay(gpib_board_t *board, struct nec7210_priv *priv, unsigned int nano_sec); void nec7210_return_to_local(const gpib_board_t *board, struct nec7210_priv *priv); diff --git a/drivers/staging/gpib/ines/ines.h b/drivers/staging/gpib/ines/ines.h index 3918737fa21a..57c3a4f8a546 100644 --- a/drivers/staging/gpib/ines/ines.h +++ b/drivers/staging/gpib/ines/ines.h @@ -60,7 +60,7 @@ void ines_parallel_poll_response(gpib_board_t *board, int ist); void ines_serial_poll_response(gpib_board_t *board, uint8_t status); uint8_t ines_serial_poll_status(gpib_board_t *board); int ines_line_status(const gpib_board_t *board); -unsigned int ines_t1_delay(gpib_board_t *board, unsigned int nano_sec); +int ines_t1_delay(gpib_board_t *board, unsigned int nano_sec); void ines_return_to_local(gpib_board_t *board); // interrupt service routines diff --git a/drivers/staging/gpib/ines/ines_gpib.c b/drivers/staging/gpib/ines/ines_gpib.c index 22a05a287bce..2aac58ca7ed4 100644 --- a/drivers/staging/gpib/ines/ines_gpib.c +++ b/drivers/staging/gpib/ines/ines_gpib.c @@ -63,7 +63,7 @@ void ines_set_xfer_counter(struct ines_priv *priv, unsigned int count) ines_outb(priv, count & 0xff, XFER_COUNT_LOWER); } -unsigned int ines_t1_delay(gpib_board_t *board, unsigned int nano_sec) +int ines_t1_delay(gpib_board_t *board, unsigned int nano_sec) { struct ines_priv *ines_priv = board->private_data; struct nec7210_priv *nec_priv = &ines_priv->nec7210_priv; diff --git a/drivers/staging/gpib/lpvo_usb_gpib/lpvo_usb_gpib.c b/drivers/staging/gpib/lpvo_usb_gpib/lpvo_usb_gpib.c index 85322af62c23..84f9bde6f8d1 100644 --- a/drivers/staging/gpib/lpvo_usb_gpib/lpvo_usb_gpib.c +++ b/drivers/staging/gpib/lpvo_usb_gpib/lpvo_usb_gpib.c @@ -1120,8 +1120,7 @@ static uint8_t usb_gpib_serial_poll_status(gpib_board_t *board) } /* t1_delay */ - -static unsigned int usb_gpib_t1_delay(gpib_board_t *board, unsigned int nano_sec) +static int usb_gpib_t1_delay(gpib_board_t *board, unsigned int nano_sec) { dev_alert(board->gpib_dev, "%s:%s - currently a NOP\n", NAME, __func__); return 0; diff --git a/drivers/staging/gpib/nec7210/nec7210.c b/drivers/staging/gpib/nec7210/nec7210.c index c9a837fad96e..34ef2b1ff6e1 100644 --- a/drivers/staging/gpib/nec7210/nec7210.c +++ b/drivers/staging/gpib/nec7210/nec7210.c @@ -373,7 +373,7 @@ void nec7210_release_rfd_holdoff(gpib_board_t *board, struct nec7210_priv *priv) } EXPORT_SYMBOL(nec7210_release_rfd_holdoff); -unsigned int nec7210_t1_delay(gpib_board_t *board, struct nec7210_priv *priv, +int nec7210_t1_delay(gpib_board_t *board, struct nec7210_priv *priv, unsigned int nano_sec) { unsigned int retval; 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;
Dan Carpenter <dan.carpenter@linaro.org>Better to "return retval;"
} board->t1_nano_sec = actual_ns; ni_usb_soft_update_status(board, ibsta, 0);
Dan Carpenter <dan.carpenter@linaro.org>regards, dan carpenter
diff --git a/drivers/staging/gpib/pc2/pc2_gpib.c b/drivers/staging/gpib/pc2/pc2_gpib.c index 3eccd4c54afa..863217aea818 100644 --- a/drivers/staging/gpib/pc2/pc2_gpib.c +++ b/drivers/staging/gpib/pc2/pc2_gpib.c @@ -215,7 +215,7 @@ static uint8_t pc2_serial_poll_status(gpib_board_t *board) return nec7210_serial_poll_status(board, &priv->nec7210_priv); } -static unsigned int pc2_t1_delay(gpib_board_t *board, unsigned int nano_sec) +static int pc2_t1_delay(gpib_board_t *board, unsigned int nano_sec) { struct pc2_priv *priv = board->private_data; diff --git a/drivers/staging/gpib/tnt4882/tnt4882_gpib.c b/drivers/staging/gpib/tnt4882/tnt4882_gpib.c index 2e1c3cbebaca..924af43025ed 100644 --- a/drivers/staging/gpib/tnt4882/tnt4882_gpib.c +++ b/drivers/staging/gpib/tnt4882/tnt4882_gpib.c @@ -176,7 +176,7 @@ static int tnt4882_line_status(const gpib_board_t *board) return status; } -static unsigned int tnt4882_t1_delay(gpib_board_t *board, unsigned int nano_sec) +static int tnt4882_t1_delay(gpib_board_t *board, unsigned int nano_sec) { struct tnt4882_priv *tnt_priv = board->private_data; struct nec7210_priv *nec_priv = &tnt_priv->nec7210_priv; -- 2.47.0
due a change of the return type of t1_delay function, checkpatch was triggering a style error. Signed-off-by: Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com>
Dan Carpenter <dan.carpenter@linaro.org>This should be folded into the previous patch. Generally we avoid introducing checkpatch warnings and then fixing them later. Sometimes that ignoring checkpatch and then fixing it later can make the review easier if a patch is really massive but this change is only medium sized. regards, dan carpenter
--- drivers/staging/gpib/include/nec7210.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/gpib/include/nec7210.h b/drivers/staging/gpib/include/nec7210.h index aa03c234aa60..604085e3fe4f 100644 --- a/drivers/staging/gpib/include/nec7210.h +++ b/drivers/staging/gpib/include/nec7210.h @@ -109,7 +109,7 @@ void nec7210_parallel_poll_response(gpib_board_t *board, uint8_t nec7210_serial_poll_status(gpib_board_t *board, struct nec7210_priv *priv); int nec7210_t1_delay(gpib_board_t *board, - struct nec7210_priv *priv, unsigned int nano_sec); + struct nec7210_priv *priv, unsigned int nano_sec); void nec7210_return_to_local(const gpib_board_t *board, struct nec7210_priv *priv); // utility functions -- 2.47.0