All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Revert "nvme-fc: fold t fc_update_appid into fc_appid_store"
@ 2022-08-01 16:27 Ewan D. Milne
  2022-08-01 18:27 ` Christoph Hellwig
  0 siblings, 1 reply; 5+ messages in thread
From: Ewan D. Milne @ 2022-08-01 16:27 UTC (permalink / raw)
  To: linux-nvme; +Cc: hch, muneendra.kumar, james.smart, stable

This reverts commit c814153c83a892dfd42026eaa661ae2c1f298792.

The commit c814153c83a8 "nvme-fc: fold t fc_update_appid into fc_appid_store"
changed the userspace interface, because the code that decrements "count"
to remove a trailing '\n' in the parsing results in the decremented value being
incorrectly be returned from the sysfs write.  Fix this by revering the commit.

Cc: stable@vger.kernel.org
Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/nvme/host/fc.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 9987797620b6..27f6dfad5d3b 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -3873,10 +3873,12 @@ static int fc_parse_cgrpid(const char *buf, u64 *id)
 }
 
 /*
- * Parse and update the appid in the blkcg associated with the cgroupid.
+ * fc_update_appid: Parse and update the appid in the blkcg associated with
+ * cgroupid.
+ * @buf: buf contains both cgrpid and appid info
+ * @count: size of the buffer
  */
-static ssize_t fc_appid_store(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t count)
+static int fc_update_appid(const char *buf, size_t count)
 {
 	u64 cgrp_id;
 	int appid_len = 0;
@@ -3904,6 +3906,17 @@ static ssize_t fc_appid_store(struct device *dev,
 		return ret;
 	return count;
 }
+
+static ssize_t fc_appid_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	int ret  = 0;
+
+	ret = fc_update_appid(buf, count);
+	if (ret < 0)
+		return -EINVAL;
+	return count;
+}
 static DEVICE_ATTR(appid_store, 0200, NULL, fc_appid_store);
 #endif /* CONFIG_BLK_CGROUP_FC_APPID */
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] Revert "nvme-fc: fold t fc_update_appid into fc_appid_store"
  2022-08-01 16:27 [PATCH] Revert "nvme-fc: fold t fc_update_appid into fc_appid_store" Ewan D. Milne
@ 2022-08-01 18:27 ` Christoph Hellwig
  2022-08-01 21:03   ` James Smart
  0 siblings, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2022-08-01 18:27 UTC (permalink / raw)
  To: Ewan D. Milne; +Cc: linux-nvme, hch, muneendra.kumar, james.smart, stable

On Mon, Aug 01, 2022 at 12:27:13PM -0400, Ewan D. Milne wrote:
> This reverts commit c814153c83a892dfd42026eaa661ae2c1f298792.
> 
> The commit c814153c83a8 "nvme-fc: fold t fc_update_appid into fc_appid_store"
> changed the userspace interface, because the code that decrements "count"
> to remove a trailing '\n' in the parsing results in the decremented value being
> incorrectly be returned from the sysfs write.  Fix this by revering the commit.

Wouldn't something like the patch below be much simpler and cleaner:


diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 9987797620b6d..e24ab688f00d5 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -3878,6 +3878,7 @@ static int fc_parse_cgrpid(const char *buf, u64 *id)
 static ssize_t fc_appid_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t count)
 {
+	size_t orig_count = count;
 	u64 cgrp_id;
 	int appid_len = 0;
 	int cgrpid_len = 0;
@@ -3902,7 +3903,7 @@ static ssize_t fc_appid_store(struct device *dev,
 	ret = blkcg_set_fc_appid(app_id, cgrp_id, sizeof(app_id));
 	if (ret < 0)
 		return ret;
-	return count;
+	return orig_count;
 }
 static DEVICE_ATTR(appid_store, 0200, NULL, fc_appid_store);
 #endif /* CONFIG_BLK_CGROUP_FC_APPID */

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] Revert "nvme-fc: fold t fc_update_appid into fc_appid_store"
  2022-08-01 18:27 ` Christoph Hellwig
@ 2022-08-01 21:03   ` James Smart
  2022-08-03  4:38     ` Muneendra Kumar M
  0 siblings, 1 reply; 5+ messages in thread
From: James Smart @ 2022-08-01 21:03 UTC (permalink / raw)
  To: Christoph Hellwig, Ewan D. Milne
  Cc: linux-nvme, muneendra.kumar, james.smart, stable

On 8/1/2022 11:27 AM, Christoph Hellwig wrote:
> On Mon, Aug 01, 2022 at 12:27:13PM -0400, Ewan D. Milne wrote:
>> This reverts commit c814153c83a892dfd42026eaa661ae2c1f298792.
>>
>> The commit c814153c83a8 "nvme-fc: fold t fc_update_appid into fc_appid_store"
>> changed the userspace interface, because the code that decrements "count"
>> to remove a trailing '\n' in the parsing results in the decremented value being
>> incorrectly be returned from the sysfs write.  Fix this by revering the commit.
> 
> Wouldn't something like the patch below be much simpler and cleaner:
> 
> 
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 9987797620b6d..e24ab688f00d5 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -3878,6 +3878,7 @@ static int fc_parse_cgrpid(const char *buf, u64 *id)
>   static ssize_t fc_appid_store(struct device *dev,
>   		struct device_attribute *attr, const char *buf, size_t count)
>   {
> +	size_t orig_count = count;
>   	u64 cgrp_id;
>   	int appid_len = 0;
>   	int cgrpid_len = 0;
> @@ -3902,7 +3903,7 @@ static ssize_t fc_appid_store(struct device *dev,
>   	ret = blkcg_set_fc_appid(app_id, cgrp_id, sizeof(app_id));
>   	if (ret < 0)
>   		return ret;
> -	return count;
> +	return orig_count;
>   }
>   static DEVICE_ATTR(appid_store, 0200, NULL, fc_appid_store);
>   #endif /* CONFIG_BLK_CGROUP_FC_APPID */
> 

Reviewed-by: James Smart <jsmart2021@gmail.com>

looks good on my end.

-- james

^ permalink raw reply	[flat|nested] 5+ messages in thread
* RE: [PATCH] Revert "nvme-fc: fold t fc_update_appid into fc_appid_store"
  2022-08-01 21:03   ` James Smart
@ 2022-08-03  4:38     ` Muneendra Kumar M
  2022-08-03 16:34       ` Ewan D. Milne
  0 siblings, 1 reply; 5+ messages in thread
From: Muneendra Kumar M @ 2022-08-03  4:38 UTC (permalink / raw)
  To: James Smart, Christoph Hellwig, Ewan D. Milne
  Cc: linux-nvme, James Smart, stable

[-- Attachment #1: Type: text/plain, Size: 2773 bytes --]

I have tested the below changes suggested by Christoph and it is working as
expected.


Tested-by: <muneendra.kumar@broadcom.com>



-----Original Message-----
From: James Smart [mailto:jsmart2021@gmail.com]
Sent: Tuesday, August 2, 2022 2:33 AM
To: Christoph Hellwig <hch@lst.de>; Ewan D. Milne <emilne@redhat.com>
Cc: linux-nvme@lists.infradead.org; muneendra.kumar@broadcom.com;
james.smart@broadcom.com; stable@vger.kernel.org
Subject: Re: [PATCH] Revert "nvme-fc: fold t fc_update_appid into
fc_appid_store"

On 8/1/2022 11:27 AM, Christoph Hellwig wrote:
> On Mon, Aug 01, 2022 at 12:27:13PM -0400, Ewan D. Milne wrote:
>> This reverts commit c814153c83a892dfd42026eaa661ae2c1f298792.
>>
>> The commit c814153c83a8 "nvme-fc: fold t fc_update_appid into
>> fc_appid_store"
>> changed the userspace interface, because the code that decrements "count"
>> to remove a trailing '\n' in the parsing results in the decremented
>> value being incorrectly be returned from the sysfs write.  Fix this by
>> revering the commit.
>
> Wouldn't something like the patch below be much simpler and cleaner:
>
>
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c index
> 9987797620b6d..e24ab688f00d5 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -3878,6 +3878,7 @@ static int fc_parse_cgrpid(const char *buf, u64 *id)
>   static ssize_t fc_appid_store(struct device *dev,
>   		struct device_attribute *attr, const char *buf, size_t count)
>   {
> +	size_t orig_count = count;
>   	u64 cgrp_id;
>   	int appid_len = 0;
>   	int cgrpid_len = 0;
> @@ -3902,7 +3903,7 @@ static ssize_t fc_appid_store(struct device *dev,
>   	ret = blkcg_set_fc_appid(app_id, cgrp_id, sizeof(app_id));
>   	if (ret < 0)
>   		return ret;
> -	return count;
> +	return orig_count;
>   }
>   static DEVICE_ATTR(appid_store, 0200, NULL, fc_appid_store);
>   #endif /* CONFIG_BLK_CGROUP_FC_APPID */
>

Reviewed-by: James Smart <jsmart2021@gmail.com>

looks good on my end.

-- james

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4220 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] Revert "nvme-fc: fold t fc_update_appid into fc_appid_store"
  2022-08-03  4:38     ` Muneendra Kumar M
@ 2022-08-03 16:34       ` Ewan D. Milne
  0 siblings, 0 replies; 5+ messages in thread
From: Ewan D. Milne @ 2022-08-03 16:34 UTC (permalink / raw)
  To: Muneendra Kumar M, James Smart, Christoph Hellwig
  Cc: linux-nvme, James Smart, stable

On Wed, 2022-08-03 at 10:08 +0530, Muneendra Kumar M wrote:
> I have tested the below changes suggested by Christoph and it is working as
> expected.
> 
> 
> Tested-by: <muneendra.kumar@broadcom.com>
> 

Fine with me.

Reviewed-by: Ewan D. Milne <emilne@redhat.com>



^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-08-03 16:34 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-01 16:27 [PATCH] Revert "nvme-fc: fold t fc_update_appid into fc_appid_store" Ewan D. Milne
2022-08-01 18:27 ` Christoph Hellwig
2022-08-01 21:03   ` James Smart
2022-08-03  4:38     ` Muneendra Kumar M
2022-08-03 16:34       ` Ewan D. Milne

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.