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
