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 B3CB9ECAAA1 for ; Tue, 30 Aug 2022 16:48:23 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id AE5CB8171B; Tue, 30 Aug 2022 18:48:21 +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="bK7w36pL"; dkim=permerror (0-bit key) header.d=suse.de header.i=@suse.de header.b="eNphlUJQ"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 29AC58021D; Tue, 30 Aug 2022 18:48:20 +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 DE3118482F for ; Tue, 30 Aug 2022 18:48:17 +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 4E8F621D15; Tue, 30 Aug 2022 16:48:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1661878092; 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=soqERe1IZlv/3Ynh5u8bKaIU6Xv7clqBsaS9Pe1sdyQ=; b=bK7w36pLOo+uY26LWwcjlojoGLxEjOs2cHIiCTn9KzNF4Lyw5evOT4NZT5kYmnDVs4iVJj Dj/RRMA9LAP/tp9flNuIU403icAAN7owlyBwXOukEP215OOqJ7giyp2/BqB9dX+/gSraKe H4ST0zpdkGFsBwc2Gns0PtecIqB/At4= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1661878092; 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=soqERe1IZlv/3Ynh5u8bKaIU6Xv7clqBsaS9Pe1sdyQ=; b=eNphlUJQKuPLrvWiFt4rLyHCDf3EDmiBIoQzk1bOqwln4YMOwWCYzzcVrNR8QMR0ppdxHi FydaXCT17vL4OTDw== 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 2323D2C141; Tue, 30 Aug 2022 16:48:12 +0000 (UTC) Date: Tue, 30 Aug 2022 18:48:10 +0200 From: Michal =?iso-8859-1?Q?Such=E1nek?= To: Simon Glass Cc: U-Boot Mailing List , Marek Vasut , Viktor =?utf-8?B?S8WZaXbDoWs=?= , Pavel Herrmann , Tomas Hlavacek Subject: Re: [PATCH v3] dm: core: Do not stop uclass iteration on error Message-ID: <20220830164810.GP28810@kitsune.suse.cz> References: <20220818194640.GC28810@kitsune.suse.cz> <20220819202322.25701-1-msuchanek@suse.de> <20220830102303.GO28810@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: 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 Tue, Aug 30, 2022 at 09:56:52AM -0600, Simon Glass wrote: > Hi Michal, > > On Tue, 30 Aug 2022 at 04:23, Michal Suchánek wrote: > > > > On Sat, Aug 27, 2022 at 07:52:27PM -0600, Simon Glass wrote: > > > Hi Michal, > > > > > > On Fri, 19 Aug 2022 at 14:23, Michal Suchanek wrote: > > > > > > > > When probing a device fails NULL pointer is returned, and other devices > > > > cannot be iterated. Skip to next device on error instead. > > > > > > > > Fixes: 6494d708bf ("dm: Add base driver model support") > > > > > > I think you should drop this as you are doing a change of behaviour, > > > not fixing a bug! > > > > You can hardly fix a bug without a change in behavior. > > > > These functions are used for iterating devices, and are not iterating > > devices. That's clearly a bug. > > If it were clear I would have changed this long ago. The new way you > have this function ignores errors, so they cannot be reported. > > We should almost always report errors, which is why I think your > methods should be named differently. > > > > > > > Signed-off-by: Michal Suchanek > > > > --- > > > > v2: - Fix up tests > > > > v3: - Fix up API doc > > > > - Correctly forward error from uclass_get > > > > - Do not return an error when last device fails to probe > > > > - Drop redundant initialization > > > > - Wrap at 80 columns > > > > --- > > > > drivers/core/uclass.c | 32 ++++++++++++++++++++++++-------- > > > > include/dm/uclass.h | 13 ++++++++----- > > > > test/dm/test-fdt.c | 20 ++++++++++++++++---- > > > > 3 files changed, 48 insertions(+), 17 deletions(-) > > > > > > Unfortunately this still fails one test. Try 'make qcheck' to see it - > > > it is ethernet. > > > > I will look at that. > > > > > I actually think you should create new functions for this feature, > > > e.g.uclass_first_device_ok(), since it makes it impossible to see what > > > when wrong with a device in the middle. > > > > > > I have long had all this in my mind. One idea for a future change is > > > to return the error, but set dev, so that the caller knows there is a > > > device, which failed. When we are at the end, dev is set to NULL. > > > > We already have uclass_first_device_check() and > > uclass_next_device_check() to iterate all devices, including broken > > ones, and getting the errors as well. > > > > That's for the case you want all the details, and these are for the case > > you just want to get devices and don't care about the details. > > > > That's AFAICT as much as this iteration interface can provide, and we > > have both cases covered. > > I see three cases: > - want to see the next device, returning the error if it cannot be > probed - uclass_first_device() And the point of this is what exactly? The device order in the uclass is not well defined - at any time a new device which will become the first can be added, fail probe, and block what was assumed a loop iterating the uclass from returning any devices at all. That's exactly what happened with the new sysreset. What is exactly the point of returning the error and not the pointer to the next device? The only point of these simplified iterators is that the caller can check only one value (device pointer) and then not check the error because they don't care. If they do cate uclass_first_device_check() provides all the details available. > - want to get the next device that can successfully probe - your new functions > - want to see each device, along with any errors - uclass_first_device_check() Thanks Michal