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

2018-03-14 Thread Desnes Augusto Nunes do Rosário 

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!

--
Desnes A. Nunes do Rosário
--

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

2018-03-14 Thread Bjorn Helgaas 
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 
> > > ---
> > >   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 &= ~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!

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

2018-03-14 Thread Desnes Augusto Nunes do Rosário 

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 
---
  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 &= ~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,

--
Desnes A. Nunes do Rosário

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

2018-03-14 Thread Desnes Augusto Nunes do Rosário 

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
 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 :00:00.0: Disabling memory decoding and releasing 
memory resources

[0.692324] pci :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 Rosário

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

2018-03-14 Thread Bjorn Helgaas 
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 
> ---
>  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 &= ~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, 0xfff0);
> 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
>

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

2018-03-14 Thread Andy Shevchenko 
On Wed, Mar 14, 2018 at 7:41 PM, Andy Shevchenko
 wrote:

> I would rather name it _NO_PCI_REALIGN_MSG

I meant _NO_REALIGN_MSG of course (PCI is already at the beginning).

-- 
With Best Regards,
Andy Shevchenko

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

2018-03-14 Thread Andy Shevchenko 
On Wed, Mar 14, 2018 at 6:34 PM, 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.

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

>  };

-- 
With Best Regards,
Andy Shevchenko

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

2018-03-14 Thread Desnes A. Nunes do Rosario 
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 
---
 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 &= ~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, 0xfff0);
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