Re: [PATCH v2 1/1] vpci: Add resizable bar support

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2024/12/10 19:25, Roger Pau Monné wrote:
> On Tue, Dec 10, 2024 at 10:54:43AM +0100, Jan Beulich wrote:
>> On 10.12.2024 08:57, Chen, Jiqian wrote:
>>> On 2024/12/10 15:17, Jan Beulich wrote:
>>>> On 10.12.2024 08:07, Chen, Jiqian wrote:
>>>>> On 2024/12/9 21:59, Jan Beulich wrote:
>>>>>> On 02.12.2024 07:09, Jiqian Chen wrote:
>>>>>>> +        if ( rc )
>>>>>>> +        {
>>>>>>> +            printk("%pp: add register for PCI_REBAR_CAP failed 
>>>>>>> (rc=%d)\n",
>>>>>>> +                   &pdev->sbdf, rc);
>>>>>>> +            break;
>>>>>>> +        }
>>>>>>> +
>>>>>>> +        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, 
>>>>>>> rebar_ctrl_write,
>>>>>>> +                               rebar_offset + PCI_REBAR_CTRL, 4,
>>>>>>> +                               pdev->vpci->header.bars);
>>>>>>> +        if ( rc )
>>>>>>> +        {
>>>>>>> +            printk("%pp: add register for PCI_REBAR_CTRL failed %d\n",
>>>>>>> +                   &pdev->sbdf, rc);
>>>>>>> +            break;
>>>>>>
>>>>>> Is it correct to keep the other handler installed? After all ...
>>>>> Will change to "return rc;" here and above in next version.
>>>>
>>>> I'm not convinced this is what we want, as per ...
>>>>
>>>>>>> +        }
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    return 0;
>>>>>>
>>>>>> ... you - imo sensibly - aren't communicating the error back up (to allow
>>>>>> the device to be used without BAR resizing.
>>>>
>>>> ... what I said here.
>>> Sorry, I didn’t understand.
>>> Do you mean it is not enough to return error code once a handler failed to 
>>> be installed, I need to remove the already installed handlers?
>>
>> No, if you return an error here, nothing else needs doing. However, I
>> question that returning an error here is good or even necessary. In
>> the event of an error, the device ought to still be usable, just
>> without the BAR-resizing capability.
> 
> So you suggest that the capability should be hidden in that case?  We
> have logic to hide capabilities, just not used for the hardware
> domain.  It would need some extra wiring to be capable of hiding
> failed capabilities.
Can you give me a guidance on how to hide a failed capability?
What codes are current logic to hide capabilities?
Then maybe I can add a patch to implement it.

> 
> Regards, Roger.

-- 
Best regards,
Jiqian Chen.