vdpa/sfc: make MCDI memzone name unique

PATCH Superseded [email protected] 3 participants 9 messages Jan 11, 2022 – Feb 17, 2022 virtio core

lore

Share & tools

lore

[email protected]

author

2022-01-11 05:33 UTC #0 lore raw

PATCH 1/1 vdpa/sfc: make MCDI memzone name unique

From: Abhimanyu Saini <[email protected]>

Buffer for MCDI channel is allocated using rte_memzone_reserve_aligned with zone name 'mcdi'. Since multiple MCDI channels are needed to support multiple VF(s) and rte_memzone_reserve_aligned expects unique zone names, append PCI address to zone name to make it unique.

Signed-off-by Abhimanyu Saini <[email protected]>

---
 drivers/vdpa/sfc/sfc_vdpa_hw.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

drivers/vdpa/sfc/sfc_vdpa_hw.c +12 -3

index fd1fee7..a7018b1 100644
@@ -25,21 +25,30 @@
 {
        uint64_t mcdi_iova;
        size_t mcdi_buff_size;
+       char mz_name[RTE_MEMZONE_NAMESIZE];
        const struct rte_memzone *mz = NULL;
        int numa_node = sva->pdev->device.numa_node;
        int ret;
 
        mcdi_buff_size = RTE_ALIGN_CEIL(len, PAGE_SIZE);
+       ret = snprintf(mz_name, RTE_MEMZONE_NAMESIZE, "%s_%s",
+                      sva->pdev->name, name);
+       if (ret < 0 || ret >= RTE_MEMZONE_NAMESIZE) {
+               sfc_vdpa_err(sva, "%s_%s too long to fit in mz_name",
+                            sva->pdev->name, name);
+               return -EINVAL;
+       }
 
-       sfc_vdpa_log_init(sva, "name=%s, len=%zu", name, len);
+       sfc_vdpa_log_init(sva, "name=%s, len=%zu", mz_name, len);
 
-       mz = rte_memzone_reserve_aligned(name, mcdi_buff_size,
+       mz = rte_memzone_reserve_aligned(mz_name, mcdi_buff_size,
                                         numa_node,
                                         RTE_MEMZONE_IOVA_CONTIG,
                                         PAGE_SIZE);
        if (mz == NULL) {
                sfc_vdpa_err(sva, "cannot reserve memory for %s: len=%#x: %s",
-                            name, (unsigned int)len, rte_strerror(rte_errno));
+                            mz_name, (unsigned int)len,
+                            rte_strerror(rte_errno));
                return -ENOMEM;
        }

--
1.8.3.1

This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Xia, Chenbo

2022-01-14 07:07 UTC #1 lore raw

Hi Abhimanyu,

Quoted 10 lines

-----Original Message-----
From: [email protected] <[email protected]>
Sent: Tuesday, January 11, 2022 1:33 PM
To: [email protected]
Cc: Xia, Chenbo <[email protected]>; [email protected]; [email protected]; Abhimanyu Saini <[email protected]>
Subject: [PATCH] vdpa/sfc: make MCDI memzone name unique
From: Abhimanyu Saini <[email protected]>

Quoted 7 lines

Buffer for MCDI channel is allocated using rte_memzone_reserve_aligned with zone name 'mcdi'. Since multiple MCDI channels are needed to support multiple VF(s) and rte_memzone_reserve_aligned expects unique zone names, append PCI address to zone name to make it unique.
Signed-off-by Abhimanyu Saini <[email protected]>
---

Could you help with the apply issue and checkpatch issue, then send new version?

Thanks,
Chenbo

[email protected]

author

2022-01-17 11:30 UTC #2 lore raw

PATCH v2 1/1 vdpa/sfc: make MCDI memzone name unique

In reply to [email protected]

From: Abhimanyu Saini <[email protected]>

Signed-off-by Abhimanyu Saini <[email protected]>

---
v2:
* Formatting changes

 drivers/vdpa/sfc/sfc_vdpa_hw.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

drivers/vdpa/sfc/sfc_vdpa_hw.c +12 -3

index fd1fee7..a7018b1 100644
@@ -25,21 +25,30 @@
 {
        uint64_t mcdi_iova;
        size_t mcdi_buff_size;
+       char mz_name[RTE_MEMZONE_NAMESIZE];
        const struct rte_memzone *mz = NULL;
        int numa_node = sva->pdev->device.numa_node;
        int ret;
 
        mcdi_buff_size = RTE_ALIGN_CEIL(len, PAGE_SIZE);
+       ret = snprintf(mz_name, RTE_MEMZONE_NAMESIZE, "%s_%s",
+                      sva->pdev->name, name);
+       if (ret < 0 || ret >= RTE_MEMZONE_NAMESIZE) {
+               sfc_vdpa_err(sva, "%s_%s too long to fit in mz_name",
+                            sva->pdev->name, name);
+               return -EINVAL;
+       }
 
-       sfc_vdpa_log_init(sva, "name=%s, len=%zu", name, len);
+       sfc_vdpa_log_init(sva, "name=%s, len=%zu", mz_name, len);
 
-       mz = rte_memzone_reserve_aligned(name, mcdi_buff_size,
+       mz = rte_memzone_reserve_aligned(mz_name, mcdi_buff_size,
                                         numa_node,
                                         RTE_MEMZONE_IOVA_CONTIG,
                                         PAGE_SIZE);
        if (mz == NULL) {
                sfc_vdpa_err(sva, "cannot reserve memory for %s: len=%#x: %s",
-                            name, (unsigned int)len, rte_strerror(rte_errno));
+                            mz_name, (unsigned int)len,
+                            rte_strerror(rte_errno));
                return -ENOMEM;
        }

--
1.8.3.1

Maxime Coquelin

2022-02-01 08:21 UTC #3 lore raw

Hi Abhimanyu,

Quoted 7 lines · On 1/17/22 12:29, [email protected] wrote:

From: Abhimanyu Saini <[email protected]>
Buffer for MCDI channel is allocated using rte_memzone_reserve_aligned with zone name 'mcdi'. Since multiple MCDI channels are needed to support multiple VF(s) and rte_memzone_reserve_aligned expects unique zone names, append PCI address to zone name to make it unique.

Quoted 8 lines

Signed-off-by Abhimanyu Saini <[email protected]>
---
v2:
* Formatting changes
  drivers/vdpa/sfc/sfc_vdpa_hw.c | 15 ++++++++++++---
  1 file changed, 12 insertions(+), 3 deletions(-)

The patch itself looks goof to me, but it seems you have issues with its formatting (I guess you need to configure indents properly in your editor):

WARNING:LEADING_SPACE: please, no spaces at the start of a line #153: FILE: drivers/vdpa/sfc/sfc_vdpa_hw.c:28:
+ char mz_name[RTE_MEMZONE_NAMESIZE];$

WARNING:LEADING_SPACE: please, no spaces at the start of a line #159: FILE: drivers/vdpa/sfc/sfc_vdpa_hw.c:34:
+ ret = snprintf(mz_name, RTE_MEMZONE_NAMESIZE, "%s_%s",$

ERROR:CODE_INDENT: code indent should use tabs where possible #160: FILE: drivers/vdpa/sfc/sfc_vdpa_hw.c:35:
+ sva->pdev->name, name);$

WARNING:LEADING_SPACE: please, no spaces at the start of a line #160: FILE: drivers/vdpa/sfc/sfc_vdpa_hw.c:35:
+ sva->pdev->name, name);$

WARNING:LEADING_SPACE: please, no spaces at the start of a line #161: FILE: drivers/vdpa/sfc/sfc_vdpa_hw.c:36:
+ if (ret < 0 || ret >= RTE_MEMZONE_NAMESIZE) {$

WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (7, 15) #161: FILE: drivers/vdpa/sfc/sfc_vdpa_hw.c:36:
+ if (ret < 0 || ret >= RTE_MEMZONE_NAMESIZE) {
+ sfc_vdpa_err(sva, "%s_%s too long to fit in mz_name",

ERROR:CODE_INDENT: code indent should use tabs where possible #162: FILE: drivers/vdpa/sfc/sfc_vdpa_hw.c:37:
+ sfc_vdpa_err(sva, "%s_%s too long to fit in mz_name",$

WARNING:LEADING_SPACE: please, no spaces at the start of a line #162: FILE: drivers/vdpa/sfc/sfc_vdpa_hw.c:37:
+ sfc_vdpa_err(sva, "%s_%s too long to fit in mz_name",$

ERROR:CODE_INDENT: code indent should use tabs where possible #163: FILE: drivers/vdpa/sfc/sfc_vdpa_hw.c:38:
+ sva->pdev->name, name);$

WARNING:LEADING_SPACE: please, no spaces at the start of a line #163: FILE: drivers/vdpa/sfc/sfc_vdpa_hw.c:38:
+ sva->pdev->name, name);$

ERROR:CODE_INDENT: code indent should use tabs where possible #164: FILE: drivers/vdpa/sfc/sfc_vdpa_hw.c:39:
+ return -EINVAL;$

WARNING:LEADING_SPACE: please, no spaces at the start of a line #164: FILE: drivers/vdpa/sfc/sfc_vdpa_hw.c:39:
+ return -EINVAL;$

WARNING:LEADING_SPACE: please, no spaces at the start of a line #165: FILE: drivers/vdpa/sfc/sfc_vdpa_hw.c:40:
+ }$

WARNING:LEADING_SPACE: please, no spaces at the start of a line #168: FILE: drivers/vdpa/sfc/sfc_vdpa_hw.c:42:
+ sfc_vdpa_log_init(sva, "name=%s, len=%zu", mz_name, len);$

WARNING:LEADING_SPACE: please, no spaces at the start of a line #171: FILE: drivers/vdpa/sfc/sfc_vdpa_hw.c:44:
+ mz = rte_memzone_reserve_aligned(mz_name, mcdi_buff_size,$

ERROR:CODE_INDENT: code indent should use tabs where possible #178: FILE: drivers/vdpa/sfc/sfc_vdpa_hw.c:50:
+ mz_name, (unsigned int)len,$

WARNING:LEADING_SPACE: please, no spaces at the start of a line #178: FILE: drivers/vdpa/sfc/sfc_vdpa_hw.c:50:
+ mz_name, (unsigned int)len,$

ERROR:CODE_INDENT: code indent should use tabs where possible #179: FILE: drivers/vdpa/sfc/sfc_vdpa_hw.c:51:
+ rte_strerror(rte_errno));$

WARNING:LEADING_SPACE: please, no spaces at the start of a line #179: FILE: drivers/vdpa/sfc/sfc_vdpa_hw.c:51:
+ rte_strerror(rte_errno));$

total: 6 errors, 13 warnings, 33 lines checked

[email protected]

author

2022-02-14 10:52 UTC #4 lore raw

PATCH v3 1/1 vdpa/sfc: make MCDI memzone name unique

In reply to [email protected]

From: Abhimanyu Saini <[email protected]>

Signed-off-by Abhimanyu Saini <[email protected]>

---
v2:
- Formatting changes
v3:
- Formatting changes

 drivers/vdpa/sfc/sfc_vdpa_hw.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

drivers/vdpa/sfc/sfc_vdpa_hw.c +12 -3

index fd1fee7..a7018b1 100644
@@ -25,21 +25,30 @@
 {
 	uint64_t mcdi_iova;
 	size_t mcdi_buff_size;
+	char mz_name[RTE_MEMZONE_NAMESIZE];
 	const struct rte_memzone *mz = NULL;
 	int numa_node = sva->pdev->device.numa_node;
 	int ret;
 
 	mcdi_buff_size = RTE_ALIGN_CEIL(len, PAGE_SIZE);
+	ret = snprintf(mz_name, RTE_MEMZONE_NAMESIZE, "%s_%s",
+		       sva->pdev->name, name);
+	if (ret < 0 || ret >= RTE_MEMZONE_NAMESIZE) {
+		sfc_vdpa_err(sva, "%s_%s too long to fit in mz_name",
+			     sva->pdev->name, name);
+		return -EINVAL;
+	}
 
-	sfc_vdpa_log_init(sva, "name=%s, len=%zu", name, len);
+	sfc_vdpa_log_init(sva, "name=%s, len=%zu", mz_name, len);
 
-	mz = rte_memzone_reserve_aligned(name, mcdi_buff_size,
+	mz = rte_memzone_reserve_aligned(mz_name, mcdi_buff_size,
 					 numa_node,
 					 RTE_MEMZONE_IOVA_CONTIG,
 					 PAGE_SIZE);
 	if (mz == NULL) {
 		sfc_vdpa_err(sva, "cannot reserve memory for %s: len=%#x: %s",
-			     name, (unsigned int)len, rte_strerror(rte_errno));
+			     mz_name, (unsigned int)len,
+			     rte_strerror(rte_errno));
 		return -ENOMEM;
 	}

Signature · 1 line

1.8.3.1

Maxime Coquelin

2022-02-15 10:56 UTC #5 lore raw

Quoted 24 lines · On 2/14/22 11:51, [email protected] wrote:

From: Abhimanyu Saini <[email protected]>
Buffer for MCDI channel is allocated using rte_memzone_reserve_aligned with zone name 'mcdi'. Since multiple MCDI channels are needed to support multiple VF(s) and rte_memzone_reserve_aligned expects unique zone names, append PCI address to zone name to make it unique.
Signed-off-by Abhimanyu Saini <[email protected]>
---
v2:
- Formatting changes
v3:
- Formatting changes
  drivers/vdpa/sfc/sfc_vdpa_hw.c | 15 ++++++++++++---
  1 file changed, 12 insertions(+), 3 deletions(-)
drivers/vdpa/sfc/sfc_vdpa_hw.c +0 -0
index fd1fee7..a7018b1 100644
@@ -25,21 +25,30 @@
  {
  	uint64_t mcdi_iova;

drivers/vdpa/sfc/sfc_vdpa_hw.c line 36

Quoted 10 lines

drivers/vdpa/sfc/sfc_vdpa_hw.c +4 -0

  	size_t mcdi_buff_size;
+	char mz_name[RTE_MEMZONE_NAMESIZE];
  	const struct rte_memzone *mz = NULL;
  	int numa_node = sva->pdev->device.numa_node;
  	int ret;
 
  	mcdi_buff_size = RTE_ALIGN_CEIL(len, PAGE_SIZE);
+	ret = snprintf(mz_name, RTE_MEMZONE_NAMESIZE, "%s_%s",
+		       sva->pdev->name, name);
+	if (ret < 0 || ret >= RTE_MEMZONE_NAMESIZE) {

From the man page:
"
The functions snprintf() and vsnprintf() do not write more than size bytes (including the terminating null byte ('\0')). "

you might want to pass RTE_MEMZONE_NAMESIZE - 1 as size arg to snprintf, so you can just check ret >= 0?

Quoted 21 lines

drivers/vdpa/sfc/sfc_vdpa_hw.c +8 -3

+		sfc_vdpa_err(sva, "%s_%s too long to fit in mz_name",
+			     sva->pdev->name, name);
+		return -EINVAL;
+	}
 
-	sfc_vdpa_log_init(sva, "name=%s, len=%zu", name, len);
+	sfc_vdpa_log_init(sva, "name=%s, len=%zu", mz_name, len);
 
-	mz = rte_memzone_reserve_aligned(name, mcdi_buff_size,
+	mz = rte_memzone_reserve_aligned(mz_name, mcdi_buff_size,
  					 numa_node,
  					 RTE_MEMZONE_IOVA_CONTIG,
  					 PAGE_SIZE);
  	if (mz == NULL) {
  		sfc_vdpa_err(sva, "cannot reserve memory for %s: len=%#x: %s",
-			     name, (unsigned int)len, rte_strerror(rte_errno));
+			     mz_name, (unsigned int)len,
+			     rte_strerror(rte_errno));
  		return -ENOMEM;
  	}

Maxime Coquelin

2022-02-15 10:59 UTC #6 lore raw

Quoted 42 lines · Maxime Coquelin

Quoted 23 lines · On 2/14/22 11:51, [email protected] wrote:
From: Abhimanyu Saini <[email protected]>
Buffer for MCDI channel is allocated using rte_memzone_reserve_aligned with zone name 'mcdi'. Since multiple MCDI channels are needed to support multiple VF(s) and rte_memzone_reserve_aligned expects unique zone names, append PCI address to zone name to make it unique.
Signed-off-by Abhimanyu Saini <[email protected]>
---
v2:
- Formatting changes
v3:
- Formatting changes
drivers/vdpa/sfc/sfc_vdpa_hw.c | 15 ++++++++++++---
  1 file changed, 12 insertions(+), 3 deletions(-)
drivers/vdpa/sfc/sfc_vdpa_hw.c +0 -0
index fd1fee7..a7018b1 100644
@@ -25,21 +25,30 @@
   {
drivers/vdpa/sfc/sfc_vdpa_hw.c line 35
Quoted 10 lines
drivers/vdpa/sfc/sfc_vdpa_hw.c +4 -0
       uint64_t mcdi_iova;
       size_t mcdi_buff_size;
+    char mz_name[RTE_MEMZONE_NAMESIZE];
       const struct rte_memzone *mz = NULL;
       int numa_node = sva->pdev->device.numa_node;
       int ret;
       mcdi_buff_size = RTE_ALIGN_CEIL(len, PAGE_SIZE);
+    ret = snprintf(mz_name, RTE_MEMZONE_NAMESIZE, "%s_%s",
+               sva->pdev->name, name);
+    if (ret < 0 || ret >= RTE_MEMZONE_NAMESIZE) {
From the man page:
"
The functions snprintf() and vsnprintf() do not write more than size bytes (including the terminating null byte ('\0')). "

Quoted 2 lines

you might want to pass RTE_MEMZONE_NAMESIZE - 1 as size arg to snprintf, so you can just check ret >= 0?

Sorry, I misread, the return value can indeed be greater than size:
"
If the output was truncated due to this limit, then the return value is the number of characters (excluding the terminating null byte) which would have been written to the final string if enough space had been available. Thus, a return value of size or more means that the output was truncated.
"

Quoted 18 lines

drivers/vdpa/sfc/sfc_vdpa_hw.c +8 -3

+        sfc_vdpa_err(sva, "%s_%s too long to fit in mz_name",
+                 sva->pdev->name, name);
+        return -EINVAL;
+    }
-    sfc_vdpa_log_init(sva, "name=%s, len=%zu", name, len);
+    sfc_vdpa_log_init(sva, "name=%s, len=%zu", mz_name, len);
-    mz = rte_memzone_reserve_aligned(name, mcdi_buff_size,
+    mz = rte_memzone_reserve_aligned(mz_name, mcdi_buff_size,
                        numa_node,
                        RTE_MEMZONE_IOVA_CONTIG,
                        PAGE_SIZE);
       if (mz == NULL) {
           sfc_vdpa_err(sva, "cannot reserve memory for %s: len=%#x: %s",
-                 name, (unsigned int)len, rte_strerror(rte_errno));
+                 mz_name, (unsigned int)len,
+                 rte_strerror(rte_errno));
           return -ENOMEM;
       }

Maxime Coquelin

Reviewed-by on v3 1/1

2022-02-15 12:21 UTC #7 lore raw

In reply to [email protected]

Quoted 8 lines · On 2/14/22 11:51, [email protected] wrote:

From: Abhimanyu Saini <[email protected]>
Buffer for MCDI channel is allocated using rte_memzone_reserve_aligned with zone name 'mcdi'. Since multiple MCDI channels are needed to support multiple VF(s) and rte_memzone_reserve_aligned expects unique zone names, append PCI address to zone name to make it unique.
Signed-off-by Abhimanyu Saini <[email protected]>

Quoted 9 lines

---
v2:
- Formatting changes
v3:
- Formatting changes
  drivers/vdpa/sfc/sfc_vdpa_hw.c | 15 ++++++++++++---
  1 file changed, 12 insertions(+), 3 deletions(-)

Reviewed-by Maxime Coquelin <[email protected]>

Thanks,
Maxime

Maxime Coquelin

Applied

2022-02-17 08:55 UTC #8 lore raw

In reply to [email protected]

Quoted 45 lines · On 2/14/22 11:51, [email protected] wrote:

From: Abhimanyu Saini <[email protected]>

Signed-off-by Abhimanyu Saini <[email protected]>

---
v2:
- Formatting changes
v3:
- Formatting changes

  drivers/vdpa/sfc/sfc_vdpa_hw.c | 15 ++++++++++++---
  1 file changed, 12 insertions(+), 3 deletions(-)

drivers/vdpa/sfc/sfc_vdpa_hw.c +10 -2

index fd1fee7..a7018b1 100644
@@ -25,21 +25,30 @@
  {
  	uint64_t mcdi_iova;
  	size_t mcdi_buff_size;
+	char mz_name[RTE_MEMZONE_NAMESIZE];
  	const struct rte_memzone *mz = NULL;
  	int numa_node = sva->pdev->device.numa_node;
  	int ret;
 
  	mcdi_buff_size = RTE_ALIGN_CEIL(len, PAGE_SIZE);
+	ret = snprintf(mz_name, RTE_MEMZONE_NAMESIZE, "%s_%s",
+		       sva->pdev->name, name);
+	if (ret < 0 || ret >= RTE_MEMZONE_NAMESIZE) {
+		sfc_vdpa_err(sva, "%s_%s too long to fit in mz_name",
+			     sva->pdev->name, name);
+		return -EINVAL;
+	}
 
-	sfc_vdpa_log_init(sva, "name=%s, len=%zu", name, len);
+	sfc_vdpa_log_init(sva, "name=%s, len=%zu", mz_name, len);
 
-	mz = rte_memzone_reserve_aligned(name, mcdi_buff_size,
+	mz = rte_memzone_reserve_aligned(mz_name, mcdi_buff_size,
  					 numa_node,

drivers/vdpa/sfc/sfc_vdpa_hw.c line 54

Quoted 10 lines

drivers/vdpa/sfc/sfc_vdpa_hw.c +2 -1

  					 RTE_MEMZONE_IOVA_CONTIG,
  					 PAGE_SIZE);
  	if (mz == NULL) {
  		sfc_vdpa_err(sva, "cannot reserve memory for %s: len=%#x: %s",
-			     name, (unsigned int)len, rte_strerror(rte_errno));
+			     mz_name, (unsigned int)len,
+			     rte_strerror(rte_errno));
  		return -ENOMEM;
  	}

Applied to dpdk-next-virtio/main.

Thanks,
Maxime