From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 0BA17A0096 for ; Fri, 7 Jun 2019 10:32:50 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id B91B42E8F; Fri, 7 Jun 2019 10:32:50 +0200 (CEST) Received: from mail-it1-f181.google.com (mail-it1-f181.google.com [209.85.166.181]) by dpdk.org (Postfix) with ESMTP id 85D4C2E8F for ; Fri, 7 Jun 2019 10:32:49 +0200 (CEST) Received: by mail-it1-f181.google.com with SMTP id j204so1496475ite.4 for ; Fri, 07 Jun 2019 01:32:49 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=+2pjAgF0AGbiVv6umNKpzUV0Yk0bW0ubO0xXfRCdQj8=; b=kDaaXPzZW+0/tDt1hCAdnhOlbigBGEX/TfcmdJ8FWrdKRQ12E5oYI8I4FdxQ8NXu2W MkJ5WBAkb+AMhWOnJOhoieiuIXAamBE4RBLVujmHJUSy+fTPs4TgDXYYQ7CXlm1q5z3+ Ogd/k6CUJ3LZNRGFkd0lxRc7ZABkrGsSMurXlvQ5OIJEpFsDCmgefMk/1pQboohOANIe MQBc5Kapt5Stz8IjwNf6KGljiYdn5ohcgrbW//i8DdqCi3b/YJimc6jKlDPoku1+FOdl NYKdP7y1GlsYUzsAlT/LgX+OVp3hbPkhcGFRwgaTSE8aNNJJONTkHdw8VzkTCiedPWLT RT7Q== X-Gm-Message-State: APjAAAWYe+LZCE98bCHRf0cjUOp8baFxj/5Q2nFDyxki4gxxQB1kIl2x cJlX3tk6U2j9BVaOKPkZHZghkN+dORGbvE0uGH8sDA== X-Google-Smtp-Source: APXvYqxpOUWOUPMHUzi4IREYSePOfNZFly9H0RKtOchCT2ldFkXK6yel+PdlHfxB3qF4XNizU+FlEuEEL3a7xLu3iAs= X-Received: by 2002:a24:edc1:: with SMTP id r184mr3066887ith.133.1559896368897; Fri, 07 Jun 2019 01:32:48 -0700 (PDT) MIME-Version: 1.0 References: <20190530132526.3496-1-i.maximets@samsung.com> <20190606100228.19959-1-i.maximets@samsung.com> In-Reply-To: <20190606100228.19959-1-i.maximets@samsung.com> From: David Marchand Date: Fri, 7 Jun 2019 10:32:37 +0200 Message-ID: To: Ilya Maximets , Thomas Monjalon , Anatoly Burakov Cc: dev , Jan Blunck , Qi Zhang , Kevin Traynor , dpdk stable , Gaetan Rivet Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-stable] [PATCH v2] eal: fix positive error codes from probe/remove X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org Sender: "stable" On Thu, Jun 6, 2019 at 12:03 PM Ilya Maximets wrote: > According to API, 'rte_dev_probe()' and 'rte_dev_remove()' must > return 0 or negative error code. Bus code returns positive values > if device wasn't recognized by any driver, so the result of > 'bus->plug/unplug()' must be converted. 'local_dev_probe()' and > 'local_dev_remove()' also has their internal API, so the conversion > should be done there. > > Positive on remove means that device not found by driver. > For backports, it is safer to add the check on > 0. The patch looks good to me. Reviewed-by: David Marchand But I have some comments on the current state of the code. After inspecting the eal and buses, this problem is not supposed to happen on the rte_dev_remove path. rte_dev_remove() ensures that it calls local_dev_remove() after checking that the device is attached to a driver (see the check on !rte_dev_probed()). Anatoly, - When handling a detach operation in the primary process https://git.dpdk.org/dpdk/tree/lib/librte_eal/common/hotplug_mp.c#n124, we signal all other secondary processes to detach right away. Then we do a bus/device lookup. Then we call the bus unplug. Would not it be better to check the device exists _and_ check if the device is attached to a driver in the primary process before calling other secondary processes? Thomas, - Calling unplug on a device that is not attached is a bit weird to me, all the more so that we have rte_dev_probed(). But there might be users calling directly the bus unplug api and not the official api... Does this enter the ABI stability perimeter? If not, I would be for changing unplug api so that we only deal with 0 or < 0 on remove path. On the plug side, is there a reason why we do not check for rte_dev_probed() and let the bus replies that the device is already probed? Does it have something to do with representors ? Only guessing. - On the plug side again, can't we have an indication from the buses that they have a driver that can handle the device rather than this odd (and historical) > 0 return code? This should not change the current behavior, just make the code a bit easier to understand. I know you are travelling, so this can wait anyway. -- David Marchand