From: Andrew Donnellan <ajd@linux.ibm.com> To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> Cc: Bjorn Helgaas <helgaas@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Peter Zijlstra <peterz@infradead.org>, Benjamin Herrenschmidt <benh@kernel.crashing.org>, "H. Peter Anvin" <hpa@zytor.com>, "Oliver O'Halloran" <oohall@gmail.com>, Russell Currey <ruscur@russell.cc>, Jiri Olsa <jolsa@redhat.com>, Christoph Hellwig <hch@lst.de>, Stefano Stabellini <sstabellini@kernel.org>, Mathias Nyman <mathias.nyman@intel.com>, Michael Ellerman <mpe@ellerman.id.au>, x86@kernel.org, Alexander Shishkin <alexander.shishkin@linux.intel.com>, Ingo Molnar <mingo@redhat.com>, linux-pci@vger.kernel.org, xen-devel@lists.xenproject.org, Juergen Gross <jgross@suse.com>, Arnd Bergmann <arnd@arndb.de>, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, Arnaldo Carvalho de Melo <acme@kernel.org>, Borislav Petkov <bp@alien8.de>, Bjorn Helgaas <bhelgaas@google.com>, Namhyung Kim <namhyung@kernel.org>, Thomas Gleixner <tglx@linutronix.de>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, linux-usb@vger.kernel.org, linux-perf-users@vger.kernel.org, kernel@pengutronix.de, Frederic Barrat <fbarrat@linux.ibm.com>, Paul Mackerras <paulus@samba.org>, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH v5 10/11] PCI: Replace pci_dev::driver usage by pci_dev::dev.driver Date: Thu, 30 Sep 2021 01:44:44 +1000 [thread overview] Message-ID: <1414b3c5-167c-c271-baed-d3d7f6cd0309@linux.ibm.com> (raw) In-Reply-To: <20210929134330.e5c57t7mtwu5iner@pengutronix.de> On 29/9/21 11:43 pm, Uwe Kleine-König wrote:> I'm not a huge fan either, I used it to keep the control flow as is and > without introducing several calls to to_pci_driver. > > The whole code looks as follows: > > list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) { > struct pci_driver *afu_drv; > if (afu_dev->dev.driver && > (afu_drv = to_pci_driver(afu_dev->dev.driver))->err_handler && > afu_drv->err_handler->resume) > afu_drv->err_handler->resume(afu_dev); > } > > Without assignment in the if it could look as follows: > > list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) { > struct pci_driver *afu_drv; > > if (!afu_dev->dev.driver) > continue; > > afu_drv = to_pci_driver(afu_dev->dev.driver)); > > if (afu_drv->err_handler && afu_drv->err_handler->resume) > afu_drv->err_handler->resume(afu_dev); > } > > Fine for me. This looks fine. As an aside while writing my email I discovered the existence of container_of_safe(), a version of container_of() that handles the null and err ptr cases... if to_pci_driver() used that, the null check in the caller could be moved until after the to_pci_driver() call which would be neater. But then, grep tells me that container_of_safe() is used precisely zero times in the entire tree. Interesting. > (Sidenote: What happens if the device is unbound directly after the > check for afu_dev->dev.driver? This is a problem the old code had, too > (assuming it is a real problem, didn't check deeply).) Looking at any of the cxl PCI error handling paths brings back nightmares from a few years ago... Fred: I wonder if we need to add a lock here? -- Andrew Donnellan OzLabs, ADL Canberra ajd@linux.ibm.com IBM Australia Limited
WARNING: multiple messages have this Message-ID (diff)
From: Andrew Donnellan <ajd@linux.ibm.com> To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> Cc: Mark Rutland <mark.rutland@arm.com>, Peter Zijlstra <peterz@infradead.org>, "H. Peter Anvin" <hpa@zytor.com>, Oliver O'Halloran <oohall@gmail.com>, Jiri Olsa <jolsa@redhat.com>, Christoph Hellwig <hch@lst.de>, Stefano Stabellini <sstabellini@kernel.org>, Arnd Bergmann <arnd@arndb.de>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, x86@kernel.org, Alexander Shishkin <alexander.shishkin@linux.intel.com>, Ingo Molnar <mingo@redhat.com>, Bjorn Helgaas <helgaas@kernel.org>, linux-pci@vger.kernel.org, xen-devel@lists.xenproject.org, Mathias Nyman <mathias.nyman@intel.com>, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, Arnaldo Carvalho de Melo <acme@kernel.org>, Borislav Petkov <bp@alien8.de>, Bjorn Helgaas <bhelgaas@google.com>, Namhyung Kim <namhyung@kernel.org>, Thomas Gleixner <tglx@linutronix.de>, Juergen Gross <jgross@suse.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, linux-usb@vger.kernel.org, linux-perf-users@vger.kernel.org, kernel@pengutronix.de, Frederic Barrat <fbarrat@linux.ibm.com>, Paul Mackerras <paulus@samba.org>, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH v5 10/11] PCI: Replace pci_dev::driver usage by pci_dev::dev.driver Date: Thu, 30 Sep 2021 01:44:44 +1000 [thread overview] Message-ID: <1414b3c5-167c-c271-baed-d3d7f6cd0309@linux.ibm.com> (raw) In-Reply-To: <20210929134330.e5c57t7mtwu5iner@pengutronix.de> On 29/9/21 11:43 pm, Uwe Kleine-König wrote:> I'm not a huge fan either, I used it to keep the control flow as is and > without introducing several calls to to_pci_driver. > > The whole code looks as follows: > > list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) { > struct pci_driver *afu_drv; > if (afu_dev->dev.driver && > (afu_drv = to_pci_driver(afu_dev->dev.driver))->err_handler && > afu_drv->err_handler->resume) > afu_drv->err_handler->resume(afu_dev); > } > > Without assignment in the if it could look as follows: > > list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) { > struct pci_driver *afu_drv; > > if (!afu_dev->dev.driver) > continue; > > afu_drv = to_pci_driver(afu_dev->dev.driver)); > > if (afu_drv->err_handler && afu_drv->err_handler->resume) > afu_drv->err_handler->resume(afu_dev); > } > > Fine for me. This looks fine. As an aside while writing my email I discovered the existence of container_of_safe(), a version of container_of() that handles the null and err ptr cases... if to_pci_driver() used that, the null check in the caller could be moved until after the to_pci_driver() call which would be neater. But then, grep tells me that container_of_safe() is used precisely zero times in the entire tree. Interesting. > (Sidenote: What happens if the device is unbound directly after the > check for afu_dev->dev.driver? This is a problem the old code had, too > (assuming it is a real problem, didn't check deeply).) Looking at any of the cxl PCI error handling paths brings back nightmares from a few years ago... Fred: I wonder if we need to add a lock here? -- Andrew Donnellan OzLabs, ADL Canberra ajd@linux.ibm.com IBM Australia Limited
next prev parent reply other threads:[~2021-09-29 15:46 UTC|newest] Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-09-29 8:52 [PATCH v5 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver Uwe Kleine-König 2021-09-29 8:52 ` Uwe Kleine-König 2021-09-29 8:52 ` [PATCH v5 01/11] PCI: Simplify pci_device_remove() Uwe Kleine-König 2021-09-29 8:52 ` [PATCH v5 02/11] PCI: Drop useless check from pci_device_probe() Uwe Kleine-König 2021-09-29 8:52 ` [PATCH v5 03/11] xen/pci: Drop some checks that are always true Uwe Kleine-König 2021-09-29 8:52 ` [PATCH v5 04/11] bcma: simplify reference to the driver's name Uwe Kleine-König 2021-09-29 8:53 ` [PATCH v5 05/11] powerpc/eeh: Don't use driver member of struct pci_dev and further cleanups Uwe Kleine-König 2021-09-29 8:53 ` Uwe Kleine-König 2021-09-29 8:53 ` [PATCH v5 06/11] ssb: Simplify determination of driver name Uwe Kleine-König 2021-09-29 11:13 ` Michael Büsch 2021-09-29 8:53 ` [PATCH v5 07/11] PCI: Replace pci_dev::driver usage that gets the " Uwe Kleine-König 2021-09-29 8:53 ` Uwe Kleine-König 2021-09-29 10:17 ` Simon Horman 2021-09-29 10:17 ` Simon Horman 2021-09-29 14:44 ` Ido Schimmel 2021-09-29 14:44 ` Ido Schimmel 2021-09-29 15:21 ` Ido Schimmel 2021-09-29 15:21 ` Ido Schimmel 2021-09-29 8:53 ` [PATCH v5 08/11] scsi: message: fusion: Remove unused parameter of mpt_pci driver's probe() Uwe Kleine-König 2021-09-29 8:53 ` [PATCH v5 09/11] crypto: qat - simplify adf_enable_aer() Uwe Kleine-König 2021-09-29 8:53 ` [PATCH v5 10/11] PCI: Replace pci_dev::driver usage by pci_dev::dev.driver Uwe Kleine-König 2021-09-29 8:53 ` Uwe Kleine-König 2021-09-29 13:15 ` Andrew Donnellan 2021-09-29 13:15 ` Andrew Donnellan 2021-09-29 13:43 ` Uwe Kleine-König 2021-09-29 13:43 ` Uwe Kleine-König 2021-09-29 15:44 ` Andrew Donnellan [this message] 2021-09-29 15:44 ` Andrew Donnellan 2021-09-30 13:48 ` Frederic Barrat 2021-09-30 13:48 ` Frederic Barrat 2021-09-29 8:53 ` [PATCH v5 11/11] PCI: Drop duplicated tracking of a pci_dev's bound driver Uwe Kleine-König
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1414b3c5-167c-c271-baed-d3d7f6cd0309@linux.ibm.com \ --to=ajd@linux.ibm.com \ --cc=acme@kernel.org \ --cc=alexander.shishkin@linux.intel.com \ --cc=arnd@arndb.de \ --cc=benh@kernel.crashing.org \ --cc=bhelgaas@google.com \ --cc=boris.ostrovsky@oracle.com \ --cc=bp@alien8.de \ --cc=fbarrat@linux.ibm.com \ --cc=gregkh@linuxfoundation.org \ --cc=hch@lst.de \ --cc=helgaas@kernel.org \ --cc=hpa@zytor.com \ --cc=jgross@suse.com \ --cc=jolsa@redhat.com \ --cc=kernel@pengutronix.de \ --cc=konrad.wilk@oracle.com \ --cc=linux-pci@vger.kernel.org \ --cc=linux-perf-users@vger.kernel.org \ --cc=linux-usb@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mark.rutland@arm.com \ --cc=mathias.nyman@intel.com \ --cc=mingo@redhat.com \ --cc=mpe@ellerman.id.au \ --cc=namhyung@kernel.org \ --cc=oohall@gmail.com \ --cc=paulus@samba.org \ --cc=peterz@infradead.org \ --cc=ruscur@russell.cc \ --cc=sstabellini@kernel.org \ --cc=tglx@linutronix.de \ --cc=u.kleine-koenig@pengutronix.de \ --cc=x86@kernel.org \ --cc=xen-devel@lists.xenproject.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.