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 694ABA0C41; Fri, 16 Apr 2021 09:02:33 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 55122141B1F; Fri, 16 Apr 2021 09:02:33 +0200 (CEST) Received: from szxga04-in.huawei.com (szxga04-in.huawei.com [45.249.212.190]) by mails.dpdk.org (Postfix) with ESMTP id E5C8640140 for ; Fri, 16 Apr 2021 09:02:31 +0200 (CEST) Received: from DGGEMS408-HUB.china.huawei.com (unknown [172.30.72.59]) by szxga04-in.huawei.com (SkyGuard) with ESMTP id 4FM6Xp6Ytnz17QqX; Fri, 16 Apr 2021 15:00:10 +0800 (CST) Received: from [10.67.103.128] (10.67.103.128) by DGGEMS408-HUB.china.huawei.com (10.3.19.208) with Microsoft SMTP Server id 14.3.498.0; Fri, 16 Apr 2021 15:02:26 +0800 To: Ferruh Yigit , CC: , References: <1618046334-39857-1-git-send-email-humin29@huawei.com> <1618484959-4360-1-git-send-email-humin29@huawei.com> <4f0b93e4-3490-7823-ecf9-7cc499a1571b@intel.com> From: "Min Hu (Connor)" Message-ID: Date: Fri, 16 Apr 2021 15:02:27 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 MIME-Version: 1.0 In-Reply-To: <4f0b93e4-3490-7823-ecf9-7cc499a1571b@intel.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.128] X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH v5] ethdev: add sanity checks in control APIs 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 Sender: "dev" Hi, Ferruh, v6 has been sent to fix it, please check it out, thanks. 在 2021/4/15 23:38, Ferruh Yigit 写道: > On 4/15/2021 12:09 PM, Min Hu (Connor) wrote: >> This patch adds more sanity checks in control path APIs. >> >> Fixes: 214ed1acd125 ("ethdev: add iterator to match devargs input") >> Fixes: 3d98f921fbe9 ("ethdev: unify prefix for static functions and >> variables") >> Fixes: 0366137722a0 ("ethdev: check for invalid device name") >> Fixes: d948f596fee2 ("ethdev: fix port data mismatched in multiple >> process model") >> Fixes: 5b7ba31148a8 ("ethdev: add port ownership") >> Fixes: f8244c6399d9 ("ethdev: increase port id range") >> Cc: stable@dpdk.org >> >> Signed-off-by: Min Hu (Connor) >> Reviewed-by: Andrew Rybchenko > > Hi Connor, > > Please find a few comments below, with they are addressed: > > Reviewed-by: Ferruh Yigit > >> @@ -738,23 +769,30 @@ rte_eth_dev_owner_delete(const uint64_t owner_id) >>   int >>   rte_eth_dev_owner_get(const uint16_t port_id, struct >> rte_eth_dev_owner *owner) >>   { >> -    int ret = 0; >> -    struct rte_eth_dev *ethdev = &rte_eth_devices[port_id]; >> - >> -    eth_dev_shared_data_prepare(); >> +    struct rte_eth_dev *ethdev; >> -    rte_spinlock_lock(ð_dev_shared_data->ownership_lock); >> +    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -1); > > Not sure '-1' is a good return value, what do you think sending > '-ENODEV' as it is mostly done in the file? > > Also emptly line can be removed as suggested by Andrew. > > <...> > >> @@ -2138,8 +2181,14 @@ rte_eth_rx_hairpin_queue_setup(uint16_t >> port_id, uint16_t rx_queue_id, >>       int count; >>       RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >> - >>       dev = &rte_eth_devices[port_id]; >> + >> +    if (conf == NULL) { >> +        RTE_ETHDEV_LOG(ERR, "Failed to setup ethdev port %u Rx >> hairpin queue to NULL\n", >> +            port_id); > > When the line is long, can you break the message part to next line, to > reduce the length at least a few columns, like: > > RTE_ETHDEV_LOG(ERR, >     "Failed to setup ethdev port %u Rx hairpin queue to NULL\n", >     port_id); > > Same for all long messages. > > <...> > >> @@ -5619,6 +5853,11 @@ rte_eth_representor_id_get(const struct >> rte_eth_dev *ethdev, >>       struct rte_eth_representor_info *info = NULL; >>       size_t size; >> +    if (ethdev == NULL) { >> +        RTE_ETHDEV_LOG(ERR, "Failed to get device representor info >> from NULL\n"); >> +        return -EINVAL; >> +    } >> + > > This is also an internal function, not sure if we should verify the > 'ehtdev' here. > .