Hi Leif,

pon., 14 sty 2019 o 23:58 Leif Lindholm <leif.lindh...@linaro.org> napisaƂ(a):
>
> On Thu, Jan 10, 2019 at 02:44:33AM +0100, Marcin Wojtas wrote:
> > Introduce new callback that can provide information
> > about GPIO SoC controllers, as well as on-board
> > I2C IO expanders. According ArmadaSoCDescLib
> > ArmadaBoardDescLib routines are used for
> > obtaining required data.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <m...@semihalf.com>
> > ---
> >  Silicon/Marvell/Include/Protocol/BoardDesc.h       |  8 ++++
> >  Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c | 43 
> > ++++++++++++++++++++
> >  2 files changed, 51 insertions(+)
> >
> > diff --git a/Silicon/Marvell/Include/Protocol/BoardDesc.h 
> > b/Silicon/Marvell/Include/Protocol/BoardDesc.h
> > index 1d57a16..02905ea 100644
> > --- a/Silicon/Marvell/Include/Protocol/BoardDesc.h
> > +++ b/Silicon/Marvell/Include/Protocol/BoardDesc.h
> > @@ -50,6 +50,13 @@ EFI_STATUS
> >
> >  typedef
> >  EFI_STATUS
> > +(EFIAPI *MV_BOARD_GPIO_DESCRIPTION_GET) (
> > +  IN MARVELL_BOARD_DESC_PROTOCOL    *This,
> > +  IN OUT MV_BOARD_GPIO_DESCRIPTION **GpioDescription
> > +  );
> > +
> > +typedef
> > +EFI_STATUS
> >  (EFIAPI *MV_BOARD_DESC_I2C_GET) (
> >    IN MARVELL_BOARD_DESC_PROTOCOL  *This,
> >    IN OUT MV_BOARD_I2C_DESC       **I2cDesc
> > @@ -113,6 +120,7 @@ struct _MARVELL_BOARD_DESC_PROTOCOL {
> >    MV_BOARD_DESC_UTMI_GET         BoardDescUtmiGet;
> >    MV_BOARD_DESC_XHCI_GET         BoardDescXhciGet;
> >    MV_BOARD_DESC_FREE             BoardDescFree;
> > +  MV_BOARD_GPIO_DESCRIPTION_GET  GpioDescriptionGet;
> >  };
> >
> >  #endif // __MARVELL_BOARD_DESC_PROTOCOL_H__
> > diff --git a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c 
> > b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c
> > 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:/

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.

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