[Add] Transcend plugin command for PLP health #2355

WBJisMyName · 2024-05-27T08:10:18Z

This function will support the Transcend NVMe Devices.
Please feel free to contact me if you have any question.
Thanks.

igaw · 2024-06-04T13:19:10Z

There are code style issues which need to be addressed. see also the output from checkpatch

The rest looks reasonable.

Please also use proper commit subject lines, e.g. with the correct prefix.

thanks

plugins/transcend/transcend-nvme.c

meson.build

plugins/transcend/transcend-nvme.c

WBJisMyName

update coding style and some errors

ikegami-t · 2024-10-21T21:39:47Z

Build failed and some other ~~error~~ errors also detected by the checks.

ikegami-t · 2024-10-22T15:14:47Z

Looks all commits should be squashed to a patch since no need to separate the patches to fix the first patch.
The commit message should be followed the description below.
https://github.com/linux-nvme/nvme-cli/pull/2539/files
Seems basically should be confirmed the contribution guide below at first.
https://github.com/linux-nvme/nvme-cli/blob/master/README.md#how-to-contribute
The camel case definition and variable naming should be fixed as below for example.

- #define PLPRecordPath "PLPRec.txt"
+ #define PLP_RECORD_PATH "PLPRec.txt"
  ...
- static const int iDis = 20;
+ static const int i_dis = 20;
  ...
- enum PLPErrorCode {
+ enum plp_error_code {

Note: Sorry still not checked detail more so if needed let me comment later.

WBJisMyName · 2024-10-22T15:46:11Z

Thanks for your reply, I will fix these errors.

Signed-off-by: WBJisMyName <[email protected]>

ikegami-t

Looks better to be mergeged all commits into a commit.

ikegami-t · 2024-11-02T11:46:49Z

plugins/transcend/transcend-nvme.c

 }
+
+int read_useful_plp_value(const char *device)


Should be a static function.

ikegami-t · 2024-11-02T11:47:18Z

plugins/transcend/transcend-nvme.c

+    }
+}
+
+void record_plp_value(const char *device, int value, bool is_replace)


Static function.

ikegami-t · 2024-11-02T11:50:09Z

plugins/transcend/transcend-nvme.h


-#include "define_cmd.h"
+#include "define_cmd.h"


Should be added a new line code on the end of the line (before the end of file).

ikegami-t · 2024-11-02T11:54:11Z

plugins/transcend/transcend-nvme.c

+static const double full_value = 170;
+
+enum plp_error_code {
+    PLP_ERROR_NO_MATCH = -1,


All indentation should be chagned to 8 character tab instead of the whitespace basically.

ikegami-t · 2024-11-02T11:55:58Z

plugins/transcend/transcend-nvme.c

+    PLP_ERROR_DATA_EXPIRED = -2
+};
+
+const char *string_list[] = {


Those variables seem better to be static variables.

ikegami-t · 2024-11-02T11:58:56Z

plugins/transcend/transcend-nvme.h

@@ -5,17 +5,19 @@
 #if !defined(TRANSCEND_NVME) || defined(CMD_HEADER_MULTI_READ)
 #define TRANSCEND_NVME

+#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))


This definition is dupulicated with the definition in common.h.

Signed-off-by: WBJisMyName <[email protected]>

ikegami-t · 2024-11-04T08:57:14Z

plugins/transcend/transcend-nvme.c

+};
+
+static const char *string_list[] = {
+    "UTE210T",   "MTE712P",   "MTE560P",   "MTE662P", "MTE662P-I", "MTS970P",


Still indentation errors. (Should be used 8 characters tab instead of 4 white spaces.)

ikegami-t · 2024-11-04T08:57:47Z

plugins/transcend/transcend-nvme.c

+
+static int get_health_value(int argc, char **argv, struct command *cmd, struct plugin *plugin)
+{
+    struct nvme_smart_log smart_log;


These are also indentation errors.

FYI: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.12-rc6#n18

Thanks for your quick reply, I will correct it.

checkpatch.pl is helpful to detect these kind of formatting issues. You can run it locally. Fetch the linux kernel source and run linux/scripts/checkpatch.pl -g HEAD or similar. Keep in mind checkpatch.pl is very opinionated and we should just be ignored some of the advice.

2. Change function to Static function Signed-off-by: WBJisMyName <[email protected]>

ikegami-t · 2024-11-09T12:30:58Z

plugins/transcend/transcend-nvme.c

+}
+
+static const char *format_char_array(char *str, int str_size,
+				   unsigned char *chr, int chr_size)


Indentation error.

tatic const char *format_char_array(char *str, int str_size, - unsigned char *chr, int chr_size) + unsigned char *chr, int chr_size)

ikegami-t · 2024-11-09T12:31:46Z

plugins/transcend/transcend-nvme.c

+}
+
+static int get_plp_health(int argc, char **argv, struct command *cmd,
+			 struct plugin *plugin)


Indentation error.

static int get_plp_health(int argc, char **argv, struct command *cmd, - struct plugin *plugin) + struct plugin *plugin)

ikegami-t · 2024-11-09T12:33:32Z

plugins/transcend/transcend-nvme.c

+	}
+
+	/* Calculate PLP health */
+	{


Seems better to split this part into a separated function.

ikegami-t · 2024-11-09T12:40:47Z

plugins/transcend/transcend-nvme.c

-		return -1;
+	ret = parse_and_open(&dev, argc, argv, desc, opts);
+	if (ret) {
+		pr_err("Device not found\n");


Seems better to use fprintf(stderr, ...) and printf() instead of the pr_err and pr_info since only this file using the pr_***() funcitons.

ikegami-t · 2024-11-09T12:43:10Z

plugins/transcend/transcend-nvme.c

+	} else {
+		remove(tmp_path);
+	}
+}


Need to insert a line break after } before EOF.

ikegami-t · 2024-11-09T12:46:33Z

plugins/transcend/transcend-nvme.c

+	fclose(fp_in);
+	fclose(fp_out);
+
+	if (remove(log_path) == 0) {


No need to use braces { and } for the single statement.

ikegami-t · 2024-11-09T12:51:35Z

plugins/transcend/transcend-nvme.c

+static int read_useful_plp_value(const char *device);
+static void record_plp_value(const char *device, int value, bool is_replace);
+
+static int get_health_value(int argc, char **argv, struct command *cmd,


Seems better to split this function related changes to other patch.

ikegami-t · 2024-11-09T12:53:39Z

plugins/transcend/transcend-nvme.c

 }

-static int getBadblock(int argc, char **argv, struct command *cmd, struct plugin *plugin)
+static int get_bad_block(int argc, char **argv, struct command *cmd,


Seems better to split this function related changes also to other patch since not related to the plphealthvalue command.

igaw · 2024-11-11T12:35:26Z

There are build errors which need to be address, e.g. there is no pr_err or pr_info macro or function. And the three unused function as well.

The documentation should also be hooked up:

--- a/Documentation/meson.build
+++ b/Documentation/meson.build
@@ -148,6 +148,7 @@ adoc_sources = [
   'nvme-toshiba-vs-smart-add-log',
   'nvme-transcend-badblock',
   'nvme-transcend-healthvalue',
+  'nvme-transcend-plphealthvalue',
   'nvme-verify',
   'nvme-virtium-save-smart-to-vtview-log',
   'nvme-virtium-show-identify',

The rest looks almost there (after squashing all changes into a single patch). A few small nitpicks which I'd address myself but as it doesn't build right now, please fix the build and I'd cleanup the rest.

igaw · 2024-12-20T12:00:33Z

Are you planing to address the remaining issues? Or is this PR abandoned?

JerryWang added 2 commits May 27, 2024 15:45

[Add] Transcend plugin command for PLP health

0b4398d

[Add] Transcend plugin command for PLP health

2049e29

ikegami-t reviewed Jun 28, 2024

View reviewed changes

plugins/transcend/transcend-nvme.c Outdated Show resolved Hide resolved

martin-belanger reviewed Jun 28, 2024

View reviewed changes

meson.build Outdated Show resolved Hide resolved

igaw requested changes Jun 28, 2024

View reviewed changes

plugins/transcend/transcend-nvme.c Outdated Show resolved Hide resolved

plugins/transcend/transcend-nvme.c Outdated Show resolved Hide resolved

plugins/transcend/transcend-nvme.c Outdated Show resolved Hide resolved

plugins/transcend/transcend-nvme.c Show resolved Hide resolved

WBJisMyName commented Oct 21, 2024

View reviewed changes

coding sytle fixed

55133c4

WBJisMyName requested a review from igaw October 21, 2024 07:30

JerryWang added 3 commits October 22, 2024 15:59

Build error fixed

25d6e90

Build error fixed

69701d5

Fixed for coding sytle

3f47f5c

Correct coding style and naming rules

1f9aa25

Signed-off-by: WBJisMyName <[email protected]>

ikegami-t reviewed Nov 2, 2024

View reviewed changes

Add static to function and make minor adjustments.

69e6c76

Signed-off-by: WBJisMyName <[email protected]>

ikegami-t reviewed Nov 4, 2024

View reviewed changes

1. Fixed indentation errors. (Use 8 characters tab)

a2c64f7

2. Change function to Static function Signed-off-by: WBJisMyName <[email protected]>

ikegami-t reviewed Nov 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Add] Transcend plugin command for PLP health #2355

[Add] Transcend plugin command for PLP health #2355

WBJisMyName commented May 27, 2024

igaw commented Jun 4, 2024 •

edited

Loading

WBJisMyName left a comment

ikegami-t commented Oct 21, 2024 •

edited

Loading

ikegami-t commented Oct 22, 2024 •

edited

Loading

WBJisMyName commented Oct 22, 2024

ikegami-t left a comment

ikegami-t Nov 2, 2024

ikegami-t Nov 2, 2024

ikegami-t Nov 2, 2024

ikegami-t Nov 2, 2024

ikegami-t Nov 2, 2024

ikegami-t Nov 2, 2024

ikegami-t Nov 4, 2024

ikegami-t Nov 4, 2024

ikegami-t Nov 4, 2024

WBJisMyName Nov 4, 2024

igaw Nov 4, 2024

ikegami-t Nov 9, 2024

ikegami-t Nov 9, 2024

ikegami-t Nov 9, 2024

ikegami-t Nov 9, 2024

ikegami-t Nov 9, 2024

ikegami-t Nov 9, 2024

ikegami-t Nov 9, 2024

ikegami-t Nov 9, 2024

igaw commented Nov 11, 2024

igaw commented Dec 20, 2024


		#include "define_cmd.h"
		#include "define_cmd.h"

[Add] Transcend plugin command for PLP health #2355

Are you sure you want to change the base?

[Add] Transcend plugin command for PLP health #2355

Conversation

WBJisMyName commented May 27, 2024

igaw commented Jun 4, 2024 • edited Loading

WBJisMyName left a comment

Choose a reason for hiding this comment

ikegami-t commented Oct 21, 2024 • edited Loading

ikegami-t commented Oct 22, 2024 • edited Loading

WBJisMyName commented Oct 22, 2024

ikegami-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igaw commented Nov 11, 2024

igaw commented Dec 20, 2024

igaw commented Jun 4, 2024 •

edited

Loading

ikegami-t commented Oct 21, 2024 •

edited

Loading

ikegami-t commented Oct 22, 2024 •

edited

Loading