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 X-Spam-Level: X-Spam-Status: No, score=-6.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D7E37C4338F for ; Thu, 19 Aug 2021 14:44:54 +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 C5E5F61157 for ; Thu, 19 Aug 2021 14:44:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org C5E5F61157 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org 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 5320181F7B; Thu, 19 Aug 2021 16:44:51 +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="Cf7+L/Cc"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 64D2C82BFA; Thu, 19 Aug 2021 16:44:49 +0200 (CEST) Received: from mail-wr1-x436.google.com (mail-wr1-x436.google.com [IPv6:2a00:1450:4864:20::436]) (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 CEB1680EE9 for ; Thu, 19 Aug 2021 16:44:44 +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-wr1-x436.google.com with SMTP id q11so9503823wrr.9 for ; Thu, 19 Aug 2021 07:44:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=2VGgCcwT2KsYu32N8e1FeS8H4mzPAKODdWQffus6YNE=; b=Cf7+L/Cc5VaeyuCbepMN724I2rGrgviyqtaFjCGfOe9R5idmoQ1bzp81YbWilnrmus 0mkA94mhYUbTqdIIgx3WOq02SvMrYo/JWNHFatSqXLRxaqLunVmo2GhOJo04TA6EMcnP JpTpf+8kBtpEqwIxhGbeYV6sryTM8Z0qKlh+A= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=2VGgCcwT2KsYu32N8e1FeS8H4mzPAKODdWQffus6YNE=; b=q9FtRlnuIjlIY1fgGG480Vtx47NfWQub0CWZVmQ7JtZdFELJJguCPUIpnh41fbtB+2 b8OO0sliCpPTj7L9XWD+ke55nDKFQM3ONUuhPFualnTljbj9D9tWI9jZcqKjnqHcDf+i bvbgh1+DjWMwAtdrj1dt/jXlDXsE0Fd11bmkD8qQg1fhWV7ZSHhUa4fspUDnZcRR0+wQ M3V2QhVZFpLK1Vm1IKNFYDrU6q5tD79X3LKwrDVrN2jXC7CpwXus6K71Wpcyadry70Y0 hOU5D5Chgu8JD2NwvEnAhZdsnaGki7/oZQjQUVTRAj4JqXN+SddecuDMcg3j+gsPBJlB WcLw== X-Gm-Message-State: AOAM531UUGMsKtY93IAzopFQzLKcCcbeQaa46UsRdqyad6V517ibV3mf qmZVhtNhYRdEo7yhBhNClh4TZur7HmjY4piBINs+PGIc+yYrWQ== X-Google-Smtp-Source: ABdhPJylMCDSJohMR1ceW0SAv9i0nC7yz+buZpzHP1krhBqaEjyYq/61uwJHG4Cx7VcJYLQlEST7CfdoLbcNHJoF3Ck= X-Received: by 2002:adf:d083:: with SMTP id y3mr4350255wrh.56.1629384283989; Thu, 19 Aug 2021 07:44:43 -0700 (PDT) MIME-Version: 1.0 References: <20210819095706.3585923-1-rasmus.villemoes@prevas.dk> <20210819095706.3585923-11-rasmus.villemoes@prevas.dk> <57359.1629373593@gemini.denx.de> <62540f7b-0e07-8759-8e12-125527c2edec@prevas.dk> <61767.1629376321@gemini.denx.de> <20210819123540.GV858@bill-the-cat> <64046.1629378226@gemini.denx.de> <20210819130806.GW858@bill-the-cat> <66367.1629382572@gemini.denx.de> In-Reply-To: <66367.1629382572@gemini.denx.de> From: Simon Glass Date: Thu, 19 Aug 2021 08:44:31 -0600 Message-ID: Subject: Re: [PATCH v6 10/12] watchdog: add gpio watchdog driver To: Wolfgang Denk Cc: Tom Rini , Rasmus Villemoes , U-Boot Mailing List , Stefan Roese Content-Type: text/plain; charset="UTF-8" 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 Hi, On Thu, 19 Aug 2021 at 08:16, Wolfgang Denk wrote: > > Dear Tom, > > In message <20210819130806.GW858@bill-the-cat> you wrote: > > > > > So we have now a policy to wave through code, and ask others to > > > clean it up later? That's ... sad. > > > > No, we continue to have the policy of expecting reviewers to follow the > > whole discussion and relevant subsystems. > > Once upon a time there has also been a policy that if a function > might return error codes, these need to be checked and handled. > > > Changing _every_ caller of dev_get_priv to check for NULL and > > then, what? is clearly not the right answer. Note that we should not check these calls, as the priv data is allocated by driver model and a device cannot be probed until it gets past this step. I know that sometimes people add this check, but whenever I see it I ask them to remove it. It is pointless and confusing, since it suggests that driver model may not have allocated it yet. This sort of confusion can really make things hard to understand for new people. > > Then what is the right answer in your opinion? If a device is probed, you can rely on the priv data being set up. The only way to access a probed device is via the device-internal.h or uclass-internal.h APIs. Be careful with those if you must use them. > > I mean, look at the implementation of dev_get_priv(): > > 628 void *dev_get_priv(const struct udevice *dev) > 629 { > 630 if (!dev) { > 631 dm_warn("%s: null device\n", __func__); > 632 return NULL; > 633 } > 634 > 635 return dm_priv_to_rw(dev->priv_); > 636 } > > If there is guaranteed no way that dev_get_priv() can return a NULL > pointer, that means that it must be guaranteed that the "dev" > argument can never be a NULL pointer, either. So why do we check it > at all? > > The same applies for all functions in "drivers/core/device.c" - they > all check for valid input parameters, like any code should do. I think did a series on this many years ago - a way to turn on checks for this and that the right struct is obtained when you call dev_get_priv(), etc. We could certainly add this with a CONFIG option for debugging purposes. The runtime cost is actually not terrible but it does require collusion with malloc() to be efficient. > > > If you think you see a problem here please go audit the DM code > > itself more and propose some changes. > > I can see that the DM code itself implements proper error checking > and reporting; it's the callers where negligent code prevails. > > > If you are consequent, you must decide what you want: > > - Either we want robust and reliable code - then we have to handle > the error codes which functions like dev_get_priv() etc. return. > > - Or you don't care about software quality, then we can omit such > handling, but then it would also be consequent to remove all the > error checking from "drivers/core/device.c" etc. - hey, that would > even save a few hundred bytes of code size. > > > Sugarcoating code which fails to handle error codes because "these > errors can never happen" does not seem to be a clever approach to > software engineering to me. > > > I stop here. You know my opinion. You are the maintainer. There is a wider issue here about arrg checking. We sometimes use assert but at present that only appears in the code if DEBUG is enabled. Also it just halts. OTOH if we add arg checking everywhere it cluttles the code and can substantially increase the size (I know of a project where it doubled the size). I like to distinguish between: - programming errors - security errors where user input is insufficiently checked IMO the former should not be present if you have sufficient tests and trying to catch them in the field at runtime is not very kind to your users. Regards, Simon