From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: None (no SPF record) identity=mailfrom; client-ip=2607:f8b0:4864:20::142; helo=mail-it1-x142.google.com; envelope-from=mw@semihalf.com; receiver=edk2-devel@lists.01.org Received: from mail-it1-x142.google.com (mail-it1-x142.google.com [IPv6:2607:f8b0:4864:20::142]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 67FBA211B63DE for ; Mon, 14 Jan 2019 22:05:52 -0800 (PST) Received: by mail-it1-x142.google.com with SMTP id z7so3418012iti.0 for ; Mon, 14 Jan 2019 22:05:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=lNb4zDM7nzNyXw38SAPucpYa/k9naP/soCBdtQbiBk8=; b=1LQxpFi1ZmZxtt5MYwLbwSg5fcuzAOWKE/bbyzQoGfx5LiR7aCP1gOdtXWOd6xZB9V ywIyf1G52ujGmhAEhCOVYwCy9FZW1aPd/4t0N8NW3jziR4e9rTc+kibskbF/eqdCgoEs Rc98v+Dpya4YsKk5mjK0r5tUpUNtvug5Em9KlvgPPpq/TjgIjMnoPtJoA4BvWIIYo2+l DNguo66MOqPbcydHzvjCDGU+SA2oJ0zC7tqUVZxoF+S1J6kwWcvm6Y3W7grpBVB2R/ma dETKFcjy3w2oFkZIYN8im2wLvLtPVKxv1r7i2GmSSmWHQwq++gKp2/qQmokCBeLJMnsM ES6A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=lNb4zDM7nzNyXw38SAPucpYa/k9naP/soCBdtQbiBk8=; b=iK19uAvGSlnIjNcg7ivms4pOKcfOAxjE7I50fE4iqBAYun+G8hAgnV7uw9wRiBV97a tkdiiddSP4ZLozrLV5AKuFPGuc8AeBgmQpiJAxv/E/bn217zpla3RTy//nEhnPAB86ds tVfRFPMKkZn5zf7XaQ1tyB2OCDn9L4+NvfM+xgSv946geClLtNNAeAF96n3sK+U/iv/+ QPLgScAVbRI9RD4CDmCsfaJ5nxNqxvEZl2477IN4FBZNTAWbsFz5dAvmGTQYuOXjPE3A iN8F11N191aIT9aDREwgV3XD9gH59YrAbxd+3/gXh0BSVDWpMBCxUv097gmhhwwcmU9q SAyA== X-Gm-Message-State: AJcUukegpGjmRRL2EE6lrUwD43hm5pJI+3MW+b0rs3zrdA3at5TNGsW0 aA7gqCG5vOVgQGuCKJ01IL354moXVnmwgBnKv/2RUg== X-Google-Smtp-Source: ALg8bN5P+gZYwDSUPpsBm38D7+1pX0SF7Zuf2JZMljKmgGE0mCtt6C8aYYMuoZv5//F/vlu7EIAzjC+uhlzS62wF6Do= X-Received: by 2002:a02:5ec9:: with SMTP id h192mr1198484jab.112.1547532351588; Mon, 14 Jan 2019 22:05:51 -0800 (PST) MIME-Version: 1.0 References: <1547084679-29597-1-git-send-email-mw@semihalf.com> <1547084679-29597-7-git-send-email-mw@semihalf.com> <20190114225845.3cnoerfpbbclsmdj@bivouac.eciton.net> In-Reply-To: <20190114225845.3cnoerfpbbclsmdj@bivouac.eciton.net> From: Marcin Wojtas Date: Tue, 15 Jan 2019 07:05:42 +0100 Message-ID: To: Leif Lindholm Cc: edk2-devel-01 , Ard Biesheuvel , nadavh@marvell.com, "jsd@semihalf.com" , Grzegorz Jaszczyk , Kostya Porotchkin Subject: Re: [platforms: PATCH v2 06/12] Marvell/Drivers: MvBoardDesc: Extend protocol with GPIO support X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 X-List-Received-Date: Tue, 15 Jan 2019 06:05:52 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Leif, pon., 14 sty 2019 o 23:58 Leif Lindholm napisa= =C5=82(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 > > --- > > 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/Mar= vell/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/Silic= on/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 =3D ArmadaSoCGpioGet (&SoCGpio, &SoCGpioCount); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > + > > + /* Get per-board information about all available I2C IO expanders */ > > GPIO OK. > > > + Status =3D ArmadaBoardGpioExpanderGet (&GpioExpanders, &GpioExpander= Count); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > + > > + /* Allocate and fill board description */ > > + Description =3D 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 =3D=3D NULL) { > > + DEBUG ((DEBUG_ERROR, "%a: Cannot allocate memory\n", __FUNCTION__)= ); > > + return EFI_OUT_OF_RESOURCES; > > + } > > + > > + Description->SoCGpio =3D SoCGpio; > > + Description->GpioDeviceCount =3D SoCGpioCount; > > + Description->GpioExpanders =3D GpioExpanders; > > + Description->GpioExpanderCount =3D GpioExpanderCount; > > + > > + *GpioDescription =3D 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 =3D MvBoardDescUtmiGet; > > BoardDescProtocol->BoardDescXhciGet =3D MvBoardDescXhciGet; > > BoardDescProtocol->BoardDescFree =3D MvBoardDescFree; > > + BoardDescProtocol->GpioDescriptionGet =3D MvBoardGpioDescriptionGet; > > > > return EFI_SUCCESS; > > } > > -- > > 2.7.4 > >