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 9A1F5C433FE for ; Mon, 10 Oct 2022 21:33:13 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 7B52684EF6; Mon, 10 Oct 2022 23:33:11 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=suse.de header.i=@suse.de header.b="IsqBzDWb"; dkim=permerror (0-bit key) header.d=suse.de header.i=@suse.de header.b="kX/E0pUr"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id F128384F09; Mon, 10 Oct 2022 23:33:08 +0200 (CEST) Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id D8CAC84ADF for ; Mon, 10 Oct 2022 23:33:02 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=msuchanek@suse.de Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id A5B15228CD; Mon, 10 Oct 2022 21:33:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1665437582; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=duTVL2TqHlAV/UHUIm37AjMJHDaHS4SFFdTuYVLcwiI=; b=IsqBzDWbO2Lzl1pBasDRoaYLpS3QuIffiLEy+YqeTAvOsrA7eAfiZXa/nilghCKy8Wc2Uf geU4KGKYS5dzeuidoG9qmMKw17uTRgCcjqA8qYA8a1BjdX3jpIXlhDJFdmDSVo/OJ/ysUm xKouwGjLHhszggIoeG06biNEFDKq+1E= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1665437582; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=duTVL2TqHlAV/UHUIm37AjMJHDaHS4SFFdTuYVLcwiI=; b=kX/E0pUrMs4BAK0gnYPKskVKprmVP9hoY/0Gyj8JtqFngx6xKW7P+dOMJ2+4ADj+TV3XSb QdkyuhexWILkgdDA== Received: from kitsune.suse.cz (kitsune.suse.cz [10.100.12.127]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 803052C142; Mon, 10 Oct 2022 21:33:02 +0000 (UTC) Date: Mon, 10 Oct 2022 23:33:01 +0200 From: Michal =?iso-8859-1?Q?Such=E1nek?= To: Simon Glass Cc: U-Boot Mailing List , Heinrich Schuchardt , AKASHI Takahiro , Bin Meng , Stefan Roese Subject: Re: [PATCH v4 04/21] dm: blk: Add probe in blk_first_device/blk_next_device Message-ID: <20221010213301.GX28810@kitsune.suse.cz> References: <76875ef0301a144797b7aee456ecb0a699a0418f.1664093812.git.msuchanek@suse.de> <20221002193443.GC28810@kitsune.suse.cz> <20221010194920.GW28810@kitsune.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20221010194920.GW28810@kitsune.suse.cz> User-Agent: Mutt/1.10.1 (2018-07-13) 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.6 at phobos.denx.de X-Virus-Status: Clean On Mon, Oct 10, 2022 at 09:49:20PM +0200, Michal Suchánek wrote: > On Sun, Oct 02, 2022 at 07:10:40PM -0600, Simon Glass wrote: > > Hi Michal, > > > > On Sun, 2 Oct 2022 at 13:34, Michal Suchánek wrote: > > > > > > On Thu, Sep 29, 2022 at 04:00:26AM -0600, Simon Glass wrote: > > > > Hi Michal, > > > > > > > > On Sun, 25 Sept 2022 at 02:28, Michal Suchanek wrote: > > > > > > > > > > The description claims that the device is probed but it isn't. > > > > > > > > > > Add the device_probe() call. > > > > > > > > > > Also consolidate the iteration into one function. > > > > > > > > > > Fixes: 8a5cbc065d ("dm: blk: Use uclass_find_first/next_device() in blk_first/next_device()") > > > > > Signed-off-by: Michal Suchanek > > > > > --- > > > > > drivers/block/blk-uclass.c | 46 ++++++++++++++++++-------------------- > > > > > 1 file changed, 22 insertions(+), 24 deletions(-) > > > > > > > > > > diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c > > > > > index 21c5209bb6..992f8ad3da 100644 > > > > > --- a/drivers/block/blk-uclass.c > > > > > +++ b/drivers/block/blk-uclass.c > > > > > @@ -361,45 +361,43 @@ int blk_dselect_hwpart(struct blk_desc *desc, int hwpart) > > > > > return blk_select_hwpart(desc->bdev, hwpart); > > > > > } > > > > > > > > > > -int blk_first_device(int if_type, struct udevice **devp) > > > > > +static int _blk_next_device(int if_type, struct udevice **devp) > > > > > { > > > > > struct blk_desc *desc; > > > > > - int ret; > > > > > + int ret = 0; > > > > > + > > > > > + for (; *devp; uclass_find_next_device(devp)) { > > > > > + desc = dev_get_uclass_plat(*devp); > > > > > + if (desc->if_type == if_type) { > > > > > + ret = device_probe(*devp); > > > > > + if (!ret) > > > > > + return 0; > > > > > + } > > > > > + } > > > > > > > > > > - ret = uclass_find_first_device(UCLASS_BLK, devp); > > > > > if (ret) > > > > > return ret; > > > > > - if (!*devp) > > > > > - return -ENODEV; > > > > > - do { > > > > > - desc = dev_get_uclass_plat(*devp); > > > > > - if (desc->if_type == if_type) > > > > > - return 0; > > > > > - ret = uclass_find_next_device(devp); > > > > > - if (ret) > > > > > - return ret; > > > > > - } while (*devp); > > > > > > > > This looks wrong since a media device may have other devices under it, > > > > e.g. UCLASS_BOOTDEV so I think you should keep the existing code and > > > > just call uclass_probe() at the end. > > > > > > > > You could add a test for this by checking that only the BLK device is probed. > > > > > > The description says that it returns ready to use device, and that's not > > > possible when the device is only probed at the end when it is to be > > > returned. > > > > Why is that? > > There are two options: > > - probe the device, and skip it if it fails, potentially probing > multiple devices before returning one > - decide what device to return, probe it, and if it fails return > non-activated device > > > > There are some tests of this function but very few users so it may be OK > > > to change the semantic again to resemble the _check variant uclass > > > iterator and retorn broken devices but I don't think that was the intent > > > here with using uclass_first_device/uclass_next_device originally. > > > > I agree. > > > > > > > > Also this change only makes a difference to the amount of devices probed > > > for callers that only call the blk_first_device and never move on to the > > > next. Callers that use the functions for iteration will move on to the > > > next device and probe it anyway. > > > > OK, perhaps I understand this. But don't you need to update the > > comment in the header file to say that devices that don't probe are > > silently skipped? > > They are not ready to use so they cannot be returned by the current > description? > > > > > Also it really does need a test. > > Right, tests are good to prevent similar regression in the future. But we don't have the boilerplate for testing failure in block devices, only in the special probe test class. Or do we? Thanks Michal