From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 78DBF42386; Thu, 12 Jan 2023 03:44:12 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2278240A7D; Thu, 12 Jan 2023 03:44:12 +0100 (CET) Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by mails.dpdk.org (Postfix) with ESMTP id C7C63400EF for ; Thu, 12 Jan 2023 03:44:10 +0100 (CET) Received: from kwepemm600004.china.huawei.com (unknown [172.30.72.54]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4Nspjz5TwSz16MZn; Thu, 12 Jan 2023 10:42:31 +0800 (CST) Received: from [10.67.103.231] (10.67.103.231) by kwepemm600004.china.huawei.com (7.193.23.242) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.34; Thu, 12 Jan 2023 10:44:06 +0800 Message-ID: <461a4536-fbbd-3848-af8d-2aa1ebaf8ccb@huawei.com> Date: Thu, 12 Jan 2023 10:44:06 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCH V4 1/5] drivers/bus: restore driver assignment at front of probing To: Ferruh Yigit , , CC: , , , References: <20220825024425.10534-1-lihuisong@huawei.com> <20221206092649.8287-1-lihuisong@huawei.com> <20221206092649.8287-2-lihuisong@huawei.com> <79192ffa-eb80-e56c-5c64-4a22374c7a1a@amd.com> From: "lihuisong (C)" In-Reply-To: <79192ffa-eb80-e56c-5c64-4a22374c7a1a@amd.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.231] X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To kwepemm600004.china.huawei.com (7.193.23.242) X-CFilter-Loop: Reflected X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 在 2023/1/11 20:51, Ferruh Yigit 写道: > On 12/6/2022 9:26 AM, Huisong Li wrote: >> The driver assignment was moved back at the end of the device probing >> because there is no something to use rte_driver during the phase of >> probing. See commit 391797f04208 ("drivers/bus: move driver assignment >> to end of probing") >> >> However, it is necessary for probing callback to reference rte_driver >> before probing. For example, probing callback may call some APIs which >> access the rte_pci_driver::driver by the device::driver pointer to get >> driver information. In this case, a segment fault will occur in probing >> callback if there is not this assignment. >> > Probing callback gets driver as parameter, so callback function can > access it via 'drv->driver', is there a specific usecase that > 'dev->device->driver' needs to be accessed explicitly? > > I assume this is related to coming patches that setting up device in > testpmd event callback, but can you please clarify exact need. For example, rte_eth_dev_info_get is called in this event callback to get driver name. > >> Further, some comments in code need to be updated if we do that. The >> driver pointer in rte_device is set before probing and needs to be reset >> if probing failed. And rte_dev_is_probed can not be called inside probing. >> >> Fixes: 391797f04208 ("drivers/bus: move driver assignment to end of probing") >> Cc: stable@dpdk.org >> >> Signed-off-by: Huisong Li >> --- >> drivers/bus/auxiliary/auxiliary_common.c | 9 +++++++-- >> drivers/bus/dpaa/dpaa_bus.c | 9 +++++++-- >> drivers/bus/fslmc/fslmc_bus.c | 8 +++++++- >> drivers/bus/ifpga/ifpga_bus.c | 12 +++++++++--- >> drivers/bus/pci/pci_common.c | 9 +++++++-- >> drivers/bus/vdev/vdev.c | 10 ++++++++-- >> drivers/bus/vmbus/vmbus_common.c | 9 +++++++-- >> 7 files changed, 52 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/bus/auxiliary/auxiliary_common.c b/drivers/bus/auxiliary/auxiliary_common.c >> index ff1369353a..13cb3fe0f8 100644 >> --- a/drivers/bus/auxiliary/auxiliary_common.c >> +++ b/drivers/bus/auxiliary/auxiliary_common.c >> @@ -132,16 +132,21 @@ rte_auxiliary_probe_one_driver(struct rte_auxiliary_driver *drv, >> } >> >> dev->driver = drv; >> + /* >> + * Reference rte_driver before probing so as to this pointer can >> + * be used to get driver information in case of segment fault in >> + * probing callback. >> + */ >> + dev->device.driver = &drv->driver; >> >> AUXILIARY_LOG(INFO, "Probe auxiliary driver: %s device: %s (NUMA node %i)", >> drv->driver.name, dev->name, dev->device.numa_node); >> ret = drv->probe(drv, dev); >> if (ret != 0) { >> dev->driver = NULL; >> + dev->device.driver = NULL; >> rte_intr_instance_free(dev->intr_handle); >> dev->intr_handle = NULL; >> - } else { >> - dev->device.driver = &drv->driver; >> } >> >> return ret; >> diff --git a/drivers/bus/dpaa/dpaa_bus.c b/drivers/bus/dpaa/dpaa_bus.c >> index e57159f5d8..f1b817e58c 100644 >> --- a/drivers/bus/dpaa/dpaa_bus.c >> +++ b/drivers/bus/dpaa/dpaa_bus.c >> @@ -693,17 +693,22 @@ rte_dpaa_bus_probe(void) >> (dev->device.devargs && >> dev->device.devargs->policy == RTE_DEV_BLOCKED)) >> continue; >> - >> + /* >> + * Reference rte_driver before probing so as to this >> + * pointer can be used to get driver information in case >> + * of segment fault in probing callback. >> + */ >> + dev->device.driver = &drv->driver; >> if (probe_all || >> (dev->device.devargs && >> dev->device.devargs->policy == RTE_DEV_ALLOWED)) { >> ret = drv->probe(drv, dev); >> if (ret) { >> + dev->device.driver = NULL; >> DPAA_BUS_ERR("unable to probe:%s", >> dev->name); >> } else { >> dev->driver = drv; >> - dev->device.driver = &drv->driver; >> } >> } >> break; >> diff --git a/drivers/bus/fslmc/fslmc_bus.c b/drivers/bus/fslmc/fslmc_bus.c >> index 57bfb5111a..4bc0c6d3d4 100644 >> --- a/drivers/bus/fslmc/fslmc_bus.c >> +++ b/drivers/bus/fslmc/fslmc_bus.c >> @@ -471,15 +471,21 @@ rte_fslmc_probe(void) >> continue; >> } >> >> + /* >> + * Reference rte_driver before probing so as to this >> + * pointer can be used to get driver information in case >> + * of segment fault in probing callback. >> + */ >> + dev->device.driver = &drv->driver; >> if (probe_all || >> (dev->device.devargs && >> dev->device.devargs->policy == RTE_DEV_ALLOWED)) { >> ret = drv->probe(drv, dev); >> if (ret) { >> + dev->device.driver = NULL; >> DPAA2_BUS_ERR("Unable to probe"); >> } else { >> dev->driver = drv; >> - dev->device.driver = &drv->driver; >> } >> } >> break; >> diff --git a/drivers/bus/ifpga/ifpga_bus.c b/drivers/bus/ifpga/ifpga_bus.c >> index bb943b58b5..5f23446f41 100644 >> --- a/drivers/bus/ifpga/ifpga_bus.c >> +++ b/drivers/bus/ifpga/ifpga_bus.c >> @@ -293,13 +293,19 @@ ifpga_probe_one_driver(struct rte_afu_driver *drv, >> >> /* reference driver structure */ >> afu_dev->driver = drv; >> + /* >> + * Reference rte_driver before probing so as to this pointer can >> + * be used to get driver information in case of segment fault in >> + * probing callback. >> + */ >> + afu_dev->device.driver = &drv->driver; >> >> /* call the driver probe() function */ >> ret = drv->probe(afu_dev); >> - if (ret) >> + if (ret) { >> afu_dev->driver = NULL; >> - else >> - afu_dev->device.driver = &drv->driver; >> + afu_dev->device.driver = NULL; >> + } >> >> return ret; >> } >> diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c >> index bc3a7f39fe..efaa1792e9 100644 >> --- a/drivers/bus/pci/pci_common.c >> +++ b/drivers/bus/pci/pci_common.c >> @@ -302,6 +302,12 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr, >> return ret; >> } >> } >> + /* >> + * Reference rte_driver before probing so as to this pointer can >> + * be used to get driver information in case of segment fault in >> + * probing callback. >> + */ >> + dev->device.driver = &dr->driver; >> } >> >> RTE_LOG(INFO, EAL, "Probe PCI driver: %s (%x:%x) device: "PCI_PRI_FMT" (socket %i)\n", >> @@ -314,6 +320,7 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr, >> return ret; /* no rollback if already succeeded earlier */ >> if (ret) { >> dev->driver = NULL; >> + dev->device.driver = NULL; >> if ((dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) && >> /* Don't unmap if device is unsupported and >> * driver needs mapped resources. >> @@ -325,8 +332,6 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr, >> dev->vfio_req_intr_handle = NULL; >> rte_intr_instance_free(dev->intr_handle); >> dev->intr_handle = NULL; >> - } else { >> - dev->device.driver = &dr->driver; >> } >> >> return ret; >> diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c >> index 41bc07dde7..2e3f0f2e12 100644 >> --- a/drivers/bus/vdev/vdev.c >> +++ b/drivers/bus/vdev/vdev.c >> @@ -207,9 +207,15 @@ vdev_probe_all_drivers(struct rte_vdev_device *dev) >> return -1; >> } >> >> + /* >> + * Reference rte_driver before probing so as to this pointer can >> + * be used to get driver information in case of segment fault in >> + * probing callback. >> + */ >> + dev->device.driver = &driver->driver; >> ret = driver->probe(dev); >> - if (ret == 0) >> - dev->device.driver = &driver->driver; >> + if (ret != 0) >> + dev->device.driver = NULL; >> return ret; >> } >> >> diff --git a/drivers/bus/vmbus/vmbus_common.c b/drivers/bus/vmbus/vmbus_common.c >> index 8d32d66504..feb1651984 100644 >> --- a/drivers/bus/vmbus/vmbus_common.c >> +++ b/drivers/bus/vmbus/vmbus_common.c >> @@ -110,6 +110,12 @@ vmbus_probe_one_driver(struct rte_vmbus_driver *dr, >> >> /* reference driver structure */ >> dev->driver = dr; >> + /* >> + * Reference rte_driver before probing so as to this pointer can >> + * be used to get driver information in case of segment fault in >> + * probing callback. >> + */ >> + dev->device.driver = &dr->driver; >> >> if (dev->device.numa_node < 0 && rte_socket_count() > 1) >> VMBUS_LOG(INFO, "Device %s is not NUMA-aware", guid); >> @@ -119,9 +125,8 @@ vmbus_probe_one_driver(struct rte_vmbus_driver *dr, >> ret = dr->probe(dr, dev); >> if (ret) { >> dev->driver = NULL; >> + dev->device.driver = NULL; >> rte_vmbus_unmap_device(dev); >> - } else { >> - dev->device.driver = &dr->driver; >> } >> >> return ret; > .