From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f43.google.com (mail-pa0-f43.google.com [209.85.220.43]) by dpdk.org (Postfix) with ESMTP id 082025683 for ; Tue, 17 Feb 2015 11:26:46 +0100 (CET) Received: by pabrd3 with SMTP id rd3so5339942pab.4 for ; Tue, 17 Feb 2015 02:26:45 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:content-type :content-transfer-encoding; bh=7Q3C5Yu3xw62k7q/JuR80oovPJKa12yo+emqc5F8yjo=; b=YLtNn/BbN6Rmo+MY2IZ1H/+TZaVsKn0+I9fG8hxMdJdiuKfDMaCI931dXOcW5cYjkg EN8lT50BvSprvy/Hu0llzsM0NXkgUiY7U0+QeettX9xP5JrjLbrPiFAxQBcGWNiHTb75 rxqTzmEiCLhtwKi0xBS08gF44eIVmk0Ps5JgvWxouztP8MOOGElkkT0A6OzPKFIG3VqU ZHcPv8owuFET4BzLSlDw9L/KITSfXAPgN5mb6abvPUxchoJM5Xie90onESGeLlEI+SLA MsffgFEO+BiQqDJMmcqwnBZR0JipekG1KoQvHyxlNiAd6Qg9IccKr467vPBfDpAtLyxu bq3A== X-Gm-Message-State: ALoCoQmyquWimoeWMDnAC4SkMPJ6o25Rz9Sf1ND+9tnm2YBnF7xj6hrrpadhgEgJOB/toOh+kMJX X-Received: by 10.68.228.7 with SMTP id se7mr47768340pbc.45.1424168805274; Tue, 17 Feb 2015 02:26:45 -0800 (PST) Received: from [10.16.129.101] (napt.igel.co.jp. [219.106.231.132]) by mx.google.com with ESMTPSA id ei3sm16370396pbc.91.2015.02.17.02.26.43 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 17 Feb 2015 02:26:44 -0800 (PST) Message-ID: <54E31762.5070106@igel.co.jp> Date: Tue, 17 Feb 2015 19:26:42 +0900 From: Tetsuya Mukawa User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Thomas Monjalon References: <1423470639-15744-2-git-send-email-mukawa@igel.co.jp> <83798909.brLNWNC9tj@xps13> <54E300F8.8000503@igel.co.jp> <1733734.Ud8Ok1nOed@xps13> In-Reply-To: <1733734.Ud8Ok1nOed@xps13> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Feb 2015 10:26:46 -0000 On 2015/02/17 18:23, Thomas Monjalon wrote: > 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 =3D=3D NULL) || (port_id =3D=3D 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 sear= ching 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 devi= ce >> 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_i= d. >> - "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 an= d > pci devices. I strongly agree with it. But I haven't investigated how to remove it so far. To be honest, I want to submit hotplug patches to DPDK-2.0. Is above functionality needed to merge the hotplug patches? I guess I will not be able to remove it by 23rd. Regards, Tetsuya >> 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 =3D 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 =3D=3D NULL) || (port_id =3D=3D NULL)) >>>> + goto err0; >>>> + >>>> + args =3D strdup(vdevargs); >>>> + if (args =3D=3D 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 > >