From: Bjorn Helgaas <helgaas@kernel.org> To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> Cc: "Mark Rutland" <mark.rutland@arm.com>, "Giovanni Cabiddu" <giovanni.cabiddu@intel.com>, "Rafał Miłecki" <zajec5@gmail.com>, "Peter Zijlstra" <peterz@infradead.org>, linux-pci@vger.kernel.org, "Alexander Duyck" <alexanderduyck@fb.com>, "Russell Currey" <ruscur@russell.cc>, "Sathya Prakash" <sathya.prakash@broadcom.com>, oss-drivers@corigine.com, "Paul Mackerras" <paulus@samba.org>, "H. Peter Anvin" <hpa@zytor.com>, "Jiri Olsa" <jolsa@redhat.com>, "Boris Ostrovsky" <boris.ostrovsky@oracle.com>, linux-perf-users@vger.kernel.org, "Stefano Stabellini" <sstabellini@kernel.org>, "Herbert Xu" <herbert@gondor.apana.org.au>, linux-scsi@vger.kernel.org, "Michael Ellerman" <mpe@ellerman.id.au>, "Ido Schimmel" <idosch@nvidia.com>, x86@kernel.org, qat-linux@intel.com, "Alexander Shishkin" <alexander.shishkin@linux.intel.com>, "Ingo Molnar" <mingo@redhat.com>, "Benjamin Herrenschmidt" <benh@kernel.crashing.org>, linux-wireless@vger.kernel.org, "Jakub Kicinski" <kuba@kernel.org>, "Mathias Nyman" <mathias.nyman@intel.com>, "Yisen Zhuang" <yisen.zhuang@huawei.com>, "Fiona Trahe" <fiona.trahe@intel.com>, "Andrew Donnellan" <ajd@linux.ibm.com>, "Arnd Bergmann" <arnd@arndb.de>, "Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>, "Suganath Prabu Subramani" <suganath-prabu.subramani@broadcom.com>, "Simon Horman" <simon.horman@corigine.com>, "Arnaldo Carvalho de Melo" <acme@kernel.org>, "Borislav Petkov" <bp@alien8.de>, "Michael Buesch" <m@bues.ch>, "Jiri Pirko" <jiri@nvidia.com>, "Bjorn Helgaas" <bhelgaas@google.com>, "Namhyung Kim" <namhyung@kernel.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Andy Shevchenko" <andriy.shevchenko@intel.com>, "Juergen Gross" <jgross@suse.com>, "Salil Mehta" <salil.mehta@huawei.com>, "Sreekanth Reddy" <sreekanth.reddy@broadcom.com>, xen-devel@lists.xenproject.org, "Vadym Kochan" <vkochan@marvell.com>, MPT-FusionLinux.pdl@broadcom.com, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, linux-usb@vger.kernel.org, "Wojciech Ziemba" <wojciech.ziemba@intel.com>, linux-kernel@vger.kernel.org, "Taras Chornyi" <tchornyi@marvell.com>, "Zhou Wang" <wangzhou1@hisilicon.com>, linux-crypto@vger.kernel.org, kernel@pengutronix.de, netdev@vger.kernel.org, "Frederic Barrat" <fbarrat@linux.ibm.com>, "Oliver O'Halloran" <oohall@gmail.com>, linuxppc-dev@lists.ozlabs.org, "David S. Miller" <davem@davemloft.net> Subject: Re: [PATCH v2 0/6] PCI: Drop duplicated tracking of a pci_dev's bound driver Date: Fri, 6 Aug 2021 16:24:52 -0500 [thread overview] Message-ID: <20210806212452.GA1867870@bjorn-Precision-5520> (raw) In-Reply-To: <20210806064623.3lxl4clzbjpmchef@pengutronix.de> On Fri, Aug 06, 2021 at 08:46:23AM +0200, Uwe Kleine-König wrote: > On Thu, Aug 05, 2021 at 06:42:34PM -0500, Bjorn Helgaas wrote: > > I looked at all the bus_type.probe() methods, it looks like pci_dev is > > not the only offender here. At least the following also have a driver > > pointer in the device struct: > > > > parisc_device.driver > > acpi_device.driver > > dio_dev.driver > > hid_device.driver > > pci_dev.driver > > pnp_dev.driver > > rio_dev.driver > > zorro_dev.driver > > Right, when I converted zorro_dev it was pointed out that the code was > copied from pci and the latter has the same construct. :-) > See > https://lore.kernel.org/r/20210730191035.1455248-5-u.kleine-koenig@pengutronix.de > for the patch, I don't find where pci was pointed out, maybe it was on > irc only. Oh, thanks! I looked to see if you'd done something similar elsewhere, but I missed this one. > > Looking through the places that care about pci_dev.driver (the ones > > updated by patch 5/6), many of them are ... a little dubious to begin > > with. A few need the "struct pci_error_handlers *err_handler" > > pointer, so that's probably legitimate. But many just need a name, > > and should probably be using dev_driver_string() instead. > > Yeah, I considered adding a function to get the driver name from a > pci_dev and a function to get the error handlers. Maybe it's an idea to > introduce these two and then use to_pci_driver(pdev->dev.driver) for the > few remaining users? Maybe doing that on top of my current series makes > sense to have a clean switch from pdev->driver to pdev->dev.driver?! I'd propose using dev_driver_string() for these places: eeh_driver_name() (could change callers to use dev_driver_string()) bcma_host_pci_probe() qm_alloc_uacce() hns3_get_drvinfo() prestera_pci_probe() mlxsw_pci_probe() nfp_get_drvinfo() ssb_pcihost_probe() The use in mpt_device_driver_register() looks unnecessary: it's only to get a struct pci_device_id *, which is passed to ->probe() functions that don't need it. The use in adf_enable_aer() looks wrong: it sets the err_handler pointer in one of the adf_driver structs. I think those structs should be basically immutable, and the drivers that call adf_enable_aer() from their .probe() methods should set ".err_handler = &adf_err_handler" in their static adf_driver definitions instead. I think that basically leaves these: uncore_pci_probe() # .id_table, custom driver "registration" match_id() # .id_table, arch/x86/kernel/probe_roms.c xhci_pci_quirks() # .id_table pci_error_handlers() # roll-your-own AER handling, drivers/misc/cxl/guest.c I think it would be fine to use to_pci_driver(pdev->dev.driver) for these few. Bjorn
WARNING: multiple messages have this Message-ID (diff)
From: Bjorn Helgaas <helgaas@kernel.org> To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> Cc: "Mark Rutland" <mark.rutland@arm.com>, "Giovanni Cabiddu" <giovanni.cabiddu@intel.com>, "Rafał Miłecki" <zajec5@gmail.com>, "Peter Zijlstra" <peterz@infradead.org>, linux-pci@vger.kernel.org, "Alexander Duyck" <alexanderduyck@fb.com>, x86@kernel.org, oss-drivers@corigine.com, netdev@vger.kernel.org, "Paul Mackerras" <paulus@samba.org>, "H. Peter Anvin" <hpa@zytor.com>, "Jiri Olsa" <jolsa@redhat.com>, "Thomas Gleixner" <tglx@linutronix.de>, "Taras Chornyi" <tchornyi@marvell.com>, "Stefano Stabellini" <sstabellini@kernel.org>, "Herbert Xu" <herbert@gondor.apana.org.au>, linux-scsi@vger.kernel.org, "Sathya Prakash" <sathya.prakash@broadcom.com>, qat-linux@intel.com, "Alexander Shishkin" <alexander.shishkin@linux.intel.com>, "Ingo Molnar" <mingo@redhat.com>, "Jakub Kicinski" <kuba@kernel.org>, "Yisen Zhuang" <yisen.zhuang@huawei.com>, "Suganath Prabu Subramani" <suganath-prabu.subramani@broadcom.com>, "Fiona Trahe" <fiona.trahe@intel.com>, "Oliver O'Halloran" <oohall@gmail.com>, "Andrew Donnellan" <ajd@linux.ibm.com>, "Mathias Nyman" <mathias.nyman@intel.com>, "Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>, "Ido Schimmel" <idosch@nvidia.com>, "Arnaldo Carvalho de Melo" <acme@kernel.org>, "Frederic Barrat" <fbarrat@linux.ibm.com>, "Borislav Petkov" <bp@alien8.de>, "Michael Buesch" <m@bues.ch>, "Jiri Pirko" <jiri@nvidia.com>, "Bjorn Helgaas" <bhelgaas@google.com>, "Namhyung Kim" <namhyung@kernel.org>, "Boris Ostrovsky" <boris.ostrovsky@oracle.com>, "Andy Shevchenko" <andriy.shevchenko@intel.com>, "Juergen Gross" <jgross@suse.com>, "Salil Mehta" <salil.mehta@huawei.com>, "Sreekanth Reddy" <sreekanth.reddy@broadcom.com>, xen-devel@lists.xenproject.org, "Vadym Kochan" <vkochan@marvell.com>, MPT-FusionLinux.pdl@broadcom.com, linux-usb@vger.kernel.org, linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org, "Zhou Wang" <wangzhou1@hisilicon.com>, "Arnd Bergmann" <arnd@arndb.de>, linux-crypto@vger.kernel.org, kernel@pengutronix.de, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "Simon Horman" <simon.horman@corigine.com>, "Wojciech Ziemba" <wojciech.ziemba@intel.com>, linuxppc-dev@lists.ozlabs.org, "David S. Miller" <davem@davemloft.net> Subject: Re: [PATCH v2 0/6] PCI: Drop duplicated tracking of a pci_dev's bound driver Date: Fri, 6 Aug 2021 16:24:52 -0500 [thread overview] Message-ID: <20210806212452.GA1867870@bjorn-Precision-5520> (raw) In-Reply-To: <20210806064623.3lxl4clzbjpmchef@pengutronix.de> On Fri, Aug 06, 2021 at 08:46:23AM +0200, Uwe Kleine-König wrote: > On Thu, Aug 05, 2021 at 06:42:34PM -0500, Bjorn Helgaas wrote: > > I looked at all the bus_type.probe() methods, it looks like pci_dev is > > not the only offender here. At least the following also have a driver > > pointer in the device struct: > > > > parisc_device.driver > > acpi_device.driver > > dio_dev.driver > > hid_device.driver > > pci_dev.driver > > pnp_dev.driver > > rio_dev.driver > > zorro_dev.driver > > Right, when I converted zorro_dev it was pointed out that the code was > copied from pci and the latter has the same construct. :-) > See > https://lore.kernel.org/r/20210730191035.1455248-5-u.kleine-koenig@pengutronix.de > for the patch, I don't find where pci was pointed out, maybe it was on > irc only. Oh, thanks! I looked to see if you'd done something similar elsewhere, but I missed this one. > > Looking through the places that care about pci_dev.driver (the ones > > updated by patch 5/6), many of them are ... a little dubious to begin > > with. A few need the "struct pci_error_handlers *err_handler" > > pointer, so that's probably legitimate. But many just need a name, > > and should probably be using dev_driver_string() instead. > > Yeah, I considered adding a function to get the driver name from a > pci_dev and a function to get the error handlers. Maybe it's an idea to > introduce these two and then use to_pci_driver(pdev->dev.driver) for the > few remaining users? Maybe doing that on top of my current series makes > sense to have a clean switch from pdev->driver to pdev->dev.driver?! I'd propose using dev_driver_string() for these places: eeh_driver_name() (could change callers to use dev_driver_string()) bcma_host_pci_probe() qm_alloc_uacce() hns3_get_drvinfo() prestera_pci_probe() mlxsw_pci_probe() nfp_get_drvinfo() ssb_pcihost_probe() The use in mpt_device_driver_register() looks unnecessary: it's only to get a struct pci_device_id *, which is passed to ->probe() functions that don't need it. The use in adf_enable_aer() looks wrong: it sets the err_handler pointer in one of the adf_driver structs. I think those structs should be basically immutable, and the drivers that call adf_enable_aer() from their .probe() methods should set ".err_handler = &adf_err_handler" in their static adf_driver definitions instead. I think that basically leaves these: uncore_pci_probe() # .id_table, custom driver "registration" match_id() # .id_table, arch/x86/kernel/probe_roms.c xhci_pci_quirks() # .id_table pci_error_handlers() # roll-your-own AER handling, drivers/misc/cxl/guest.c I think it would be fine to use to_pci_driver(pdev->dev.driver) for these few. Bjorn
next prev parent reply other threads:[~2021-08-06 21:24 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-08-03 10:01 [PATCH v2 0/6] PCI: Drop duplicated tracking of a pci_dev's bound driver Uwe Kleine-König 2021-08-03 10:01 ` Uwe Kleine-König 2021-08-03 10:01 ` [PATCH v2 1/6] PCI: Simplify pci_device_remove() Uwe Kleine-König 2021-08-03 10:01 ` [PATCH v2 2/6] PCI: Drop useless check from pci_device_probe() Uwe Kleine-König 2021-08-03 10:01 ` [PATCH v2 3/6] xen/pci: Drop some checks that are always true Uwe Kleine-König 2021-08-03 13:50 ` Boris Ostrovsky 2021-08-03 10:01 ` [PATCH v2 4/6] PCI: Provide wrapper to access a pci_dev's bound driver Uwe Kleine-König 2021-08-03 10:01 ` Uwe Kleine-König 2021-08-03 14:37 ` Andy Shevchenko 2021-08-03 14:37 ` Andy Shevchenko 2021-08-03 10:01 ` [PATCH v2 5/6] PCI: Adapt all code locations to not use struct pci_dev::driver directly Uwe Kleine-König 2021-08-03 10:01 ` Uwe Kleine-König 2021-08-03 13:53 ` Boris Ostrovsky 2021-08-03 13:53 ` Boris Ostrovsky 2021-08-03 14:38 ` Andy Shevchenko 2021-08-03 14:38 ` Andy Shevchenko 2021-08-03 14:46 ` Ido Schimmel 2021-08-03 14:46 ` Ido Schimmel 2021-08-05 15:09 ` Andrew Donnellan 2021-08-05 15:09 ` Andrew Donnellan 2021-08-03 10:01 ` [PATCH v2 6/6] PCI: Drop duplicated tracking of a pci_dev's bound driver Uwe Kleine-König 2021-08-03 10:01 ` Uwe Kleine-König 2021-08-05 23:42 ` [PATCH v2 0/6] " Bjorn Helgaas 2021-08-05 23:42 ` Bjorn Helgaas 2021-08-06 6:46 ` Uwe Kleine-König 2021-08-06 6:46 ` Uwe Kleine-König 2021-08-06 21:24 ` Bjorn Helgaas [this message] 2021-08-06 21:24 ` Bjorn Helgaas 2021-08-07 9:26 ` Uwe Kleine-König 2021-08-07 9:26 ` Uwe Kleine-König 2021-08-09 18:14 ` Bjorn Helgaas 2021-08-09 18:14 ` Bjorn Helgaas
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=20210806212452.GA1867870@bjorn-Precision-5520 \ --to=helgaas@kernel.org \ --cc=MPT-FusionLinux.pdl@broadcom.com \ --cc=acme@kernel.org \ --cc=ajd@linux.ibm.com \ --cc=alexander.shishkin@linux.intel.com \ --cc=alexanderduyck@fb.com \ --cc=andriy.shevchenko@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=davem@davemloft.net \ --cc=fbarrat@linux.ibm.com \ --cc=fiona.trahe@intel.com \ --cc=giovanni.cabiddu@intel.com \ --cc=gregkh@linuxfoundation.org \ --cc=herbert@gondor.apana.org.au \ --cc=hpa@zytor.com \ --cc=idosch@nvidia.com \ --cc=jgross@suse.com \ --cc=jiri@nvidia.com \ --cc=jolsa@redhat.com \ --cc=kernel@pengutronix.de \ --cc=konrad.wilk@oracle.com \ --cc=kuba@kernel.org \ --cc=linux-crypto@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=linux-perf-users@vger.kernel.org \ --cc=linux-scsi@vger.kernel.org \ --cc=linux-usb@vger.kernel.org \ --cc=linux-wireless@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=m@bues.ch \ --cc=mark.rutland@arm.com \ --cc=mathias.nyman@intel.com \ --cc=mingo@redhat.com \ --cc=mpe@ellerman.id.au \ --cc=namhyung@kernel.org \ --cc=netdev@vger.kernel.org \ --cc=oohall@gmail.com \ --cc=oss-drivers@corigine.com \ --cc=paulus@samba.org \ --cc=peterz@infradead.org \ --cc=qat-linux@intel.com \ --cc=ruscur@russell.cc \ --cc=salil.mehta@huawei.com \ --cc=sathya.prakash@broadcom.com \ --cc=simon.horman@corigine.com \ --cc=sreekanth.reddy@broadcom.com \ --cc=sstabellini@kernel.org \ --cc=suganath-prabu.subramani@broadcom.com \ --cc=tchornyi@marvell.com \ --cc=tglx@linutronix.de \ --cc=u.kleine-koenig@pengutronix.de \ --cc=vkochan@marvell.com \ --cc=wangzhou1@hisilicon.com \ --cc=wojciech.ziemba@intel.com \ --cc=x86@kernel.org \ --cc=xen-devel@lists.xenproject.org \ --cc=yisen.zhuang@huawei.com \ --cc=zajec5@gmail.com \ /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.