* [PATCH 0/4] PCI: Tidy up messages @ 2016-11-16 22:13 Bjorn Helgaas 2016-11-16 22:13 ` [PATCH 1/4] PCI: Remove service driver load/unload messages Bjorn Helgaas ` (3 more replies) 0 siblings, 4 replies; 9+ messages in thread From: Bjorn Helgaas @ 2016-11-16 22:13 UTC (permalink / raw) To: linux-pci Remove a few messages that don't contain any useful information, e.g., pcie_pme 0000:00:02.0:pcie01: service driver pcie_pme loaded pci_hotplug: PCI Hot Plug PCI Core version: 0.5 pciehp 0000:80:02.0:pcie04: service driver pciehp loaded pciehp: PCI Express Hot Plug Controller Driver version: 0.4 --- Bjorn Helgaas (4): PCI: Remove service driver load/unload messages PCI: hotplug: Remove hotplug core message PCI: pciehp: Remove loading message PCI: Expand "VPD access disabled" quirk message drivers/pci/hotplug/pci_hotplug_core.c | 10 +++------- drivers/pci/hotplug/pciehp_core.c | 9 ++++----- drivers/pci/pcie/portdrv_core.c | 3 --- drivers/pci/quirks.c | 2 +- 4 files changed, 8 insertions(+), 16 deletions(-) ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/4] PCI: Remove service driver load/unload messages 2016-11-16 22:13 [PATCH 0/4] PCI: Tidy up messages Bjorn Helgaas @ 2016-11-16 22:13 ` Bjorn Helgaas 2016-11-17 13:40 ` Lukas Wunner 2016-11-16 22:13 ` [PATCH 2/4] PCI: hotplug: Remove hotplug core message Bjorn Helgaas ` (2 subsequent siblings) 3 siblings, 1 reply; 9+ messages in thread From: Bjorn Helgaas @ 2016-11-16 22:13 UTC (permalink / raw) To: linux-pci Remove the "service driver %s loaded" and unloaded messages. I don't think these add any useful information. Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> --- drivers/pci/pcie/portdrv_core.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c index e9270b4..9698289 100644 --- a/drivers/pci/pcie/portdrv_core.c +++ b/drivers/pci/pcie/portdrv_core.c @@ -499,7 +499,6 @@ static int pcie_port_probe_service(struct device *dev) if (status) return status; - dev_printk(KERN_DEBUG, dev, "service driver %s loaded\n", driver->name); get_device(dev); return 0; } @@ -524,8 +523,6 @@ static int pcie_port_remove_service(struct device *dev) pciedev = to_pcie_device(dev); driver = to_service_driver(dev->driver); if (driver && driver->remove) { - dev_printk(KERN_DEBUG, dev, "unloading service driver %s\n", - driver->name); driver->remove(pciedev); put_device(dev); } ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] PCI: Remove service driver load/unload messages 2016-11-16 22:13 ` [PATCH 1/4] PCI: Remove service driver load/unload messages Bjorn Helgaas @ 2016-11-17 13:40 ` Lukas Wunner 2016-11-17 20:49 ` Bjorn Helgaas 0 siblings, 1 reply; 9+ messages in thread From: Lukas Wunner @ 2016-11-17 13:40 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: linux-pci On Wed, Nov 16, 2016 at 04:13:10PM -0600, Bjorn Helgaas wrote: > Remove the "service driver %s loaded" and unloaded messages. I don't think > these add any useful information. I think those particular ones have a value to some extent because they communicate for which of the port's capabilities a driver is available and loaded. For ports below a hotplug port they also comunicate the steps to unbind the ports from their drivers when the device is unplugged. E.g. when I plug in the Apple Thunderbolt Ethernet adapter I get this because a new hotplug port appears below the hotplug port of the host controller: [ 141.926865] pciehp 0000:0a:00.0:pcie204: service driver pciehp loaded On unplug I get this: [ 202.497548] pciehp 0000:0a:00.0:pcie204: unloading service driver pciehp Thanks, Lukas > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/pci/pcie/portdrv_core.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c > index e9270b4..9698289 100644 > --- a/drivers/pci/pcie/portdrv_core.c > +++ b/drivers/pci/pcie/portdrv_core.c > @@ -499,7 +499,6 @@ static int pcie_port_probe_service(struct device *dev) > if (status) > return status; > > - dev_printk(KERN_DEBUG, dev, "service driver %s loaded\n", driver->name); > get_device(dev); > return 0; > } > @@ -524,8 +523,6 @@ static int pcie_port_remove_service(struct device *dev) > pciedev = to_pcie_device(dev); > driver = to_service_driver(dev->driver); > if (driver && driver->remove) { > - dev_printk(KERN_DEBUG, dev, "unloading service driver %s\n", > - driver->name); > driver->remove(pciedev); > put_device(dev); > } > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] PCI: Remove service driver load/unload messages 2016-11-17 13:40 ` Lukas Wunner @ 2016-11-17 20:49 ` Bjorn Helgaas 2016-11-18 10:00 ` Lukas Wunner 0 siblings, 1 reply; 9+ messages in thread From: Bjorn Helgaas @ 2016-11-17 20:49 UTC (permalink / raw) To: Lukas Wunner; +Cc: Bjorn Helgaas, linux-pci On Thu, Nov 17, 2016 at 02:40:42PM +0100, Lukas Wunner wrote: > On Wed, Nov 16, 2016 at 04:13:10PM -0600, Bjorn Helgaas wrote: > > Remove the "service driver %s loaded" and unloaded messages. I don't think > > these add any useful information. > > I think those particular ones have a value to some extent because > they communicate for which of the port's capabilities a driver is > available and loaded. For ports below a hotplug port they also > comunicate the steps to unbind the ports from their drivers when > the device is unplugged. None of the service drivers can be modules anymore. I think if we want a clue about whether a service driver is attached to a device, we should add something to the service driver, where we can print more useful information. In general I want to move towards thinking about the service drivers as optional parts of the PCI core instead of separate "drivers." > E.g. when I plug in the Apple Thunderbolt Ethernet adapter I get this > because a new hotplug port appears below the hotplug port of the host > controller: > [ 141.926865] pciehp 0000:0a:00.0:pcie204: service driver pciehp loaded For example, for pciehp, you should already see something like this: pciehp 0000:80:02.0:pcie04: Slot #6 AttnBtn+ PwrCtrl+ MRL- AttnInd+ PwrInd+ HotPlug+ Surprise- Interlock- NoCompl- LLActRep+ Is that enough? Or maybe we don't emit this message in your case for some reason? > On unplug I get this: > [ 202.497548] pciehp 0000:0a:00.0:pcie204: unloading service driver pciehp I'm not sure we print anything when we remove a device; maybe we should. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > --- > > drivers/pci/pcie/portdrv_core.c | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c > > index e9270b4..9698289 100644 > > --- a/drivers/pci/pcie/portdrv_core.c > > +++ b/drivers/pci/pcie/portdrv_core.c > > @@ -499,7 +499,6 @@ static int pcie_port_probe_service(struct device *dev) > > if (status) > > return status; > > > > - dev_printk(KERN_DEBUG, dev, "service driver %s loaded\n", driver->name); > > get_device(dev); > > return 0; > > } > > @@ -524,8 +523,6 @@ static int pcie_port_remove_service(struct device *dev) > > pciedev = to_pcie_device(dev); > > driver = to_service_driver(dev->driver); > > if (driver && driver->remove) { > > - dev_printk(KERN_DEBUG, dev, "unloading service driver %s\n", > > - driver->name); > > driver->remove(pciedev); > > put_device(dev); > > } > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] PCI: Remove service driver load/unload messages 2016-11-17 20:49 ` Bjorn Helgaas @ 2016-11-18 10:00 ` Lukas Wunner 2016-11-18 14:45 ` Bjorn Helgaas 0 siblings, 1 reply; 9+ messages in thread From: Lukas Wunner @ 2016-11-18 10:00 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: linux-pci On Thu, Nov 17, 2016 at 02:49:34PM -0600, Bjorn Helgaas wrote: > On Thu, Nov 17, 2016 at 02:40:42PM +0100, Lukas Wunner wrote: > > On Wed, Nov 16, 2016 at 04:13:10PM -0600, Bjorn Helgaas wrote: > > > Remove the "service driver %s loaded" and unloaded messages. I don't think > > > these add any useful information. > > > > I think those particular ones have a value to some extent because > > they communicate for which of the port's capabilities a driver is > > available and loaded. For ports below a hotplug port they also > > comunicate the steps to unbind the ports from their drivers when > > the device is unplugged. > > None of the service drivers can be modules anymore. I think if we > want a clue about whether a service driver is attached to a device, we > should add something to the service driver, where we can print more > useful information. In general I want to move towards thinking about > the service drivers as optional parts of the PCI core instead of > separate "drivers." The issue with making the port services modules was that user space doesn't load them when they're needed. We could have solved this in the kernel by amending get_port_device_capability() to call find_module() and then try_module_get() to load the module for a port service and acquire a reference on it upon discovery of the services supported by a probed port. Then in pcie_port_device_remove() the references on the loaded modules would have to be released with module_put(). Most users just run the kernel that comes with their distro, and distro vendors frequently use a "one size fits all" kernel package regardless if the machine is a tiny netbook or a big-iron server. They will enable all port services to support all configurations and because this is all compiled in, the kernel keeps that code resident in memory even if the port services are never used. An uncompressed x86_64 vmlinux with the standard Debian configuration currently clocks in at around 30 MByte... > > E.g. when I plug in the Apple Thunderbolt Ethernet adapter I get this > > because a new hotplug port appears below the hotplug port of the host > > controller: > > [ 141.926865] pciehp 0000:0a:00.0:pcie204: service driver pciehp loaded > > For example, for pciehp, you should already see something like this: > > pciehp 0000:80:02.0:pcie04: Slot #6 AttnBtn+ PwrCtrl+ MRL- AttnInd+ PwrInd+ HotPlug+ Surprise- Interlock- NoCompl- LLActRep+ > > Is that enough? Or maybe we don't emit this message in your case for > some reason? It's sufficient but I'm not sure all port services print such a welcome message. Thanks, Lukas > > > On unplug I get this: > > [ 202.497548] pciehp 0000:0a:00.0:pcie204: unloading service driver pciehp > > I'm not sure we print anything when we remove a device; maybe we > should. > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > > --- > > > drivers/pci/pcie/portdrv_core.c | 3 --- > > > 1 file changed, 3 deletions(-) > > > > > > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c > > > index e9270b4..9698289 100644 > > > --- a/drivers/pci/pcie/portdrv_core.c > > > +++ b/drivers/pci/pcie/portdrv_core.c > > > @@ -499,7 +499,6 @@ static int pcie_port_probe_service(struct device *dev) > > > if (status) > > > return status; > > > > > > - dev_printk(KERN_DEBUG, dev, "service driver %s loaded\n", driver->name); > > > get_device(dev); > > > return 0; > > > } > > > @@ -524,8 +523,6 @@ static int pcie_port_remove_service(struct device *dev) > > > pciedev = to_pcie_device(dev); > > > driver = to_service_driver(dev->driver); > > > if (driver && driver->remove) { > > > - dev_printk(KERN_DEBUG, dev, "unloading service driver %s\n", > > > - driver->name); > > > driver->remove(pciedev); > > > put_device(dev); > > > } > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] PCI: Remove service driver load/unload messages 2016-11-18 10:00 ` Lukas Wunner @ 2016-11-18 14:45 ` Bjorn Helgaas 0 siblings, 0 replies; 9+ messages in thread From: Bjorn Helgaas @ 2016-11-18 14:45 UTC (permalink / raw) To: Lukas Wunner; +Cc: linux-pci On Fri, Nov 18, 2016 at 11:00:43AM +0100, Lukas Wunner wrote: > On Thu, Nov 17, 2016 at 02:49:34PM -0600, Bjorn Helgaas wrote: > > On Thu, Nov 17, 2016 at 02:40:42PM +0100, Lukas Wunner wrote: > > > On Wed, Nov 16, 2016 at 04:13:10PM -0600, Bjorn Helgaas wrote: > > > > Remove the "service driver %s loaded" and unloaded messages. I don't think > > > > these add any useful information. > > > > > > I think those particular ones have a value to some extent because > > > they communicate for which of the port's capabilities a driver is > > > available and loaded. For ports below a hotplug port they also > > > comunicate the steps to unbind the ports from their drivers when > > > the device is unplugged. > > > > None of the service drivers can be modules anymore. I think if we > > want a clue about whether a service driver is attached to a device, we > > should add something to the service driver, where we can print more > > useful information. In general I want to move towards thinking about > > the service drivers as optional parts of the PCI core instead of > > separate "drivers." > > The issue with making the port services modules was that user space > doesn't load them when they're needed. We could have solved this in > the kernel by amending get_port_device_capability() to call > find_module() and then try_module_get() to load the module for a port > service and acquire a reference on it upon discovery of the services > supported by a probed port. Then in pcie_port_device_remove() the > references on the loaded modules would have to be released with > module_put(). That's a possibility, although I'm skeptical of find_module() because there are only a handful of callers, and a generally useful feature should be much more common. But obviously I don't know much about this and I'm sure it could be done. My more serious objection is that a driver generally owns a piece of hardware exclusively, and that's not the case with these service drivers. They have to cooperate with each other and the core much more than normal drivers do. I also think there are some pieces that can't reasonably be done as modules because they really should be done during device enumeration. I think there are some AER/firmware-first issues that would be complicated if AER were a module. > > > E.g. when I plug in the Apple Thunderbolt Ethernet adapter I get this > > > because a new hotplug port appears below the hotplug port of the host > > > controller: > > > [ 141.926865] pciehp 0000:0a:00.0:pcie204: service driver pciehp loaded > > > > For example, for pciehp, you should already see something like this: > > > > pciehp 0000:80:02.0:pcie04: Slot #6 AttnBtn+ PwrCtrl+ MRL- AttnInd+ PwrInd+ HotPlug+ Surprise- Interlock- NoCompl- LLActRep+ > > > > Is that enough? Or maybe we don't emit this message in your case for > > some reason? > > It's sufficient but I'm not sure all port services print such a > welcome message. I'm not sure about that either. If there are some where we need a message, I'd rather add it to the service instead of logging the registration/unregistration. We don't do that for any other driver registration interfaces, AFAIK. Bjorn ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/4] PCI: hotplug: Remove hotplug core message 2016-11-16 22:13 [PATCH 0/4] PCI: Tidy up messages Bjorn Helgaas 2016-11-16 22:13 ` [PATCH 1/4] PCI: Remove service driver load/unload messages Bjorn Helgaas @ 2016-11-16 22:13 ` Bjorn Helgaas 2016-11-16 22:13 ` [PATCH 3/4] PCI: pciehp: Remove loading message Bjorn Helgaas 2016-11-16 22:13 ` [PATCH 4/4] PCI: Expand "VPD access disabled" quirk message Bjorn Helgaas 3 siblings, 0 replies; 9+ messages in thread From: Bjorn Helgaas @ 2016-11-16 22:13 UTC (permalink / raw) To: linux-pci Remove the "PCI Hot Plug PCI Core" version message. I don't think it contains any useful information. Remove unused #defines and move the author information to a comment. Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> --- drivers/pci/hotplug/pci_hotplug_core.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c index fea0b8b..56013d0 100644 --- a/drivers/pci/hotplug/pci_hotplug_core.c +++ b/drivers/pci/hotplug/pci_hotplug_core.c @@ -23,6 +23,9 @@ * * Send feedback to <kristen.c.accardi@intel.com> * + * Authors: + * Greg Kroah-Hartman <greg@kroah.com> + * Scott Murray <scottm@somanetworks.com> */ #include <linux/module.h> /* try_module_get & module_put */ @@ -50,15 +53,9 @@ #define info(format, arg...) printk(KERN_INFO "%s: " format, MY_NAME, ## arg) #define warn(format, arg...) printk(KERN_WARNING "%s: " format, MY_NAME, ## arg) - /* local variables */ static bool debug; -#define DRIVER_VERSION "0.5" -#define DRIVER_AUTHOR "Greg Kroah-Hartman <greg@kroah.com>, Scott Murray <scottm@somanetworks.com>" -#define DRIVER_DESC "PCI Hot Plug PCI Core" - - static LIST_HEAD(pci_hotplug_slot_list); static DEFINE_MUTEX(pci_hp_mutex); @@ -534,7 +531,6 @@ static int __init pci_hotplug_init(void) return result; } - info(DRIVER_DESC " version: " DRIVER_VERSION "\n"); return result; } device_initcall(pci_hotplug_init); ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/4] PCI: pciehp: Remove loading message 2016-11-16 22:13 [PATCH 0/4] PCI: Tidy up messages Bjorn Helgaas 2016-11-16 22:13 ` [PATCH 1/4] PCI: Remove service driver load/unload messages Bjorn Helgaas 2016-11-16 22:13 ` [PATCH 2/4] PCI: hotplug: Remove hotplug core message Bjorn Helgaas @ 2016-11-16 22:13 ` Bjorn Helgaas 2016-11-16 22:13 ` [PATCH 4/4] PCI: Expand "VPD access disabled" quirk message Bjorn Helgaas 3 siblings, 0 replies; 9+ messages in thread From: Bjorn Helgaas @ 2016-11-16 22:13 UTC (permalink / raw) To: linux-pci Remove the "PCI Express Hot Plug Controller Driver" version message. I don't think it contains any useful information. Remove unused #defines and move the author information to a comment. Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> --- drivers/pci/hotplug/pciehp_core.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c index 7d32fa33..35d8484 100644 --- a/drivers/pci/hotplug/pciehp_core.c +++ b/drivers/pci/hotplug/pciehp_core.c @@ -25,6 +25,10 @@ * * Send feedback to <greg@kroah.com>, <kristen.c.accardi@intel.com> * + * Authors: + * Dan Zink <dan.zink@compaq.com> + * Greg Kroah-Hartman <greg@kroah.com> + * Dely Sy <dely.l.sy@intel.com>" */ #include <linux/moduleparam.h> @@ -42,10 +46,6 @@ bool pciehp_poll_mode; int pciehp_poll_time; static bool pciehp_force; -#define DRIVER_VERSION "0.4" -#define DRIVER_AUTHOR "Dan Zink <dan.zink@compaq.com>, Greg Kroah-Hartman <greg@kroah.com>, Dely Sy <dely.l.sy@intel.com>" -#define DRIVER_DESC "PCI Express Hot Plug Controller Driver" - /* * not really modular, but the easiest way to keep compat with existing * bootargs behaviour is to continue using module_param here. @@ -333,7 +333,6 @@ static int __init pcied_init(void) retval = pcie_port_service_register(&hpdriver_portdrv); dbg("pcie_port_service_register = %d\n", retval); - info(DRIVER_DESC " version: " DRIVER_VERSION "\n"); if (retval) dbg("Failure to register service\n"); ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 4/4] PCI: Expand "VPD access disabled" quirk message 2016-11-16 22:13 [PATCH 0/4] PCI: Tidy up messages Bjorn Helgaas ` (2 preceding siblings ...) 2016-11-16 22:13 ` [PATCH 3/4] PCI: pciehp: Remove loading message Bjorn Helgaas @ 2016-11-16 22:13 ` Bjorn Helgaas 3 siblings, 0 replies; 9+ messages in thread From: Bjorn Helgaas @ 2016-11-16 22:13 UTC (permalink / raw) To: linux-pci It's not very enlightening to see pci 0000:07:00.0: [Firmware Bug]: VPD access disabled in the dmesg log because there's no clue about what the firmware bug is. Expand the message to explain why we're disabling VPD. Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> --- drivers/pci/quirks.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index c232729..7329796 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -2156,7 +2156,7 @@ static void quirk_blacklist_vpd(struct pci_dev *dev) { if (dev->vpd) { dev->vpd->len = 0; - dev_warn(&dev->dev, FW_BUG "VPD access disabled\n"); + dev_warn(&dev->dev, FW_BUG "disabling VPD access (can't determine size of non-standard VPD format)\n"); } } ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-11-18 14:46 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-16 22:13 [PATCH 0/4] PCI: Tidy up messages Bjorn Helgaas 2016-11-16 22:13 ` [PATCH 1/4] PCI: Remove service driver load/unload messages Bjorn Helgaas 2016-11-17 13:40 ` Lukas Wunner 2016-11-17 20:49 ` Bjorn Helgaas 2016-11-18 10:00 ` Lukas Wunner 2016-11-18 14:45 ` Bjorn Helgaas 2016-11-16 22:13 ` [PATCH 2/4] PCI: hotplug: Remove hotplug core message Bjorn Helgaas 2016-11-16 22:13 ` [PATCH 3/4] PCI: pciehp: Remove loading message Bjorn Helgaas 2016-11-16 22:13 ` [PATCH 4/4] PCI: Expand "VPD access disabled" quirk message Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).