From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wj0-f177.google.com (mail-wj0-f177.google.com [209.85.210.177]) by dpdk.org (Postfix) with ESMTP id 9FDC132A5 for ; Tue, 6 Dec 2016 21:52:29 +0100 (CET) Received: by mail-wj0-f177.google.com with SMTP id tg4so81966667wjb.1 for ; Tue, 06 Dec 2016 12:52:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=Krm5beSB8ZqtAJE8pfS2u5sNP/b+AVWQfdzCXOsSuIA=; b=ZyviBdYym1pjpK1amssVUGVKl4efaf4l5/NLA/KUO+YxNsUvnTa+hXD9tn9mujC9TL a21rXyiGco9q4tYUqAsaweQ6bCnjRRZqpvR3HB26rTerzg091K84FYWoac/hmxCFY88+ Ye6kJdii2I9/DNCwQ6bj2EcyPmyMkEJ9mWmc3M3L/LeqbY3uPTNcsLqk7nWBWMpyQ4oM fnpL1QvkrT94IcaaBvIvzA2U838MZ2XZK+gIgUbg6kgCK2dahuqRcJLDfXZhq51ucbtp EQY2hlsd8GLZaEUcZL79RmML8d5iApZuAtYEd52m9BOOzAxF/TkdRldB/duDhy+zEfaA w8kg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=Krm5beSB8ZqtAJE8pfS2u5sNP/b+AVWQfdzCXOsSuIA=; b=hQAU0yBQgTFFMsbTSGIBDthBdDvAqcWi2CBhzbpHIDEuQUZAJSaiG4Dqf3y58Ebqfc WFql4aDGI+zupVCrg+gbfwZj/bOtSpN2FRFL6ju0pvm+bl4KW9lMVQQdUplPX09BFzx/ Udlw+CKkmLouWDK1d9jwKhNoMiMIi7oG699DaoM7PyBV0PWoKTymYP3mamQXau2s/Zlv SJuhLP9UJ4V0y6egtlnphK/kSN42g+2l4svJOKqeUQQ/DRItEMUOiHPowxlSt6wCAUUO IM6aChh6NJZ2jDnw7C6HZ15N8njPwaTBDwOpIyXCSWuN3F2u6lo3VxGNnW1p5u5QDXXa P3uQ== X-Gm-Message-State: AKaTC01wjKxOavFWPhC7y870+YWgI6IiHWwPzqfnTfOgzGWVYzv9Hg9BnA98DrWNJiIJy8hDbiLkEZ6ioCdzpu7U X-Received: by 10.194.126.38 with SMTP id mv6mr55682528wjb.142.1481057549303; Tue, 06 Dec 2016 12:52:29 -0800 (PST) MIME-Version: 1.0 Received: by 10.28.19.73 with HTTP; Tue, 6 Dec 2016 12:52:08 -0800 (PST) In-Reply-To: <1480846288-2517-1-git-send-email-shreyansh.jain@nxp.com> References: <1480846288-2517-1-git-send-email-shreyansh.jain@nxp.com> From: David Marchand Date: Tue, 6 Dec 2016 21:52:08 +0100 Message-ID: To: Shreyansh Jain Cc: "dev@dpdk.org" , Thomas Monjalon Content-Type: text/plain; charset=UTF-8 Subject: Re: [dpdk-dev] [PATCH 00/13] Introducing EAL Bus-Device-Driver Model 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: , X-List-Received-Date: Tue, 06 Dec 2016 20:52:29 -0000 "Big patchset and a lot of things to look at. Here is a first look at it. On Sun, Dec 4, 2016 at 11:11 AM, Shreyansh Jain wrote: > In continuation to the RFC posted on 17/Nov [9], > A series of patches is being posted which attempts to create: > 1. A basic bus model > `- define rte_bus and associated methods/helpers > `- test infrastructure to test the Bus infra > 2. Changes in EAL to support PCI as a bus > `- a "pci" bus is registered > `- existing scan/match/probe are modified to allow for bus integration > `- PCI Device and Driver list, which were global entities, have been > moved to rte_bus->[device/driver]_list > > I have sanity tested this patch over a XeonD X552 available with me, as > well as part of PoC for verifying NXP's DPAA2 PMD (being pushed out in a > separate series). Exhaustive testing is still pending. I saw some checkpatch issues for patch 1 (I would ignore this one) and 4 (please check). > :: Brief about Patch Layout :: > > 0001: Container_of patch from [3] > 0002~0003: Introducing the basic Bus model and associated test case > 0005: Support insertion of device rather than addition to tail Patch 2 and 5 could be squashed. The constructor priority stuff seems unneeded as long as we use explicit reference to a global (or local, did not check) bus symbol rather than a runtime lookup. > 0004: Add scan and match callbacks for the Bus and updated test case Why do you push back the bus object in the 'scan' method ? This method is bus specific which means that the code "knows" the object registered with the callback. Is is that you want to have a single scan method used by multiple buses ? > 0006: Integrate bus scan/match with EAL, without any effective driver Hard to find a right balance in patch splittng, but patch 4 and 6 are linked, I would squash them into one. > 0007: rte_pci_driver->probe replaced with rte_driver->probe This patch is too big, please separate in two patches: eal changes then ethdev/driver changes. I almost missed that mlx4 has been broken : you moved the drv_flags from the mlx4 pci driver to rte_driver. Why do you push back the driver object in the 'probe' method ? (idem rte_bus->scan). > 0008: Integrate probe of drivers with EAL This patch does nothing about "remove" while its title talks about it. + What about hotplug code ? I suppose this is for later. > 0009: Split the existing PCI probe into match and probe You don't need to expose rte_eal_pci_match_default() (and I am not sure what the "default" means here). This method should be internal to the pci bus object ? > 0010: Make PCI probe/match work on rte_driver/device rather than > rte_pci_device/rte_pci_driver > 0011: Patch from Ben [8], part of series [2] > 0012~0013: Enable Scan/Match/probe on Bus from EAL and remove unused > functions and lists Same thing as earlier in the series, you don't need to check for the bus object. The pci bus code can't be called if the bus was not registered and consequently, you are sure that the pci bus object is the pci_bus symbol. Regards, -- David Marchand