| Message ID | 20241016120048.1355-1-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/1] PCI: Convert pdev_sort_resources() to use resource name helper | expand |
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; > } >
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. :-)
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.
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 >
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.
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; }
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(-)