From: "Zhou, Danny" <danny.zhou@intel.com>
To: Thomas Monjalon <thomas.monjalon@6wind.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v1 1/3] eal: enable uio_pci_generic support
Date: Thu, 19 Feb 2015 15:48:02 +0000 [thread overview]
Message-ID: <DFDF335405C17848924A094BC35766CF0AAA3AB9@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <22608095.lA2lToQDvY@xps13>
Thomas, thanks for review and I added comments inline.
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, February 18, 2015 9:40 PM
> To: Zhou, Danny
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v1 1/3] eal: enable uio_pci_generic support
>
> Hi Danny,
>
> I wanted to apply this patchset which was reviewed. But when having a quick
> overview, I've seen some strange additions.
>
> 2015-01-29 17:28, Danny Zhou:
> > 1) Unify procedure to retrieve BAR resource mapping information.
> > 2) Setup bus master bit in NIC's PCIe configuration space for uio_pci_generic.
> >
> > Signed-off-by: Danny Zhou <danny.zhou@intel.com>
> > Tested-by: Qun Wan <qun.wan@intel.com>
> [...]
> > --- a/lib/librte_eal/common/include/rte_pci.h
> > +++ b/lib/librte_eal/common/include/rte_pci.h
> > @@ -148,6 +148,7 @@ struct rte_pci_device {
> > struct rte_pci_id id; /**< PCI ID. */
> > struct rte_pci_resource mem_resource[PCI_MAX_RESOURCE]; /**< PCI Memory Resource */
> > struct rte_intr_handle intr_handle; /**< Interrupt handle */
> > + char driver_name[BUFSIZ]; /**< driver name */
>
> Why not embedding this field in driver struct?
> The name and comment should be more precise.
> There is also driver->name and hotplug patchset is adding a kernel driver name.
> Please bring the light in all these driver names :)
>
This driver_name is the name of kernel driver(e.g. vfio_pci, igb_uio, uio_pci_generic) while the driver->name is
a user-defined name for user space driver. I am going to change it to kernel_driver_name with precise comment in V2
patch, and when the V2 patch is applied, I think the function pci_get_kernel_driver_by_path() in hotplug patchset is not
necessary then as it could directly retrieve the kernel driver name from this variable.
> > const struct rte_pci_driver *driver; /**< Associated driver */
> [...]
> > --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> > +#define IORESOURCE_MEM 0x00000200
>
> Please comment this value.
Will do.
>
> > --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> > +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> > @@ -50,8 +50,14 @@ enum rte_intr_handle_type {
> >
> > /** Handle for interrupts. */
> > struct rte_intr_handle {
> > - int vfio_dev_fd; /**< VFIO device file descriptor */
> > - int fd; /**< file descriptor */
> > + union {
> > + int vfio_dev_fd; /**< VFIO device file descriptor */
> > + };
> > + union {
> > + int uio_cfg_fd; /**< UIO config file descriptor
> > + for uio_pci_generic */
> > + };
>
> Apart the indent, it seems there is a mistake here.
> Why 2 unions with 1 field each?
It is a mistake I made during code merge, will fix it in V2.
>
> > + int fd; /**< interrupt event file descriptor */
> > enum rte_intr_handle_type type; /**< handle type */
> > };
next prev parent reply other threads:[~2015-02-19 15:48 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-01-29 9:28 [dpdk-dev] [PATCH 0/3] Enable " Danny Zhou
2015-01-29 9:28 ` [dpdk-dev] [PATCH v1 1/3] eal: enable " Danny Zhou
2015-02-18 13:39 ` Thomas Monjalon
2015-02-19 15:48 ` Zhou, Danny [this message]
2015-01-29 9:28 ` [dpdk-dev] [PATCH v1 2/3] eal: add interrupt enable/disable routines for uio_pci_generic Danny Zhou
2015-01-29 9:28 ` [dpdk-dev] [PATCH v1 3/3] tools: enable binding NIC device to uio_pci_generic Danny Zhou
2015-01-29 9:34 ` [dpdk-dev] [PATCH 0/3] Enable uio_pci_generic support Qiu, Michael
2015-02-09 14:30 ` Bruce Richardson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DFDF335405C17848924A094BC35766CF0AAA3AB9@SHSMSX104.ccr.corp.intel.com \
--to=danny.zhou@intel.com \
--cc=dev@dpdk.org \
--cc=thomas.monjalon@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).