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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> 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:
>> Hi Jan,
>> 
>>> 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.
>>> 
>>> --- 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.

Cheers
Bertrand

> 
> Thanks, Roger.