nvmem: rmem: Fix return value of rmem_read()
Subsystems (2)
PATCH v2 1/1 nvmem: rmem: Fix return value of rmem_read()
reg_read() callback registered with nvmem core expects an integer error as a return value but rmem_read() returns the number of bytes read, as a result error checks in nvmem core fail even when they shouldn't.
Return 0 on success where number of bytes read match the number of bytes requested and a negative error -EINVAL on all other cases.
--- drivers/nvmem/rmem.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/nvmem/rmem.c +6 -1
@@ -46,7 +46,12 @@ static int rmem_read(void *context, unsigned int offset,memunmap (addr); -return count; +if (count != bytes) { +dev_err (priv->dev,"Failed read memory (%d)\n" , count); +return -EINVAL ; + } + +return 0 ; }static int rmem_probe (struct platform_device *pdev)
Signature · 1 line
2.43.0.594.gd9cf4e227d-goog
Greg Kroah-Hartman
maintainerQuoted 19 lines · Joy Chakraborty
reg_read()callback registered with nvmem core expects an integer error as a return value butrmem_read()returns the number of bytes read, as a result error checks in nvmem core fail even when they shouldn't.Return 0 on success where number of bytes read match the number of bytes requested and a negative error
-EINVALon all other cases.Fixes 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem")Signed-off-by Joy Chakraborty <[email protected]>--- drivers/nvmem/rmem.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/nvmem/rmem.c +0
-0
drivers/nvmem/rmem.c static int rmem_read(void *context, unsigned int offset, · line 51
Quoted 7 lines
drivers/nvmem/rmem.c +3
-1@@ -46,7 +46,12 @@ static int rmem_read(void *context, unsigned int offset,memunmap (addr); -return count; +if (count != bytes) { +dev_err (priv->dev,"Failed read memory (%d)\n" , count); +return -EINVAL ;
Why is a "short read" somehow illegal here? What internal changes need to be made now that this has changed?
And what will userspace do with this error message in the kernel log?
thanks,
greg k-h
Quoted 24 lines · Greg Kroah-Hartman
Quoted 22 lines · Joy Chakraborty
reg_read()callback registered with nvmem core expects an integer error as a return value butrmem_read()returns the number of bytes read, as a result error checks in nvmem core fail even when they shouldn't.Return 0 on success where number of bytes read match the number of bytes requested and a negative error
-EINVALon all other cases.Fixes 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem")Signed-off-by Joy Chakraborty <[email protected]>--- drivers/nvmem/rmem.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/nvmem/rmem.c +0
-0@@ -46,7 +46,12 @@ static int rmem_read(void *context, unsigned int offset,memunmap (addr);
Quoted 7 lines
drivers/nvmem/rmem.c static int rmem_read(void *context, unsigned int offset, · line 51
Quoted 4 lines
drivers/nvmem/rmem.c +3
-1-return count; +if (count != bytes) { +dev_err (priv->dev,"Failed read memory (%d)\n" , count); +return -EINVAL ;Why is a "short read" somehow illegal here? What internal changes need to be made now that this has changed?
In my opinion "short read" should be illegal for cases where if the nvmem core is unable to read the required size of data to fill up a nvmem cell then data returned might have truncated value.
No internal changes should be made since the registered reg_read() is called from __nvmem_reg_read() which eventually passes on the error code to nvmem_reg_read() whose return code is already checked and passed to nvmem consumers.
Currently rmem driver is incorrectly passing a positive value for success.
Quoted 2 lines
And what will userspace do with this error message in the kernel log?
User space currently is not seeing this error for nvmem device/eeprom reads due to the following code at nvmem/core.c in bin_attr_nvmem_read():
"
rc = nvmem_reg_read(nvmem, pos, buf, count);
if (rc)
return rc;
return count;"
since it expects to return the number of bytes.
Userspace will see a false error with nvmem cell reads from nvmem_cell_attr_read() in current code, which should be fixed on returning 0 for success.
Quoted 3 lines
thanks,
greg k-h
Thanks
Joy
Greg Kroah-Hartman
maintainerQuoted 34 lines · Joy Chakraborty
Quoted 24 lines · Greg Kroah-Hartman
Quoted 22 lines · Joy Chakraborty
reg_read()callback registered with nvmem core expects an integer error as a return value butrmem_read()returns the number of bytes read, as a result error checks in nvmem core fail even when they shouldn't.Return 0 on success where number of bytes read match the number of bytes requested and a negative error
-EINVALon all other cases.Fixes 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem")Signed-off-by Joy Chakraborty <[email protected]>--- drivers/nvmem/rmem.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/nvmem/rmem.c +0
-0@@ -46,7 +46,12 @@ static int rmem_read(void *context, unsigned int offset,memunmap (addr);Quoted 7 lines
drivers/nvmem/rmem.c static int rmem_read(void *context, unsigned int offset, · line 51
Quoted 4 lines
drivers/nvmem/rmem.c +3
-1-return count; +if (count != bytes) { +dev_err (priv->dev,"Failed read memory (%d)\n" , count); +return -EINVAL ;Why is a "short read" somehow illegal here? What internal changes need to be made now that this has changed?
Quoted 3 lines
In my opinion "short read" should be illegal for cases where if the nvmem core is unable to read the required size of data to fill up a nvmem cell then data returned might have truncated value.
But that's kind of against what a read() call normally expects.
Quoted 5 lines
No internal changes should be made since the registered
reg_read()is called from__nvmem_reg_read()which eventually passes on the error code tonvmem_reg_read()whose return code is already checked and passed to nvmem consumers.
Currently rmem driver is incorrectly passing a positive value for success.
So this is an internal api issue and not a general issue? Unwinding the read callbacks here is hard.
Also, in looking at the code, how can this ever be a short read? You are using memory_read_from_buffer() which unless the values passed into it are incorrect, will always return the expected read amount.
Quoted 12 lines
Quoted 1 line
And what will userspace do with this error message in the kernel log?
User space currently is not seeing this error for nvmem device/eeprom reads due to the following code at
nvmem/core.cinbin_attr_nvmem_read():
"rc = nvmem_reg_read(nvmem, pos, buf, count); if (rc) return rc; return count;
Quoted 6 lines
"
since it expects to return the number of bytes.Userspace will see a false error with nvmem cell reads from
nvmem_cell_attr_read()in current code, which should be fixed on returning 0 for success.
So maybe fix this all up to allow the read to return the actual amount read? That feels more "correct" to me.
thanks,
greg k-h
Quoted 36 lines · Greg Kroah-Hartman
Quoted 34 lines · Joy Chakraborty
Quoted 31 lines · Greg Kroah-Hartman
Quoted 19 lines · Joy Chakraborty
reg_read()callback registered with nvmem core expects an integer error as a return value butrmem_read()returns the number of bytes read, as a result error checks in nvmem core fail even when they shouldn't.Return 0 on success where number of bytes read match the number of bytes requested and a negative error
-EINVALon all other cases.Fixes 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem")Signed-off-by Joy Chakraborty <[email protected]>--- drivers/nvmem/rmem.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/nvmem/rmem.c +0
-0drivers/nvmem/rmem.c static int rmem_read(void *context, unsigned int offset, · line 51
Quoted 7 lines
drivers/nvmem/rmem.c +3
-1@@ -46,7 +46,12 @@ static int rmem_read(void *context, unsigned int offset,memunmap (addr); -return count; +if (count != bytes) { +dev_err (priv->dev,"Failed read memory (%d)\n" , count); +return -EINVAL ;Why is a "short read" somehow illegal here? What internal changes need to be made now that this has changed?
Quoted 5 lines
Quoted 3 lines
In my opinion "short read" should be illegal for cases where if the nvmem core is unable to read the required size of data to fill up a nvmem cell then data returned might have truncated value.
But that's kind of against what a
read()call normally expects.
That is fair, maybe the size check should be there at the nvmem core layer to catch any truncations but the actual size read is not passed from provider to the core layer.
Quoted 9 lines
Quoted 6 lines
No internal changes should be made since the registered
reg_read()is called from__nvmem_reg_read()which eventually passes on the error code tonvmem_reg_read()whose return code is already checked and passed to nvmem consumers.
Currently rmem driver is incorrectly passing a positive value for success.So this is an internal api issue and not a general issue? Unwinding the read callbacks here is hard.
Yes, this is an internal API issue with how the return type of the reg_read() function pointer passed to nvmem_register() is handled. The function prototype in nvmem-provider.h does not define how the return value is treated in nvmem core.
"
typedef int (*nvmem_reg_read_t)(void *priv, unsigned int offset, void *val, size_t bytes);
"
Currently it is always checked against 0 for success in nvmem/core.c which all nvmem-providers adhere to i.e. return 0 on success. Actual size read from the provider is considered to be equal to the requested size from core as the provider does not relay that information.
Quoted 5 lines
Also, in looking at the code, how can this ever be a short read? You are using
memory_read_from_buffer()which unless the values passed into it are incorrect, will always return the expected read amount.
Correct, we will have an error only if the returned value from memory_read_from_buffer() is negative.
So to work with the current nvmem core implementation, I can return the value as is if negative and 0 for positive.
Quoted 15 lines
Quoted 15 lines
Quoted 1 line
And what will userspace do with this error message in the kernel log?
User space currently is not seeing this error for nvmem device/eeprom reads due to the following code at
nvmem/core.cinbin_attr_nvmem_read():
"rc = nvmem_reg_read(nvmem, pos, buf, count); if (rc) return rc; return count;"
since it expects to return the number of bytes.
Quoted 7 lines
Quoted 3 lines
Userspace will see a false error with nvmem cell reads from
nvmem_cell_attr_read()in current code, which should be fixed on returning 0 for success.So maybe fix this all up to allow the read to return the actual amount read? That feels more "correct" to me.
If I change the behavior of the nvmem_reg_read_t callback to negative for error and number of bytes actually read for success then, other than the core driver I would also have to change all the nvmem-provider drivers.
Is it okay to do so ?
Quoted 3 lines
thanks,
greg k-h
Thanks
Joy
Greg Kroah-Hartman
maintainerQuoted 8 lines · Joy Chakraborty
Quoted 7 lines
Quoted 3 lines
Userspace will see a false error with nvmem cell reads from
nvmem_cell_attr_read()in current code, which should be fixed on returning 0 for success.So maybe fix this all up to allow the read to return the actual amount read? That feels more "correct" to me.
Quoted 5 lines
If I change the behavior of the
nvmem_reg_read_tcallback to negative for error and number of bytes actually read for success then, other than the core driver I would also have to change all the nvmem-provider drivers.
Is it okay to do so ?
Sure, why not? That seems like the correct fix to me, right?
thanks,
greg k-h
Quoted 10 lines · Greg Kroah-Hartman
Quoted 8 lines · Joy Chakraborty
Quoted 7 lines
Quoted 3 lines
Userspace will see a false error with nvmem cell reads from
nvmem_cell_attr_read()in current code, which should be fixed on returning 0 for success.So maybe fix this all up to allow the read to return the actual amount read? That feels more "correct" to me.
Quoted 7 lines
Quoted 5 lines
If I change the behavior of the
nvmem_reg_read_tcallback to negative for error and number of bytes actually read for success then, other than the core driver I would also have to change all the nvmem-provider drivers.
Is it okay to do so ?Sure, why not? That seems like the correct fix to me, right?
Sure, I can do that.
Is it okay to change the if checks on the return code to "if (rc < 0)" instead of "if (rc)" as a fix for the immediate issue with how return value from rmem is handled which can be applied to older kernels. In a separate patch I can change the definition of nvmem_reg_read_t() to return ssize_t instead of int and make corresponding changes to nvmem-provider drivers.
Does that sound okay ?
Quoted 4 lines
thanks,
greg k-h
Thanks
Joy
Quoted 23 lines · Joy Chakraborty
Quoted 10 lines · Greg Kroah-Hartman
Quoted 8 lines · Joy Chakraborty
Quoted 7 lines
Quoted 3 lines
Userspace will see a false error with nvmem cell reads from
nvmem_cell_attr_read()in current code, which should be fixed on returning 0 for success.So maybe fix this all up to allow the read to return the actual amount read? That feels more "correct" to me.
Quoted 7 lines
Quoted 5 lines
If I change the behavior of the
nvmem_reg_read_tcallback to negative for error and number of bytes actually read for success then, other than the core driver I would also have to change all the nvmem-provider drivers.
Is it okay to do so ?Sure, why not? That seems like the correct fix to me, right?
Sure, I can do that.
Quoted 8 lines
Is it okay to change the if checks on the return code to "if (rc < 0)" instead of "if (rc)" as a fix for the immediate issue with how return value from rmem is handled which can be applied to older kernels. In a separate patch I can change the definition of
nvmem_reg_read_t()to returnssize_tinstead of int and make corresponding changes to nvmem-provider drivers.Does that sound okay ?
Hi Greg,
Sent a patch https://lore.kernel.org/all/[email protected]/
to change the return type for read/write callbacks.
Do I mark that with the "Fixes:" tag as well ?
It affects a lot of files so might not be able to easily pick to an older kernel when needed.
Thanks
Joy
Quoted 7 lines
Quoted 4 lines
thanks,
greg k-h
Thanks
Joy
Greg Kroah-Hartman
maintainerQuoted 31 lines · Joy Chakraborty
Quoted 30 lines · Joy Chakraborty
Quoted 10 lines · Greg Kroah-Hartman
Quoted 8 lines · Joy Chakraborty
Quoted 7 lines
Quoted 3 lines
Userspace will see a false error with nvmem cell reads from
nvmem_cell_attr_read()in current code, which should be fixed on returning 0 for success.So maybe fix this all up to allow the read to return the actual amount read? That feels more "correct" to me.
Quoted 7 lines
Quoted 5 lines
If I change the behavior of the
nvmem_reg_read_tcallback to negative for error and number of bytes actually read for success then, other than the core driver I would also have to change all the nvmem-provider drivers.
Is it okay to do so ?Sure, why not? That seems like the correct fix to me, right?
Sure, I can do that.
Is it okay to change the if checks on the return code to "if (rc < 0)" instead of "if (rc)" as a fix for the immediate issue with how return value from rmem is handled which can be applied to older kernels. In a separate patch I can change the definition of
nvmem_reg_read_t()to returnssize_tinstead of int and make corresponding changes to nvmem-provider drivers.
Quoted 7 lines
Quoted 1 line
Does that sound okay ?
Hi Greg,
Sent a patch https://lore.kernel.org/all/[email protected]/
to change the return type for read/write callbacks.
Do I mark that with the "Fixes:" tag as well ?
Is it actually fixing a bug? Or just evolving the api to be more correct over time? I think the latter.
Quoted 2 lines
It affects a lot of files so might not be able to easily pick to an older kernel when needed.
What is it fixing that older kernels need?
And do not worry about stable kernels while doing development, only take that into consideration after your changes are accepted.
thanks,
greg k-h
Quoted 35 lines · Greg Kroah-Hartman
Quoted 33 lines · Joy Chakraborty
Quoted 31 lines · Joy Chakraborty
Quoted 10 lines · Greg Kroah-Hartman
Quoted 8 lines · Joy Chakraborty
Quoted 7 lines
Quoted 3 lines
Userspace will see a false error with nvmem cell reads from
nvmem_cell_attr_read()in current code, which should be fixed on returning 0 for success.So maybe fix this all up to allow the read to return the actual amount read? That feels more "correct" to me.
Quoted 7 lines
Quoted 5 lines
If I change the behavior of the
nvmem_reg_read_tcallback to negative for error and number of bytes actually read for success then, other than the core driver I would also have to change all the nvmem-provider drivers.
Is it okay to do so ?Sure, why not? That seems like the correct fix to me, right?
Sure, I can do that.
Is it okay to change the if checks on the return code to "if (rc < 0)" instead of "if (rc)" as a fix for the immediate issue with how return value from rmem is handled which can be applied to older kernels. In a separate patch I can change the definition of
nvmem_reg_read_t()to returnssize_tinstead of int and make corresponding changes to nvmem-provider drivers.Does that sound okay ?
Quoted 8 lines
Quoted 5 lines
Hi Greg,
Sent a patch https://lore.kernel.org/all/[email protected]/
to change the return type for read/write callbacks.
Do I mark that with the "Fixes:" tag as well ?Is it actually fixing a bug? Or just evolving the api to be more correct over time? I think the latter.
It is more of the latter i.e. evolving the API to be correct but indirectly it ends up fixing this bug where positive value returned by this rmem driver is treated as an error in nvmem core.
Quoted 5 lines
Quoted 3 lines
It affects a lot of files so might not be able to easily pick to an older kernel when needed.
What is it fixing that older kernels need?
It is fixing the handling of a positive value returned by this rmem driver which will be treated as an error in older kernels as the nvmem core expects 0 on success without changing API behavior.
Quoted 3 lines
And do not worry about stable kernels while doing development, only take that into consideration after your changes are accepted.
Got it, thank you for your input .
Quoted 4 lines
thanks,
greg k-h
Thanks
Joy
Quoted 19 lines · Joy Chakraborty
reg_read()callback registered with nvmem core expects an integer error as a return value butrmem_read()returns the number of bytes read, as a result error checks in nvmem core fail even when they shouldn't.Return 0 on success where number of bytes read match the number of bytes requested and a negative error
-EINVALon all other cases.Fixes 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem")Signed-off-by Joy Chakraborty <[email protected]>--- drivers/nvmem/rmem.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/nvmem/rmem.c +0
-0
drivers/nvmem/rmem.c static int rmem_read(void *context, unsigned int offset, · line 49
Quoted 5 lines
drivers/nvmem/rmem.c +1
-1@@ -46,7 +46,12 @@ static int rmem_read(void *context, unsigned int offset,memunmap (addr); -return count; +if (count != bytes) {
How can this fail unless the values set in priv->mem->size is incorrect
Only case I see this failing with short reads is when offset cross the boundary of priv->mem->size.
can you provide more details on the failure usecase, may be with actual values of offsets, bytes and priv->mem->size?
drivers/nvmem/rmem.c static int rmem_read(void *context, unsigned int offset,
Quoted 6 lines
drivers/nvmem/rmem.c +5
-0+dev_err (priv->dev,"Failed read memory (%d)\n" , count); +return -EINVAL ; + } + +return 0 ;
thanks,
srini
Quoted 3 lines
drivers/nvmem/rmem.c +0
-0}static int
rmem_probe(struct platform_device *pdev)
Quoted 24 lines · Srinivas Kandagatla
Quoted 20 lines · Joy Chakraborty
reg_read()callback registered with nvmem core expects an integer error as a return value butrmem_read()returns the number of bytes read, as a result error checks in nvmem core fail even when they shouldn't.Return 0 on success where number of bytes read match the number of bytes requested and a negative error
-EINVALon all other cases.Fixes 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem")Signed-off-by Joy Chakraborty <[email protected]>--- drivers/nvmem/rmem.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/nvmem/rmem.c +0
-0@@ -46,7 +46,12 @@ static int rmem_read(void *context, unsigned int offset,
Quoted 7 lines
drivers/nvmem/rmem.c static int rmem_read(void *context, unsigned int offset, · line 49
Quoted 4 lines
drivers/nvmem/rmem.c +1
-1memunmap (addr); -return count; +if (count != bytes) {How can this fail unless the values set in
priv->mem->sizeis incorrect
That should be correct since it would be fetched from the reserved memory definition in the device tree.
Quoted 7 lines
Only case I see this failing with short reads is when offset cross the boundary of
priv->mem->size.can you provide more details on the failure usecase, may be with actual values of offsets, bytes and
priv->mem->size?
This could very well happen if a fixed-layout defined for the reserved memory has a cell which defines an offset and size greater than the actual size of the reserved mem.
For E.g. if the device tree node is as follows
reserved-memory {
#address-cells = <1>;
#size-cells = <1>;
ranges;
nvmem@1000 {
compatible = "nvmem-rmem";
reg = <0x1000 0x400>;
no-map;
nvmem-layout {
compatible = "fixed-layout";
#address-cells = <1>;
#size-cells = <1>;
calibration@13ff {
reg = <0x13ff 0x2>;
};
};
};
};If we try to read the cell "calibration" which crosses the boundary of the reserved memory then it will lead to a short read. Though, one might argue that the protection against such cell definition should be there during fixed-layout parsing in core itself but that is not there now and would not be a fix.
What I am trying to fix here is not exactly short reads but how the return value of rmem_read() is treated by the nvmem core, where it treats a non-zero return from read as an error currently. Hence returning the number of bytes read leads to false failures if we try to read a cell.
Quoted 14 lines
drivers/nvmem/rmem.c static int rmem_read(void *context, unsigned int offset,
Quoted 7 lines
drivers/nvmem/rmem.c +5
-0+dev_err (priv->dev,"Failed read memory (%d)\n" , count); +return -EINVAL ; + } + +return 0 ;thanks,
sriniQuoted 3 lines
drivers/nvmem/rmem.c +0
-0}static int
rmem_probe(struct platform_device *pdev)
Quoted 41 lines · Joy Chakraborty
Quoted 24 lines · Srinivas Kandagatla
Quoted 20 lines · Joy Chakraborty
reg_read()callback registered with nvmem core expects an integer error as a return value butrmem_read()returns the number of bytes read, as a result error checks in nvmem core fail even when they shouldn't.Return 0 on success where number of bytes read match the number of bytes requested and a negative error
-EINVALon all other cases.Fixes 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem")Signed-off-by Joy Chakraborty <[email protected]>--- drivers/nvmem/rmem.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/nvmem/rmem.c +0
-0@@ -46,7 +46,12 @@ static int rmem_read(void *context, unsigned int offset,Quoted 7 lines
drivers/nvmem/rmem.c static int rmem_read(void *context, unsigned int offset, · line 49
Quoted 4 lines
drivers/nvmem/rmem.c +1
-1memunmap (addr); -return count; +if (count != bytes) {How can this fail unless the values set in
priv->mem->sizeis incorrectThat should be correct since it would be fetched from the reserved memory definition in the device tree.
Quoted 4 lines
Only case I see this failing with short reads is when offset cross the boundary of
priv->mem->size.
Quoted 7 lines
Quoted 3 lines
can you provide more details on the failure usecase, may be with actual values of offsets, bytes and
priv->mem->size?This could very well happen if a fixed-layout defined for the reserved memory has a cell which defines an offset and size greater than the actual size of the reserved mem.
No that should just be blocked from core layer, atleast which is what is checked bin_attr_nvmem_read(), if checks are missing in other places then that needs fixing.
Quoted 15 lines
For E.g. if the device tree node is as follows
reserved-memory { #address-cells = <1>; #size-cells = <1>; ranges; nvmem@1000 { compatible = "nvmem-rmem"; reg = <0x1000 0x400>; no-map; nvmem-layout { compatible = "fixed-layout"; #address-cells = <1>; #size-cells = <1>; calibration@13ff { reg = <0x13ff 0x2>;
this is out of range, core should just err out.
--srini
Quoted 31 lines
}; }; }; };If we try to read the cell "calibration" which crosses the boundary of the reserved memory then it will lead to a short read. Though, one might argue that the protection against such cell definition should be there during fixed-layout parsing in core itself but that is not there now and would not be a fix.
What I am trying to fix here is not exactly short reads but how the return value of
rmem_read()is treated by the nvmem core, where it treats a non-zero return from read as an error currently. Hence returning the number of bytes read leads to false failures if we try to read a cell.Quoted 14 lines
drivers/nvmem/rmem.c static int rmem_read(void *context, unsigned int offset,
Quoted 7 lines
drivers/nvmem/rmem.c +5
-0+dev_err (priv->dev,"Failed read memory (%d)\n" , count); +return -EINVAL ; + } + +return 0 ;thanks,
sriniQuoted 3 lines
drivers/nvmem/rmem.c +0
-0}static int
rmem_probe(struct platform_device *pdev)
Quoted 49 lines · Srinivas Kandagatla
Quoted 45 lines · Joy Chakraborty
Quoted 24 lines · Srinivas Kandagatla
Quoted 20 lines · Joy Chakraborty
reg_read()callback registered with nvmem core expects an integer error as a return value butrmem_read()returns the number of bytes read, as a result error checks in nvmem core fail even when they shouldn't.Return 0 on success where number of bytes read match the number of bytes requested and a negative error
-EINVALon all other cases.Fixes 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem")Signed-off-by Joy Chakraborty <[email protected]>--- drivers/nvmem/rmem.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/nvmem/rmem.c +0
-0@@ -46,7 +46,12 @@ static int rmem_read(void *context, unsigned int offset,Quoted 7 lines
drivers/nvmem/rmem.c static int rmem_read(void *context, unsigned int offset, · line 49
Quoted 4 lines
drivers/nvmem/rmem.c +1
-1memunmap (addr); -return count; +if (count != bytes) {How can this fail unless the values set in
priv->mem->sizeis incorrectThat should be correct since it would be fetched from the reserved memory definition in the device tree.
Quoted 7 lines
Only case I see this failing with short reads is when offset cross the boundary of
priv->mem->size.can you provide more details on the failure usecase, may be with actual values of offsets, bytes and
priv->mem->size?
Quoted 8 lines
Quoted 3 lines
This could very well happen if a fixed-layout defined for the reserved memory has a cell which defines an offset and size greater than the actual size of the reserved mem.
No that should just be blocked from core layer, atleast which is what is checked
bin_attr_nvmem_read(), if checks are missing in other places then that needs fixing.
Sure.
Quoted 17 lines
Quoted 16 lines
For E.g. if the device tree node is as follows
reserved-memory { #address-cells = <1>; #size-cells = <1>; ranges; nvmem@1000 { compatible = "nvmem-rmem"; reg = <0x1000 0x400>; no-map; nvmem-layout { compatible = "fixed-layout"; #address-cells = <1>; #size-cells = <1>; calibration@13ff { reg = <0x13ff 0x2>;
Quoted 2 lines
this is out of range, core should just err out.
Cells are currently unchecked, I can fix that in a different patch.
Quoted 33 lines
--srini
Quoted 31 lines
}; }; }; };If we try to read the cell "calibration" which crosses the boundary of the reserved memory then it will lead to a short read. Though, one might argue that the protection against such cell definition should be there during fixed-layout parsing in core itself but that is not there now and would not be a fix.
What I am trying to fix here is not exactly short reads but how the return value of
rmem_read()is treated by the nvmem core, where it treats a non-zero return from read as an error currently. Hence returning the number of bytes read leads to false failures if we try to read a cell.Quoted 14 lines
drivers/nvmem/rmem.c static int rmem_read(void *context, unsigned int offset,
Quoted 7 lines
drivers/nvmem/rmem.c +5
-0+dev_err (priv->dev,"Failed read memory (%d)\n" , count); +return -EINVAL ; + } + +return 0 ;thanks,
sriniQuoted 3 lines
drivers/nvmem/rmem.c +0
-0}static int
rmem_probe(struct platform_device *pdev)
Thanks
Joy