CR-1156386 Default and Backup FPT sysfs nodes impl #7715

prapulkrishnamurthy · 2023-09-20T22:00:30Z

Problem solved by the commit

Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered

This is an implementation of sysfs nodes for debug purposes. Sysfs nodes for reading raw meta data from Default FPT and Backup FPT through VMR.

How problem was solved, alternative solutions (if any) and why they were rejected

Added changes in both XRT and corresponding VMR compatible changes.

Risks (if any) associated the changes in the commit

What has been tested and how, request additional testing if necessary

Tested on V70 base 3 2023.2 and found the hexdump to be matching between default and backup dumps
Test results are attached in the VMR PR here:- https://github.com/Xilinx/VMR/pull/335

Documentation impact (if any)

gbuildx · 2023-09-20T22:02:02Z

Can one of the admins verify this patch?

prapulkrishnamurthy · 2023-09-20T22:02:02Z

src/runtime_src/core/include/xgq_cmd_vmr.h

+	XGQ_CMD_LOG_PLM_LOG		 = 0x8,
+	XGQ_CMD_LOG_APU_LOG		 = 0x9,
+	XGQ_CMD_LOG_SHELL_INTERFACE_UUID = 0xa,
+	XGQ_CMD_LOG_DEFAULT_FPT		 = 0xb,


alignment changes made to keep consistency with vmr common header file

look ok as long as the vmr changes made in vmr.

xdavidz

This looks ok, but I realized that there is a potential bug in our driver, that every log_page request might return a big size back. We need to valid the size before call vmalloc.

I think just add a code to make sure the returned size should be within request size. Does this make sense?

xdavidz · 2023-09-20T22:10:03Z

This looks ok, but I realized that there is a potential bug in our driver, that every log_page request might return a big size back. We need to valid the size before call vmalloc.

I think just add a code to make sure the returned size should be within request size. Does this make sense?

Adjust my request a little bit. Actually, we did have checking code in the log_page request for each set of data.
But given the request off might be not zero. Thus, I don't think we should cache those data unless we have to.

Can you please take a look at the vmr_apu_log_read as an example?
aka. We only temporary copy data to buf and free the memory. We don't cache those in the driver.

Including fpt, system dtb, fw_log, plm_log, apu_log. The shell interface uuid is special, this one can be cached.

xdavidz · 2023-09-21T04:18:11Z

ok to test

maxzhen · 2023-09-22T15:50:45Z

src/runtime_src/core/pcie/driver/linux/xocl/subdev/xgq_vmr.c

+
+        mutex_lock(&xgq->xgq_lock);
+
+        blob = xgq->xgq_vmr_backup_fpt;
+        size = xgq->xgq_vmr_backup_fpt_size;
+
+        if (off >= size)
+                goto out;
+
+        if (off + count > size)
+                count = size - off;
+        memcpy(buf, blob + off, count);
+
+        ret = count;
+out:
+        mutex_unlock(&xgq->xgq_lock);
+        return ret;


worth creating a function for here and L3062-3078?

there are indeed 3+ places dup code. could prapul fix this?

xdavidz · 2023-09-23T22:11:32Z

reset this please

prapulkrishnamurthy · 2023-09-26T00:42:46Z

The commit 87af11f involves eliminating the need for caching sysfs read data for plm, system dtb, default and backup fpt data in driver as per @xdavidz suggested.

xdavidz · 2023-09-26T05:41:20Z

src/runtime_src/core/pcie/driver/linux/xocl/subdev/xgq_vmr.c

-	 */
-	if (off == 0)
-		ret = xgq_refresh_system_dtb(xgq);
+	if (off > 0)


this looks incorrect. why off cannot be bigger than 0? It might be more than 4096 per linux sysfs request for the blob data. we are also able to return data with any offset.

Non-zero offset means there is some data returned per read request. This is more like a boundary check to avoid infinite looping of this API call because, after read and return, the offset seeks back to zero and automatically keeps calling this API by the behavior of sysfs.

src/runtime_src/core/pcie/driver/linux/xocl/subdev/xgq_vmr.c

xdavidz

overall look ok to me, those nits are optional.
But the memory leak has to be fixed in all 4 places.

xdavidz · 2023-09-27T23:31:24Z

src/runtime_src/core/pcie/driver/linux/xocl/subdev/xgq_vmr.c

+        ret = count;
+done:
+	vfree(log_buf);
+        XGQ_INFO(xgq,"Offset  = %d, count = %d, ret = %d\n",(int)off,(int)count,(int)ret);


nits: to avoid compiling warning, the more cleaner way: offset and count is %ld, ret is %d

Signed-off-by: Prapul Krishnamurthy <[email protected]>

manikandan-xilinx · 2023-09-28T21:26:37Z

retest this please.

gbuildx · 2023-09-29T00:44:50Z

Build Passed!

prapulkrishnamurthy requested review from NishadSaraf and xdavidz September 20, 2023 22:00

prapulkrishnamurthy requested review from maxzhen and stsoe as code owners September 20, 2023 22:00

prapulkrishnamurthy commented Sep 20, 2023

View reviewed changes

xdavidz requested changes Sep 20, 2023

View reviewed changes

stsoe removed their request for review September 20, 2023 22:03

prapulkrishnamurthy changed the title ~~Default and Backup FPT sysfs nodes impl~~ CR-1156386 Default and Backup FPT sysfs nodes impl Sep 20, 2023

NishadSaraf approved these changes Sep 20, 2023

View reviewed changes

maxzhen reviewed Sep 22, 2023

View reviewed changes

maxzhen approved these changes Sep 22, 2023

View reviewed changes

xdavidz approved these changes Sep 24, 2023

View reviewed changes

prapulkrishnamurthy force-pushed the CR-1156386_default_FPT_backup_FPT_sync branch from 7b71112 to 87af11f Compare September 26, 2023 00:40

prapulkrishnamurthy requested a review from xdavidz September 26, 2023 00:40

xdavidz reviewed Sep 26, 2023

View reviewed changes

prapulkrishnamurthy force-pushed the CR-1156386_default_FPT_backup_FPT_sync branch from 87af11f to 82814d3 Compare September 27, 2023 22:32

prapulkrishnamurthy requested review from NishadSaraf and xdavidz September 27, 2023 22:40

xdavidz reviewed Sep 27, 2023

View reviewed changes

src/runtime_src/core/pcie/driver/linux/xocl/subdev/xgq_vmr.c Outdated Show resolved Hide resolved

xdavidz reviewed Sep 27, 2023

View reviewed changes

src/runtime_src/core/pcie/driver/linux/xocl/subdev/xgq_vmr.c Outdated Show resolved Hide resolved

xdavidz reviewed Sep 27, 2023

View reviewed changes

src/runtime_src/core/pcie/driver/linux/xocl/subdev/xgq_vmr.c Outdated Show resolved Hide resolved

NishadSaraf approved these changes Sep 27, 2023

View reviewed changes

xdavidz requested changes Sep 27, 2023

View reviewed changes

prapulkrishnamurthy force-pushed the CR-1156386_default_FPT_backup_FPT_sync branch from 82814d3 to 8b9ef21 Compare September 27, 2023 23:10

xdavidz reviewed Sep 27, 2023

View reviewed changes

xdavidz approved these changes Sep 27, 2023

View reviewed changes

Default and Backup FPT sysfs nodes impl

c19308c

Signed-off-by: Prapul Krishnamurthy <[email protected]>

prapulkrishnamurthy force-pushed the CR-1156386_default_FPT_backup_FPT_sync branch from 8b9ef21 to c19308c Compare September 27, 2023 23:49

maxzhen merged commit 5833de0 into Xilinx:master Sep 29, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CR-1156386 Default and Backup FPT sysfs nodes impl #7715

CR-1156386 Default and Backup FPT sysfs nodes impl #7715

prapulkrishnamurthy commented Sep 20, 2023

gbuildx commented Sep 20, 2023

prapulkrishnamurthy Sep 20, 2023

xdavidz Sep 26, 2023

xdavidz left a comment

xdavidz commented Sep 20, 2023 •

edited

Loading

xdavidz commented Sep 21, 2023

maxzhen Sep 22, 2023

xdavidz Sep 23, 2023

xdavidz commented Sep 23, 2023

prapulkrishnamurthy commented Sep 26, 2023 •

edited

Loading

xdavidz Sep 26, 2023

prapulkrishnamurthy Sep 26, 2023 •

edited

Loading

xdavidz left a comment

xdavidz Sep 27, 2023

manikandan-xilinx commented Sep 28, 2023

gbuildx commented Sep 29, 2023

CR-1156386 Default and Backup FPT sysfs nodes impl #7715

CR-1156386 Default and Backup FPT sysfs nodes impl #7715

Conversation

prapulkrishnamurthy commented Sep 20, 2023

Problem solved by the commit

Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered

How problem was solved, alternative solutions (if any) and why they were rejected

Risks (if any) associated the changes in the commit

What has been tested and how, request additional testing if necessary

Documentation impact (if any)

gbuildx commented Sep 20, 2023

prapulkrishnamurthy Sep 20, 2023

Choose a reason for hiding this comment

xdavidz Sep 26, 2023

Choose a reason for hiding this comment

xdavidz left a comment

Choose a reason for hiding this comment

xdavidz commented Sep 20, 2023 • edited Loading

xdavidz commented Sep 21, 2023

maxzhen Sep 22, 2023

Choose a reason for hiding this comment

xdavidz Sep 23, 2023

Choose a reason for hiding this comment

xdavidz commented Sep 23, 2023

prapulkrishnamurthy commented Sep 26, 2023 • edited Loading

xdavidz Sep 26, 2023

Choose a reason for hiding this comment

prapulkrishnamurthy Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

xdavidz left a comment

Choose a reason for hiding this comment

xdavidz Sep 27, 2023

Choose a reason for hiding this comment

manikandan-xilinx commented Sep 28, 2023

gbuildx commented Sep 29, 2023

xdavidz commented Sep 20, 2023 •

edited

Loading

prapulkrishnamurthy commented Sep 26, 2023 •

edited

Loading

prapulkrishnamurthy Sep 26, 2023 •

edited

Loading