On 17.12.2024 09:22, Chen, Jiqian wrote:
> On 2024/12/16 19:24, Jan Beulich wrote:
>> On 13.12.2024 06:42, Jiqian Chen wrote:
>>> +static int cf_check init_rebar(struct pci_dev *pdev)
>>> +{
>>> + uint32_t ctrl;
>>> + unsigned int rebar_offset, nbars;
>>> +
>>> + rebar_offset = pci_find_ext_capability(pdev->sbdf,
>>> PCI_EXT_CAP_ID_REBAR);
>>> +
>>> + if ( !rebar_offset )
>>> + return 0;
>>> +
>>> + if ( !is_hardware_domain(pdev->domain) )
>>> + {
>>> + printk("ReBar is not supported for domUs\n");
>>> + return -EOPNOTSUPP;
>>> + }
>>> +
>>> + ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL);
>>> + nbars = MASK_EXTR(ctrl, PCI_REBAR_CTRL_NBAR_MASK);
>>> +
>>> + for ( unsigned int i = 0; i < nbars; i++, rebar_offset +=
>>> PCI_REBAR_CTRL )
>>
>> PCI_REBAR_CTRL is an offset; it can't be used to bump rebar_offset here.
>> That'll need a separate constant, even if both evaluate to 8.
> I will add a new macro to represent the '8' in rebar.c
> Maybe I can name it "PCI_REBAR_SINGLE_BAR_LEN" ?
Naming is a 2nd step only, I think (and no really suitable name comes to mind).
Before thinking of names, I think the approach of doing the accesses here wants
reconsidering. It isn't quite right to bump rebar_offset. When using #define-s,
I'd instead expect to first move _just_ past the capability header, and then
use constants to get at capability and control registers. Alternatively, if we
want to express everything relative to rebar_offset, I think we'd want
#define PCI_REBAR_CAP(n) (4 + 8 *(n))
#define PCI_REBAR_CTRL(n) (8 + 8 *(n))
eliminating the need to alter rebar_offset (and hence disconnecting variable
name from its purpose).
Jan
