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 CA5D4ECAAD3 for ; Wed, 31 Aug 2022 17:45:00 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id ABE78848F8; Wed, 31 Aug 2022 19:44:54 +0200 (CEST) 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="ToQOHP12"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 38052849EE; Wed, 31 Aug 2022 19:44:50 +0200 (CEST) Received: from mail-yb1-xb30.google.com (mail-yb1-xb30.google.com [IPv6:2607:f8b0:4864:20::b30]) (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 B1689849B9 for ; Wed, 31 Aug 2022 19:44:45 +0200 (CEST) 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-yb1-xb30.google.com with SMTP id j204so5235627ybj.2 for ; Wed, 31 Aug 2022 10:44:45 -0700 (PDT) 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; bh=WrVWCUsiy46kSC0vR9VQb9rmXGagLMs0wE7G09NXM2Q=; b=ToQOHP129k//OAv4tk8kM1BGu6NoAY5kFbzSkFAYTXMqA1aXiPoy2Cs4/vI7myj6DL HyIodyrqH24+nzWf10hMYakT0vAgVQVV+SH6ESug1xPu7ozalxuulouA68xGoxbR8cC7 iFyF3lstqWdNznzDdJFxGxsNSWCPuInmPWKFI= 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; bh=WrVWCUsiy46kSC0vR9VQb9rmXGagLMs0wE7G09NXM2Q=; b=1rjroXOLfZoiKMeZUw4VEAy1sf34oOyzNxHCzuEn/fujwx9/XTLQZTyCAWtWsN4rbx nRNsz7AK+i2Q1dmynbaN0UvIy5+BU/cIoh61XMKHQZ3Jvh8o3rb/BoJCzHllI4vYC+3h w+4RO2H+z+z3B1b7u4tHTkvFB61cIzPkL6pOkcWeFfeKVb8xTgIwDRaAtm/FOE6IQ9zN 9gdkj8o3SojvMMqARrQE4Afknkdf3Bn57gcc6DE9QP536uHbo2SNQaaiNUHW/KB76XgY XvoPu0ma3V+/Pt84ByT6mwtQvekJeyAb6+cORX0hQiDj8e4/LDUcBvXIOzkUBd0jtbcm 3xow== X-Gm-Message-State: ACgBeo0bbZ0V9o6aPK8pNk6RMQ9Z3NLTIgyn3UNp7OedyDDe06njt36t LkWY/Hb0HZOOoBtQOsD5pe9bBeTMKL5cAXX0wVY/Mw== X-Google-Smtp-Source: AA6agR6RGIQnwVdCnFlerG1KrlzDlppDt6/o8k7AvAXQKfKrWApDxm1neBVUXnRqRcIQ4jzaRru6dVAip4WootCzuzM= X-Received: by 2002:a25:558:0:b0:696:4e84:6367 with SMTP id 85-20020a250558000000b006964e846367mr15773768ybf.412.1661967884219; Wed, 31 Aug 2022 10:44:44 -0700 (PDT) MIME-Version: 1.0 References: <20220818194640.GC28810@kitsune.suse.cz> <20220819202322.25701-1-msuchanek@suse.de> <20220830102303.GO28810@kitsune.suse.cz> <20220830164810.GP28810@kitsune.suse.cz> <20220831073903.GQ28810@kitsune.suse.cz> In-Reply-To: <20220831073903.GQ28810@kitsune.suse.cz> From: Simon Glass Date: Wed, 31 Aug 2022 11:44:32 -0600 Message-ID: Subject: Re: [PATCH v3] dm: core: Do not stop uclass iteration on error To: =?UTF-8?Q?Michal_Such=C3=A1nek?= Cc: U-Boot Mailing List , Marek Vasut , =?UTF-8?B?VmlrdG9yIEvFmWl2w6Fr?= , Pavel Herrmann , Tomas Hlavacek 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 Hi Michal, On Wed, 31 Aug 2022 at 01:39, Michal Such=C3=A1nek wrot= e: > > Hello, > > On Tue, Aug 30, 2022 at 09:15:12PM -0600, Simon Glass wrote: > > Hi Michal, > > > > On Tue, 30 Aug 2022 at 10:48, Michal Such=C3=A1nek = wrote: > > > > > > 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=C3=A1nek 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 oth= er 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 behav= iour, > > > > > > 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 itera= ting > > > > > devices. That's clearly a bug. > > > > > > > > If it were clear I would have changed this long ago. The new way yo= u > > > > 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 s= ee it - > > > > > > it is ethernet. > > > > > > > > > > I will look at that. > > > > > > > > > > > I actually think you should create new functions for this featu= re, > > > > > > e.g.uclass_first_device_ok(), since it makes it impossible to s= ee what > > > > > > when wrong with a device in the middle. > > > > > > > > > > > > I have long had all this in my mind. One idea for a future chan= ge is > > > > > > to return the error, but set dev, so that the caller knows ther= e is a > > > > > > device, which failed. When we are at the end, dev is set to NUL= L. > > > > > > > > > > We already have uclass_first_device_check() and > > > > > uclass_next_device_check() to iterate all devices, including brok= en > > > > > ones, and getting the errors as well. > > > > > > > > > > That's for the case you want all the details, and these are for t= he 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, an= d 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? > > > > Please can you adjust your tone, It seems too aggressive for this > > mailing list. Thank you. > > > > > > > > The device order in the uclass is not well defined - at any time a ne= w > > > device which will become the first can be added, fail probe, and bloc= k > > > what was assumed a loop iterating the uclass from returning any devic= es > > > at all. That's exactly what happened with the new sysreset. > > > > The order only changes if the device is unbound and rebound. Otherwise > > the order set by the device tree is used. > > So the order is defined by device tree. That does not make it > well-defined from the point of view of any kind of code. > > The point of device tree is that it can be replaced with another device > tree describing another board and the code should still work. Otherwise > we would not need device trees, and could keep using board files. We do use the raw ordering in test code, but in general we use the sequence number (from DT ordering or aliases) to provide the official ordering (the uclass...seq() calls). > > > > What is exactly the point of returning the error and not the pointer = to > > > the next device? > > > > Partly, we have existing code which uses the interface, checking 'dev' > > to see if the device is valid. I would be happy to change that, so > > that the device is always returned. In fact I think it would be > > better. But it does need a bit of work with coccinelle, etc. > > I suppose changing the return type to void would catch the users that do > something with the return value but it would still need building all > the code. > > And it does not work for users of uclass_first_device_err which is > basically useless after this change but pretty much all users use the > return value. > > > > 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. > > > > Yes I think we can have just two sets of iterators, but in that case > > it should be: > > > > - want to see the next device, returning the error if it cannot be > > probed, with dev updated to the next device in any case - new version > > of uclass_first_device() - basically rename > > uclass_first_device_check() to that > > About 2/3 of users of uclass_first_device don't use the return value at > all in current code. Changing uclass_first_device to > uclass_first_device_check is counterproductive. The current > documentation basically implies the new behavior, and there are a lot of > examples in the core code that use uclass_first_device in a for loop > without assigning the return value at all. > > Also renaming uclass_first_device_check would break the 3 existing users > of it. > > > - want to see next device which probes OK - your new function, perhaps > > uclass_first_device_ok() ? > > I don't think any amount of renaming is going to solve the problem at > hand: we have bazillion of users of uclass_first_device, and because it > was not documented that it does not in fact iterate uclass devices there > are users that use it for the purpose. There are also users that expect > maningful return value which is basically bogus - they do get a return > value of something, but not something specific. > > What can be done is adding the simple iterator under new name, convert > the obvious existing users, and mark the old function deprecated in some > way so that any code that uses it generates a warning. I'm OK with that. But let's rename uclass_first_device() to uclass_old_first_device() or something like that. Regards, Simon