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

[Dave Penkler <dpenkler@gmail.com>]

On Tue, Feb 25, 2025 at 11:59:56AM +0300, Dan Carpenter wrote:
> 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
Thanks Dan.
I will submit separate patches once Greg has merged a new baseline.
-dave