wt., 15 sty 2019 o 10:56 Leif Lindholm <leif.lindh...@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
> > > >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
