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

[Tetsuya Mukawa <mukawa-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org>]

On 2015/03/26 19:03, Iremonger, Bernard wrote:
>
>> -----Original Message-----
>> From: Tetsuya Mukawa [mailto:mukawa-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org]
>> Sent: Thursday, March 26, 2015 4:19 AM
>> To: Stephen Hemminger
>> Cc: Iremonger, Bernard; david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org; dev-VfR2kkLFssw@public.gmane.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-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org> wrote:
>>>
>>>> On 2015/03/25 3:33, Stephen Hemminger wrote:
>>>>> On Tue, 24 Mar 2015 13:18:33 +0900
>>>>> Tetsuya Mukawa <mukawa-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org> 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-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org>
>>>>>> ---
>>>>>>  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.

Hi Bernard,

Thanks for comments. I will focus on BSD thing.

Regards,
Tetsuya

> Regards,
>
> Bernard.
>
>
>