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=-16.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 E71D3C432BE for ; Mon, 30 Aug 2021 14:56:10 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id CD66360E90 for ; Mon, 30 Aug 2021 14:56:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237206AbhH3O5D (ORCPT ); Mon, 30 Aug 2021 10:57:03 -0400 Received: from mail.kernel.org ([198.145.29.99]:49706 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233162AbhH3O5C (ORCPT ); Mon, 30 Aug 2021 10:57:02 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 5CFF560E90; Mon, 30 Aug 2021 14:56:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1630335368; bh=pRLIow2bOne8zY9MmBi2l3flpVgazhDViJowLgZg0lg=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=H3CaWCJvRgHAHIshKvBsqQ33imfBflTIsjfytsqSg2chdx5VvdQTuENne2jHDlDjg GN3xgZa5SpJYshTu2MZpCjhVQmNMiiykmAXgvKM8DwNjTBtOukUtTBgyqssU49JhsN TyU/uwLiF7ev0CEaC+cAticxpTXi41OA5hcYqnk8M7ApshMXmuGNnuLkg0MK0Fi2gN Jtj2M4V7XuYXrnADy9gPzzKu0RDofUdVWVX8/sJ2YrWrV2z/ErA1rljAqwsTwpbbpu 4S1nludJ5zP5VykTn/AbbBT9zsPMvZ0wBgNQfmppp2Nzs/mPSSTMUqKQqgAoJfybWo zEd99bPN7XyOw== Received: by mail-ed1-f48.google.com with SMTP id g21so22061205edw.4; Mon, 30 Aug 2021 07:56:08 -0700 (PDT) X-Gm-Message-State: AOAM530ACpd20wLXuVP6GTDmKwESNldPKsjZlFWcHGLZEoUlZg36Cb/0 O10TnIPorv8+4ANGADZoBivH3D9ViYAVoQb+KA== X-Google-Smtp-Source: ABdhPJxokTq2aAaklRnJkPvbh83JaHPms31ufMUIF1QhZ0YzE3zlCUXE9CvjIfCwd3EH9dvxOdOfMX28MiS3gTVJZqc= X-Received: by 2002:a50:9b52:: with SMTP id a18mr24290884edj.165.1630335367017; Mon, 30 Aug 2021 07:56:07 -0700 (PDT) MIME-Version: 1.0 References: <20210825083425.32740-1-yajun.deng@linux.dev> <63e1e9ea1e4b74b56aeafcc6695ecfa8@linux.dev> In-Reply-To: From: Rob Herring Date: Mon, 30 Aug 2021 09:55:54 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH linux-next] PCI: Fix the order in unregister path To: Yajun Deng Cc: Bjorn Helgaas , Arnd Bergmann , Lorenzo Pieralisi , PCI , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Aug 26, 2021 at 9:39 PM wrote: > > August 26, 2021 8:01 PM, "Rob Herring" wrote: > > > On Wed, Aug 25, 2021 at 10:57 PM wrote: > > > >> August 25, 2021 9:55 PM, "Rob Herring" wrote: > >> > >> On Wed, Aug 25, 2021 at 3:34 AM Yajun Deng wrote: > >> > >> device_del() should be called first and then called put_device() in > >> unregister path, becase if that the final reference count, the device > >> will be cleaned up via device_release() above. So use device_unregister() > >> instead. > >> > >> Fixes: 9885440b16b8 (PCI: Fix pci_host_bridge struct device release/free handling) > >> Signed-off-by: Yajun Deng > >> --- > >> drivers/pci/probe.c | 4 +--- > >> 1 file changed, 1 insertion(+), 3 deletions(-) > >> > >> NAK. > >> > >> The current code is correct. Go read the comments for device_add/device_del. > >> > >> But the device_unregister() is only contains device_del() and put_device(). It just put > >> device_del() before put_device(). > > > > And that is the wrong order as we want to undo what the code above > > did. The put_device here is for the get_device we did. The put_device > > in device_unregister is for the get_device that device_register did > > (on success only). > > > > Logically, it is wrong too to call unregister if register failed. That > > would be like doing this: You are right that the register and unregister are different devices. However, your change is still wrong. The device_register is actually irrelevant. > > > > p = malloc(1); > > if (!p) > > free(p); > > > This is the raw code: > err = device_register(&bus->dev); > if (err) > goto unregister; > unregister: > put_device(&bridge->dev); > device_del(&bridge->dev); The pertinent parts are this: err = device_add(&bridge->dev); // which calls get_device() itself, so there's the first ref if (err) { put_device(&bridge->dev); goto free; } bus->bridge = get_device(&bridge->dev); // This is the 2nd ref which the PCI core holds ... unregister: put_device(&bridge->dev); // This is the put for the get_device just above here. device_del(&bridge->dev); // Then this does the 2nd put. The get_device and put_device are paired, and the device_add and device_del are paired. As I said earlier, go read the kerneldoc for device_add. For your convenience, here's the important part: device_add: * Rule of thumb is: if device_add() succeeds, you should call * device_del() when you want to get rid of it. If device_add() has * *not* succeeded, use *only* put_device() to drop the reference * count. device_del: * NOTE: this should be called manually _iff_ device_add() was * also called manually. Rob