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 BDF40C433F5 for ; Wed, 16 Mar 2022 16:18:58 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 2447182093; Wed, 16 Mar 2022 17:18:56 +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="ImuX0PBj"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id C0BA78396F; Wed, 16 Mar 2022 17:18:53 +0100 (CET) Received: from mail-qv1-xf2c.google.com (mail-qv1-xf2c.google.com [IPv6:2607:f8b0:4864:20::f2c]) (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 E5DB9809D1 for ; Wed, 16 Mar 2022 17:18:48 +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-qv1-xf2c.google.com with SMTP id kk16so2209265qvb.5 for ; Wed, 16 Mar 2022 09:18:48 -0700 (PDT) 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=OI6mvbnbwtC/ClJHJe7P7v4JUahTnzjTjn/+G9AyAOM=; b=ImuX0PBjhkaYnurmGv4q2fb8m68r9sfYA7bfV7kEefBNDZRvx+qpnbdlK5ThDJZb9K TbReg5RLUH2SBdEQXk8z43VTo3FCbGVTpgUz4b4op82Y3p9qfNjoeQN3uTZqchEQsw9P 5OvTz14u2ZKw4xs9ch0MZfYDrN7pvX0PQDfQlgoaQ8CkS0BiwylzvYOl+VXAFhQRVRGt ofZ6jlK1RXMSpVOye2RMu6zmeu0r24Iwmk2Ukd06IW1G+ziJ/61jGmOU8lLs03v/VVj+ aNgG34Zx/oJPS9K9K/TyHPJJg2wVMDxZaaHUESJuaL6dVFRJO2Wy/mon9xeYj9E2VHtM 8vlw== 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=OI6mvbnbwtC/ClJHJe7P7v4JUahTnzjTjn/+G9AyAOM=; b=UFmAYvZZ7oxZws/aONrhIvnrHuQlyg7raa0mJ0VvagNMSI/x2RVtVXbI3UFLkfEYT3 exOAO0w1tdM3CTUtdUE9kLNVPsc2/8SSVgBm9gBvfozj6PAjudzrEManyM4pFbfoFw3Q bvAFwlzaiopK3iOnYunH3MyWLbKoz3BAoIPfd1T/7f1fVzRkBAEANnGx6wiD6K1ISE54 WdkFq+uDsEjrKaaCxnf6FEdwDV8Isb3ltTwcH1O5QWII9dds1HvVTGvWyYAb+bVqqJGp sND3BImvQ1SXc5OJBlXhUt9C+0FjRKl3JTvW+evhG0kC0wW7+WCm0X/AC94lZMWlbYDO PBsg== X-Gm-Message-State: AOAM5328rszkbxpSZe8tn2YbdzERvJi+i3+pGFIE2QUns4jyVnx5snci liTwh494/vn8/WWvXqRX55c= X-Google-Smtp-Source: ABdhPJwRS02jLQJF7B8rWpP6z6EXNxrZkf3F6u2B8jjGGG44Bm1eO5s0elgG1SnOHuz/t/WLPxJSqA== X-Received: by 2002:ad4:4ee2:0:b0:435:4327:f5c1 with SMTP id dv2-20020ad44ee2000000b004354327f5c1mr112718qvb.88.1647447527144; Wed, 16 Mar 2022 09:18:47 -0700 (PDT) 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 q8-20020a05622a030800b002e1c9304db8sm1556663qtw.38.2022.03.16.09.18.45 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 16 Mar 2022 09:18:46 -0700 (PDT) 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> <9cc38f2d-2c1f-e656-56ca-b7888ff0df63@gmail.com> <4bdef436-8cca-6879-f017-20b721f9a8dc@gmail.com> <75aed704-b354-a6c3-892a-9640ecad858f@gmail.com> From: Sean Anderson Message-ID: Date: Wed, 16 Mar 2022 12:18:45 -0400 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 3/1/22 9:58 AM, Simon Glass wrote: > Hi Sean, > > On Sun, 27 Feb 2022 at 12:38, Sean Anderson wrote: >> >> On 2/26/22 1:36 PM, Simon Glass wrote: >>> Hi Sean, >>> >>> On Tue, 1 Feb 2022 at 21:24, Sean Anderson wrote: >>>> >>>> On 2/1/22 10:59 PM, Simon Glass wrote: >>>>> Hi Sean, >>>>> >>>>> On Tue, 1 Feb 2022 at 07:49, Sean Anderson wrote: >>>>>> >>>>>> On 1/27/22 4:35 PM, Simon Glass wrote: >>>>>>> Hi Sean, >>>>>>> >>>>>>> On Thu, 27 Jan 2022 at 08:43, Sean Anderson wrote: >>>>>>>> >>>>>>>> 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. >>>>>>> >>>>>>> What does this all look like in Linux? >>>>>> >>>>>> Their equivalent (clk_put) returns void, and generally so do most other >>>>>> cleanup functions, since .device_remove also returns void. >>>>> >>>>> We really cannot ignore errors from device_remove(). >>>> >>>> Once you are at device_remove, all the users are gone and it's up to the >>>> device to clean up after itself. And often there is nothing we can do >>>> once remove is called. As you yourself say in device_remove, >>>> >>>> /* We can't put the children back */ >>> >>> Well this assumes that device_remove() is actually removing the >>> device, not just disabling DMA, etc. >>> >>>> >>>> Really the only sensible thing is to print an error and continue booting >>>> if possible. >>>> >>>> And of course no clock drivers actually use this function anyway, nor do >>>> (all but 5) users check it. >>>> >>>>> Anyway I think what you say about the 'thick pointer' makes sense. But >>>>> my expectation was that removing a clock might turn off a clock above >>>>> it in the tree, for example. >>>> >>>> No, this just frees resources (as is documented). If you want to turn >>>> off a clock, you have to call clk_disable. In fact, a very common use >>>> case is just like the example above, where the consmer frees the clock >>>> after enabling it. >>>> >>>> (This is also why clk->enable_count/rate are basically useless for >>>> anything other than CCF clocks) >>> >>> How about a clock provided by an audio codec on an I2C bus? Should >>> clk_free() do anything in that case? I assume not. I think the >>> compelling part of your argument is that it is a 'think pointer' and >>> disable does nothing. So can you update clk_rfree() etc. to document >>> what is allowed to be done in that function? >> >> The ideal case would be if you wanted to allocate some per-struct-clk >> data. Then, the correct place to free it would be rfree. But no one >> does this, and if they did it would probably be better to free things >> in remove. >> >> Actually... no one in clk, reset, or power-domain does anything with >> rfree. So I am inclined to just remove it altogether. > > Well, I suppose it is easy enough to add later, if needed. What does > Linux use this for? As far as I can tell, Linux doesn't have request/free. There's of_clk_provider->get(), but that corresponds more to of_xlate. Similarly, there's reset_controller_dev->of_xlate(), but no request/free. --Sean