Re: [dpdk-dev] [PATCH v2 2/6] eal: Close file descriptor of uio configuration

["Iremonger, Bernard" <bernard.iremonger@intel.com>]

> -----Original Message-----
> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> Sent: Thursday, March 26, 2015 4:19 AM
> To: Stephen Hemminger
> Cc: Iremonger, Bernard; david.marchand@6wind.com; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 2/6] eal: Close file descriptor of uio configuration
> 
> On 2015/03/25 14:07, Stephen Hemminger wrote:
> > On Wed, 25 Mar 2015 12:17:32 +0900
> > Tetsuya Mukawa <mukawa@igel.co.jp> wrote:
> >
> >> On 2015/03/25 3:33, Stephen Hemminger wrote:
> >>> On Tue, 24 Mar 2015 13:18:33 +0900
> >>> Tetsuya Mukawa <mukawa@igel.co.jp> wrote:
> >>>
> >>>> When pci_uio_unmap_resource() is called, a file descriptor that is
> >>>> used for uio configuration should be closed.
> >>>>
> >>>> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> >>>> ---
> >>>>  lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 6 +++++-
> >>>>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> >>>> b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> >>>> index 9cdf24f..f0277be 100644
> >>>> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> >>>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> >>>> @@ -459,8 +459,12 @@ pci_uio_unmap_resource(struct rte_pci_device
> >>>> *dev)
> >>>>
> >>>>  	/* close fd if in primary process */
> >>>>  	close(dev->intr_handle.fd);
> >>>> -
> >>>>  	dev->intr_handle.fd = -1;
> >>>> +
> >>>> +	/* close cfg_fd if in primary process */
> >>>> +	close(dev->intr_handle.uio_cfg_fd);
> >>>> +	dev->intr_handle.uio_cfg_fd = -1;
> >>>> +
> >>>>  	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;  }  #endif /*
> >>>> RTE_LIBRTE_EAL_HOTPLUG */
> >>> For the Qlogic/Broadcom driver it needed the config fd handle, and I
> >>> added generic config space access functions.
> >> Hi Stephen,
> >>
> >> Is this the patch you mentioned?
> >> http://dpdk.org/dev/patchwork/patch/3024/
> >>
> >>
> >> Hi David, Bernard, Stephen
> >>
> >> I guess here are works we will need to do.
> >> 1. Add close(dev->config_fd) in Stephen's patch.
> >> 2. Write a patch for uio to merge "dev->intr_handle->uio_cfg_fd" and
> >> "dev->config_fd".
> >> 3. Write a patch for vfio to merge "dev->intr_handle->vfio_cfg_fd"
> >> and "dev->config_fd".
> >>
> >> If we already have these patches, I guess it may be nice to merge
> >> above patches first.
> >> Do you have a suggestion how to merge patches related with pci config fd?
> >>
> >> Thanks,
> >> Tetsuya
> >>
> > Yeah, that is the patch. It reopens config fd, it seems to overlap
> > this.
> 
> Hi Stephen, David, Bernard,
> 
> If you are OK, I will cherry pick below Stephen's patch, then put it on top of my patches.
> - http://dpdk.org/dev/patchwork/patch/3024/
> 
> Also I will write patches to merge following fds.
>  - dev->config_fd
>  - dev->intr_handle->uio_cfg_fd
>  - dev->intr_handle->vfio_cfg_fd
> 
> Is this direction OK?
> 
> 
> Stephen,
> 
> For uio, I guess it will be OK that I just replace pread/pwrite by your APIs.
> But for vfio, I need to write a function to wrap vfio ioctl.
> May be rte_eal_pci_ioctl_config()?
> And replace all vfio ioctls by the function.
> Is this correct way to adopt your APIs for vfio?
> 
> Regards,
> Tetsuya

Hi Tetsuya,

It would be better not to broaden the scope of the BSD cleanup patch set at this point.
It would be better to have a separate patch set  to apply Stephen's changes to the BSD code base.
Also, discussion is still ongoing on Stephen's changes so it might be better to wait until the discussion is completed.

Regards,

Bernard.