[dpdk-dev] [PATCH v2 5/6] eal: Use map_idx in pci_uio_map_resource() of bsdapp to work same as linuxapp

[Tetsuya Mukawa]

On 2015/03/26 19:41, Iremonger, Bernard wrote:
>
>> -Original Message-
>> From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp]
>> Sent: Thursday, March 26, 2015 2:51 AM
>> To: Iremonger, Bernard; dev at dpdk.org
>> Cc: Richardson, Bruce; david.marchand at 6wind.com
>> Subject: Re: [PATCH v2 5/6] eal: Use map_idx in pci_uio_map_resource() of 
>> bsdapp to work same as
>> linuxapp
>>
>> On 2015/03/26 0:27, Iremonger, Bernard wrote:
 -Original Message-
 From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp]
 Sent: Tuesday, March 24, 2015 4:19 AM
 To: dev at dpdk.org
 Cc: Iremonger, Bernard; Richardson, Bruce; david.marchand at 6wind.com;
 Tetsuya Mukawa
 Subject: [PATCH v2 5/6] eal: Use map_idx in pci_uio_map_resource() of
 bsdapp to work same as linuxapp

 This patch changes code that maps pci resources in bsdapp.
 Linuxapp has almost same code. To consolidate both, fix
 implementation of bsdapp to work same as linuxapp.

 Signed-off-by: Tetsuya Mukawa 
 ---
  lib/librte_eal/bsdapp/eal/eal_pci.c | 24 
  1 file changed, 12 insertions(+), 12 deletions(-)

 diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c
 b/lib/librte_eal/bsdapp/eal/eal_pci.c
 index 85f8671..08b91b4 100644
 --- a/lib/librte_eal/bsdapp/eal/eal_pci.c
 +++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
 @@ -195,7 +195,7 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
 static int pci_uio_map_resource(struct rte_pci_device *dev)  {
 -  int i, j;
 +  int i, map_idx;
char devname[PATH_MAX]; /* contains the /dev/uioX */
void *mapaddr;
uint64_t phaddr;
 @@ -247,31 +247,31 @@ pci_uio_map_resource(struct rte_pci_device *dev)
pagesz = sysconf(_SC_PAGESIZE);

maps = uio_res->maps;
 -  for (i = uio_res->nb_maps = 0; i != PCI_MAX_RESOURCE; i++) {
 +  for (i = 0, map_idx = 0; i != PCI_MAX_RESOURCE; i++) {

 -  j = uio_res->nb_maps;
/* skip empty BAR */
if ((phaddr = dev->mem_resource[i].phys_addr) == 0)
continue;

/* if matching map is found, then use it */
offset = i * pagesz;
 -  maps[j].offset = offset;
 -  maps[j].phaddr = dev->mem_resource[i].phys_addr;
 -  maps[j].size = dev->mem_resource[i].len;
 -  if (maps[j].addr != NULL ||
 -  (mapaddr = pci_map_resource(NULL, devname, (off_t)offset,
 -  (size_t)maps[j].size)
 -  ) == NULL) {
 +  maps[map_idx].offset = offset;
 +  maps[map_idx].phaddr = dev->mem_resource[i].phys_addr;
 +  maps[map_idx].size = dev->mem_resource[i].len;
 +  mapaddr = pci_map_resource(NULL, devname, (off_t)offset,
 +  (size_t)maps[map_idx].size);
 +  if ((maps[map_idx].addr != NULL) || (mapaddr == NULL)) {
>>> Hi Tetsuya,
>>>
>>> Should be checking for  if  (mapaddr == MAP_FAILED) here.
>>> Seems to be fixed in patch 6/6 though.
>> Hi Bernard,
>>
>> Here, bsdapp still has old pci_map_resource().
>> Old pci_map_resource() return NULL when mapping is failed.
>> And this behavior will be changed in 6/6. This is why MAP_FAILED is checked 
>> in 6/6.
>>
>> Regards,
>> Tetsuya
>>
> Hi Tetsuya,
>
> Would it make sense to squash patches 5/6 and 6/6 ?

Hi Bernard,

I don't have strong opinion about it.
So let's squash patches. And then if you feel to want to split it again,
let me know.

Regards,
Tetsuya

>
> Regards,
>
> Bernard.
>

[dpdk-dev] [PATCH v2 5/6] eal: Use map_idx in pci_uio_map_resource() of bsdapp to work same as linuxapp

[Tetsuya Mukawa]

On 2015/03/26 0:27, Iremonger, Bernard wrote:
>
>> -Original Message-
>> From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp]
>> Sent: Tuesday, March 24, 2015 4:19 AM
>> To: dev at dpdk.org
>> Cc: Iremonger, Bernard; Richardson, Bruce; david.marchand at 6wind.com; 
>> Tetsuya Mukawa
>> Subject: [PATCH v2 5/6] eal: Use map_idx in pci_uio_map_resource() of bsdapp 
>> to work same as
>> linuxapp
>>
>> This patch changes code that maps pci resources in bsdapp.
>> Linuxapp has almost same code. To consolidate both, fix implementation of 
>> bsdapp to work same as
>> linuxapp.
>>
>> Signed-off-by: Tetsuya Mukawa 
>> ---
>>  lib/librte_eal/bsdapp/eal/eal_pci.c | 24 
>>  1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c 
>> b/lib/librte_eal/bsdapp/eal/eal_pci.c
>> index 85f8671..08b91b4 100644
>> --- a/lib/librte_eal/bsdapp/eal/eal_pci.c
>> +++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
>> @@ -195,7 +195,7 @@ pci_uio_map_secondary(struct rte_pci_device *dev)  
>> static int
>> pci_uio_map_resource(struct rte_pci_device *dev)  {
>> -int i, j;
>> +int i, map_idx;
>>  char devname[PATH_MAX]; /* contains the /dev/uioX */
>>  void *mapaddr;
>>  uint64_t phaddr;
>> @@ -247,31 +247,31 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>>  pagesz = sysconf(_SC_PAGESIZE);
>>
>>  maps = uio_res->maps;
>> -for (i = uio_res->nb_maps = 0; i != PCI_MAX_RESOURCE; i++) {
>> +for (i = 0, map_idx = 0; i != PCI_MAX_RESOURCE; i++) {
>>
>> -j = uio_res->nb_maps;
>>  /* skip empty BAR */
>>  if ((phaddr = dev->mem_resource[i].phys_addr) == 0)
>>  continue;
>>
>>  /* if matching map is found, then use it */
>>  offset = i * pagesz;
>> -maps[j].offset = offset;
>> -maps[j].phaddr = dev->mem_resource[i].phys_addr;
>> -maps[j].size = dev->mem_resource[i].len;
>> -if (maps[j].addr != NULL ||
>> -(mapaddr = pci_map_resource(NULL, devname, (off_t)offset,
>> -(size_t)maps[j].size)
>> -) == NULL) {
>> +maps[map_idx].offset = offset;
>> +maps[map_idx].phaddr = dev->mem_resource[i].phys_addr;
>> +maps[map_idx].size = dev->mem_resource[i].len;
>> +mapaddr = pci_map_resource(NULL, devname, (off_t)offset,
>> +(size_t)maps[map_idx].size);
>> +if ((maps[map_idx].addr != NULL) || (mapaddr == NULL)) {
> Hi Tetsuya,
>
> Should be checking for  if  (mapaddr == MAP_FAILED) here.
> Seems to be fixed in patch 6/6 though.

Hi Bernard,

Here, bsdapp still has old pci_map_resource().
Old pci_map_resource() return NULL when mapping is failed.
And this behavior will be changed in 6/6. This is why MAP_FAILED is
checked in 6/6.

Regards,
Tetsuya

> Regards,
>
> Bernard.
>
>>  rte_free(uio_res);
>>  return -1;
>>  }
>>
>> -maps[j].addr = mapaddr;
>> -uio_res->nb_maps++;
>> +maps[map_idx].addr = mapaddr;
>> +map_idx++;
>>  dev->mem_resource[i].addr = mapaddr;
>>  }
>>
>> +uio_res->nb_maps = map_idx;
>> +
>>  TAILQ_INSERT_TAIL(uio_res_list, uio_res, next);
>>
>>  return 0;
>> --
>> 1.9.1

[dpdk-dev] [PATCH v2 5/6] eal: Use map_idx in pci_uio_map_resource() of bsdapp to work same as linuxapp

[Iremonger, Bernard]

> -Original Message-
> From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp]
> Sent: Thursday, March 26, 2015 2:51 AM
> To: Iremonger, Bernard; dev at dpdk.org
> Cc: Richardson, Bruce; david.marchand at 6wind.com
> Subject: Re: [PATCH v2 5/6] eal: Use map_idx in pci_uio_map_resource() of 
> bsdapp to work same as
> linuxapp
> 
> On 2015/03/26 0:27, Iremonger, Bernard wrote:
> >
> >> -Original Message-
> >> From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp]
> >> Sent: Tuesday, March 24, 2015 4:19 AM
> >> To: dev at dpdk.org
> >> Cc: Iremonger, Bernard; Richardson, Bruce; david.marchand at 6wind.com;
> >> Tetsuya Mukawa
> >> Subject: [PATCH v2 5/6] eal: Use map_idx in pci_uio_map_resource() of
> >> bsdapp to work same as linuxapp
> >>
> >> This patch changes code that maps pci resources in bsdapp.
> >> Linuxapp has almost same code. To consolidate both, fix
> >> implementation of bsdapp to work same as linuxapp.
> >>
> >> Signed-off-by: Tetsuya Mukawa 
> >> ---
> >>  lib/librte_eal/bsdapp/eal/eal_pci.c | 24 
> >>  1 file changed, 12 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c
> >> b/lib/librte_eal/bsdapp/eal/eal_pci.c
> >> index 85f8671..08b91b4 100644
> >> --- a/lib/librte_eal/bsdapp/eal/eal_pci.c
> >> +++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
> >> @@ -195,7 +195,7 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
> >> static int pci_uio_map_resource(struct rte_pci_device *dev)  {
> >> -  int i, j;
> >> +  int i, map_idx;
> >>char devname[PATH_MAX]; /* contains the /dev/uioX */
> >>void *mapaddr;
> >>uint64_t phaddr;
> >> @@ -247,31 +247,31 @@ pci_uio_map_resource(struct rte_pci_device *dev)
> >>pagesz = sysconf(_SC_PAGESIZE);
> >>
> >>maps = uio_res->maps;
> >> -  for (i = uio_res->nb_maps = 0; i != PCI_MAX_RESOURCE; i++) {
> >> +  for (i = 0, map_idx = 0; i != PCI_MAX_RESOURCE; i++) {
> >>
> >> -  j = uio_res->nb_maps;
> >>/* skip empty BAR */
> >>if ((phaddr = dev->mem_resource[i].phys_addr) == 0)
> >>continue;
> >>
> >>/* if matching map is found, then use it */
> >>offset = i * pagesz;
> >> -  maps[j].offset = offset;
> >> -  maps[j].phaddr = dev->mem_resource[i].phys_addr;
> >> -  maps[j].size = dev->mem_resource[i].len;
> >> -  if (maps[j].addr != NULL ||
> >> -  (mapaddr = pci_map_resource(NULL, devname, (off_t)offset,
> >> -  (size_t)maps[j].size)
> >> -  ) == NULL) {
> >> +  maps[map_idx].offset = offset;
> >> +  maps[map_idx].phaddr = dev->mem_resource[i].phys_addr;
> >> +  maps[map_idx].size = dev->mem_resource[i].len;
> >> +  mapaddr = pci_map_resource(NULL, devname, (off_t)offset,
> >> +  (size_t)maps[map_idx].size);
> >> +  if ((maps[map_idx].addr != NULL) || (mapaddr == NULL)) {
> > Hi Tetsuya,
> >
> > Should be checking for  if  (mapaddr == MAP_FAILED) here.
> > Seems to be fixed in patch 6/6 though.
> 
> Hi Bernard,
> 
> Here, bsdapp still has old pci_map_resource().
> Old pci_map_resource() return NULL when mapping is failed.
> And this behavior will be changed in 6/6. This is why MAP_FAILED is checked 
> in 6/6.
> 
> Regards,
> Tetsuya
> 

Hi Tetsuya,

Would it make sense to squash patches 5/6 and 6/6 ?

Regards,

Bernard.

[dpdk-dev] [PATCH v2 5/6] eal: Use map_idx in pci_uio_map_resource() of bsdapp to work same as linuxapp

[Iremonger, Bernard]

> -Original Message-
> From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp]
> Sent: Tuesday, March 24, 2015 4:19 AM
> To: dev at dpdk.org
> Cc: Iremonger, Bernard; Richardson, Bruce; david.marchand at 6wind.com; 
> Tetsuya Mukawa
> Subject: [PATCH v2 5/6] eal: Use map_idx in pci_uio_map_resource() of bsdapp 
> to work same as
> linuxapp
> 
> This patch changes code that maps pci resources in bsdapp.
> Linuxapp has almost same code. To consolidate both, fix implementation of 
> bsdapp to work same as
> linuxapp.
> 
> Signed-off-by: Tetsuya Mukawa 
> ---
>  lib/librte_eal/bsdapp/eal/eal_pci.c | 24 
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c 
> b/lib/librte_eal/bsdapp/eal/eal_pci.c
> index 85f8671..08b91b4 100644
> --- a/lib/librte_eal/bsdapp/eal/eal_pci.c
> +++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
> @@ -195,7 +195,7 @@ pci_uio_map_secondary(struct rte_pci_device *dev)  static 
> int
> pci_uio_map_resource(struct rte_pci_device *dev)  {
> - int i, j;
> + int i, map_idx;
>   char devname[PATH_MAX]; /* contains the /dev/uioX */
>   void *mapaddr;
>   uint64_t phaddr;
> @@ -247,31 +247,31 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>   pagesz = sysconf(_SC_PAGESIZE);
> 
>   maps = uio_res->maps;
> - for (i = uio_res->nb_maps = 0; i != PCI_MAX_RESOURCE; i++) {
> + for (i = 0, map_idx = 0; i != PCI_MAX_RESOURCE; i++) {
> 
> - j = uio_res->nb_maps;
>   /* skip empty BAR */
>   if ((phaddr = dev->mem_resource[i].phys_addr) == 0)
>   continue;
> 
>   /* if matching map is found, then use it */
>   offset = i * pagesz;
> - maps[j].offset = offset;
> - maps[j].phaddr = dev->mem_resource[i].phys_addr;
> - maps[j].size = dev->mem_resource[i].len;
> - if (maps[j].addr != NULL ||
> - (mapaddr = pci_map_resource(NULL, devname, (off_t)offset,
> - (size_t)maps[j].size)
> - ) == NULL) {
> + maps[map_idx].offset = offset;
> + maps[map_idx].phaddr = dev->mem_resource[i].phys_addr;
> + maps[map_idx].size = dev->mem_resource[i].len;
> + mapaddr = pci_map_resource(NULL, devname, (off_t)offset,
> + (size_t)maps[map_idx].size);
> + if ((maps[map_idx].addr != NULL) || (mapaddr == NULL)) {

Hi Tetsuya,

Should be checking for  if  (mapaddr == MAP_FAILED) here.
Seems to be fixed in patch 6/6 though.

Regards,

Bernard.

>   rte_free(uio_res);
>   return -1;
>   }
> 
> - maps[j].addr = mapaddr;
> - uio_res->nb_maps++;
> + maps[map_idx].addr = mapaddr;
> + map_idx++;
>   dev->mem_resource[i].addr = mapaddr;
>   }
> 
> + uio_res->nb_maps = map_idx;
> +
>   TAILQ_INSERT_TAIL(uio_res_list, uio_res, next);
> 
>   return 0;
> --
> 1.9.1

[dpdk-dev] [PATCH v2 5/6] eal: Use map_idx in pci_uio_map_resource() of bsdapp to work same as linuxapp

[Tetsuya Mukawa]

This patch changes code that maps pci resources in bsdapp.
Linuxapp has almost same code. To consolidate both, fix implementation
of bsdapp to work same as linuxapp.

Signed-off-by: Tetsuya Mukawa 
---
 lib/librte_eal/bsdapp/eal/eal_pci.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c 
b/lib/librte_eal/bsdapp/eal/eal_pci.c
index 85f8671..08b91b4 100644
--- a/lib/librte_eal/bsdapp/eal/eal_pci.c
+++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
@@ -195,7 +195,7 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
 static int
 pci_uio_map_resource(struct rte_pci_device *dev)
 {
-   int i, j;
+   int i, map_idx;
char devname[PATH_MAX]; /* contains the /dev/uioX */
void *mapaddr;
uint64_t phaddr;
@@ -247,31 +247,31 @@ pci_uio_map_resource(struct rte_pci_device *dev)
pagesz = sysconf(_SC_PAGESIZE);

maps = uio_res->maps;
-   for (i = uio_res->nb_maps = 0; i != PCI_MAX_RESOURCE; i++) {
+   for (i = 0, map_idx = 0; i != PCI_MAX_RESOURCE; i++) {

-   j = uio_res->nb_maps;
/* skip empty BAR */
if ((phaddr = dev->mem_resource[i].phys_addr) == 0)
continue;

/* if matching map is found, then use it */
offset = i * pagesz;
-   maps[j].offset = offset;
-   maps[j].phaddr = dev->mem_resource[i].phys_addr;
-   maps[j].size = dev->mem_resource[i].len;
-   if (maps[j].addr != NULL ||
-   (mapaddr = pci_map_resource(NULL, devname, (off_t)offset,
-   (size_t)maps[j].size)
-   ) == NULL) {
+   maps[map_idx].offset = offset;
+   maps[map_idx].phaddr = dev->mem_resource[i].phys_addr;
+   maps[map_idx].size = dev->mem_resource[i].len;
+   mapaddr = pci_map_resource(NULL, devname, (off_t)offset,
+   (size_t)maps[map_idx].size);
+   if ((maps[map_idx].addr != NULL) || (mapaddr == NULL)) {
rte_free(uio_res);
return -1;
}

-   maps[j].addr = mapaddr;
-   uio_res->nb_maps++;
+   maps[map_idx].addr = mapaddr;
+   map_idx++;
dev->mem_resource[i].addr = mapaddr;
}

+   uio_res->nb_maps = map_idx;
+
TAILQ_INSERT_TAIL(uio_res_list, uio_res, next);

return 0;
-- 
1.9.1