[1/3] pci: move check for existing devfn into new pci_bus_devfn_available() helper

Also touch up the logic in do_pci_register_device() accordingly.

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

On 07/07/2017 10:44, Mark Cave-Ayland wrote:
> Also touch up the logic in do_pci_register_device() accordingly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/pci/pci.c |   14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 0c6f74a..04e6edb 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -951,6 +951,15 @@ uint16_t pci_requester_id(PCIDevice *dev)
>       return pci_req_id_cache_extract(&dev->requester_id_cache);
>   }
> 


Hi Mark,

> +static bool pci_bus_devfn_available(PCIBus *bus, int devfn)
> +{
> +    if (bus->devices[devfn]) {
> +        return false;
> +    } else {
> +        return true;
> +    }
> +}
> +
The function may simply return bus->devices[devfn], right?
(the return type should take care of the rest)



>   /* -1 for devfn means auto assign */
>   static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>                                            const char *name, int devfn,
> @@ -974,14 +983,15 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>       if (devfn < 0) {
>           for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
>               devfn += PCI_FUNC_MAX) {
> -            if (!bus->devices[devfn])
> +            if (pci_bus_devfn_available(bus, devfn)) {

I am all for making the code more readable, but in this
case I am not sure if it worth it. "bus->devices[devfn]"
is self explanatory, but maybe is a matter of taste.

Thanks,
Marcel

>                   goto found;
> +            }
>           }
>           error_setg(errp, "PCI: no slot/function available for %s, all in use",
>                      name);
>           return NULL;
>       found: ;
> -    } else if (bus->devices[devfn]) {
> +    } else if (!pci_bus_devfn_available(bus, devfn)) {
>           error_setg(errp, "PCI: slot %d function %d not available for %s,"
>                      " in use by %s",
>                      PCI_SLOT(devfn), PCI_FUNC(devfn), name,
>

On 10/07/17 08:24, Marcel Apfelbaum wrote:

> On 07/07/2017 10:44, Mark Cave-Ayland wrote:
>> Also touch up the logic in do_pci_register_device() accordingly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/pci/pci.c |   14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 0c6f74a..04e6edb 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -951,6 +951,15 @@ uint16_t pci_requester_id(PCIDevice *dev)
>>       return pci_req_id_cache_extract(&dev->requester_id_cache);
>>   }
>>
> 
> 
> Hi Mark,
> 
>> +static bool pci_bus_devfn_available(PCIBus *bus, int devfn)
>> +{
>> +    if (bus->devices[devfn]) {
>> +        return false;
>> +    } else {
>> +        return true;
>> +    }
>> +}
>> +
> The function may simply return bus->devices[devfn], right?
> (the return type should take care of the rest)
> 
> 
> 
>>   /* -1 for devfn means auto assign */
>>   static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus
>> *bus,
>>                                            const char *name, int devfn,
>> @@ -974,14 +983,15 @@ static PCIDevice
>> *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>>       if (devfn < 0) {
>>           for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
>>               devfn += PCI_FUNC_MAX) {
>> -            if (!bus->devices[devfn])
>> +            if (pci_bus_devfn_available(bus, devfn)) {
> 
> I am all for making the code more readable, but in this
> case I am not sure if it worth it. "bus->devices[devfn]"
> is self explanatory, but maybe is a matter of taste.

Yes I agree that on its own it comes across as a more cosmetic patch,
however I felt that it made the final logic in patch 3 much more
readable in terms of determining the behaviour for reserved vs.
unavailable slots.

Does anyone else have any strong opinions?


ATB,

Mark.