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 D56A7A0096 for ; Fri, 7 Jun 2019 10:32:52 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E73DF4C99; Fri, 7 Jun 2019 10:32:50 +0200 (CEST) Received: from mail-it1-f170.google.com (mail-it1-f170.google.com [209.85.166.170]) by dpdk.org (Postfix) with ESMTP id 8B1D73237 for ; Fri, 7 Jun 2019 10:32:49 +0200 (CEST) Received: by mail-it1-f170.google.com with SMTP id l21so1514282ita.2 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=YcsyknPUMuYi6wd/RdZJtl+9xpFaJNdtJdXpiDvOPVZZ9MsknCRdvVw5ZxmeU97UK3 oXWLPOokyQi4LjIXgWijtUVq2OmHKcfMWmRi14eW9ZVe3f23rnoDJXzPBBtwfTpMR7rq HMst0FLMaMBWT8uCYUgU8UVJpXKEvsPNtudLqqgNoydRFvMbN230oAS2MMWN3uX5pAO/ qa7hogkhxF5uHTjl4E7EcmxAUfYzHo9SNRifSZSAhBNNXM+FLc/d1wq8atp2aI1htVsM YUYrKF0buIkyZ757BvjhN5x+KU+rt7vn5U+JHvWS515HoEBLxaDejZDPBxvbloUvzkQ+ 3Lhw== X-Gm-Message-State: APjAAAVntfV915L0zeVdWLbSm/Pmd443klIiGH5g/JNITqJQ1qAz+7UM /ancf+IwRr757Z7qNQtH6XsFwKZrGGGpF0R1llVREw== 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-dev] [PATCH v2] eal: fix positive error codes from probe/remove X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" 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