Hi Philippe,
On 2/27/20 8:51 AM, Philippe Mathieu-Daudé wrote:
> The smmu_find_smmu_pcibus() function was introduced (in commit
> cac994ef43b) in a code format that could return an incorrect
> pointer, which was then fixed by the previous commit.
> We could have avoid this by writing the if() statement differently.
nit: avoided
> Do it now, in case this function is re-used. The code is easier to
> review (harder to miss bugs).
>
> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com>
> ---
> This patch is easier to review with 'git-diff -w' (--ignore-all-space)
> ---
> hw/arm/smmu-common.c | 25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 67d7b2d0fd..e13a5f4a7c 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -290,20 +290,21 @@ inline int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova,
> IOMMUAccessFlags perm,
> SMMUPciBus *smmu_find_smmu_pcibus(SMMUState *s, uint8_t bus_num)
> {
> SMMUPciBus *smmu_pci_bus = s->smmu_pcibus_by_bus_num[bus_num];
> + GHashTableIter iter;
>
> - if (!smmu_pci_bus) {
> - GHashTableIter iter;
> -
> - g_hash_table_iter_init(&iter, s->smmu_pcibus_by_busptr);
> - while (g_hash_table_iter_next(&iter, NULL, (void **)&smmu_pci_bus)) {
> - if (pci_bus_num(smmu_pci_bus->bus) == bus_num) {
> - s->smmu_pcibus_by_bus_num[bus_num] = smmu_pci_bus;
> - return smmu_pci_bus;
> - }
> - }
> - smmu_pci_bus = NULL;
> + if (smmu_pci_bus) {
> + return smmu_pci_bus;
> }
> - return smmu_pci_bus;
> +
> + g_hash_table_iter_init(&iter, s->smmu_pcibus_by_busptr);
> + while (g_hash_table_iter_next(&iter, NULL, (void **)&smmu_pci_bus)) {
> + if (pci_bus_num(smmu_pci_bus->bus) == bus_num) {
> + s->smmu_pcibus_by_bus_num[bus_num] = smmu_pci_bus;
> + return smmu_pci_bus;
> + }
> + }
> +
> + return NULL;
> }
>
> static AddressSpace *smmu_find_add_as(PCIBus *bus, void *opaque, int devfn)
>
Acked-by: Eric Auger <eric.au...@redhat.com>
Thanks!
Eric
