Skip to contentSkip to conversation

crypto: hisilicon - allow smaller reads in debugfs

PATCH In review Dan Carpenter 3 participants 4 messages Jun 2, 2020 – Jun 18, 2020

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 crypto: hisilicon - allow smaller reads in debugfs

Originally this code rejected any read less than 256 bytes. There is no need for this artificial limit. We should just use the normal helper functions to read a string from the kernel.

---
v2: Use simple_read_from_buffer(). The v1 was slightly half arsed because I left the original check for:

	if (*pos)
		return 0;

So it could result in partial reads. The new code means that if you want to read the buffer one byte at a time, that's fine or if you want to read it in one 256 byte chunk that's also fine. Plus it deletes 21 lines of code and is a lot cleaner.

 drivers/crypto/hisilicon/qm.c | 33 ++++++---------------------------
 1 file changed, 6 insertions(+), 27 deletions(-)
drivers/crypto/hisilicon/qm.c +6 -27
index 9bb263cec6c30..13ccb9e29a2e1 100644
@@ -1064,19 +1064,10 @@ static ssize_t qm_cmd_read(struct file *filp, char __user *buffer,
 	char buf[QM_DBG_READ_LEN];
 	int len;
 
-	if (*pos)
-		return 0;
-
-	if (count < QM_DBG_READ_LEN)
-		return -ENOSPC;
+	len = scnprintf(buf, QM_DBG_READ_LEN, "%s\n",
+			"Please echo help to cmd to get help information");
 
-	len = snprintf(buf, QM_DBG_READ_LEN, "%s\n",
-		       "Please echo help to cmd to get help information");
-
-	if (copy_to_user(buffer, buf, len))
-		return -EFAULT;
-
-	return (*pos = len);
+	return simple_read_from_buffer(buffer, count, pos, buf, len);
 }
 
 static void *qm_ctx_alloc(struct hisi_qm *qm, size_t ctx_size,
@@ -2691,24 +2682,12 @@ static ssize_t qm_status_read(struct file *filp, char __user *buffer,
 {
 	struct hisi_qm *qm = filp->private_data;
 	char buf[QM_DBG_READ_LEN];
-	int val, cp_len, len;
-
-	if (*pos)
-		return 0;
-
-	if (count < QM_DBG_READ_LEN)
-		return -ENOSPC;
+	int val, len;
 
 	val = atomic_read(&qm->status.flags);
-	len = snprintf(buf, QM_DBG_READ_LEN, "%s\n", qm_s[val]);
-	if (!len)
-		return -EFAULT;
-
-	cp_len = copy_to_user(buffer, buf, len);
-	if (cp_len)
-		return -EFAULT;
+	len = scnprintf(buf, QM_DBG_READ_LEN, "%s\n", qm_s[val]);
 
-	return (*pos = len);
+	return simple_read_from_buffer(buffer, count, pos, buf, len);
 }
 
 static const struct file_operations qm_status_fops = {
Signature · 1 line
2.26.2
#1 lore raw

Hi Dan,

Quoted 9 lines · Dan Carpenter

Originally this code rejected any read less than 256 bytes. There is no need for this artificial limit. We should just use the normal helper functions to read a string from the kernel.

---
v2: Use simple_read_from_buffer(). The v1 was slightly half arsed because I left the original check for:

Quoted 8 lines
	if (*pos)
		return 0;

So it could result in partial reads. The new code means that if you want to read the buffer one byte at a time, that's fine or if you want to read it in one 256 byte chunk that's also fine. Plus it deletes 21 lines of code and is a lot cleaner.

In fact, In our original design, we do not hope the user do the partial reads. Thank you for your work, but I still insist on adding this limit.

Thanks,
Shukun

Quoted 59 lines
 drivers/crypto/hisilicon/qm.c | 33 ++++++---------------------------
 1 file changed, 6 insertions(+), 27 deletions(-)
drivers/crypto/hisilicon/qm.c +6 -27
index 9bb263cec6c30..13ccb9e29a2e1 100644
@@ -1064,19 +1064,10 @@ static ssize_t qm_cmd_read(struct file *filp, char __user *buffer,
 	char buf[QM_DBG_READ_LEN];
 	int len;
 
-	if (*pos)
-		return 0;
-
-	if (count < QM_DBG_READ_LEN)
-		return -ENOSPC;
+	len = scnprintf(buf, QM_DBG_READ_LEN, "%s\n",
+			"Please echo help to cmd to get help information");
 
-	len = snprintf(buf, QM_DBG_READ_LEN, "%s\n",
-		       "Please echo help to cmd to get help information");
-
-	if (copy_to_user(buffer, buf, len))
-		return -EFAULT;
-
-	return (*pos = len);
+	return simple_read_from_buffer(buffer, count, pos, buf, len);
 }
 
 static void *qm_ctx_alloc(struct hisi_qm *qm, size_t ctx_size,
@@ -2691,24 +2682,12 @@ static ssize_t qm_status_read(struct file *filp, char __user *buffer,
 {
 	struct hisi_qm *qm = filp->private_data;
 	char buf[QM_DBG_READ_LEN];
-	int val, cp_len, len;
-
-	if (*pos)
-		return 0;
-
-	if (count < QM_DBG_READ_LEN)
-		return -ENOSPC;
+	int val, len;
 
 	val = atomic_read(&qm->status.flags);
-	len = snprintf(buf, QM_DBG_READ_LEN, "%s\n", qm_s[val]);
-	if (!len)
-		return -EFAULT;
-
-	cp_len = copy_to_user(buffer, buf, len);
-	if (cp_len)
-		return -EFAULT;
+	len = scnprintf(buf, QM_DBG_READ_LEN, "%s\n", qm_s[val]);
 
-	return (*pos = len);
+	return simple_read_from_buffer(buffer, count, pos, buf, len);
 }
 
 static const struct file_operations qm_status_fops = {
#2 lore raw
Quoted 15 lines · Shukun Tan

Hi Dan,

Quoted 12 lines · Dan Carpenter

Originally this code rejected any read less than 256 bytes. There is no need for this artificial limit. We should just use the normal helper functions to read a string from the kernel.

---
v2: Use simple_read_from_buffer(). The v1 was slightly half arsed because I left the original check for:

	if (*pos)
		return 0;
Quoted 8 lines
Quoted 5 lines

So it could result in partial reads. The new code means that if you want to read the buffer one byte at a time, that's fine or if you want to read it in one 256 byte chunk that's also fine. Plus it deletes 21 lines of code and is a lot cleaner.

In fact, In our original design, we do not hope the user do the partial reads. Thank you for your work, but I still insist on adding this limit.

This not how POSIX filesystems work... :( Last time you said that this literally breaks cat.

This doesn't break anything if the user chooses not to read a single byte at a time. That's obviously a crazy way to read a file. It just allows them to if they want. Or if they want to read 256 bytes at a time then that also works. My patch makes *everything* work.

regards,
dan carpenter

maintainer
#3 lore raw
In reply to Dan Carpenter
Quoted 12 lines · Dan Carpenter

Originally this code rejected any read less than 256 bytes. There is no need for this artificial limit. We should just use the normal helper functions to read a string from the kernel.

---
v2: Use simple_read_from_buffer(). The v1 was slightly half arsed because I left the original check for:

	if (*pos)
		return 0;
Quoted 7 lines

So it could result in partial reads. The new code means that if you want to read the buffer one byte at a time, that's fine or if you want to read it in one 256 byte chunk that's also fine. Plus it deletes 21 lines of code and is a lot cleaner.

 drivers/crypto/hisilicon/qm.c | 33 ++++++---------------------------
 1 file changed, 6 insertions(+), 27 deletions(-)

Patch applied. Thanks.

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt