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 82260A0543; Tue, 25 Oct 2022 05:25:45 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 270E4410FB; Tue, 25 Oct 2022 05:25:45 +0200 (CEST) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id 0785A410D3 for ; Tue, 25 Oct 2022 05:25:42 +0200 (CEST) Received: from dggemv703-chm.china.huawei.com (unknown [172.30.72.54]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4MxHJl210PzVj1C; Tue, 25 Oct 2022 11:20:55 +0800 (CST) Received: from kwepemm600004.china.huawei.com (7.193.23.242) by dggemv703-chm.china.huawei.com (10.3.19.46) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Tue, 25 Oct 2022 11:25:40 +0800 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.31; Tue, 25 Oct 2022 11:25:40 +0800 Message-ID: <77e478de-4e48-93f1-ecf4-a0510e3977d4@huawei.com> Date: Tue, 25 Oct 2022 11:25:39 +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 V2 1/6] bus/pci: fix a segfault when call callback To: Thomas Monjalon CC: , , , , References: <20220825024425.10534-1-lihuisong@huawei.com> <20220915124522.5407-1-lihuisong@huawei.com> <20220915124522.5407-2-lihuisong@huawei.com> <11178860.MucGe3eQFb@thomas> From: "lihuisong (C)" In-Reply-To: <11178860.MucGe3eQFb@thomas> 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 Hi Thomas, I missed this e-mail, I'm sorry for late reply. 在 2022/10/11 3:49, Thomas Monjalon 写道: > 15/09/2022 14:45, Huisong Li: >> After the driver probe is executed, the callback in application will >> be called. The callback in application may call some APIs which access the >> rte_pci_driver::driver by the device::driver pointer to get driver >> information. If the rte_pci_device::device::driver pointer isn't pointed to >> rte_pci_driver::driver in rte_pci_probe_one_driver, a segfault will occur. >> For example, when ethdev driver probe completes, the callback in >> application call rte_eth_dev_info_get which use dev->device->driver->name. >> So rte_pci_device::device::driver should point to rte_pci_driver::driver >> before executing the driver probe. >> >> Fixes: c752998b5e2e ("pci: introduce library and driver") >> Cc: stable@dpdk.org > To be more precise, it is reverting > 391797f04208 ("drivers/bus: move driver assignment to end of probing") > > dev->device.driver is used by rte_dev_is_probed(): > > int > rte_dev_is_probed(const struct rte_device *dev) > { > /* The field driver should be set only when the probe is successful. */ > return dev->driver != NULL; > } > > And the field comment is clear in rte_device: > > const struct rte_driver *driver; /**< Driver assigned after probing */ > > That's why I am not enthusiastic about setting this pointer before probing. +1 > > I understand it is more convenient to use this pointer > in a probing callback. > We need to check it is not breaking rte_dev_is_probed() usage. > It may be OK if there is no parallel probing, > and rte_dev_is_probed() is not called inside probing. From all the places where it's used, first of all, it is a internal interface and only used when scan device on bus, remove device and hotplug device. In process of scanning or probing device, the core code checks whether the device is already probed by calling rte_dev_is_probed(). So I think it is ok for parallel probing(if exist) to set dev->device.driver before probing successful. The Concurrency between probing and removing should not be possible. The application removes a device after device probing successful. Currently, rte_dev_is_probed() isn't called inside probing. > > At the very minimum, we need to update some comments in the code, > to mention that the pointer is set before probing, > and reset if probing failed. If it is ok for you, I will update some comments and add this modification to other bus type in next version. > > > .