[1/2,pci] pci: Addition of PCI_DEV_FLAGS_QUIET_PCI_REALIGN attribute to the PCI subsystem

Message ID 20180314163455.15854-2-desnesn@linux.vnet.ibm.com
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series
  • Silent PCI realignment messages during boot
Related show

Commit Message

Desnes A. Nunes do Rosario March 14, 2018, 4:34 p.m.
Add PCI_DEV_FLAGS_QUIET_PCI_REALIGN to pci_dev_flags and use it to
silent PCI realignment messages if the flag is turned on by a driver.

Signed-off-by: Desnes A. Nunes do Rosario <desnesn@linux.vnet.ibm.com>
---
 drivers/pci/pci.c       | 3 ++-
 drivers/pci/setup-res.c | 3 ++-
 include/linux/pci.h     | 2 ++
 3 files changed, 6 insertions(+), 2 deletions(-)

Comments

Andy Shevchenko March 14, 2018, 5:41 p.m. | #1
On Wed, Mar 14, 2018 at 6:34 PM, Desnes A. Nunes do Rosario
<desnesn@linux.vnet.ibm.com> wrote:
> Add PCI_DEV_FLAGS_QUIET_PCI_REALIGN to pci_dev_flags and use it to
> silent PCI realignment messages if the flag is turned on by a driver.

It doesn't explain "Why?"
Why the driver needs that?

Another approach is to increase level of the message. Would it be
accepted by you (in case Bjorn agrees)?

> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -205,6 +205,8 @@ enum pci_dev_flags {
>         PCI_DEV_FLAGS_NO_FLR_RESET = (__force pci_dev_flags_t) (1 << 10),
>         /* Don't use Relaxed Ordering for TLPs directed at this device */
>         PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11),
> +       /* Silent PCI resource realignment messages */
> +       PCI_DEV_FLAGS_QUIET_PCI_REALIGN = (__force pci_dev_flags_t) (1 << 12),

I would rather name it _NO_PCI_REALIGN_MSG

>  };
Andy Shevchenko March 14, 2018, 5:42 p.m. | #2
On Wed, Mar 14, 2018 at 7:41 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

> I would rather name it _NO_PCI_REALIGN_MSG

I meant _NO_REALIGN_MSG of course (PCI is already at the beginning).
Bjorn Helgaas March 14, 2018, 6:06 p.m. | #3
On Wed, Mar 14, 2018 at 01:34:54PM -0300, Desnes A. Nunes do Rosario wrote:
> Add PCI_DEV_FLAGS_QUIET_PCI_REALIGN to pci_dev_flags and use it to
> silent PCI realignment messages if the flag is turned on by a driver.
> 
> Signed-off-by: Desnes A. Nunes do Rosario <desnesn@linux.vnet.ibm.com>
> ---
>  drivers/pci/pci.c       | 3 ++-
>  drivers/pci/setup-res.c | 3 ++-
>  include/linux/pci.h     | 2 ++
>  3 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8c71d1a66cdd..be197c944e5f 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5505,7 +5505,8 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
>  		return;
>  	}
>  
> -	pci_info(dev, "Disabling memory decoding and releasing memory resources\n");
> +	if (!(dev->dev_flags & PCI_DEV_FLAGS_QUIET_PCI_REALIGN))
> +		pci_info(dev, "Disabling memory decoding and releasing memory resources\n");
>  	pci_read_config_word(dev, PCI_COMMAND, &command);
>  	command &= ~PCI_COMMAND_MEMORY;
>  	pci_write_config_word(dev, PCI_COMMAND, command);
> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> index 369d48d6c6f1..00a538def763 100644
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c
> @@ -172,7 +172,8 @@ EXPORT_SYMBOL(pci_claim_resource);
>  
>  void pci_disable_bridge_window(struct pci_dev *dev)
>  {
> -	pci_info(dev, "disabling bridge mem windows\n");
> +	if (!(dev->dev_flags & PCI_DEV_FLAGS_QUIET_PCI_REALIGN))
> +		pci_info(dev, "disabling bridge mem windows\n");

As far as I'm concerned, we can just remove these messages completely.
I don't think there's any real value there.

>  	/* MMIO Base/Limit */
>  	pci_write_config_dword(dev, PCI_MEMORY_BASE, 0x0000fff0);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index e057e8cc63e7..993f9c7dcc00 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -205,6 +205,8 @@ enum pci_dev_flags {
>  	PCI_DEV_FLAGS_NO_FLR_RESET = (__force pci_dev_flags_t) (1 << 10),
>  	/* Don't use Relaxed Ordering for TLPs directed at this device */
>  	PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11),
> +	/* Silent PCI resource realignment messages */
> +	PCI_DEV_FLAGS_QUIET_PCI_REALIGN = (__force pci_dev_flags_t) (1 << 12),
>  };
>  
>  enum pci_irq_reroute_variant {
> -- 
> 2.14.3
>
Desnes A. Nunes do Rosario March 14, 2018, 6:09 p.m. | #4
Hello Andy,

On 03/14/2018 02:41 PM, Andy Shevchenko wrote:
> On Wed, Mar 14, 2018 at 6:34 PM, Desnes A. Nunes do Rosario
> <desnesn@linux.vnet.ibm.com> wrote:
>> Add PCI_DEV_FLAGS_QUIET_PCI_REALIGN to pci_dev_flags and use it to
>> silent PCI realignment messages if the flag is turned on by a driver.
> 
> It doesn't explain "Why?"
> Why the driver needs that?

I have written down the reason on the cover letter, but I concur on 
creating a second version of the patch's comment. Basically, all PCI 
resources on powerpc are printing out expected realignment messages 
which are flooding the systems logs.

Perhaps this would be better?
---
"Some architectures such as powerpc has aligned all of its PCI resources 
to its PAGE_SIZE during boot, thus the system logs will be flooded by 
expected realignment messages, which can be interpreted as a false 
positive for total PCI failure on the system.

[root@system user]# dmesg | grep -i disabling
[    0.692270] pci 0000:00:00.0: Disabling memory decoding and releasing 
memory resources
[    0.692324] pci 0000:00:00.0: disabling bridge mem windows
[    0.729134] pci 0001:00:00.0: Disabling memory decoding and releasing 
memory resources
[    0.737352] pci 0001:00:00.0: disabling bridge mem windows
[    0.776295] pci 0002:00:00.0: Disabling memory decoding and releasing 
memory resources
[    0.784509] pci 0002:00:00.0: disabling bridge mem windows
... and goes on for all PCI devices ...

Thus, this patch adds PCI_DEV_FLAGS_NO_REALIGN_MSG to pci_dev_flags and 
uses it to silent PCI realignment messages if the flag is turned on by a 
driver.
"
---

> 
> Another approach is to increase level of the message. Would it be
> accepted by you (in case Bjorn agrees)?
> 
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -205,6 +205,8 @@ enum pci_dev_flags {
>>          PCI_DEV_FLAGS_NO_FLR_RESET = (__force pci_dev_flags_t) (1 << 10),
>>          /* Don't use Relaxed Ordering for TLPs directed at this device */
>>          PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11),
>> +       /* Silent PCI resource realignment messages */
>> +       PCI_DEV_FLAGS_QUIET_PCI_REALIGN = (__force pci_dev_flags_t) (1 << 12),
> 
> I would rather name it _NO_PCI_REALIGN_MSG

I concur on changing it to PCI_DEV_FLAGS_NO_REALIGN_MSG in a second 
version of the patchset.

> 
>>   };
> 

Thank you very much for your review,
Desnes A. Nunes do Rosario March 14, 2018, 6:22 p.m. | #5
Hello Bjorn,

On 03/14/2018 03:06 PM, Bjorn Helgaas wrote:
> On Wed, Mar 14, 2018 at 01:34:54PM -0300, Desnes A. Nunes do Rosario wrote:
>> Add PCI_DEV_FLAGS_QUIET_PCI_REALIGN to pci_dev_flags and use it to
>> silent PCI realignment messages if the flag is turned on by a driver.
>>
>> Signed-off-by: Desnes A. Nunes do Rosario <desnesn@linux.vnet.ibm.com>
>> ---
>>   drivers/pci/pci.c       | 3 ++-
>>   drivers/pci/setup-res.c | 3 ++-
>>   include/linux/pci.h     | 2 ++
>>   3 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 8c71d1a66cdd..be197c944e5f 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -5505,7 +5505,8 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
>>   		return;
>>   	}
>>   
>> -	pci_info(dev, "Disabling memory decoding and releasing memory resources\n");
>> +	if (!(dev->dev_flags & PCI_DEV_FLAGS_QUIET_PCI_REALIGN))
>> +		pci_info(dev, "Disabling memory decoding and releasing memory resources\n");
>>   	pci_read_config_word(dev, PCI_COMMAND, &command);
>>   	command &= ~PCI_COMMAND_MEMORY;
>>   	pci_write_config_word(dev, PCI_COMMAND, command);
>> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
>> index 369d48d6c6f1..00a538def763 100644
>> --- a/drivers/pci/setup-res.c
>> +++ b/drivers/pci/setup-res.c
>> @@ -172,7 +172,8 @@ EXPORT_SYMBOL(pci_claim_resource);
>>   
>>   void pci_disable_bridge_window(struct pci_dev *dev)
>>   {
>> -	pci_info(dev, "disabling bridge mem windows\n");
>> +	if (!(dev->dev_flags & PCI_DEV_FLAGS_QUIET_PCI_REALIGN))
>> +		pci_info(dev, "disabling bridge mem windows\n");
> 
> As far as I'm concerned, we can just remove these messages completely.
> I don't think there's any real value there.

After I found out that this was happening to all PCI devices on powerpc 
due to the __weak
pcibios_default_alignment() interface (necessary for VFIO passthrough 
and performance), I confess that this was my first approach to this 
matter; however I couldn't vouch the need of these messages on other 
architectures.

If there are no further concerns, I definitely prefer sending a second 
version of this patch only eliminating these messages and attesting the 
reason why.

Thank you very much for your review Bjorn,
Bjorn Helgaas March 14, 2018, 6:55 p.m. | #6
On Wed, Mar 14, 2018 at 03:22:44PM -0300, Desnes Augusto Nunes do Rosário wrote:
> Hello Bjorn,
> 
> On 03/14/2018 03:06 PM, Bjorn Helgaas wrote:
> > On Wed, Mar 14, 2018 at 01:34:54PM -0300, Desnes A. Nunes do Rosario wrote:
> > > Add PCI_DEV_FLAGS_QUIET_PCI_REALIGN to pci_dev_flags and use it to
> > > silent PCI realignment messages if the flag is turned on by a driver.
> > > 
> > > Signed-off-by: Desnes A. Nunes do Rosario <desnesn@linux.vnet.ibm.com>
> > > ---
> > >   drivers/pci/pci.c       | 3 ++-
> > >   drivers/pci/setup-res.c | 3 ++-
> > >   include/linux/pci.h     | 2 ++
> > >   3 files changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 8c71d1a66cdd..be197c944e5f 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -5505,7 +5505,8 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
> > >   		return;
> > >   	}
> > > -	pci_info(dev, "Disabling memory decoding and releasing memory resources\n");
> > > +	if (!(dev->dev_flags & PCI_DEV_FLAGS_QUIET_PCI_REALIGN))
> > > +		pci_info(dev, "Disabling memory decoding and releasing memory resources\n");
> > >   	pci_read_config_word(dev, PCI_COMMAND, &command);
> > >   	command &= ~PCI_COMMAND_MEMORY;
> > >   	pci_write_config_word(dev, PCI_COMMAND, command);
> > > diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> > > index 369d48d6c6f1..00a538def763 100644
> > > --- a/drivers/pci/setup-res.c
> > > +++ b/drivers/pci/setup-res.c
> > > @@ -172,7 +172,8 @@ EXPORT_SYMBOL(pci_claim_resource);
> > >   void pci_disable_bridge_window(struct pci_dev *dev)
> > >   {
> > > -	pci_info(dev, "disabling bridge mem windows\n");
> > > +	if (!(dev->dev_flags & PCI_DEV_FLAGS_QUIET_PCI_REALIGN))
> > > +		pci_info(dev, "disabling bridge mem windows\n");
> > 
> > As far as I'm concerned, we can just remove these messages completely.
> > I don't think there's any real value there.
> 
> After I found out that this was happening to all PCI devices on powerpc due
> to the __weak
> pcibios_default_alignment() interface (necessary for VFIO passthrough and
> performance), I confess that this was my first approach to this matter;
> however I couldn't vouch the need of these messages on other architectures.
> 
> If there are no further concerns, I definitely prefer sending a second
> version of this patch only eliminating these messages and attesting the
> reason why.
> 
> Thank you very much for your review Bjorn,

No problem, welcome to PCI, and I hope we see more of your work!
Desnes A. Nunes do Rosario March 14, 2018, 9:12 p.m. | #7
On 03/14/2018 03:55 PM, Bjorn Helgaas wrote:

>>> As far as I'm concerned, we can just remove these messages completely.
>>> I don't think there's any real value there.
>>
>> After I found out that this was happening to all PCI devices on powerpc due
>> to the __weak
>> pcibios_default_alignment() interface (necessary for VFIO passthrough and
>> performance), I confess that this was my first approach to this matter;
>> however I couldn't vouch the need of these messages on other architectures.
>>
>> If there are no further concerns, I definitely prefer sending a second
>> version of this patch only eliminating these messages and attesting the
>> reason why.
>>
>> Thank you very much for your review Bjorn,
> 
> No problem, welcome to PCI, and I hope we see more of your work!

Bjorn,

Awesome! A second version of this fix has been sent with a new title: 
"[PATCH, pci, v2] pci: Delete PCI disabling informational messages"

Thanks for the review and warm welcome!

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 8c71d1a66cdd..be197c944e5f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5505,7 +5505,8 @@  void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 		return;
 	}
 
-	pci_info(dev, "Disabling memory decoding and releasing memory resources\n");
+	if (!(dev->dev_flags & PCI_DEV_FLAGS_QUIET_PCI_REALIGN))
+		pci_info(dev, "Disabling memory decoding and releasing memory resources\n");
 	pci_read_config_word(dev, PCI_COMMAND, &command);
 	command &= ~PCI_COMMAND_MEMORY;
 	pci_write_config_word(dev, PCI_COMMAND, command);
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 369d48d6c6f1..00a538def763 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -172,7 +172,8 @@  EXPORT_SYMBOL(pci_claim_resource);
 
 void pci_disable_bridge_window(struct pci_dev *dev)
 {
-	pci_info(dev, "disabling bridge mem windows\n");
+	if (!(dev->dev_flags & PCI_DEV_FLAGS_QUIET_PCI_REALIGN))
+		pci_info(dev, "disabling bridge mem windows\n");
 
 	/* MMIO Base/Limit */
 	pci_write_config_dword(dev, PCI_MEMORY_BASE, 0x0000fff0);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index e057e8cc63e7..993f9c7dcc00 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -205,6 +205,8 @@  enum pci_dev_flags {
 	PCI_DEV_FLAGS_NO_FLR_RESET = (__force pci_dev_flags_t) (1 << 10),
 	/* Don't use Relaxed Ordering for TLPs directed at this device */
 	PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11),
+	/* Silent PCI resource realignment messages */
+	PCI_DEV_FLAGS_QUIET_PCI_REALIGN = (__force pci_dev_flags_t) (1 << 12),
 };
 
 enum pci_irq_reroute_variant {