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 552D0C433FE for ; Thu, 27 Jan 2022 15:43:08 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 700DD837AB; Thu, 27 Jan 2022 16:43:06 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="jwoHtTBo"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id F1D4C8354B; Thu, 27 Jan 2022 16:43:04 +0100 (CET) Received: from mail-qt1-x833.google.com (mail-qt1-x833.google.com [IPv6:2607:f8b0:4864:20::833]) (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 8092F82F91 for ; Thu, 27 Jan 2022 16:43:01 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=seanga2@gmail.com Received: by mail-qt1-x833.google.com with SMTP id v5so2785663qto.7 for ; Thu, 27 Jan 2022 07:43:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=3bcJ44sDTEQh+l1fu2hMh2Y+MfbfX0ZTwHChoOAX+1c=; b=jwoHtTBoERw4r9UaE673q+/qnqp6tFx769UulDpb/dk+ywygnKxxFK7mYN3FfFFpIL y6uQ6/EPVFNdFmssDO+y+BMxnkmhhDD49UOEXC4p5OjmOKzUZYNNJ+un/DRUmxRCC3er NOoqbXrEz4Ffi7MXLRQmInajYe2bFgDWBAwOXeR6yMrSGigSNjr8byMzoLSwDzgHMz24 +i+JMT63J3CCzHGZR0HQA3nZC7YLUAcJMlV9i/J9BEApMz2vwCtYWfUQT0PNW/GLhm7L boLIWoC6sQEeZHfJwSsizxIqoAgvbduOD2BHdOGDOdRzqs71408t1pjsqOIzOVniBGH5 l0AA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=3bcJ44sDTEQh+l1fu2hMh2Y+MfbfX0ZTwHChoOAX+1c=; b=0uwkgbCawW+fufbLclHS5bnlzRlzdPun5DLA1JfDFiAehu7GwE8NbXy1JpRuWH+Jff YPZi2qw8p2sxZbYIu+ST3h0P1/RD6HArNJaBwywEH/ctPEforvtvfsULK6P4idNr4G84 lFz8NAkx7MfX5Y7uv27PEgigIhSOkkcAAbkvs3r8T/++fh9Yiv4sXjYY7i+LcAoQpi0d HsnxVfeOSV0wEDe3ozm70eRqWroTfoUkQ2Mdm4Xpq8v8lWRVZhFwP5r3kv7Ti8+mzJAV dCg1qS1WGkc41vFeHsdvnndyqWoPuF/cq1lfdGBY2RmTLJCufm+ibj/ZGxRiunew4cVG D5Xw== X-Gm-Message-State: AOAM532ATa8Zr1g3sqrMKBWUiCT9X7eGgZAZ0apDxyzdgBpZjxH+etw9 KDVjAketLfvXQAALykqbc5SmB9YnDjQ= X-Google-Smtp-Source: ABdhPJyW0N3l/oVy9cREIeK6HGsWdl932UnCdVJQ7RQHeBL031lOFI4NT8EF1gtk+VF4BJweLFQ//g== X-Received: by 2002:a05:622a:13cb:: with SMTP id p11mr3162597qtk.210.1643298180271; Thu, 27 Jan 2022 07:43:00 -0800 (PST) Received: from [192.168.1.201] (pool-108-18-137-133.washdc.fios.verizon.net. [108.18.137.133]) by smtp.googlemail.com with ESMTPSA id o1sm1693332qkp.49.2022.01.27.07.42.59 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 27 Jan 2022 07:42:59 -0800 (PST) Subject: Re: [PATCH 1/7] clk: Make rfree return void To: Simon Glass Cc: U-Boot Mailing List , Lukasz Majewski References: <20220115222504.617013-1-seanga2@gmail.com> <20220115222504.617013-2-seanga2@gmail.com> From: Sean Anderson Message-ID: <9cc38f2d-2c1f-e656-56ca-b7888ff0df63@gmail.com> Date: Thu, 27 Jan 2022 10:42:59 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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.5 at phobos.denx.de X-Virus-Status: Clean On 1/27/22 10:05 AM, Simon Glass wrote: > Hi Sean, > > On Sat, 15 Jan 2022 at 15:25, Sean Anderson wrote: >> >> When freeing a clock there is not much we can do if there is an error, and >> most callers do not actually check the return value. Even e.g. checking to >> make sure that clk->id is valid should have been done in request() in the >> first place (unless someone is messing with the driver behind our back). >> Just return void and don't bother returning an error. >> >> Signed-off-by: Sean Anderson >> --- >> >> drivers/clk/clk-uclass.c | 7 +++---- >> drivers/clk/clk_sandbox.c | 6 +++--- >> include/clk-uclass.h | 8 +++----- >> 3 files changed, 9 insertions(+), 12 deletions(-) >> > > We have the same thing in other places too, but I am a little worried > about removing error checking. We try to avoid checking arguments too > much in U-Boot, due to code-size concerns, so I suppose I agree that > an invalid clk should be caught by a debug assertion rather than a > full check. But with driver model we have generally added an error > return to every uclass method, for consistency and to permit returning > error information if needed. > > Regards, > Simon > So there are a few reasons why I don't think a return value is useful here. To illustrate this, consider a typical user of the clock API: struct clk a, b; ret = clk_get_by_name(dev, "a", &a); if (ret) return ret; ret = clk_get_by_name(dev, "b", &b); if (ret) goto free_a; ret = clk_set_rate(&a, 5000000); if (ret) goto free_b; ret = clk_enable(&b); free_b: clk_free(&b); free_a: clk_free(&a); return ret; - Because a and b are "thick pointers" they do not need any cleanup to free their own resources. The only cleanup might be if the clock driver has allocated something in clk_request (more on this below) - By the time we call clk_free, the mutable portions of the function have already completed. In effect, the function has succeeded, regardless of whether clk_free fails. Additionally, we cannot take any action if it fails, since we still have to free both clocks. - clk_free occurs during the error path of the function. Even if it errored, we do not want to override the existing error from one of the functions doing "real" work. The last thing is that no clock driver actually does anything in rfree. The only driver with this function is the sandbox driver. I would like to remove the function altogether. As I understand it, the existing API is inspired by the reset drivers, so I would like to review its usage in the reset subsystem before removing it for the clock subsystem. I also want to make some changes to how rates and enables/disables are calculated which might provide a case for rfree. But once that is complete I think there will be no users still. --Sean