[2/2] pci: allow DeviceClass fw_name to override pci_dev_fw_name() if set

The current implementation of pci_dev_fw_name() scans through a fixed
set of PCI class descriptions to determine the firmware device name
but in some cases this isn't always appropriate, for example with
the macio device which uses a class code of 0xff (unassigned).

Rather than add a new entry for the macio device and risk a potential
clash with another unassigned device later, add a check to
pcibus_get_fw_dev_path() that will use DeviceClass fw_name if set in
preference to pci_dev_fw_name().

This enables PCI devices such as macio to set dc->fw_name as required
to match the name specified in the firmware.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/pci/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

On 06/23/2018 05:50 AM, Mark Cave-Ayland wrote:
> The current implementation of pci_dev_fw_name() scans through a fixed
> set of PCI class descriptions to determine the firmware device name
> but in some cases this isn't always appropriate, for example with
> the macio device which uses a class code of 0xff (unassigned).
> 
> Rather than add a new entry for the macio device and risk a potential
> clash with another unassigned device later, add a check to
> pcibus_get_fw_dev_path() that will use DeviceClass fw_name if set in
> preference to pci_dev_fw_name().
> 
> This enables PCI devices such as macio to set dc->fw_name as required
> to match the name specified in the firmware.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/pci/pci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 80bc45930d..126dd683dc 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2411,12 +2411,14 @@ static char *pci_dev_fw_name(DeviceState *dev, char *buf, int len)
>  
>  static char *pcibus_get_fw_dev_path(DeviceState *dev)
>  {
> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
>      PCIDevice *d = (PCIDevice *)dev;
>      char path[50], name[33];
>      int off;
>  
>      off = snprintf(path, sizeof(path), "%s@%x",
> -                   pci_dev_fw_name(dev, name, sizeof name),
> +                   dc->fw_name ? dc->fw_name :
> +                       pci_dev_fw_name(dev, name, sizeof name),
>                     PCI_SLOT(d->devfn));
>      if (PCI_FUNC(d->devfn))
>          snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
>

On Sat, Jun 23, 2018 at 09:50:28AM +0100, Mark Cave-Ayland wrote:
> The current implementation of pci_dev_fw_name() scans through a fixed
> set of PCI class descriptions to determine the firmware device name
> but in some cases this isn't always appropriate, for example with
> the macio device which uses a class code of 0xff (unassigned).
> 
> Rather than add a new entry for the macio device and risk a potential
> clash with another unassigned device later, add a check to
> pcibus_get_fw_dev_path() that will use DeviceClass fw_name if set in
> preference to pci_dev_fw_name().
> 
> This enables PCI devices such as macio to set dc->fw_name as required
> to match the name specified in the firmware.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Do we know no existing pci device sets this?

> ---
>  hw/pci/pci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 80bc45930d..126dd683dc 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2411,12 +2411,14 @@ static char *pci_dev_fw_name(DeviceState *dev, char *buf, int len)
>  
>  static char *pcibus_get_fw_dev_path(DeviceState *dev)
>  {
> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
>      PCIDevice *d = (PCIDevice *)dev;
>      char path[50], name[33];
>      int off;
>  
>      off = snprintf(path, sizeof(path), "%s@%x",
> -                   pci_dev_fw_name(dev, name, sizeof name),
> +                   dc->fw_name ? dc->fw_name :
> +                       pci_dev_fw_name(dev, name, sizeof name),
>                     PCI_SLOT(d->devfn));
>      if (PCI_FUNC(d->devfn))
>          snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
> -- 
> 2.11.0

On 24/06/18 04:55, Michael S. Tsirkin wrote:

> On Sat, Jun 23, 2018 at 09:50:28AM +0100, Mark Cave-Ayland wrote:
>> The current implementation of pci_dev_fw_name() scans through a fixed
>> set of PCI class descriptions to determine the firmware device name
>> but in some cases this isn't always appropriate, for example with
>> the macio device which uses a class code of 0xff (unassigned).
>>
>> Rather than add a new entry for the macio device and risk a potential
>> clash with another unassigned device later, add a check to
>> pcibus_get_fw_dev_path() that will use DeviceClass fw_name if set in
>> preference to pci_dev_fw_name().
>>
>> This enables PCI devices such as macio to set dc->fw_name as required
>> to match the name specified in the firmware.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> 
> Do we know no existing pci device sets this?

Here was my basic check:

# Find all files containing PCIDevice
find . -type f -exec grep -l "PCIDevice" {} \; > /tmp/pcidevices.txt
# Of these return files containing 'dc->fw_name'
for FILE in `cat /tmp/pcidevices.txt`; do grep -H 'dc->fw_name' $FILE; done;

./hw/ppc/spapr.c:        snprintf(cpu_model, 32, "%s@%x", dc->fw_name, 
index);
./hw/ppc/spapr.c:        nodename = g_strdup_printf("%s@%x", 
dc->fw_name, index);
./hw/ppc/spapr.c:    nodename = g_strdup_printf("%s@%x", dc->fw_name, id);
./hw/pci-bridge/pci_expander_bridge.c:    dc->fw_name = "pci";
./hw/pci-host/designware.c:    dc->fw_name = "pci";
./hw/pci-host/piix.c:    dc->fw_name = "pci";
./hw/pci-host/gpex.c:    dc->fw_name = "pci";
./hw/pci-host/xilinx-pcie.c:    dc->fw_name = "pci";
./hw/pci-host/q35.c:    dc->fw_name = "pci";
./hw/pci-host/prep.c:    dc->fw_name = "pci";

 From what I can see the usage in spapr.c is for setting the DT CPU name 
whilst all of the others are derived from 
PCIE_HOST_BRIDGE/PCI_HOST_BRIDGE which are ultimately sysbus devices.


ATB,

Mark.