[dpdk-dev] [PATCH v12 4/7] bus/pci: implement sigbus handler ops

Jeff Guo jia.guo at intel.com
Thu Oct 4 05:58:15 CEST 2018


On 10/2/2018 10:39 PM, Burakov, Anatoly wrote:
> On 02-Oct-18 1:35 PM, Jeff Guo wrote:
>> This patch implements the ops for the PCI bus sigbus handler. It 
>> finds the
>> PCI device that is being hot-unplugged and calls the relevant ops of the
>> hot-unplug handler to handle the hot-unplug failure of the device.
>>
>> Signed-off-by: Jeff Guo <jia.guo at intel.com>
>> Acked-by: Shaopeng He <shaopeng.he at intel.com>
>> ---
>> v12->v11:
>> no change.
>> ---
>>   drivers/bus/pci/pci_common.c | 53 
>> ++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 53 insertions(+)
>>
>> diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
>> index d286234..f313fe9 100644
>> --- a/drivers/bus/pci/pci_common.c
>> +++ b/drivers/bus/pci/pci_common.c
>> @@ -405,6 +405,36 @@ pci_find_device(const struct rte_device *start, 
>> rte_dev_cmp_t cmp,
>>       return NULL;
>>   }
>>   +/**
>> + * find the device which encounter the failure, by iterate over all 
>> device on
>> + * PCI bus to check if the memory failure address is located in the 
>> range
>> + * of the BARs of the device.
>> + */
>> +static struct rte_pci_device *
>> +pci_find_device_by_addr(const void *failure_addr)
>> +{
>> +    struct rte_pci_device *pdev = NULL;
>> +    int i;
>> +
>> +    FOREACH_DEVICE_ON_PCIBUS(pdev) {
>> +        for (i = 0; i != RTE_DIM(pdev->mem_resource); i++) {
>> +            if ((uint64_t)(uintptr_t)failure_addr >=
>> + (uint64_t)(uintptr_t)pdev->mem_resource[i].addr &&
>> +                (uint64_t)(uintptr_t)failure_addr <
>> + (uint64_t)(uintptr_t)pdev->mem_resource[i].addr +
>> +                pdev->mem_resource[i].len) {
>
> You must *really* dislike local variables :) Suggested rewriting:
>
> const void *start, *end;
> size_t len;
>
> start = pdev->mem_resource[i].addr;
> len = pdev->mem_resource[i].len;
> end = RTE_PTR_ADD(start, len);
>
> if (failure_addr >= start && failure_addr < end) {
>     ...
> }
>
> I think this is way clearer.
>

good point, local variable might be good and helpful. Thanks.


>> +                RTE_LOG(INFO, EAL, "Failure address "
>> +                    "%16.16"PRIx64" belongs to "
>> +                    "device %s!\n",
>> +                    (uint64_t)(uintptr_t)failure_addr,
>> +                    pdev->device.name);
>
> I feel like this should have DEBUG level, rather than INFO. 
> Alternatively, if you really feel like this should be at level INFO, 
> the message should be reworded because the word "failure" might give 
> the wrong impression :)
>
> (but really, i think this is info useful for debugging purposes but 
> not interesting generally, so it should be under DEBUG IMO)
>

ok, i accept it.



More information about the dev mailing list