Re: [platforms: PATCH v2 06/12] Marvell/Drivers: MvBoardDesc: Extend protocol with GPIO support

[Marcin Wojtas <mw@semihalf.com>]

wt., 15 sty 2019 o 10:56 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
>
> On Tue, Jan 15, 2019 at 07:05:42AM +0100, Marcin Wojtas wrote:
> > > > index f71bfc4..e348b85 100644
> > > > --- a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c
> > > > +++ b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c
> > > > @@ -100,6 +100,48 @@ MvBoardDescComPhyGet (
> > > >
> > > >  STATIC
> > > >  EFI_STATUS
> > > > +MvBoardGpioDescriptionGet (
> > > > +  IN MARVELL_BOARD_DESC_PROTOCOL    *This,
> > > > +  IN OUT MV_BOARD_GPIO_DESCRIPTION **GpioDescription
> > > > +  )
> > > > +{
> > > > +  MV_BOARD_GPIO_DESCRIPTION *Description;
> > >
> > > My request on v1 was that this be refactored from a STATIC local
> > > variable to a global variable.
> > >
> > > > +  UINTN SoCGpioCount, GpioExpanderCount;
> > > > +  MV_GPIO_EXPANDER *GpioExpanders;
> > > > +  GPIO_CONTROLLER *SoCGpio;
> > > > +  EFI_STATUS Status;
> > > > +
> > > > +  /* Get SoC data about all available GPIO controllers */
> > > > +  Status = ArmadaSoCGpioGet (&SoCGpio, &SoCGpioCount);
> > > > +  if (EFI_ERROR (Status)) {
> > > > +    return Status;
> > > > +  }
> > > > +
> > > > +  /* Get per-board information about all available I2C IO expanders */
> > >
> > > GPIO
> >
> > OK.
> >
> > >
> > > > +  Status = ArmadaBoardGpioExpanderGet (&GpioExpanders, &GpioExpanderCount);
> > > > +  if (EFI_ERROR (Status)) {
> > > > +    return Status;
> > > > +  }
> > > > +
> > > > +  /* Allocate and fill board description */
> > > > +  Description = AllocateZeroPool (sizeof (MV_BOARD_GPIO_DESCRIPTION));
> > >
> > > Instead, this space is now dynamically allocated. But none of the call
> > > sites actually change, leading to potential memory leaks.
> > >
> > > I don't have a problem if you prefer an alternative solution to the
> > > one I propose, but please discuss the change first rather than
> > > submitting a new revision containing something I didn't ask for.
> >
> > Indeed, I could have asked, but didn't want to spoil your Christmas
> > time with questions, especially gitven it was 3+ weeks from review:/
>
> It's not like I was checking email :)
>
> > Anyway, I tried to play with the MV_BOARD_GPIO_DESCRIPTION to be the
> > global variable, but was not convinced by the outcome. My biggest
> > objection to the global variable and checking whether it's NULL in the
> > consumer driver is following - until now all users of the
> > BoardDescProtocol 'know' and call only the relevant protocol routine,
> > everything what happens underneath (PCDs, variable names, etc) is
> > transparent.
> >
> > The consumers should be responsible for avoiding the memory leaks and
> > this was an improvement of v2. Both MvGpioDxe and MvPca9xxxDxe have
> > now fixed error path an properly take care of freeing the memory after
> > using protocol.
>
> But the call sites don't keep track of freeing it when error handling.
>
> Which I think serves as a clear demonstrator of how magically
> allocating buffers makes for difficult code to keep correct. (Which is
> why the UEFI intefaces all require you to allocate a buffer and then
> pass that and a size as parameters.)
>
> So, since we are dealing with data that isn't changing, I prefer the
> original design - just not the original implemenation.
>
> So how about sticking with that, but moving the STATIC struct global
> in that source file (but keeping it STATIC)?
>

Good, keeping the global variable inside MvBoardDescDxe and checking
it there is a clean and easy solution (consumers won't have to bother
about the stuff additional to calling the protocol and error handling
will be simpler).

How about on top of the file I add a section for global varibles (IMO
it's worth to modify other interfaces to that scheme later) and call
it:

STATIC MV_BOARD_GPIO_DESCRIPTION gGpioDescription;
?

Thanks,
Marcin

> /
>     Leif
>
> > Please let know if above approach is acceptable.
> >
> > Best regards,
> > Marcin
> >
> > > /
> > >     Leif
> > >
> > > > +  if (Description == NULL) {
> > > > +    DEBUG ((DEBUG_ERROR, "%a: Cannot allocate memory\n", __FUNCTION__));
> > > > +    return EFI_OUT_OF_RESOURCES;
> > > > +  }
> > > > +
> > > > +  Description->SoCGpio = SoCGpio;
> > > > +  Description->GpioDeviceCount = SoCGpioCount;
> > > > +  Description->GpioExpanders = GpioExpanders;
> > > > +  Description->GpioExpanderCount = GpioExpanderCount;
> > > > +
> > > > +  *GpioDescription = Description;
> > > > +
> > > > +  return EFI_SUCCESS;
> > > > +}
> > > > +
> > > > +STATIC
> > > > +EFI_STATUS
> > > >  MvBoardDescI2cGet (
> > > >    IN MARVELL_BOARD_DESC_PROTOCOL  *This,
> > > >    IN OUT MV_BOARD_I2C_DESC       **I2cDesc
> > > > @@ -571,6 +613,7 @@ MvBoardDescInitProtocol (
> > > >    BoardDescProtocol->BoardDescUtmiGet = MvBoardDescUtmiGet;
> > > >    BoardDescProtocol->BoardDescXhciGet = MvBoardDescXhciGet;
> > > >    BoardDescProtocol->BoardDescFree = MvBoardDescFree;
> > > > +  BoardDescProtocol->GpioDescriptionGet = MvBoardGpioDescriptionGet;
> > > >
> > > >    return EFI_SUCCESS;
> > > >  }
> > > > --
> > > > 2.7.4
> > > >