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 D44B0C433FE for ; Thu, 10 Nov 2022 02:15:28 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 97541850A3; Thu, 10 Nov 2022 03:15:26 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org 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=chromium.org header.i=@chromium.org header.b="S9UcVx+I"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 6E202850D2; Thu, 10 Nov 2022 03:15:23 +0100 (CET) Received: from mail-ej1-x629.google.com (mail-ej1-x629.google.com [IPv6:2a00:1450:4864:20::629]) (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 1ECEA850A3 for ; Thu, 10 Nov 2022 03:15:19 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sjg@google.com Received: by mail-ej1-x629.google.com with SMTP id b2so1578617eja.6 for ; Wed, 09 Nov 2022 18:15:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=0Ar548ckhykEuTjd5sOtRBWB2ZS0Z5vQu3I4APUcb74=; b=S9UcVx+InmmGTf1rcjMresuSkEnCMlGslsM/TSb1QgG4ZiBFtOypHAtT+hbSFjBLEN zsrWJkqomL/BW7VxyOn2bqRczhlt92LWfP6ZKSSM/21p3iA1LcNjqbjKEnkAxiZl3dUT UDbpzd+RDICNvr10WsAVMCqk6MDcAul3495SA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=0Ar548ckhykEuTjd5sOtRBWB2ZS0Z5vQu3I4APUcb74=; b=mN2G5WMxW4n23yB0ieUlRsrrgjmtQGb0uMh5vl0VMOW+yFgihLOALZKTwKLc+5a2wj 8TmFUNsl8v/z2UjF/7XfGrDDN07VFpepbG0KhKof0U9l24SS9cRhKZVmJ1LBhOnNrdls TiUS0/JgLPJ+Q7P1XLyqmfR0ytOgh+j5pHAVGEEepEWL0NtEsSduZYXiYqXULZSLePBJ Oqr8XckGDQMHoX7TTusYNp1rJMpi+PYTWRkNtjOnvai9TsM6Os6E+R9vuY5ZbUjhMeRw cWXaUTZiR5Gy9ItUcGbNyYJA+ltPGlDUv8fyiAjDFsoObq+7slCxrJNpPAQLpP1tlWf3 X/sg== X-Gm-Message-State: ACrzQf0utuAw3D1KrkW2fqa6YpPO9zBWAmngxYf5FXrYxjFPMqbkvtE/ U2ipP0mcxefi912qNB8eehDg2Bxz68sXQM96rTWbbA== X-Google-Smtp-Source: AMsMyM42/YUslq64XwXSJk4x4eJQW13eb0US7Bo66GH2Wd/E51m1uksKmnglZJ+u6dzMSJuXkyHqiKuXWA3DlsK+1s8= X-Received: by 2002:a17:906:7f88:b0:7ad:84d1:5b56 with SMTP id f8-20020a1709067f8800b007ad84d15b56mr2222007ejr.205.1668046518297; Wed, 09 Nov 2022 18:15:18 -0800 (PST) MIME-Version: 1.0 References: <76875ef0301a144797b7aee456ecb0a699a0418f.1664093812.git.msuchanek@suse.de> <20221002193443.GC28810@kitsune.suse.cz> <20221010194920.GW28810@kitsune.suse.cz> <20221010213301.GX28810@kitsune.suse.cz> In-Reply-To: From: Simon Glass Date: Wed, 9 Nov 2022 19:15:06 -0700 Message-ID: Subject: Re: [PATCH v4 04/21] dm: blk: Add probe in blk_first_device/blk_next_device To: =?UTF-8?Q?Michal_Such=C3=A1nek?= Cc: U-Boot Mailing List , Heinrich Schuchardt , AKASHI Takahiro , Bin Meng , Stefan Roese Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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, 10 Oct 2022 at 16:33, Simon Glass wrote: > > Hi Michal, > > On Mon, 10 Oct 2022 at 15:33, Michal Such=C3=A1nek wr= ote: > > > > On Mon, Oct 10, 2022 at 09:49:20PM +0200, Michal Such=C3=A1nek 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=C3=A1nek 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_devic= e() 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-u= class.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 **de= vp) > > > > > > > { > > > > > > > struct blk_desc *desc; > > > > > > > - int ret; > > > > > > > + int ret =3D 0; > > > > > > > + > > > > > > > + for (; *devp; uclass_find_next_device(devp)) { > > > > > > > + desc =3D dev_get_uclass_plat(*devp); > > > > > > > + if (desc->if_type =3D=3D if_type) { > > > > > > > + ret =3D device_probe(*devp); > > > > > > > + if (!ret) > > > > > > > + return 0; > > > > > > > + } > > > > > > > + } > > > > > > > > > > > > > > - ret =3D uclass_find_first_device(UCLASS_BLK, devp); > > > > > > > if (ret) > > > > > > > return ret; > > > > > > > - if (!*devp) > > > > > > > - return -ENODEV; > > > > > > > - do { > > > > > > > - desc =3D dev_get_uclass_plat(*devp); > > > > > > > - if (desc->if_type =3D=3D if_type) > > > > > > > - return 0; > > > > > > > - ret =3D uclass_find_next_device(devp); > > > > > > > - if (ret) > > > > > > > - return ret; > > > > > > > - } while (*devp); > > > > > > > > > > > > This looks wrong since a media device may have other devices un= der it, > > > > > > e.g. UCLASS_BOOTDEV so I think you should keep the existing cod= e and > > > > > > just call uclass_probe() at the end. > > > > > > > > > > > > You could add a test for this by checking that only the BLK dev= ice is probed. > > > > > > > > > > The description says that it returns ready to use device, and tha= t'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 ma= y be OK > > > > > to change the semantic again to resemble the _check variant uclas= s > > > > > 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 t= o 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? > > Well you can add a new driver and a device associated with it, to test th= at. > Applied to u-boot-dm (please check)