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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 91575C433F5 for ; Wed, 20 Oct 2021 12:15:37 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id E847961359 for ; Wed, 20 Oct 2021 12:15:36 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org E847961359 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id B2184833C2; Wed, 20 Oct 2021 14:15:13 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com 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=gmail.com header.i=@gmail.com header.b="jMXGG4VX"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id DBC4E82DA1; Wed, 20 Oct 2021 07:54:46 +0200 (CEST) Received: from mail-qk1-x72e.google.com (mail-qk1-x72e.google.com [IPv6:2607:f8b0:4864:20::72e]) (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 86FF582DA1 for ; Wed, 20 Oct 2021 07:54:43 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=schspa@gmail.com Received: by mail-qk1-x72e.google.com with SMTP id 77so2167342qkh.6 for ; Tue, 19 Oct 2021 22:54:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=message-id:subject:from:to:cc:date:in-reply-to:references :user-agent:mime-version:content-transfer-encoding; bh=sySbiPbhcmyUULbUwyo3LlrIjiFwwlGaIB/NSqx+N+c=; b=jMXGG4VX4g1KYa78ceGrv0tx92QLeiXyrwfK80DBXnyWmiuaTVYeLNh4bt5jsk9nT5 8V7+Di1kTZuKGm+LXqT7J4DuGNzHoq8LsGi5sDfOLd3FJo0ctypc3AndudfwPNQ2akMj UmB994BRW1vmXD8YV7mx1639hJCfarBRM+ExFycHWC2/gjQV73rGD1iPLF/wVAlA01vE aq403XHieTLi5CORE3amBm41iLx9+NjNEUk4N0XZMNWvr9M1YPnYEoxmQgTnVXiUVwqB nzThNj3jSOerEbSy9jJxu2sPl0BTJEn1Gx7K6eGeKORKHTfL8wOEapf6rhutBsfZQu4r +/sA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=sySbiPbhcmyUULbUwyo3LlrIjiFwwlGaIB/NSqx+N+c=; b=S0uviiy7QqsJMnp0OIKKYsbnA33Hysd11n3ssDRBIev3CKgSYESQfqB9Yiklft/2j6 OrWp7e49PuvZjgenGiOegZoMgUxjVcZZ/DDgAnGErT/V/zB3f3auQwCUskB1jbLVRWpv eU08ckGraeAoqYPtfYu/UJk14OJytbNWd3cmE+9RGL0Wc+tR6cwTicKP4qZKBGNDaRuJ C9kdzR9EqODXc+esxkETGvGKodhq0aiETM4hcAAeMu4s3k97EvKEDvWVzscYHvHJxv+K 7tVleYdSZ5VqF1rXMGVAaGaP+SeI4xusjHJZNBkWQGUjY6hQRxgf0wpAEDNwIawXWw7+ Rngg== X-Gm-Message-State: AOAM5323gtA6l6/SL60qDZDMVsIq+n6TYpzhZ3akE1HPzqkCLTJaIRIE vdS/W3zeL4iHBtBGyO9qXmg= X-Google-Smtp-Source: ABdhPJzbVGbkAUldKOIPdwrt8AESmA7QXazyNOBg6ZJM8eHKiGc4t6DWy1ctXSBwUYms8F5XycVUUA== X-Received: by 2002:a05:620a:4449:: with SMTP id w9mr3621579qkp.273.1634709282153; Tue, 19 Oct 2021 22:54:42 -0700 (PDT) Received: from [127.0.0.1] (ec2-3-142-145-253.us-east-2.compute.amazonaws.com. [3.142.145.253]) by smtp.gmail.com with ESMTPSA id x123sm577386qkb.130.2021.10.19.22.54.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 19 Oct 2021 22:54:41 -0700 (PDT) Message-ID: <008fd4239bd7defd608c67e9f108eb09aadfa253.camel@gmail.com> Subject: Re: [PATCH] part: return -ENOSYS when get_info not valid. From: schspa To: Heinrich Schuchardt Cc: u-boot@lists.denx.de, Sean Anderson , Simon Glass Date: Wed, 20 Oct 2021 13:54:38 +0800 In-Reply-To: <0ce77a21-b2fd-ed12-1f46-f50e6fd18e23@gmx.de> References: <20211019133513.4021268-1-zhaohui.shi@horizon.ai> <8f915095-7fd2-8100-878e-ad9caacd7655@gmx.de> <0ce77a21-b2fd-ed12-1f46-f50e6fd18e23@gmx.de> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.40.4 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Mailman-Approved-At: Wed, 20 Oct 2021 14:14:53 +0200 X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 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.2 at phobos.denx.de X-Virus-Status: Clean 在 2021-10-20星期三的 06:44 +0200，Heinrich Schuchardt写道： > > > On 10/20/21 04:37, schspa wrote: > > 在 2021-10-19星期二的 17:23 +0200，Heinrich Schuchardt写道： > > > On 10/19/21 15:35, zhaohui.shi wrote: > > > > From: schspa > > > > > > > > In some case, get_info() interface can be NULL, add this check to > > > > stop > > > > from crash. > > > > > > Thank you for reviewing the partition driver code. > > > > > > There seems to be no driver missing a get_info implementation. > > > Where > > > did > > > you run into a problem? > > > > > Yes, I do run into a problem, In my spl build, CONFIG_SPL_FS_EXT4, > > CONFIG_SPL_FS_FAT, CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION are all > > not enabled. In this case, get_info implementation is NULL. see > > 'part_get_info_ptr' and 'part_print_ptr' and part_efi.c for detail. > > > > > Why should we only check .get_info and not .test and not .print? > > > > > > > All part type driver have .test implementation, it can't be NULL, > > and .print have NULL pointer judgement already. > > > > > > > > > > Signed-off-by: schspa > > > > > > Please, provide a name. > > > > > > > --- > > > >    disk/part.c | 7 +++++++ > > > >    1 file changed, 7 insertions(+) > > > > > > > > diff --git a/disk/part.c b/disk/part.c > > > > index a6a8f7052b..7af3240ec7 100644 > > > > --- a/disk/part.c > > > > +++ b/disk/part.c > > > > @@ -668,6 +668,13 @@ int part_get_info_by_name_type(struct > > > > blk_desc > > > > *dev_desc, const char *name, > > > >          part_drv = part_driver_lookup_type(dev_desc); > > > >          if (!part_drv) > > > >                  return -1; > > > > + > > > > +       if (!part_drv->get_info) { > > > > +               PRINTF("## Driver %s does not have the get_info() > > > > method\n", > > > > +                      part_drv->name); > > > > > > Please, use log_debug() to avoid noise on the console on every > > > boot. > > > > > > > I think it's OK to use PRINTF, because of this BUG occurs only when > > user give a bad part configuration, and this error message can prompt > > the user that a configuration error has occurred. > > Besides, 'part_get_info' use PRINTF for this null pointer protection > > too. > > Above you write that part_drv->get_info = NULL is part of your > configuration. So it will always be printed. The size of the SPL is > very > critical on many boards. We should avoid anything that increases it. > Considering this problem, I will upload a new patch to use log_debug() for error message. > Why is part_get_info_by_name_type() being called in your configuration > which does not provide a partition driver in SPL? Are there some > dependencies missing in the Kconfig files? > In my case, I have CONFIG_SPL_EFI_PARTITION=y CONFIG_SPL_LIBDISK_SUPPORT=y but forget to enable CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION. Yes, it seems we can have some dependencies in Kconfig like this: config SPL_LIBDISK_SUPPORT bool "Support disk partitions" depends on CONFIG_SPL_EXT_SUPPORT || CONFIG_SPL_FAT_SUPPORT || CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION help But there is some case for part_iso or part_mac, which have no this dependencies (because of it don't use part_get_info_ptr to wrap part_get_info_iso). How do you think about this ? Should we add such dependencies ? part_iso seems shouldn't have dependencies about ext, fat, mmcsd etc. > Best regards > > Heinrich > > > > > > Best regards > > > > > > Heinrich > > > > > > > +               return -ENOSYS; > > > > +       } > > > > + > > > >          for (i = 1; i < part_drv->max_entries; i++) { > > > >                  ret = part_drv->get_info(dev_desc, i, info); > > > >                  if (ret != 0) { > > > > > > -- schspa