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

Burakov, Anatoly anatoly.burakov at intel.com
Tue Oct 2 16:39:54 CEST 2018


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.

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

-- 
Thanks,
Anatoly


More information about the dev mailing list