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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A5045C433EF for ; Fri, 15 Oct 2021 16:46:59 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8CB2C61213 for ; Fri, 15 Oct 2021 16:46:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S241596AbhJOQtD (ORCPT ); Fri, 15 Oct 2021 12:49:03 -0400 Received: from mail.kernel.org ([198.145.29.99]:60482 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S241554AbhJOQtC (ORCPT ); Fri, 15 Oct 2021 12:49:02 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 34DF061164; Fri, 15 Oct 2021 16:46:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1634316415; bh=UazkFhnF58o243B84E27T2Q0kziPCzNQlAM0EXnPs+0=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=uJA4ra39P0Gv6naQSVcILGvPjCJywIq0xOMKparacmz05rvVX6Np5K3P9TBtMV+Q6 0fZMoqlsDqrvmOr81us08+gZah/AgsMkvZGLUZdvStWvkLQD9P6+IT1gNPD5kGzCTz OqQa78pHkuTPX/IwPxuJYSyFJgvQojor9CCfmM0cFvkyP8yz7FerO3C6Xj6Ja6G65F 0JNWOrnI2B9XClHx9c5DgYXjOqp4IU9Zrxk/YET9tVYaYS+dnGehAv9e/YWKzwcbHz D1LRfA0mXkjJ2/mnwlueLADrqQ5YuEa5UTY+rPKkoEu2iEn918jjZk3T8fIflFmK8o yGBq7Ksq4IKXA== Date: Fri, 15 Oct 2021 11:46:53 -0500 From: Bjorn Helgaas To: Andy Shevchenko Cc: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= , linux-pci , Sascha Hauer , Alexander Duyck , Alexander Shishkin , Andrew Donnellan , Arnaldo Carvalho de Melo , Arnd Bergmann , Benjamin Herrenschmidt , Bjorn Helgaas , Borislav Petkov , Boris Ostrovsky , "David S. Miller" , Fiona Trahe , Frederic Barrat , Giovanni Cabiddu , Greg Kroah-Hartman , Herbert Xu , "H. Peter Anvin" , Ido Schimmel , Ingo Molnar , Jack Xu , Jakub Kicinski , Jesse Brandeburg , Jiri Olsa , Jiri Pirko , Juergen Gross , Konrad Rzeszutek Wilk , Marco Chiappero , Mark Rutland , Mathias Nyman , Michael Buesch , Michael Ellerman , Namhyung Kim , Oliver O'Halloran , Paul Mackerras , Peter Zijlstra , =?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= , Russell Currey , Salil Mehta , Sathya Prakash , Simon Horman , Sreekanth Reddy , Stefano Stabellini , Suganath Prabu Subramani , Taras Chornyi , Thomas Gleixner , Tomaszx Kowalik , Vadym Kochan , Wojciech Ziemba , Yisen Zhuang , Zhou Wang , linux-crypto , Linux Kernel Mailing List , linux-perf-users@vger.kernel.org, "open list:LINUX FOR POWERPC PA SEMI PWRFICIENT" , linux-scsi , USB , "open list:TI WILINK WIRELES..." , MPT-FusionLinux.pdl@broadcom.com, netdev , oss-drivers@corigine.com, qat-linux@intel.com, "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" , xen-devel@lists.xenproject.org Subject: Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver Message-ID: <20211015164653.GA2108651@bhelgaas> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org On Wed, Oct 13, 2021 at 04:23:09PM +0300, Andy Shevchenko wrote: > On Wed, Oct 13, 2021 at 06:33:56AM -0500, Bjorn Helgaas wrote: > > On Wed, Oct 13, 2021 at 12:26:42PM +0300, Andy Shevchenko wrote: > > > On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas wrote: > > > > On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote: > > > > + return drv && drv->resume ? > > > > + drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev); > > > > > > One line? > > > > I don't think I touched that line. > > Then why they are both in + section? They're both in the + section of the interdiff because Uwe's v6 patch looks like this: static int pci_legacy_resume(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); - return drv && drv->resume ? - drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev); + if (pci_dev->dev.driver) { + struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver); + + if (drv->resume) + return drv->resume(pci_dev); + } + + return pci_pm_reenable_device(pci_dev); and my revision looks like this: static int pci_legacy_resume(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); - struct pci_driver *drv = pci_dev->driver; + struct pci_driver *drv = to_pci_driver(dev->driver); so compared to Uwe's v6, I restored that section to the original code. My goal here was to make the patch as simple and easy to review as possible. > > > > + struct pci_driver *drv = to_pci_driver(dev->dev.driver); > > > > const struct pci_error_handlers *err_handler = > > > > - dev->dev.driver ? to_pci_driver(dev->dev.driver)->err_handler : NULL; > > > > + drv ? drv->err_handler : NULL; > > > > > > Isn't dev->driver == to_pci_driver(dev->dev.driver)? > > > > Yes, I think so, but not sure what you're getting at here, can you > > elaborate? > > Getting pointer from another pointer seems waste of resources, why we > can't simply > > struct pci_driver *drv = dev->driver; I think this is in pci_dev_save_and_disable(), and "dev" here is a struct pci_dev *. We're removing the dev->driver member. Let me know if I'm still missing something. > > > > - "bad request in aer recovery " > > > > - "operation!\n"); > > > > + "bad request in AER recovery operation!\n"); > > > Stray change? Or is it in a separate patch in your tree? > > > > Could be skipped. The string now fits on one line so I combined it to > > make it more greppable. > > This is inconsistency in your changes, in one case you are objecting of > doing something close to the changed lines, in the other you are doing > unrelated change. You're right, this didn't make much sense in that patch. I moved the line join to the previous patch, which unindented this section and made space for this to fit on one line. Here's the revised commit: https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?id=34ab316d7287 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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3A2BBC433F5 for ; Fri, 15 Oct 2021 23:10:32 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 7BA716023D for ; Fri, 15 Oct 2021 23:10:31 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 7BA716023D Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.ozlabs.org Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4HWMSQ15L3z3cN1 for ; Sat, 16 Oct 2021 10:10:30 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256 header.s=k20201202 header.b=uJA4ra39; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=kernel.org (client-ip=198.145.29.99; helo=mail.kernel.org; envelope-from=helgaas@kernel.org; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256 header.s=k20201202 header.b=uJA4ra39; dkim-atps=neutral Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4HWBxt46KQz308J for ; Sat, 16 Oct 2021 03:46:58 +1100 (AEDT) Received: by mail.kernel.org (Postfix) with ESMTPSA id 34DF061164; Fri, 15 Oct 2021 16:46:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1634316415; bh=UazkFhnF58o243B84E27T2Q0kziPCzNQlAM0EXnPs+0=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=uJA4ra39P0Gv6naQSVcILGvPjCJywIq0xOMKparacmz05rvVX6Np5K3P9TBtMV+Q6 0fZMoqlsDqrvmOr81us08+gZah/AgsMkvZGLUZdvStWvkLQD9P6+IT1gNPD5kGzCTz OqQa78pHkuTPX/IwPxuJYSyFJgvQojor9CCfmM0cFvkyP8yz7FerO3C6Xj6Ja6G65F 0JNWOrnI2B9XClHx9c5DgYXjOqp4IU9Zrxk/YET9tVYaYS+dnGehAv9e/YWKzwcbHz D1LRfA0mXkjJ2/mnwlueLADrqQ5YuEa5UTY+rPKkoEu2iEn918jjZk3T8fIflFmK8o yGBq7Ksq4IKXA== Date: Fri, 15 Oct 2021 11:46:53 -0500 From: Bjorn Helgaas To: Andy Shevchenko Subject: Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver Message-ID: <20211015164653.GA2108651@bhelgaas> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Mailman-Approved-At: Sat, 16 Oct 2021 10:07:44 +1100 X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Giovanni Cabiddu , Mark Rutland , Sathya Prakash , Alexander Shishkin , Alexander Duyck , "maintainer:X86 ARCHITECTURE \(32-BIT AND 64-BIT\)" , qat-linux@intel.com, oss-drivers@corigine.com, Oliver O'Halloran , "H. Peter Anvin" , Jiri Olsa , Thomas Gleixner , Marco Chiappero , Stefano Stabellini , Herbert Xu , linux-scsi , =?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= , Jesse Brandeburg , Peter Zijlstra , Ingo Molnar , linux-pci , "open list:TI WILINK WIRELES..." , Jakub Kicinski , Yisen Zhuang , Suganath Prabu Subramani , Fiona Trahe , Andrew Donnellan , Arnd Bergmann , Konrad Rzeszutek Wilk , Ido Schimmel , Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= , Simon Horman , "open list:LINUX FOR POWERPC PA SEMI PWRFICIENT" , Arnaldo Carvalho de Melo , Jack Xu , Borislav Petkov , Michael Buesch , Jiri Pirko , Bjorn Helgaas , Namhyung Kim , Boris Ostrovsky , Juergen Gross , Salil Mehta , Sreekanth Reddy , xen-devel@lists.xenproject.org, Vadym Kochan , MPT-FusionLinux.pdl@broadcom.com, Greg Kroah-Hartman , USB , Wojciech Ziemba , Linux Kernel Mailing List , Mathias Nyman , Zhou Wang , linux-crypto , Sascha Hauer , netdev , Frederic Barrat , Paul Mackerras , Tomaszx Kowalik , Taras Chornyi , "David S. Miller" , linux-perf-users@vger.kernel.org Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Wed, Oct 13, 2021 at 04:23:09PM +0300, Andy Shevchenko wrote: > On Wed, Oct 13, 2021 at 06:33:56AM -0500, Bjorn Helgaas wrote: > > On Wed, Oct 13, 2021 at 12:26:42PM +0300, Andy Shevchenko wrote: > > > On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas wrote: > > > > On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote: > > > > + return drv && drv->resume ? > > > > + drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev); > > > > > > One line? > > > > I don't think I touched that line. > > Then why they are both in + section? They're both in the + section of the interdiff because Uwe's v6 patch looks like this: static int pci_legacy_resume(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); - return drv && drv->resume ? - drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev); + if (pci_dev->dev.driver) { + struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver); + + if (drv->resume) + return drv->resume(pci_dev); + } + + return pci_pm_reenable_device(pci_dev); and my revision looks like this: static int pci_legacy_resume(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); - struct pci_driver *drv = pci_dev->driver; + struct pci_driver *drv = to_pci_driver(dev->driver); so compared to Uwe's v6, I restored that section to the original code. My goal here was to make the patch as simple and easy to review as possible. > > > > + struct pci_driver *drv = to_pci_driver(dev->dev.driver); > > > > const struct pci_error_handlers *err_handler = > > > > - dev->dev.driver ? to_pci_driver(dev->dev.driver)->err_handler : NULL; > > > > + drv ? drv->err_handler : NULL; > > > > > > Isn't dev->driver == to_pci_driver(dev->dev.driver)? > > > > Yes, I think so, but not sure what you're getting at here, can you > > elaborate? > > Getting pointer from another pointer seems waste of resources, why we > can't simply > > struct pci_driver *drv = dev->driver; I think this is in pci_dev_save_and_disable(), and "dev" here is a struct pci_dev *. We're removing the dev->driver member. Let me know if I'm still missing something. > > > > - "bad request in aer recovery " > > > > - "operation!\n"); > > > > + "bad request in AER recovery operation!\n"); > > > Stray change? Or is it in a separate patch in your tree? > > > > Could be skipped. The string now fits on one line so I combined it to > > make it more greppable. > > This is inconsistency in your changes, in one case you are objecting of > doing something close to the changed lines, in the other you are doing > unrelated change. You're right, this didn't make much sense in that patch. I moved the line join to the previous patch, which unindented this section and made space for this to fit on one line. Here's the revised commit: https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?id=34ab316d7287