[v12,4/7] bus/pci: implement sigbus handler ops

  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@intel.com>
Acked-by: Shaopeng He <shaopeng.he@intel.com>
---
v12->v11:
no change.
---
 drivers/bus/pci/pci_common.c | 53 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
  

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@intel.com>
> Acked-by: Shaopeng He <shaopeng.he@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.

> +				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)
  

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@intel.com>
>> Acked-by: Shaopeng He <shaopeng.he@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.