Re: [PATCH 7/8] PCI: replace stray uses of PCI_{DEVFN,BDF}2()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.04.2022 16:13, Bertrand Marquis wrote:
>> On 13 Apr 2022, at 14:58, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>> On Wed, Apr 13, 2022 at 01:48:14PM +0000, Bertrand Marquis wrote:
>>>> On 11 Apr 2022, at 10:40, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>
>>>> There's no good reason to use these when we already have a pci_sbdf_t
>>>> type object available. This extends to the use of PCI_BUS() in
>>>> pci_ecam_map_bus() as well.
>>>>
>>>> No change to generated code (with gcc11 at least, and I have to admit
>>>> that I didn't expect compilers to necessarily be able to spot the
>>>> optimization potential on the original code).
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>> ---
>>>> Note that the Arm changes are "blind": I haven't been able to spot a way
>>>> to at least compile test the changes there; the code looks to be
>>>> entirely dead.

Note this remark.

>>>> --- a/xen/arch/arm/pci/ecam.c
>>>> +++ b/xen/arch/arm/pci/ecam.c
>>>> @@ -28,8 +28,7 @@ void __iomem *pci_ecam_map_bus(struct pc
>>>>        container_of(bridge->ops, const struct pci_ecam_ops, pci_ops);
>>>>    unsigned int devfn_shift = ops->bus_shift - 8;
>>>>    void __iomem *base;
>>>> -
>>>> -    unsigned int busn = PCI_BUS(sbdf.bdf);
>>>> +    unsigned int busn = sbdf.bus;
>>>>
>>>>    if ( busn < cfg->busn_start || busn > cfg->busn_end )
>>>>        return NULL;
>>>> @@ -37,7 +36,7 @@ void __iomem *pci_ecam_map_bus(struct pc
>>>>    busn -= cfg->busn_start;
>>>>    base = cfg->win + (busn << ops->bus_shift);
>>>>
>>>> -    return base + (PCI_DEVFN2(sbdf.bdf) << devfn_shift) + where;
>>>> +    return base + (sbdf.df << devfn_shift) + where;
>>>
>>> I think this should be sbdf.bdf instead (typo you removed the b).
>>
>> I don't think so, notice PCI_DEVFN2(sbdf.bdf) which is extracting the
>> devfn from sbdf.bdf. That's not needed, as you can just get the devfn
>> directly from sbdf.df.
>>
>> Or else the original code is wrong, and the PCI_DEVFN2 shouldn't be
>> there.
> 
> There is not df field in the sbdf structure so it should be devfn instead.

Yes indeed, thanks for noticing. But really (see the remark further up)
this is what happens if code in the tree can't even be built-tested.

Jan