* [PATCH v2] PCI: Make pci_bus_speed_strings[] public
@ 2020-02-06 10:58 Yicong Yang
2020-02-12 22:31 ` Bjorn Helgaas
0 siblings, 1 reply; 3+ messages in thread
From: Yicong Yang @ 2020-02-06 10:58 UTC (permalink / raw)
To: helgaas, linux-pci; +Cc: f.fangjian, huangdaode
pci_bus_speed_strings[] in slot.c defines universal speed information.
Currently it is only used to decode bus speed in slot.c, while elsewhere
use judgement statements repeatly to decode speed information. For
example, in PCIE_SPEED2STR and current_link_speed_show() in sysfs.
Make it public and move to probe.c so that we can reuse it for decoding
speed information in sysfs or dmesg log. Remove "PCIe" suffix of PCIe
bus speed strings to reduce redundancy.
Add pci_bus_speed_strings_size for boundary check purpose, to avoid
acquiring size of an external array by ARRAY_SIZE macro.
Link:https://lore.kernel.org/linux-pci/20200205183531.GA229621@google.com/
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
change since v1:
1. split the patch from series as suggested
2. add pci_bus_speed_strings_size for boundary check in bus_speed_read()
drivers/pci/pci.h | 2 ++
drivers/pci/probe.c | 30 ++++++++++++++++++++++++++++++
drivers/pci/slot.c | 31 +------------------------------
3 files changed, 33 insertions(+), 30 deletions(-)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6394e77..e6bcc06 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -12,6 +12,8 @@
#define PCI_VSEC_ID_INTEL_TBT 0x1234 /* Thunderbolt */
extern const unsigned char pcie_link_speed[];
+extern const char *pci_bus_speed_strings[];
+extern const int pci_bus_speed_strings_size;
extern bool pci_early_dump;
bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 512cb43..6ce47d8 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -678,6 +678,36 @@ const unsigned char pcie_link_speed[] = {
PCI_SPEED_UNKNOWN /* F */
};
+/* these strings match up with the values in pci_bus_speed */
+const char *pci_bus_speed_strings[] = {
+ "33 MHz PCI", /* 0x00 */
+ "66 MHz PCI", /* 0x01 */
+ "66 MHz PCI-X", /* 0x02 */
+ "100 MHz PCI-X", /* 0x03 */
+ "133 MHz PCI-X", /* 0x04 */
+ NULL, /* 0x05 */
+ NULL, /* 0x06 */
+ NULL, /* 0x07 */
+ NULL, /* 0x08 */
+ "66 MHz PCI-X 266", /* 0x09 */
+ "100 MHz PCI-X 266", /* 0x0a */
+ "133 MHz PCI-X 266", /* 0x0b */
+ "Unknown AGP", /* 0x0c */
+ "1x AGP", /* 0x0d */
+ "2x AGP", /* 0x0e */
+ "4x AGP", /* 0x0f */
+ "8x AGP", /* 0x10 */
+ "66 MHz PCI-X 533", /* 0x11 */
+ "100 MHz PCI-X 533", /* 0x12 */
+ "133 MHz PCI-X 533", /* 0x13 */
+ "2.5 GT/s", /* 0x14 */
+ "5.0 GT/s", /* 0x15 */
+ "8.0 GT/s", /* 0x16 */
+ "16.0 GT/s", /* 0x17 */
+ "32.0 GT/s", /* 0x18 */
+};
+const int pci_bus_speed_strings_size = ARRAY_SIZE(pci_bus_speed_strings);
+
void pcie_update_link_speed(struct pci_bus *bus, u16 linksta)
{
bus->cur_bus_speed = pcie_link_speed[linksta & PCI_EXP_LNKSTA_CLS];
diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index ae4aa0e..fb7c172 100644
--- a/drivers/pci/slot.c
+++ b/drivers/pci/slot.c
@@ -49,40 +49,11 @@ static ssize_t address_read_file(struct pci_slot *slot, char *buf)
slot->number);
}
-/* these strings match up with the values in pci_bus_speed */
-static const char *pci_bus_speed_strings[] = {
- "33 MHz PCI", /* 0x00 */
- "66 MHz PCI", /* 0x01 */
- "66 MHz PCI-X", /* 0x02 */
- "100 MHz PCI-X", /* 0x03 */
- "133 MHz PCI-X", /* 0x04 */
- NULL, /* 0x05 */
- NULL, /* 0x06 */
- NULL, /* 0x07 */
- NULL, /* 0x08 */
- "66 MHz PCI-X 266", /* 0x09 */
- "100 MHz PCI-X 266", /* 0x0a */
- "133 MHz PCI-X 266", /* 0x0b */
- "Unknown AGP", /* 0x0c */
- "1x AGP", /* 0x0d */
- "2x AGP", /* 0x0e */
- "4x AGP", /* 0x0f */
- "8x AGP", /* 0x10 */
- "66 MHz PCI-X 533", /* 0x11 */
- "100 MHz PCI-X 533", /* 0x12 */
- "133 MHz PCI-X 533", /* 0x13 */
- "2.5 GT/s PCIe", /* 0x14 */
- "5.0 GT/s PCIe", /* 0x15 */
- "8.0 GT/s PCIe", /* 0x16 */
- "16.0 GT/s PCIe", /* 0x17 */
- "32.0 GT/s PCIe", /* 0x18 */
-};
-
static ssize_t bus_speed_read(enum pci_bus_speed speed, char *buf)
{
const char *speed_string;
- if (speed < ARRAY_SIZE(pci_bus_speed_strings))
+ if (speed < pci_bus_speed_strings_size)
speed_string = pci_bus_speed_strings[speed];
else
speed_string = "Unknown";
--
2.8.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2] PCI: Make pci_bus_speed_strings[] public
2020-02-06 10:58 [PATCH v2] PCI: Make pci_bus_speed_strings[] public Yicong Yang
@ 2020-02-12 22:31 ` Bjorn Helgaas
2020-02-13 6:07 ` Yicong Yang
0 siblings, 1 reply; 3+ messages in thread
From: Bjorn Helgaas @ 2020-02-12 22:31 UTC (permalink / raw)
To: Yicong Yang; +Cc: linux-pci, f.fangjian, huangdaode
On Thu, Feb 06, 2020 at 06:58:07PM +0800, Yicong Yang wrote:
> pci_bus_speed_strings[] in slot.c defines universal speed information.
> Currently it is only used to decode bus speed in slot.c, while elsewhere
> use judgement statements repeatly to decode speed information. For
> example, in PCIE_SPEED2STR and current_link_speed_show() in sysfs.
>
> Make it public and move to probe.c so that we can reuse it for decoding
> speed information in sysfs or dmesg log. Remove "PCIe" suffix of PCIe
> bus speed strings to reduce redundancy.
>
> Add pci_bus_speed_strings_size for boundary check purpose, to avoid
> acquiring size of an external array by ARRAY_SIZE macro.
>
> Link:https://lore.kernel.org/linux-pci/20200205183531.GA229621@google.com/
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>
> change since v1:
> 1. split the patch from series as suggested
That's not what I said. What I said was:
This needs to say exactly where this change will be observed: /proc
file, /sys file, dmesg, etc. I would prefer that an observable
change be in its own patch instead of being a by-product of a
structural change like this one.
That means I want a tiny patch that changes the strings a user will
see and a specific example in the commit log to show what change the
user will see. For example, if it were in sysfs the changelog could
say something like:
-/sys/devices/pci0000:00/0000:00:1c.0/max_link_speed:8 GT/s PCIe
+/sys/devices/pci0000:00/0000:00:1c.0/max_link_speed:8 GT/s
I still don't know exactly *where* the change is (it's not in sysfs; I
just made up the example above). But something like the above would
tell me exactly what files are affected and what the change looks
like. That's the information people would need to update programs
that parse the file.
And the patch itself should be something like:
+ "8.0 GT/s", /* 0x16 */
- "8.0 GT/s PCIe", /* 0x16 */
*without* the code moving between files. This patch is basically
identical to the first one [1] except that you made it separate from
the series.
This appears to change the strings *and* move them from slot.c to
probe.c. Those are two different things, and putting them together in
one patch makes it harder for people to figure out what's changing.
There should be one patch that *only* changes the strings and another
that *only* moves them from slot.c to probe.c.
And I don't want them as separate patches *outside* the series. That
just makes the series itself not apply correctly. They should be *in*
the series so the whole thing applies cleanly on my "master" brance
(v5.6-rc1).
[1] https://lore.kernel.org/r/1579079063-5668-3-git-send-email-yangyicong@hisilicon.com
> 2. add pci_bus_speed_strings_size for boundary check in bus_speed_read()
>
> drivers/pci/pci.h | 2 ++
> drivers/pci/probe.c | 30 ++++++++++++++++++++++++++++++
> drivers/pci/slot.c | 31 +------------------------------
> 3 files changed, 33 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 6394e77..e6bcc06 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -12,6 +12,8 @@
> #define PCI_VSEC_ID_INTEL_TBT 0x1234 /* Thunderbolt */
>
> extern const unsigned char pcie_link_speed[];
> +extern const char *pci_bus_speed_strings[];
> +extern const int pci_bus_speed_strings_size;
> extern bool pci_early_dump;
>
> bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 512cb43..6ce47d8 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -678,6 +678,36 @@ const unsigned char pcie_link_speed[] = {
> PCI_SPEED_UNKNOWN /* F */
> };
>
> +/* these strings match up with the values in pci_bus_speed */
> +const char *pci_bus_speed_strings[] = {
> + "33 MHz PCI", /* 0x00 */
> + "66 MHz PCI", /* 0x01 */
> + "66 MHz PCI-X", /* 0x02 */
> + "100 MHz PCI-X", /* 0x03 */
> + "133 MHz PCI-X", /* 0x04 */
> + NULL, /* 0x05 */
> + NULL, /* 0x06 */
> + NULL, /* 0x07 */
> + NULL, /* 0x08 */
> + "66 MHz PCI-X 266", /* 0x09 */
> + "100 MHz PCI-X 266", /* 0x0a */
> + "133 MHz PCI-X 266", /* 0x0b */
> + "Unknown AGP", /* 0x0c */
> + "1x AGP", /* 0x0d */
> + "2x AGP", /* 0x0e */
> + "4x AGP", /* 0x0f */
> + "8x AGP", /* 0x10 */
> + "66 MHz PCI-X 533", /* 0x11 */
> + "100 MHz PCI-X 533", /* 0x12 */
> + "133 MHz PCI-X 533", /* 0x13 */
> + "2.5 GT/s", /* 0x14 */
> + "5.0 GT/s", /* 0x15 */
> + "8.0 GT/s", /* 0x16 */
> + "16.0 GT/s", /* 0x17 */
> + "32.0 GT/s", /* 0x18 */
> +};
> +const int pci_bus_speed_strings_size = ARRAY_SIZE(pci_bus_speed_strings);
> +
> void pcie_update_link_speed(struct pci_bus *bus, u16 linksta)
> {
> bus->cur_bus_speed = pcie_link_speed[linksta & PCI_EXP_LNKSTA_CLS];
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index ae4aa0e..fb7c172 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -49,40 +49,11 @@ static ssize_t address_read_file(struct pci_slot *slot, char *buf)
> slot->number);
> }
>
> -/* these strings match up with the values in pci_bus_speed */
> -static const char *pci_bus_speed_strings[] = {
> - "33 MHz PCI", /* 0x00 */
> - "66 MHz PCI", /* 0x01 */
> - "66 MHz PCI-X", /* 0x02 */
> - "100 MHz PCI-X", /* 0x03 */
> - "133 MHz PCI-X", /* 0x04 */
> - NULL, /* 0x05 */
> - NULL, /* 0x06 */
> - NULL, /* 0x07 */
> - NULL, /* 0x08 */
> - "66 MHz PCI-X 266", /* 0x09 */
> - "100 MHz PCI-X 266", /* 0x0a */
> - "133 MHz PCI-X 266", /* 0x0b */
> - "Unknown AGP", /* 0x0c */
> - "1x AGP", /* 0x0d */
> - "2x AGP", /* 0x0e */
> - "4x AGP", /* 0x0f */
> - "8x AGP", /* 0x10 */
> - "66 MHz PCI-X 533", /* 0x11 */
> - "100 MHz PCI-X 533", /* 0x12 */
> - "133 MHz PCI-X 533", /* 0x13 */
> - "2.5 GT/s PCIe", /* 0x14 */
> - "5.0 GT/s PCIe", /* 0x15 */
> - "8.0 GT/s PCIe", /* 0x16 */
> - "16.0 GT/s PCIe", /* 0x17 */
> - "32.0 GT/s PCIe", /* 0x18 */
> -};
> -
> static ssize_t bus_speed_read(enum pci_bus_speed speed, char *buf)
> {
> const char *speed_string;
>
> - if (speed < ARRAY_SIZE(pci_bus_speed_strings))
> + if (speed < pci_bus_speed_strings_size)
> speed_string = pci_bus_speed_strings[speed];
> else
> speed_string = "Unknown";
> --
> 2.8.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] PCI: Make pci_bus_speed_strings[] public
2020-02-12 22:31 ` Bjorn Helgaas
@ 2020-02-13 6:07 ` Yicong Yang
0 siblings, 0 replies; 3+ messages in thread
From: Yicong Yang @ 2020-02-13 6:07 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, f.fangjian, huangdaode
Hi Bjorn,
Seems I've misunderstood the comments. Sorry for that. I'll reorganize
the series and send later.
Thanks,
Yicong Yang
On 2020/2/13 6:31, Bjorn Helgaas wrote:
> On Thu, Feb 06, 2020 at 06:58:07PM +0800, Yicong Yang wrote:
>> pci_bus_speed_strings[] in slot.c defines universal speed information.
>> Currently it is only used to decode bus speed in slot.c, while elsewhere
>> use judgement statements repeatly to decode speed information. For
>> example, in PCIE_SPEED2STR and current_link_speed_show() in sysfs.
>>
>> Make it public and move to probe.c so that we can reuse it for decoding
>> speed information in sysfs or dmesg log. Remove "PCIe" suffix of PCIe
>> bus speed strings to reduce redundancy.
>>
>> Add pci_bus_speed_strings_size for boundary check purpose, to avoid
>> acquiring size of an external array by ARRAY_SIZE macro.
>>
>> Link:https://lore.kernel.org/linux-pci/20200205183531.GA229621@google.com/
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> ---
>>
>> change since v1:
>> 1. split the patch from series as suggested
> That's not what I said. What I said was:
>
> This needs to say exactly where this change will be observed: /proc
> file, /sys file, dmesg, etc. I would prefer that an observable
> change be in its own patch instead of being a by-product of a
> structural change like this one.
>
> That means I want a tiny patch that changes the strings a user will
> see and a specific example in the commit log to show what change the
> user will see. For example, if it were in sysfs the changelog could
> say something like:
>
> -/sys/devices/pci0000:00/0000:00:1c.0/max_link_speed:8 GT/s PCIe
> +/sys/devices/pci0000:00/0000:00:1c.0/max_link_speed:8 GT/s
>
> I still don't know exactly *where* the change is (it's not in sysfs; I
> just made up the example above). But something like the above would
> tell me exactly what files are affected and what the change looks
> like. That's the information people would need to update programs
> that parse the file.
>
> And the patch itself should be something like:
>
> + "8.0 GT/s", /* 0x16 */
> - "8.0 GT/s PCIe", /* 0x16 */
>
> *without* the code moving between files. This patch is basically
> identical to the first one [1] except that you made it separate from
> the series.
>
> This appears to change the strings *and* move them from slot.c to
> probe.c. Those are two different things, and putting them together in
> one patch makes it harder for people to figure out what's changing.
>
> There should be one patch that *only* changes the strings and another
> that *only* moves them from slot.c to probe.c.
>
> And I don't want them as separate patches *outside* the series. That
> just makes the series itself not apply correctly. They should be *in*
> the series so the whole thing applies cleanly on my "master" brance
> (v5.6-rc1).
>
> [1] https://lore.kernel.org/r/1579079063-5668-3-git-send-email-yangyicong@hisilicon.com
>
>> 2. add pci_bus_speed_strings_size for boundary check in bus_speed_read()
>>
>> drivers/pci/pci.h | 2 ++
>> drivers/pci/probe.c | 30 ++++++++++++++++++++++++++++++
>> drivers/pci/slot.c | 31 +------------------------------
>> 3 files changed, 33 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 6394e77..e6bcc06 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -12,6 +12,8 @@
>> #define PCI_VSEC_ID_INTEL_TBT 0x1234 /* Thunderbolt */
>>
>> extern const unsigned char pcie_link_speed[];
>> +extern const char *pci_bus_speed_strings[];
>> +extern const int pci_bus_speed_strings_size;
>> extern bool pci_early_dump;
>>
>> bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 512cb43..6ce47d8 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -678,6 +678,36 @@ const unsigned char pcie_link_speed[] = {
>> PCI_SPEED_UNKNOWN /* F */
>> };
>>
>> +/* these strings match up with the values in pci_bus_speed */
>> +const char *pci_bus_speed_strings[] = {
>> + "33 MHz PCI", /* 0x00 */
>> + "66 MHz PCI", /* 0x01 */
>> + "66 MHz PCI-X", /* 0x02 */
>> + "100 MHz PCI-X", /* 0x03 */
>> + "133 MHz PCI-X", /* 0x04 */
>> + NULL, /* 0x05 */
>> + NULL, /* 0x06 */
>> + NULL, /* 0x07 */
>> + NULL, /* 0x08 */
>> + "66 MHz PCI-X 266", /* 0x09 */
>> + "100 MHz PCI-X 266", /* 0x0a */
>> + "133 MHz PCI-X 266", /* 0x0b */
>> + "Unknown AGP", /* 0x0c */
>> + "1x AGP", /* 0x0d */
>> + "2x AGP", /* 0x0e */
>> + "4x AGP", /* 0x0f */
>> + "8x AGP", /* 0x10 */
>> + "66 MHz PCI-X 533", /* 0x11 */
>> + "100 MHz PCI-X 533", /* 0x12 */
>> + "133 MHz PCI-X 533", /* 0x13 */
>> + "2.5 GT/s", /* 0x14 */
>> + "5.0 GT/s", /* 0x15 */
>> + "8.0 GT/s", /* 0x16 */
>> + "16.0 GT/s", /* 0x17 */
>> + "32.0 GT/s", /* 0x18 */
>> +};
>> +const int pci_bus_speed_strings_size = ARRAY_SIZE(pci_bus_speed_strings);
>> +
>> void pcie_update_link_speed(struct pci_bus *bus, u16 linksta)
>> {
>> bus->cur_bus_speed = pcie_link_speed[linksta & PCI_EXP_LNKSTA_CLS];
>> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
>> index ae4aa0e..fb7c172 100644
>> --- a/drivers/pci/slot.c
>> +++ b/drivers/pci/slot.c
>> @@ -49,40 +49,11 @@ static ssize_t address_read_file(struct pci_slot *slot, char *buf)
>> slot->number);
>> }
>>
>> -/* these strings match up with the values in pci_bus_speed */
>> -static const char *pci_bus_speed_strings[] = {
>> - "33 MHz PCI", /* 0x00 */
>> - "66 MHz PCI", /* 0x01 */
>> - "66 MHz PCI-X", /* 0x02 */
>> - "100 MHz PCI-X", /* 0x03 */
>> - "133 MHz PCI-X", /* 0x04 */
>> - NULL, /* 0x05 */
>> - NULL, /* 0x06 */
>> - NULL, /* 0x07 */
>> - NULL, /* 0x08 */
>> - "66 MHz PCI-X 266", /* 0x09 */
>> - "100 MHz PCI-X 266", /* 0x0a */
>> - "133 MHz PCI-X 266", /* 0x0b */
>> - "Unknown AGP", /* 0x0c */
>> - "1x AGP", /* 0x0d */
>> - "2x AGP", /* 0x0e */
>> - "4x AGP", /* 0x0f */
>> - "8x AGP", /* 0x10 */
>> - "66 MHz PCI-X 533", /* 0x11 */
>> - "100 MHz PCI-X 533", /* 0x12 */
>> - "133 MHz PCI-X 533", /* 0x13 */
>> - "2.5 GT/s PCIe", /* 0x14 */
>> - "5.0 GT/s PCIe", /* 0x15 */
>> - "8.0 GT/s PCIe", /* 0x16 */
>> - "16.0 GT/s PCIe", /* 0x17 */
>> - "32.0 GT/s PCIe", /* 0x18 */
>> -};
>> -
>> static ssize_t bus_speed_read(enum pci_bus_speed speed, char *buf)
>> {
>> const char *speed_string;
>>
>> - if (speed < ARRAY_SIZE(pci_bus_speed_strings))
>> + if (speed < pci_bus_speed_strings_size)
>> speed_string = pci_bus_speed_strings[speed];
>> else
>> speed_string = "Unknown";
>> --
>> 2.8.1
>>
> .
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-02-13 6:07 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-06 10:58 [PATCH v2] PCI: Make pci_bus_speed_strings[] public Yicong Yang
2020-02-12 22:31 ` Bjorn Helgaas
2020-02-13 6:07 ` Yicong Yang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).