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::144; helo=mail-it1-x144.google.com; envelope-from=mw@semihalf.com; receiver=edk2-devel@lists.01.org Received: from mail-it1-x144.google.com (mail-it1-x144.google.com [IPv6:2607:f8b0:4864:20::144]) (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 7D300211B5A57 for ; Tue, 15 Jan 2019 02:05:23 -0800 (PST) Received: by mail-it1-x144.google.com with SMTP id p197so3630761itp.0 for ; Tue, 15 Jan 2019 02:05:23 -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=mrJFPGikCydBjlMLy3iLiQc3MyFUM5/4UM+rZMqjW6Y=; b=ln1/VqXltSp7YD5Io3pdjy49jgNZQtInlkS7YQDPMDDwucqwjGZ9A7r9mPrX4mzygd VrsSXHJgDHwmCKwA0X4ZYE7bGNqJ1w7zSEC//ZIYq4wdesHuY15OSL4gNvz8KEA7goUd zYbxjhY4/LNQNAV+orI/Qe/g1jjzQl6/OZR7Notu4VJwcGi24Evl+l4idqpgpQRhjJne mBZzPzPGu4RbScZBYeq0gMuARlYznadi6NFILej31VNZR+p3EliNf+JQ8KFB8L9l/hER inPnjeZYNNzZnhQ178IrYFyO69MCrAsTInwwvevmmV21LmlBoLPKzNAPl8cCKWxBfyqL /AtA== 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=mrJFPGikCydBjlMLy3iLiQc3MyFUM5/4UM+rZMqjW6Y=; b=K2cQuMzgiyhlGN+iXcGBLVlw2X+PuHLDeiN/l4Uhdt2ywKKhOjx4UYhonr8lWgXLQ8 tL3ytSxsvEOG2HRcFp7Ch/r6TsA9une22qv1XvS+F1lNgknOkINiRY+LJ+oPxRQ4/bxG OMSJ+jRR1F3FnOBXzOz47Uzb/3uZE0Jg/d+IaGbuT51rzUikpY1gExk8ITproGFsx8KJ FZ1xJsz6axEuA8E9I8m+3Dms51HS/1/1yQ4Drq2GgJesuc+BRkftDZZgq2ECh6iHHEMH MXsLnAms6MCqEfsgBTtpZW0Os274xo2hloK/S5RH0vUK81T5px9WCpEsVhc4fOSdKUAR qZZA== X-Gm-Message-State: AJcUukcVrRg8pjyaASD9br9ilAVb2mHS+feDUp9PMZM1VgCfYWUjWESl E3PQfxlFXAnBZAHlB9KyxiHprR25bVChvJE+wwKCZw== X-Google-Smtp-Source: ALg8bN6m48K2bEf77D7y2Gp74N7YcSUBNME2TC/eLcEvPTVTUGhvLwR15bQBY8vhAduhc+SY4q2XLPIvBOIdvwWjvTg= X-Received: by 2002:a02:5ec9:: with SMTP id h192mr1595082jab.112.1547546722268; Tue, 15 Jan 2019 02:05:22 -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> <20190115095612.v7qwnmttzptayhee@bivouac.eciton.net> In-Reply-To: <20190115095612.v7qwnmttzptayhee@bivouac.eciton.net> From: Marcin Wojtas Date: Tue, 15 Jan 2019 11:05:12 +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 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 X-List-Received-Date: Tue, 15 Jan 2019 10:05:23 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable wt., 15 sty 2019 o 10:56 Leif Lindholm napisa=C5= =82(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 =3D ArmadaSoCGpioGet (&SoCGpio, &SoCGpioCount); > > > > + if (EFI_ERROR (Status)) { > > > > + return Status; > > > > + } > > > > + > > > > + /* Get per-board information about all available I2C IO expander= s */ > > > > > > GPIO > > > > OK. > > > > > > > > > + Status =3D ArmadaBoardGpioExpanderGet (&GpioExpanders, &GpioExpa= nderCount); > > > > + if (EFI_ERROR (Status)) { > > > > + return Status; > > > > + } > > > > + > > > > + /* Allocate and fill board description */ > > > > + Description =3D AllocateZeroPool (sizeof (MV_BOARD_GPIO_DESCRIPT= ION)); > > > > > > Instead, this space is now dynamically allocated. But none of the cal= l > > > 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 =3D=3D NULL) { > > > > + DEBUG ((DEBUG_ERROR, "%a: Cannot allocate memory\n", __FUNCTIO= N__)); > > > > + 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 MvBoardGpioDescription= Get; > > > > > > > > return EFI_SUCCESS; > > > > } > > > > -- > > > > 2.7.4 > > > >