[PATCH 0/4] PCI: Tidy up messages

[PATCH 0/4] PCI: Tidy up messages

[Bjorn Helgaas]

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(-)

[PATCH 1/4] PCI: Remove service driver load/unload messages

[Bjorn Helgaas]

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);
 	}

[PATCH 2/4] PCI: hotplug: Remove hotplug core message

[Bjorn Helgaas]

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);

[PATCH 3/4] PCI: pciehp: Remove loading message

[Bjorn Helgaas]

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");
 

[PATCH 4/4] PCI: Expand "VPD access disabled" quirk message

[Bjorn Helgaas]

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");
 	}
 }
 

Re: [PATCH 1/4] PCI: Remove service driver load/unload messages

[Lukas Wunner]

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);
>  	}
> 

Re: [PATCH 1/4] PCI: Remove service driver load/unload messages

[Bjorn Helgaas]

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

Re: [PATCH 1/4] PCI: Remove service driver load/unload messages

[Lukas Wunner]

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);
> > >  	}
> > > 

Re: [PATCH 1/4] PCI: Remove service driver load/unload messages

[Bjorn Helgaas]

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