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.
Fixes: 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem") Cc: stable@vger.kernel.org Signed-off-by: Joy Chakraborty joychakr@google.com --- drivers/nvmem/rmem.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c index 752d0bf4445e..a74dfa279ff4 100644 --- a/drivers/nvmem/rmem.c +++ b/drivers/nvmem/rmem.c @@ -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)
On Tue, Feb 06, 2024 at 04:24:08AM +0000, Joy Chakraborty wrote:
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.
Fixes: 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem") Cc: stable@vger.kernel.org Signed-off-by: Joy Chakraborty joychakr@google.com
drivers/nvmem/rmem.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c index 752d0bf4445e..a74dfa279ff4 100644 --- a/drivers/nvmem/rmem.c +++ b/drivers/nvmem/rmem.c @@ -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
On Tue, Feb 6, 2024 at 3:00 PM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Tue, Feb 06, 2024 at 04:24:08AM +0000, Joy Chakraborty wrote:
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.
Fixes: 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem") Cc: stable@vger.kernel.org Signed-off-by: Joy Chakraborty joychakr@google.com
drivers/nvmem/rmem.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c index 752d0bf4445e..a74dfa279ff4 100644 --- a/drivers/nvmem/rmem.c +++ b/drivers/nvmem/rmem.c @@ -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?
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.
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.
thanks,
greg k-h
Thanks Joy
On Tue, Feb 06, 2024 at 04:01:02PM +0530, Joy Chakraborty wrote:
On Tue, Feb 6, 2024 at 3:00 PM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Tue, Feb 06, 2024 at 04:24:08AM +0000, Joy Chakraborty wrote:
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.
Fixes: 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem") Cc: stable@vger.kernel.org Signed-off-by: Joy Chakraborty joychakr@google.com
drivers/nvmem/rmem.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c index 752d0bf4445e..a74dfa279ff4 100644 --- a/drivers/nvmem/rmem.c +++ b/drivers/nvmem/rmem.c @@ -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?
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.
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.
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.
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.
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
On Tue, Feb 6, 2024 at 4:27 PM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Tue, Feb 06, 2024 at 04:01:02PM +0530, Joy Chakraborty wrote:
On Tue, Feb 6, 2024 at 3:00 PM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Tue, Feb 06, 2024 at 04:24:08AM +0000, Joy Chakraborty wrote:
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.
Fixes: 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem") Cc: stable@vger.kernel.org Signed-off-by: Joy Chakraborty joychakr@google.com
drivers/nvmem/rmem.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c index 752d0bf4445e..a74dfa279ff4 100644 --- a/drivers/nvmem/rmem.c +++ b/drivers/nvmem/rmem.c @@ -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?
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.
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.
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.
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.
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.
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 ?
thanks,
greg k-h
Thanks Joy
On Tue, Feb 06, 2024 at 05:22:15PM +0530, Joy Chakraborty wrote:
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 ?
Sure, why not? That seems like the correct fix to me, right?
thanks,
greg k-h
On Wed, Feb 7, 2024 at 3:04 PM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Tue, Feb 06, 2024 at 05:22:15PM +0530, Joy Chakraborty wrote:
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 ?
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 ?
thanks,
greg k-h
Thanks Joy
On Wed, Feb 7, 2024 at 8:33 PM Joy Chakraborty joychakr@google.com wrote:
On Wed, Feb 7, 2024 at 3:04 PM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Tue, Feb 06, 2024 at 05:22:15PM +0530, Joy Chakraborty wrote:
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 ?
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 ?
Hi Greg,
Sent a patch https://lore.kernel.org/all/20240219113149.2437990-2-joychakr@google.com/ 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
thanks,
greg k-h
Thanks Joy
On Tue, Feb 27, 2024 at 12:27:09PM +0530, Joy Chakraborty wrote:
On Wed, Feb 7, 2024 at 8:33 PM Joy Chakraborty joychakr@google.com wrote:
On Wed, Feb 7, 2024 at 3:04 PM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Tue, Feb 06, 2024 at 05:22:15PM +0530, Joy Chakraborty wrote:
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 ?
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 ?
Hi Greg,
Sent a patch https://lore.kernel.org/all/20240219113149.2437990-2-joychakr@google.com/ 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 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
On Tue, Feb 27, 2024 at 1:02 PM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Tue, Feb 27, 2024 at 12:27:09PM +0530, Joy Chakraborty wrote:
On Wed, Feb 7, 2024 at 8:33 PM Joy Chakraborty joychakr@google.com wrote:
On Wed, Feb 7, 2024 at 3:04 PM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Tue, Feb 06, 2024 at 05:22:15PM +0530, Joy Chakraborty wrote:
> 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 ?
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 ?
Hi Greg,
Sent a patch https://lore.kernel.org/all/20240219113149.2437990-2-joychakr@google.com/ 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.
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.
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 .
thanks,
greg k-h
Thanks Joy
On 06/02/2024 04:24, Joy Chakraborty wrote:
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.
Fixes: 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem") Cc: stable@vger.kernel.org Signed-off-by: Joy Chakraborty joychakr@google.com
drivers/nvmem/rmem.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c index 752d0bf4445e..a74dfa279ff4 100644 --- a/drivers/nvmem/rmem.c +++ b/drivers/nvmem/rmem.c @@ -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?
dev_err(priv->dev, "Failed read memory (%d)\n", count);return -EINVAL;- }
- return 0;
thanks, srini
} static int rmem_probe(struct platform_device *pdev)
On Wed, Feb 7, 2024 at 4:06 AM Srinivas Kandagatla srinivas.kandagatla@linaro.org wrote:
On 06/02/2024 04:24, Joy Chakraborty wrote:
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.
Fixes: 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem") Cc: stable@vger.kernel.org Signed-off-by: Joy Chakraborty joychakr@google.com
drivers/nvmem/rmem.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c index 752d0bf4445e..a74dfa279ff4 100644 --- a/drivers/nvmem/rmem.c +++ b/drivers/nvmem/rmem.c @@ -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
That should be correct since it would be fetched from the reserved memory definition in the device tree.
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.
dev_err(priv->dev, "Failed read memory (%d)\n", count);return -EINVAL;}
return 0;thanks, srini
}
static int rmem_probe(struct platform_device *pdev)
On 07/02/2024 06:35, Joy Chakraborty wrote:
On Wed, Feb 7, 2024 at 4:06 AM Srinivas Kandagatla srinivas.kandagatla@linaro.org wrote:
On 06/02/2024 04:24, Joy Chakraborty wrote:
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.
Fixes: 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem") Cc: stable@vger.kernel.org Signed-off-by: Joy Chakraborty joychakr@google.com
drivers/nvmem/rmem.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c index 752d0bf4445e..a74dfa279ff4 100644 --- a/drivers/nvmem/rmem.c +++ b/drivers/nvmem/rmem.c @@ -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
That should be correct since it would be fetched from the reserved memory definition in the device tree.
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.
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.
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
}; }; };}; 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.
dev_err(priv->dev, "Failed read memory (%d)\n", count);return -EINVAL;}
return 0;thanks, srini
}
static int rmem_probe(struct platform_device *pdev)
On Wed, Feb 7, 2024 at 3:00 PM Srinivas Kandagatla srinivas.kandagatla@linaro.org wrote:
On 07/02/2024 06:35, Joy Chakraborty wrote:
On Wed, Feb 7, 2024 at 4:06 AM Srinivas Kandagatla srinivas.kandagatla@linaro.org wrote:
On 06/02/2024 04:24, Joy Chakraborty wrote:
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.
Fixes: 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem") Cc: stable@vger.kernel.org Signed-off-by: Joy Chakraborty joychakr@google.com
drivers/nvmem/rmem.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c index 752d0bf4445e..a74dfa279ff4 100644 --- a/drivers/nvmem/rmem.c +++ b/drivers/nvmem/rmem.c @@ -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
That should be correct since it would be fetched from the reserved memory definition in the device tree.
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.
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.
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.
Cells are currently unchecked, I can fix that in a different patch.
--srini
}; }; };}; 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.
dev_err(priv->dev, "Failed read memory (%d)\n", count);return -EINVAL;}
return 0;thanks, srini
}
static int rmem_probe(struct platform_device *pdev)
Thanks Joy
linux-stable-mirror@lists.linaro.org
-
Greg Kroah-Hartman -
Joy Chakraborty -
Srinivas Kandagatla