[PATCH 1/2] Staging: common: iblib: Changed ibrsc function type

[PATCH 1/2] Staging: common: iblib: Changed ibrsc function type

[Thomas Andreatta]

Function ibrsc type changed from void to int.
Returning 0 for success.

Signed-off-by: Thomas Andreatta <thomas.andreatta2000@gmail.com>
---
 drivers/staging/gpib/common/iblib.c       | 4 ++--
 drivers/staging/gpib/include/gpib_proto.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/gpib/common/iblib.c b/drivers/staging/gpib/common/iblib.c
index b297261818f2..8b4280309ae7 100644
--- a/drivers/staging/gpib/common/iblib.c
+++ b/drivers/staging/gpib/common/iblib.c
@@ -418,12 +418,12 @@ int ibsic(struct gpib_board *board, unsigned int usec_duration)
 	return 0;
 }
 
-	/* FIXME make int */
-void ibrsc(struct gpib_board *board, int request_control)
+int ibrsc(struct gpib_board *board, int request_control)
 {
 	board->master = request_control != 0;
 	if (board->interface->request_system_control)
 		board->interface->request_system_control(board, request_control);
+	return 0;
 }
 
 /*
diff --git a/drivers/staging/gpib/include/gpib_proto.h b/drivers/staging/gpib/include/gpib_proto.h
index 2c7dfc02f517..9ef1cc247289 100644
--- a/drivers/staging/gpib/include/gpib_proto.h
+++ b/drivers/staging/gpib/include/gpib_proto.h
@@ -38,7 +38,7 @@ int iblines(const struct gpib_board *board, short *lines);
 int ibrd(struct gpib_board *board, uint8_t *buf, size_t length, int *end_flag, size_t *bytes_read);
 int ibrpp(struct gpib_board *board, uint8_t *buf);
 int ibrsv2(struct gpib_board *board, uint8_t status_byte, int new_reason_for_service);
-void ibrsc(struct gpib_board *board, int request_control);
+int ibrsc(struct gpib_board *board, int request_control);
 int ibsic(struct gpib_board *board, unsigned int usec_duration);
 int ibsre(struct gpib_board *board, int enable);
 int ibpad(struct gpib_board *board, unsigned int addr);
-- 
2.34.1

[PATCH 2/2] Staging: common: gpib_os: `request_system_control_ioctl` return value

[Thomas Andreatta]

`request_system_control_ioctl` now returns the value returned from the
function `ibrsc`, now int.

Signed-off-by: Thomas Andreatta <thomas.andreatta2000@gmail.com>
---
 drivers/staging/gpib/common/gpib_os.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/gpib/common/gpib_os.c b/drivers/staging/gpib/common/gpib_os.c
index 8456b97290b8..dddf7a70084d 100644
--- a/drivers/staging/gpib/common/gpib_os.c
+++ b/drivers/staging/gpib/common/gpib_os.c
@@ -1970,9 +1970,9 @@ static int request_system_control_ioctl(struct gpib_board *board, unsigned long
 	if (retval)
 		return -EFAULT;
 
-	ibrsc(board, request_control);
+	retval = ibrsc(board, request_control);
 
-	return 0;
+	return retval;
 }
 
 static int t1_delay_ioctl(struct gpib_board *board, unsigned long arg)
-- 
2.34.1

Re: [PATCH 2/2] Staging: common: gpib_os: `request_system_control_ioctl` return value

[Greg KH]

On Sat, May 10, 2025 at 05:38:00PM +0200, Thomas Andreatta wrote:
> `request_system_control_ioctl` now returns the value returned from the
> function `ibrsc`, now int.
> 
> Signed-off-by: Thomas Andreatta <thomas.andreatta2000@gmail.com>
> ---
>  drivers/staging/gpib/common/gpib_os.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/gpib/common/gpib_os.c b/drivers/staging/gpib/common/gpib_os.c
> index 8456b97290b8..dddf7a70084d 100644
> --- a/drivers/staging/gpib/common/gpib_os.c
> +++ b/drivers/staging/gpib/common/gpib_os.c
> @@ -1970,9 +1970,9 @@ static int request_system_control_ioctl(struct gpib_board *board, unsigned long
>  	if (retval)
>  		return -EFAULT;
>  
> -	ibrsc(board, request_control);
> +	retval = ibrsc(board, request_control);
>  
> -	return 0;
> +	return retval;

Why not:
	return ibrsc(board, request_control);
?

But again, as this can not fail, why is this needed?

Start way back at the "ends" of the call chain.  If they can fail, then
start propagating the error up all the way to here.  As it is, your
patch series really didn't do anything that I can tell, sorry.

greg k-h

Re: [PATCH 2/2] Staging: common: gpib_os: simplify return value handling

[Thomas Andreatta]

> But again, as this can not fail, why is this needed?

Agree, should've checked it before trying to solve the "FIXME".

> Start way back at the "ends" of the call chain.  If they can fail, then
start propagating the error up all the way to here.

Sounds logic, will add it to my check-list from now on, thanks!

Best,
Thomas

Re: [PATCH 1/2] Staging: common: iblib: Changed ibrsc function type

[Greg KH]

On Sat, May 10, 2025 at 05:37:59PM +0200, Thomas Andreatta wrote:
> Function ibrsc type changed from void to int.

You need to explain more here.

> Returning 0 for success.

Yes, but it can not fail:

> 
> Signed-off-by: Thomas Andreatta <thomas.andreatta2000@gmail.com>
> ---
>  drivers/staging/gpib/common/iblib.c       | 4 ++--
>  drivers/staging/gpib/include/gpib_proto.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/gpib/common/iblib.c b/drivers/staging/gpib/common/iblib.c
> index b297261818f2..8b4280309ae7 100644
> --- a/drivers/staging/gpib/common/iblib.c
> +++ b/drivers/staging/gpib/common/iblib.c
> @@ -418,12 +418,12 @@ int ibsic(struct gpib_board *board, unsigned int usec_duration)
>  	return 0;
>  }
>  
> -	/* FIXME make int */
> -void ibrsc(struct gpib_board *board, int request_control)
> +int ibrsc(struct gpib_board *board, int request_control)
>  {
>  	board->master = request_control != 0;
>  	if (board->interface->request_system_control)
>  		board->interface->request_system_control(board, request_control);
> +	return 0;

If this can not fail, and request_system_control() can not fail, why
does this function need to return anything?

And you aren't checking the return value of it either, so why change it?

On another note, it's not a good name for a global function :)

thanks,

greg k-h

Re: [PATCH 1/2] Staging: common: iblib: Changed ibrsc function type

[Dan Carpenter]

On Sat, May 10, 2025 at 05:37:59PM +0200, Thomas Andreatta wrote:
> Function ibrsc type changed from void to int.
> Returning 0 for success.
> 
> Signed-off-by: Thomas Andreatta <thomas.andreatta2000@gmail.com>

You need to work against linux-next.  Also you need to explain *why*
a patch is needed.

regards,
dan carpenter