From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 2DFB8C433F5 for ; Mon, 24 Jan 2022 07:38:42 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id C4BF180615; Mon, 24 Jan 2022 08:38:40 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="bNstkuDV"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 855E28069E; Mon, 24 Jan 2022 08:38:39 +0100 (CET) Received: from mail-pf1-x434.google.com (mail-pf1-x434.google.com [IPv6:2607:f8b0:4864:20::434]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 5E352802DF for ; Mon, 24 Jan 2022 08:38:36 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=takahiro.akashi@linaro.org Received: by mail-pf1-x434.google.com with SMTP id a8so9773551pfa.6 for ; Sun, 23 Jan 2022 23:38:36 -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:mail-followup-to:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=bkDJDnQ//9YLjSRZc4gvGhiMod5u9IDhOUAA3wpYqNA=; b=bNstkuDVhDYSlpl64stliZeb3XTExE58QXrSPj9FJXGVSseYNHo1IUUv2kCbBIL/eZ kfg7g9T7OGCZqAvWm2stVwp0Rc7k16lO0EKHTXQa3OkyInom3Bk2w1MdZEEKdQg9HuKW 3gFJ/NFJLdiSZ2vXoMYP/HiypK3ed42e0+XMsvtlzysyQ2UVQAPpIKFjOVvC9Xo+n0gt 0U6gbD5Zap9147RbAYyjJd8oRPUYf3AVxSycx178SzZFSjgmQwcv38z5QH9r91EDUrQP lj6wCkuJmtKw1am2VQzkzimmvVYexbJ84+Kc7q+LtV4agzk8M94gu7Y3GTycXdTpNLoP GEsQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :content-transfer-encoding:in-reply-to; bh=bkDJDnQ//9YLjSRZc4gvGhiMod5u9IDhOUAA3wpYqNA=; b=L20n0GIjd1ZqeQH+3QXzcH+qgNVexz35nwuZcoD9YewSzMyw+lUGDfBfK4EvZzT7nD 8ZUTSqGNWW4iD5xbM16IqIa3pPbIn1frfXwNq10LvmMHWOBcdZlHUYV1HAhUO1dz+syF hxFH9jUr4ZxyT7bzWdtHSXUS3a6Rl3exnww/mXgAPre5Rck0MfFXcJX2i3zSGvlAk6qX rU+IBzuzDM4ljzFJcRIH9wHwzq8Kj00dpHd1xWIWU2iz9ooJYFWlCTnp/inRZTqYIP6a N55RIv+JMHlQ93wr2q5hcm2sV0VqehQU9uXs4P1bFEapnD/67EwWmNLx1xTCWdnYfxh1 AB2w== X-Gm-Message-State: AOAM532zpP0kWjP/HuuQHzEIiPIh7M0/v3WAniuKeIfK9tU5KUM2CxLV L6rx3UY+M9V1CEMPDzHLTVE2mA== X-Google-Smtp-Source: ABdhPJzGeYGEola8n3W7bCF78HG9bzgtVUuU/NVWrCJhtRfsk2jh8hOjgHvVo9rCTx4QIZ+gYoEGMw== X-Received: by 2002:a63:6906:: with SMTP id e6mr10908040pgc.170.1643009914636; Sun, 23 Jan 2022 23:38:34 -0800 (PST) Received: from laputa (p914133-ipoe.ipoe.ocn.ne.jp. [153.243.15.132]) by smtp.gmail.com with ESMTPSA id b12sm15379945pfv.148.2022.01.23.23.38.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 23 Jan 2022 23:38:34 -0800 (PST) Date: Mon, 24 Jan 2022 16:38:18 +0900 From: AKASHI Takahiro To: Sughosh Ganu Cc: Masami Hiramatsu , u-boot@lists.denx.de, Patrick Delaunay , Patrice Chotard , Heinrich Schuchardt , Alexander Graf , Simon Glass , Bin Meng , Ilias Apalodimas , Jose Marinho , Grant Likely , Tom Rini , Etienne Carriere Subject: Re: [RFC PATCH v3 2/9] FWU: Add FWU metadata access functions for GPT partitioned block devices Message-ID: <20220124073818.GD48616@laputa> Mail-Followup-To: AKASHI Takahiro , Sughosh Ganu , Masami Hiramatsu , u-boot@lists.denx.de, Patrick Delaunay , Patrice Chotard , Heinrich Schuchardt , Alexander Graf , Simon Glass , Bin Meng , Ilias Apalodimas , Jose Marinho , Grant Likely , Tom Rini , Etienne Carriere References: <20220119185548.16730-1-sughosh.ganu@linaro.org> <20220119185548.16730-3-sughosh.ganu@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean On Mon, Jan 24, 2022 at 12:28:24PM +0530, Sughosh Ganu wrote: > hi Masami, > > On Thu, 20 Jan 2022 at 14:13, Masami Hiramatsu > wrote: > > > > Hi Sughosh, > > > > 2022年1月20日(木) 3:56 Sughosh Ganu : > > > > > +static int fwu_gpt_update_mdata(struct fwu_mdata *mdata) > > > +{ > > > + int ret; > > > + struct blk_desc *desc; > > > + u16 primary_mpart = 0, secondary_mpart = 0; > > > + > > > > I think this update_mdata() method (or fwu_update_mdata() wrapper) > > should always update mdata::crc32. calculate crc32 at each call site is > > inefficient and easy to introduce bugs. > > Okay. Will do. > > > > > > + ret = fwu_plat_get_blk_desc(&desc); > > > + if (ret < 0) { > > > + log_err("Block device not found\n"); > > > + return -ENODEV; > > > + } > > > + > > > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > > > + &secondary_mpart); > > > + > > > + if (ret < 0) { > > > + log_err("Error getting the FWU metadata partitions\n"); > > > + return -ENODEV; > > > + } > > > + > > > + /* First write the primary partition*/ > > > + ret = gpt_write_mdata_partition(desc, mdata, primary_mpart); > > > + if (ret < 0) { > > > + log_err("Updating primary FWU metadata partition failed\n"); > > > + return ret; > > > + } > > > + > > > + /* And now the replica */ > > > + ret = gpt_write_mdata_partition(desc, mdata, secondary_mpart); > > > + if (ret < 0) { > > > + log_err("Updating secondary FWU metadata partition failed\n"); > > > + return ret; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static int gpt_get_mdata(struct fwu_mdata **mdata) > > > +{ > > > + int ret; > > > + struct blk_desc *desc; > > > + u16 primary_mpart = 0, secondary_mpart = 0; > > > + > > > + ret = fwu_plat_get_blk_desc(&desc); > > > + if (ret < 0) { > > > + log_err("Block device not found\n"); > > > + return -ENODEV; > > > + } > > > + > > > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > > > + &secondary_mpart); > > > + > > > + if (ret < 0) { > > > + log_err("Error getting the FWU metadata partitions\n"); > > > + return -ENODEV; > > > + } > > > + > > > + *mdata = malloc(sizeof(struct fwu_mdata)); > > > + if (!*mdata) { > > > + log_err("Unable to allocate memory for reading FWU metadata\n"); > > > + return -ENOMEM; > > > + } > > > + > > > + ret = gpt_read_mdata(desc, *mdata, primary_mpart); > > > + if (ret < 0) { > > > + log_err("Failed to read the FWU metadata from the device\n"); > > > > Also, please release mdata inside the gpt_get_mdata() itself. > > > > I think it is not a good design to ask caller to free mdata if get_mdata() > > operation is failed because mdata may or may not allocated in error case. > > > > In success case, user must free it because it is allocated (user accessed it), > > and in error case, user can ignore it because it should not be allocated. > > This is simpler mind model and less memory leak chance. > > I think this is confusing. It would be better to be consistent and > have the caller free up the allocated/not allocated memory. If you > check, the mdata pointer is being initialised to NULL in every > instance. Calling free with a NULL pointer is a valid case, which the > free call handles. There are multiple places in u-boot where the > caller is freeing up the allocated memory. So i think this should not > be an issue. The simple rule should be that, if the function returns 0 (successfully), the area will be allocated. If not (i.e. returns an error), the area will not be allocated. This will be a much more common behavior. -Takahiro Akashi > -sughosh > > > > > Thank you, > > -- > > Masami Hiramatsu