* [PATCH 1/1] PCI: Convert pdev_sort_resources() to use resource name helper @ 2024-10-16 12:00 Ilpo Järvinen 2024-10-16 13:05 ` Philipp Stanner 0 siblings, 1 reply; 6+ messages in thread From: Ilpo Järvinen @ 2024-10-16 12:00 UTC (permalink / raw) To: Bjorn Helgaas, linux-pci, linux-kernel; +Cc: Ilpo Järvinen Use pci_resource_name() helper in pdev_sort_resources() to print resources in user-friendly format. Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> --- drivers/pci/setup-bus.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index 23082bc0ca37..071c5436b4a5 100644 --- a/drivers/pci/setup-bus.c +++ b/drivers/pci/setup-bus.c @@ -134,6 +134,7 @@ static void pdev_sort_resources(struct pci_dev *dev, struct list_head *head) int i; pci_dev_for_each_resource(dev, r, i) { + const char *r_name = pci_resource_name(dev, i); struct pci_dev_resource *dev_res, *tmp; resource_size_t r_align; struct list_head *n; @@ -146,8 +147,8 @@ static void pdev_sort_resources(struct pci_dev *dev, struct list_head *head) r_align = pci_resource_alignment(dev, r); if (!r_align) { - pci_warn(dev, "BAR %d: %pR has bogus alignment\n", - i, r); + pci_warn(dev, "%s: %pR has bogus alignment\n", + r_name, r); continue; } -- 2.39.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] PCI: Convert pdev_sort_resources() to use resource name helper 2024-10-16 12:00 [PATCH 1/1] PCI: Convert pdev_sort_resources() to use resource name helper Ilpo Järvinen @ 2024-10-16 13:05 ` Philipp Stanner 2024-10-16 13:15 ` Ilpo Järvinen 0 siblings, 1 reply; 6+ messages in thread From: Philipp Stanner @ 2024-10-16 13:05 UTC (permalink / raw) To: Ilpo Järvinen, Bjorn Helgaas, linux-pci, linux-kernel On Wed, 2024-10-16 at 15:00 +0300, Ilpo Järvinen wrote: > Use pci_resource_name() helper in pdev_sort_resources() to print > resources in user-friendly format. > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > drivers/pci/setup-bus.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > index 23082bc0ca37..071c5436b4a5 100644 > --- a/drivers/pci/setup-bus.c > +++ b/drivers/pci/setup-bus.c > @@ -134,6 +134,7 @@ static void pdev_sort_resources(struct pci_dev > *dev, struct list_head *head) > int i; > > pci_dev_for_each_resource(dev, r, i) { > + const char *r_name = pci_resource_name(dev, i); > struct pci_dev_resource *dev_res, *tmp; > resource_size_t r_align; > struct list_head *n; > @@ -146,8 +147,8 @@ static void pdev_sort_resources(struct pci_dev > *dev, struct list_head *head) > > r_align = pci_resource_alignment(dev, r); > if (!r_align) { > - pci_warn(dev, "BAR %d: %pR has bogus > alignment\n", > - i, r); > + pci_warn(dev, "%s: %pR has bogus > alignment\n", > + r_name, r); Why do you remove the BAR index number, don't you think this information is also useful? One could also consider printing r_align, would that be useful? Note that there is a similar pci_warn down below in line 1118 that does print it. Would you want to change that pci_warn()-string, too? P. > continue; > } > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] PCI: Convert pdev_sort_resources() to use resource name helper 2024-10-16 13:05 ` Philipp Stanner @ 2024-10-16 13:15 ` Ilpo Järvinen 2024-10-16 13:19 ` Ilpo Järvinen 2024-10-16 13:21 ` Philipp Stanner 0 siblings, 2 replies; 6+ messages in thread From: Ilpo Järvinen @ 2024-10-16 13:15 UTC (permalink / raw) To: Philipp Stanner; +Cc: Bjorn Helgaas, linux-pci, LKML [-- Attachment #1: Type: text/plain, Size: 2123 bytes --] On Wed, 16 Oct 2024, Philipp Stanner wrote: > On Wed, 2024-10-16 at 15:00 +0300, Ilpo Järvinen wrote: > > Use pci_resource_name() helper in pdev_sort_resources() to print > > resources in user-friendly format. > > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > --- > > drivers/pci/setup-bus.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > > index 23082bc0ca37..071c5436b4a5 100644 > > --- a/drivers/pci/setup-bus.c > > +++ b/drivers/pci/setup-bus.c > > @@ -134,6 +134,7 @@ static void pdev_sort_resources(struct pci_dev > > *dev, struct list_head *head) > > int i; > > > > pci_dev_for_each_resource(dev, r, i) { > > + const char *r_name = pci_resource_name(dev, i); > > struct pci_dev_resource *dev_res, *tmp; > > resource_size_t r_align; > > struct list_head *n; > > @@ -146,8 +147,8 @@ static void pdev_sort_resources(struct pci_dev > > *dev, struct list_head *head) > > > > r_align = pci_resource_alignment(dev, r); > > if (!r_align) { > > - pci_warn(dev, "BAR %d: %pR has bogus > > alignment\n", > > - i, r); > > + pci_warn(dev, "%s: %pR has bogus > > alignment\n", > > + r_name, r); > > Why do you remove the BAR index number, don't you think this > information is also useful? That's because of how pci_resource_name() works. The number will be included in the returned string and it won't be always same as i. So that change is done on purpose. > One could also consider printing r_align, would that be useful? As per the preceeding condition, it's known to be zero so it's not that useful for any developer looking at these code lines. > Note > that there is a similar pci_warn down below in line 1118 that does > print it. Would you want to change that pci_warn()-string, too? pci_warn() on line 1118 does NOT print i (as expected when using pci_resource_name()) and there align is not zero so I don't see how this is relevant. But thanks for taking a look anyway. :-) -- i. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] PCI: Convert pdev_sort_resources() to use resource name helper 2024-10-16 13:15 ` Ilpo Järvinen @ 2024-10-16 13:19 ` Ilpo Järvinen 2024-10-16 13:21 ` Philipp Stanner 1 sibling, 0 replies; 6+ messages in thread From: Ilpo Järvinen @ 2024-10-16 13:19 UTC (permalink / raw) To: Philipp Stanner; +Cc: Bjorn Helgaas, linux-pci, LKML [-- Attachment #1: Type: text/plain, Size: 2412 bytes --] On Wed, 16 Oct 2024, Ilpo Järvinen wrote: > On Wed, 16 Oct 2024, Philipp Stanner wrote: > > > On Wed, 2024-10-16 at 15:00 +0300, Ilpo Järvinen wrote: > > > Use pci_resource_name() helper in pdev_sort_resources() to print > > > resources in user-friendly format. > > > > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > --- > > > drivers/pci/setup-bus.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > > > index 23082bc0ca37..071c5436b4a5 100644 > > > --- a/drivers/pci/setup-bus.c > > > +++ b/drivers/pci/setup-bus.c > > > @@ -134,6 +134,7 @@ static void pdev_sort_resources(struct pci_dev > > > *dev, struct list_head *head) > > > int i; > > > > > > pci_dev_for_each_resource(dev, r, i) { > > > + const char *r_name = pci_resource_name(dev, i); > > > struct pci_dev_resource *dev_res, *tmp; > > > resource_size_t r_align; > > > struct list_head *n; > > > @@ -146,8 +147,8 @@ static void pdev_sort_resources(struct pci_dev > > > *dev, struct list_head *head) > > > > > > r_align = pci_resource_alignment(dev, r); > > > if (!r_align) { > > > - pci_warn(dev, "BAR %d: %pR has bogus > > > alignment\n", > > > - i, r); > > > + pci_warn(dev, "%s: %pR has bogus > > > alignment\n", > > > + r_name, r); > > > > Why do you remove the BAR index number, don't you think this > > information is also useful? > > That's because of how pci_resource_name() works. The number will be > included in the returned string and it won't be always same as i. > So that change is done on purpose. That being said, I now realize the other examples use the colon differently "%s %pR: ..." so I'll change that for v2. -- i. > > One could also consider printing r_align, would that be useful? > > As per the preceeding condition, it's known to be zero so it's not > that useful for any developer looking at these code lines. > > > Note > > that there is a similar pci_warn down below in line 1118 that does > > print it. Would you want to change that pci_warn()-string, too? > > pci_warn() on line 1118 does NOT print i (as expected when using > pci_resource_name()) and there align is not zero so I don't see how this > is relevant. > > But thanks for taking a look anyway. :-) > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] PCI: Convert pdev_sort_resources() to use resource name helper 2024-10-16 13:15 ` Ilpo Järvinen 2024-10-16 13:19 ` Ilpo Järvinen @ 2024-10-16 13:21 ` Philipp Stanner 2024-10-16 13:34 ` Ilpo Järvinen 1 sibling, 1 reply; 6+ messages in thread From: Philipp Stanner @ 2024-10-16 13:21 UTC (permalink / raw) To: Ilpo Järvinen; +Cc: Bjorn Helgaas, linux-pci, LKML On Wed, 2024-10-16 at 16:15 +0300, Ilpo Järvinen wrote: > On Wed, 16 Oct 2024, Philipp Stanner wrote: > > > On Wed, 2024-10-16 at 15:00 +0300, Ilpo Järvinen wrote: > > > Use pci_resource_name() helper in pdev_sort_resources() to print > > > resources in user-friendly format. > > > > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > --- > > > drivers/pci/setup-bus.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > > > index 23082bc0ca37..071c5436b4a5 100644 > > > --- a/drivers/pci/setup-bus.c > > > +++ b/drivers/pci/setup-bus.c > > > @@ -134,6 +134,7 @@ static void pdev_sort_resources(struct > > > pci_dev > > > *dev, struct list_head *head) > > > int i; > > > > > > pci_dev_for_each_resource(dev, r, i) { > > > + const char *r_name = pci_resource_name(dev, i); > > > struct pci_dev_resource *dev_res, *tmp; > > > resource_size_t r_align; > > > struct list_head *n; > > > @@ -146,8 +147,8 @@ static void pdev_sort_resources(struct > > > pci_dev > > > *dev, struct list_head *head) > > > > > > r_align = pci_resource_alignment(dev, r); > > > if (!r_align) { > > > - pci_warn(dev, "BAR %d: %pR has bogus > > > alignment\n", > > > - i, r); > > > + pci_warn(dev, "%s: %pR has bogus > > > alignment\n", > > > + r_name, r); > > > > Why do you remove the BAR index number, don't you think this > > information is also useful? > > That's because of how pci_resource_name() works. The number will be > included in the returned string and it won't be always same as i. > So that change is done on purpose. > > > One could also consider printing r_align, would that be useful? > > As per the preceeding condition, it's known to be zero so it's not > that useful for any developer looking at these code lines. Ah, right. Would it then make sense then to do: pci_warn(dev, "%s: %pR: Alignment must not be zero.\n", ...); Would tell then exactly what the problem is. Unless the devs know that a bogus alignment is definitely always 0. ? > > > Note > > that there is a similar pci_warn down below in line 1118 that does > > print it. Would you want to change that pci_warn()-string, too? > > pci_warn() on line 1118 does NOT print i (as expected when using > pci_resource_name()) and there align is not zero so I don't see how > this > is relevant. > > But thanks for taking a look anyway. :-) Sure thing > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] PCI: Convert pdev_sort_resources() to use resource name helper 2024-10-16 13:21 ` Philipp Stanner @ 2024-10-16 13:34 ` Ilpo Järvinen 0 siblings, 0 replies; 6+ messages in thread From: Ilpo Järvinen @ 2024-10-16 13:34 UTC (permalink / raw) To: Philipp Stanner; +Cc: Bjorn Helgaas, linux-pci, LKML [-- Attachment #1: Type: text/plain, Size: 2463 bytes --] On Wed, 16 Oct 2024, Philipp Stanner wrote: > On Wed, 2024-10-16 at 16:15 +0300, Ilpo Järvinen wrote: > > On Wed, 16 Oct 2024, Philipp Stanner wrote: > > > > > On Wed, 2024-10-16 at 15:00 +0300, Ilpo Järvinen wrote: > > > > Use pci_resource_name() helper in pdev_sort_resources() to print > > > > resources in user-friendly format. > > > > > > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > > --- > > > > drivers/pci/setup-bus.c | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > > > > index 23082bc0ca37..071c5436b4a5 100644 > > > > --- a/drivers/pci/setup-bus.c > > > > +++ b/drivers/pci/setup-bus.c > > > > @@ -134,6 +134,7 @@ static void pdev_sort_resources(struct > > > > pci_dev > > > > *dev, struct list_head *head) > > > > int i; > > > > > > > > pci_dev_for_each_resource(dev, r, i) { > > > > + const char *r_name = pci_resource_name(dev, i); > > > > struct pci_dev_resource *dev_res, *tmp; > > > > resource_size_t r_align; > > > > struct list_head *n; > > > > @@ -146,8 +147,8 @@ static void pdev_sort_resources(struct > > > > pci_dev > > > > *dev, struct list_head *head) > > > > > > > > r_align = pci_resource_alignment(dev, r); > > > > if (!r_align) { > > > > - pci_warn(dev, "BAR %d: %pR has bogus > > > > alignment\n", > > > > - i, r); > > > > + pci_warn(dev, "%s: %pR has bogus > > > > alignment\n", > > > > + r_name, r); > > > > > > Why do you remove the BAR index number, don't you think this > > > information is also useful? > > > > That's because of how pci_resource_name() works. The number will be > > included in the returned string and it won't be always same as i. > > So that change is done on purpose. > > > > > One could also consider printing r_align, would that be useful? > > > > As per the preceeding condition, it's known to be zero so it's not > > that useful for any developer looking at these code lines. > > Ah, right. Would it then make sense then to do: > > pci_warn(dev, "%s: %pR: Alignment must not be zero.\n", ...); > > Would tell then exactly what the problem is. Unless the devs know that > a bogus alignment is definitely always 0. > > ? While I personally don't need that given the surrounding code, I can change the string to be more precise. Thanks. -- i. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-10-16 13:34 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-10-16 12:00 [PATCH 1/1] PCI: Convert pdev_sort_resources() to use resource name helper Ilpo Järvinen 2024-10-16 13:05 ` Philipp Stanner 2024-10-16 13:15 ` Ilpo Järvinen 2024-10-16 13:19 ` Ilpo Järvinen 2024-10-16 13:21 ` Philipp Stanner 2024-10-16 13:34 ` Ilpo Järvinen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox