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
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
