| 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 | expand |
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 > };
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).
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 >
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,
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,
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!
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!
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 {
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(-)