From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::341; helo=mail-wm1-x341.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm1-x341.google.com (mail-wm1-x341.google.com [IPv6:2a00:1450:4864:20::341]) (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 CA73A203B99BB for ; Tue, 15 Jan 2019 01:56:16 -0800 (PST) Received: by mail-wm1-x341.google.com with SMTP id a62so2594198wmh.4 for ; Tue, 15 Jan 2019 01:56:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=E/Ei22QdICNbr7xeGIZ29gG8Ox1Yg7THVhAzNarjAOM=; b=Bg7a/0HKizeXNwcNUJWBZiZOpn0Z/Lp+5RYmqLxdwC7mMOhl7KWUdUQXqWhPFCSFE0 C7uZoiJVj4dGdcqzYtJtkb4p47nkxWvEtqpTscao2p3+jnlYS9894OCcno9sQnnt2+6v RGCi1XJNWdmNI1yk7fePJbcanzgiUXhg8YNK4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=E/Ei22QdICNbr7xeGIZ29gG8Ox1Yg7THVhAzNarjAOM=; b=VZ33ju+7t3huHH5Eic+YfZ8UJLTWM2qUl7kOUgH4KP6w/HLqyBWxWlxBJzgMUIiy59 Rct9214MnMcO4lm28krSDfoQ42l+87zUx/na8NsvHGEEZ52pXflBeG30bMfrtRl5WTOE 0HUgtr1BDM0ylagGwcMgTStPwnqDQq3ecgu7E1Il8kx37wcUu2sJDPiXosNfD3oVF1RO QheT31CJuYFyT2/vJJNAri0tdyW2BJRTwewrrVyxINTilJTQeW8yDQEBGXRe/lG35lb1 yIEQ72ce1rWq86xn933L4CgOZGR/foO/PNiAIuLjM5jGpN7dx+ymPOB97gu2rFTWu8vl dMJw== X-Gm-Message-State: AJcUukd0GlcRNeIO7EdIDnmU28tgQ8p/a1V323ovNwM7wFeN6sAGMPRQ sS8EWmN8UljjgueSjHqBmBrJY7WFSmc= X-Google-Smtp-Source: ALg8bN65OHH/OR+RSDjnhycMTkLTntDkaM9aZqBGECFtn05e/hw7NGhjf0T/S4v0ttH+L+mx4pU64w== X-Received: by 2002:a1c:990c:: with SMTP id b12mr2768324wme.106.1547546175137; Tue, 15 Jan 2019 01:56:15 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id e17sm127995243wri.36.2019.01.15.01.56.14 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 15 Jan 2019 01:56:14 -0800 (PST) Date: Tue, 15 Jan 2019 09:56:12 +0000 From: Leif Lindholm To: Marcin Wojtas Cc: edk2-devel-01 , Ard Biesheuvel , nadavh@marvell.com, "jsd@semihalf.com" , Grzegorz Jaszczyk , Kostya Porotchkin Message-ID: <20190115095612.v7qwnmttzptayhee@bivouac.eciton.net> References: <1547084679-29597-1-git-send-email-mw@semihalf.com> <1547084679-29597-7-git-send-email-mw@semihalf.com> <20190114225845.3cnoerfpbbclsmdj@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) 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 09:56:17 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > > >