From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <thomas.monjalon@6wind.com>
Received: from mail-wi0-f170.google.com (mail-wi0-f170.google.com
[209.85.212.170]) by dpdk.org (Postfix) with ESMTP id B0486B6E6
for <dev@dpdk.org>; Tue, 17 Feb 2015 10:23:41 +0100 (CET)
Received: by mail-wi0-f170.google.com with SMTP id hi2so30061554wib.1
for <dev@dpdk.org>; Tue, 17 Feb 2015 01:23:41 -0800 (PST)
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20130820;
h=x-gm-message-state:from:to:cc:subject:date:message-id:organization
:user-agent:in-reply-to:references:mime-version
:content-transfer-encoding:content-type;
bh=r1RSIwnTsfR3ieOWgioMe12CI3YSfSNnPvytVCKHmuw=;
b=fYBBVlQOqg65on11Jtk9P5dlp3N3nbIXOQmWpdcGYYFssoqsRFN+W3K2Et4h+2n55u
dr8FU3D1RqMO8N1aHELT+ogzcs3aa3d52ghNIAKWlyz0WiRi6b7U2nsg1k2klhT6VFAX
zO3+TdYf/olGSwYKCIWycy49eWIH5NCl2sF3hu+QoatdPTsRE3zlejVRA2j+UPvhr0nX
gvYojgsx40jkSWLxxhrucqpG/D/0WXT4fGO0tAjVYSf1Yr18ut+em7fNSJVT/3KR02ca
EG1BgYodNT0hAmCOh5rcn40r63Oe6MS0m50nMigRAvAeCLpB+wWhrKFmUDCvV3Mc55yl
R80g==
X-Gm-Message-State: ALoCoQmnxDhf/PSal6qQcUkb0yTikW0XqpwhxcWVFHBg5Swqh7+Kqxv0hIkgN6mkT6uX/NEStJ6N
X-Received: by 10.194.61.100 with SMTP id o4mr59981658wjr.28.1424165021325;
Tue, 17 Feb 2015 01:23:41 -0800 (PST)
Received: from xps13.localnet (6wind.net2.nerim.net. [213.41.180.237])
by mx.google.com with ESMTPSA id hs7sm19289659wib.4.2015.02.17.01.23.39
(version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);
Tue, 17 Feb 2015 01:23:40 -0800 (PST)
From: Thomas Monjalon <thomas.monjalon@6wind.com>
To: Tetsuya Mukawa <mukawa@igel.co.jp>
Date: Tue, 17 Feb 2015 10:23:05 +0100
Message-ID: <1733734.Ud8Ok1nOed@xps13>
Organization: 6WIND
User-Agent: KMail/4.14.4 (Linux/3.18.4-1-ARCH; KDE/4.14.4; x86_64; ; )
In-Reply-To: <54E300F8.8000503@igel.co.jp>
References: <1423470639-15744-2-git-send-email-mukawa@igel.co.jp>
<83798909.brLNWNC9tj@xps13> <54E300F8.8000503@igel.co.jp>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v8 12/14] eal/pci: Add
rte_eal_dev_attach/detach() functions
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Tue, 17 Feb 2015 09:23:41 -0000
2015-02-17 17:51, Tetsuya Mukawa:
> On 2015/02/17 10:48, Thomas Monjalon wrote:
> > 2015-02-16 13:14, Tetsuya Mukawa:
> >> +/* attach the new physical device, then store port_id of the device */
> >> +static int
> >> +rte_eal_dev_attach_pdev(struct rte_pci_addr *addr, uint8_t *port_id)
> >> +{
> >> + uint8_t new_port_id;
> >> + struct rte_eth_dev devs[RTE_MAX_ETHPORTS];
> >> +
> >> + if ((addr == NULL) || (port_id == NULL))
> >> + goto err;
> >> +
> >> + /* save current port status */
> >> + if (rte_eth_dev_save(devs, sizeof(devs)))
> >> + goto err;
> >> + /* re-construct pci_device_list */
> >> + if (rte_eal_pci_scan())
> >> + goto err;
> >> + /* invoke probe func of the driver can handle the new device */
> >> + if (rte_eal_pci_probe_one(addr))
> >> + goto err;
> > You should get the port_id from the previous function instead of searching it.
>
> I agree this will beautify this code, but actually to do like above
> changes current DPDK code much more, and it will not be clear, and not
> beautiful.
>
> Could I explain it more.
> Problem is initialization sequence of virtual device and physical device
> are completely different.
>
> (1) Attaching a physical device case
> - To return port id, pci_invoke_all_drivers() needs to return port id.
> - It means "devinit" of "struct rte_pci_driver" needs to return port_id.
> - "devinit" will be rte_eth_dev_init(). But if the device is virtual,
> this function is not implemented.
>
> (2) Attaching virtual device case
> - To return port id from rte_eal_pci_probe_one(),
> pci_invoke_all_drivers() needs to return port id.
> - It means "init" of "struct rte_driver" needs to return port_id.
> - "init" will be implemented in PMD. But this function has different
> usage in physical device and virtual device.
> - Especially, In the case of physical device, "init" doesn't allocate
> eth device, so cannot return port id.
>
> As a result, to remove rte_eth_dev_save() and
> rte_eth_dev_get_changed_port(), below different functions needs to
> return port id.
> - "devinit" of "struct rte_pci_driver".
> - "init" of "struct rte_driver".
Yes, exactly,
I think you shouldn't hesitate to improve existing EAL code.
And I also think we should try to remove differences between virtual and
pci devices.
> That is why I implement like above.
>
> >> + /* get port_id enabled by above procedures */
> >> + if (rte_eth_dev_get_changed_port(devs, &new_port_id))
> >> + goto err;
> >> +
> >> + *port_id = new_port_id;
> >> + return 0;
> >> +err:
> >> + RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n");
> >> + return -1;
> >> +}
> > [...]
> >
> >> +/* attach the new virtual device, then store port_id of the device */
> >> +static int
> >> +rte_eal_dev_attach_vdev(const char *vdevargs, uint8_t *port_id)
> >> +{
> >> + char *args;
> >> + uint8_t new_port_id;
> >> + struct rte_eth_dev devs[RTE_MAX_ETHPORTS];
> >> +
> >> + if ((vdevargs == NULL) || (port_id == NULL))
> >> + goto err0;
> >> +
> >> + args = strdup(vdevargs);
> >> + if (args == NULL)
> >> + goto err0;
> >> +
> >> + /* save current port status */
> >> + if (rte_eth_dev_save(devs, sizeof(devs)))
> >> + goto err1;
> >> + /* add the vdevargs to devargs_list */
> >> + if (rte_eal_devargs_add(RTE_DEVTYPE_VIRTUAL, args))
> >> + goto err1;
> >> + /* parse vdevargs, then retrieve device name */
> >> + get_vdev_name(args);
> >> + /* walk around dev_driver_list to find the driver of the device,
> >> + * then invoke probe function o the driver */
> >> + if (rte_eal_vdev_find_and_invoke(args, RTE_EAL_INVOKE_TYPE_PROBE))
> >> + goto err2;
> > Again, you should get port_id from the attach procedure.
> >
> >> + /* get port_id enabled by above procedures */
> >> + if (rte_eth_dev_get_changed_port(devs, &new_port_id))
> >> + goto err2;
> > [...]
> >
> >> /**
> >> + * Uninitilization function called for each device driver once.
> >> + */
> >> +typedef int (rte_dev_uninit_t)(const char *name, const char *args);
> > Why do you need args for uninit?
> >
>
> I just added for the case that finalization code of PMD needs it.
> But, probably "args" parameter can be removed.
Yes I think