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 */
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 */
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
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
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
linux-stable-mirror@lists.linaro.org
-
Christoph Hellwig -
Ewan D. Milne -
James Smart -
Muneendra Kumar M