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)?

/
    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
> > >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to