Re: [PATCH] staging: gpib: Add return value to request_control

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

On Sat, Feb 22, 2025 at 09:23:01PM +0100, Dave Penkler wrote:
> A number of drivers are unable to release control due to
> hardware or software limitations. As request_control was defined
> as void - no error could be signalled. Some drivers also did
> not implement request_control correctly by setting
> controller_in_charge instead of system_controller.
> 
> This patch changes the prototype of request_control to int
> and adds the appropriate checking and returns. In the case
> that a board cannot release control EPERM is returned. The
> faulty implementations have been corrected.
> 

This patch is hard to read because it does several things:
1) It changes the functions from returning void to int.
   This is the overwhelming noisiest part of the patch so
   it's hard to even see the other changes in amongst the
   noise.
2) Returns -EPERM if request_control is false.
3) Changes some stuff like SET_DIR_WRITE(priv); and
   ENABLE_IRQ(priv->irq_SRQ, IRQ_TYPE_EDGE_FALLING);  I can't tell if
   that's related or not.

You'll need to do it in two or three patches.  The first thing is to
just change the void to int.  That's a simple mechanical change.  The
only worry is if some functions are returning an error code on failure
and I don't know the answer to that.  (That would break git bisect so it
would be against the rules, even if it's fixed in patch 2 and 3).

The actual logic changes will hopefully be easier to understand when the
diff is smaller.

> -static void agilent_82350b_request_system_control(gpib_board_t *board, int request_control)
> +static int agilent_82350b_request_system_control(gpib_board_t *board, int request_control)
>  
>  {
>  	struct agilent_82350b_priv *a_priv = board->private_data;
> +	int retval;
>  
>  	if (request_control) {
>  		a_priv->card_mode_bits |= CM_SYSTEM_CONTROLLER_BIT;
> @@ -354,7 +355,9 @@ static void agilent_82350b_request_system_control(gpib_board_t *board, int reque
>  			writeb(0, a_priv->gpib_base + INTERNAL_CONFIG_REG);
>  	}
>  	writeb(a_priv->card_mode_bits, a_priv->gpib_base + CARD_MODE_REG);
> -	tms9914_request_system_control(board, &a_priv->tms9914_priv, request_control);
> +	retval = tms9914_request_system_control(board, &a_priv->tms9914_priv, request_control);
> +
> +	return retval;

Get rid of the retval variable.  This should be:

	return tms9914_request_system_control(board, &a_priv->tms9914_priv, request_control);

> diff --git a/drivers/staging/gpib/common/iblib.c b/drivers/staging/gpib/common/iblib.c
> index fd2874c2fff4..3d51a093fc8b 100644
> --- a/drivers/staging/gpib/common/iblib.c
> +++ b/drivers/staging/gpib/common/iblib.c
> @@ -418,12 +418,19 @@ int ibsic(gpib_board_t *board, unsigned int usec_duration)
>  	return 0;
>  }
>  
> -	/* FIXME make int */
> -void ibrsc(gpib_board_t *board, int request_control)
> +int ibrsc(gpib_board_t *board, int request_control)
>  {
> -	board->master = request_control != 0;
> +	int retval;
> +
>  	if (board->interface->request_system_control)
> -		board->interface->request_system_control(board, request_control);
> +		retval = board->interface->request_system_control(board, request_control);
> +	else
> +		retval = -EPERM;
> +
> +	if (!retval)
> +		board->master = request_control != 0;
> +
> +	return retval;
>  }
>  

It would be better to reverse some of these conditions are return earlier
where it's possible.  (Always do failure handling, not success handling).

int ibrsc(gpib_board_t *board, int request_control)
{
        int ret;

        if (!board->interface->request_system_control)
                return -EPERM;
        ret = board->interface->request_system_control(board, request_control);
        if (ret)
                return ret;

        board->master = !request_control;

        return 0;
}

regards,
dan carpenter