On 2020/2/6 2:50, Bjorn Helgaas wrote:
> On Wed, Jan 15, 2020 at 05:04:21PM +0800, Yicong Yang wrote:
>> Use pci_bus_speed_strings[] array to refactor PCIE_SPEED2STR macro.
>> Rename PCIE_SPEED2STR with PCI_SPEED2STR as it's also used to
>> decode non-PCIe speeds. Modify bus_speed_read() and
>> __pcie_print_link_status() with PCI_SPEED2STR macro.
> This looks like it should be split into two or three patches.
>
> - Rename
> - Fiddle with the "PCIe" suffix
> - Refactor with pci_bus_speed_strings[]
>
> I don't know the right order for these.
I'll split this patch in next version.
>
>> Signed-off-by: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
>> ---
>>
>> I don't add a boundary check in PCI_SPEED2STR macro because:
>> 1. we cannot get the array size of an extern one using ARRAY_SIZE
>> 2. It is the *speed* should be check valid or not when assigned,
>> rather than checking it here. Actually we do make it valid when
>> assigned, the speed is either a valid value or PCI_SPEED_UNKNOWN.
>> And it's ensured in pcie_get_speed_cap(), pci_set_bus_speed()
>> when probe, and pcie_link_speed[] array. Please check again for
>> sure.
> I'm a little nervous about this because bus->cur_bus_speed and
> bus->max_bus_speed are set several places where we can't validate them
> (powerpc, s390, faraday_pci_probe(), cpqhpc_probe(), get_max_bus_speed
> (ibmphp), shpc_get_max_bus_speed(), etc).
Seems I didn't consider such situation. I'll add the boundary check in
next version.
Thanks,
Yang
>
> I don't think we can avoid out-of-bounds references by relying on all
> those places to get it right, especially since some of these can read
> speeds from hardware, so new hardware can give us a bus speed that old
> software won't know about.
>
>> drivers/pci/pci-sysfs.c | 2 +-
>> drivers/pci/pci.c | 6 +++---
>> drivers/pci/pci.h | 10 +++-------
>> drivers/pci/slot.c | 10 +++-------
>> 4 files changed, 10 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> index 13f766d..f4eafbc 100644
>> --- a/drivers/pci/pci-sysfs.c
>> +++ b/drivers/pci/pci-sysfs.c
>> @@ -156,7 +156,7 @@ static ssize_t max_link_speed_show(struct device *dev,
>> {
>> struct pci_dev *pdev = to_pci_dev(dev);
>>
>> - return sprintf(buf, "%s\n", PCIE_SPEED2STR(pcie_get_speed_cap(pdev)));
>> + return sprintf(buf, "%s\n", PCI_SPEED2STR(pcie_get_speed_cap(pdev)));
>> }
>> static DEVICE_ATTR_RO(max_link_speed);
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index e87196c..dce32ce 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -5868,14 +5868,14 @@ void __pcie_print_link_status(struct pci_dev *dev, bool verbose)
>> if (bw_avail >= bw_cap && verbose)
>> pci_info(dev, "%u.%03u Gb/s available PCIe bandwidth (%s x%d link)\n",
>> bw_cap / 1000, bw_cap % 1000,
>> - PCIE_SPEED2STR(speed_cap), width_cap);
>> + PCI_SPEED2STR(speed_cap), width_cap);
>> else if (bw_avail < bw_cap)
>> pci_info(dev, "%u.%03u Gb/s available PCIe bandwidth, limited by %s x%d link at %s (capable of %u.%03u Gb/s with %s x%d link)\n",
>> bw_avail / 1000, bw_avail % 1000,
>> - PCIE_SPEED2STR(speed), width,
>> + PCI_SPEED2STR(speed), width,
>> limiting_dev ? pci_name(limiting_dev) : "<unknown>",
>> bw_cap / 1000, bw_cap % 1000,
>> - PCIE_SPEED2STR(speed_cap), width_cap);
>> + PCI_SPEED2STR(speed_cap), width_cap);
>> }
>>
>> /**
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 5fb1d76..5e1f810 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -291,13 +291,9 @@ struct pci_bus *pci_bus_get(struct pci_bus *bus);
>> void pci_bus_put(struct pci_bus *bus);
>>
>> /* PCIe link information */
>> -#define PCIE_SPEED2STR(speed) \
>> - ((speed) == PCIE_SPEED_32_0GT ? "32 GT/s" : \
>> - (speed) == PCIE_SPEED_16_0GT ? "16 GT/s" : \
>> - (speed) == PCIE_SPEED_8_0GT ? "8 GT/s" : \
>> - (speed) == PCIE_SPEED_5_0GT ? "5 GT/s" : \
>> - (speed) == PCIE_SPEED_2_5GT ? "2.5 GT/s" : \
>> - "Unknown speed")
>> +#define PCI_SPEED2STR(speed) \
>> + ((speed) == PCI_SPEED_UNKNOWN ? "Unknown speed" : \
>> + pci_bus_speed_strings[speed])
>>
>> /* PCIe speed to Mb/s reduced by encoding overhead */
>> #define PCIE_SPEED2MBS_ENC(speed) \
>> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
>> index 140dafb..871d598 100644
>> --- a/drivers/pci/slot.c
>> +++ b/drivers/pci/slot.c
>> @@ -51,14 +51,10 @@ static ssize_t address_read_file(struct pci_slot *slot, char *buf)
>>
>> static ssize_t bus_speed_read(enum pci_bus_speed speed, char *buf)
>> {
>> - const char *speed_string;
>> + if (speed <= PCI_SPEED_133MHz_PCIX_533)
>> + return sprintf(buf, "%s\n", PCI_SPEED2STR(speed));
>>
>> - if (speed == PCI_SPEED_UNKNOWN)
>> - speed_string = "Unknown";
>> - else
>> - speed_string = pci_bus_speed_strings[speed];
>> -
>> - return sprintf(buf, "%s\n", speed_string);
>> + return sprintf(buf, "%s PCIe\n", PCI_SPEED2STR(speed));
>> }
>>
>> static ssize_t max_speed_read_file(struct pci_slot *slot, char *buf)
>> --
>> 2.8.1
>>
> .
>
