Change nvmem read/write function definition return type to ssize_t.
Signed-off-by: Joy Chakraborty <joychakr@google.com>
---
drivers/misc/eeprom/at25.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
index 595ceb9a7126..73d60f80aea8 100644
--- a/drivers/misc/eeprom/at25.c
+++ b/drivers/misc/eeprom/at25.c
@@ -74,8 +74,8 @@ struct at25_data {
#define io_limit PAGE_SIZE /* bytes */
-static int at25_ee_read(void *priv, unsigned int offset,
- void *val, size_t count)
+static ssize_t at25_ee_read(void *priv, unsigned int offset,
+ void *val, size_t count)
{
struct at25_data *at25 = priv;
char *buf = val;
@@ -147,7 +147,7 @@ static int at25_ee_read(void *priv, unsigned int offset,
dev_dbg(&at25->spi->dev, "read %zu bytes at %d\n",
count, offset);
- return 0;
+ return count;
}
/* Read extra registers as ID or serial number */
@@ -195,10 +195,11 @@ static struct attribute *sernum_attrs[] = {
};
ATTRIBUTE_GROUPS(sernum);
-static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
+static ssize_t at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
{
struct at25_data *at25 = priv;
size_t maxsz = spi_max_transfer_size(at25->spi);
+ size_t bytes_written = count;
const char *buf = val;
int status = 0;
unsigned buf_size;
@@ -313,7 +314,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
mutex_unlock(&at25->lock);
kfree(bounce);
- return status;
+ return status < 0 ? status : bytes_written;
}
/*-------------------------------------------------------------------------*/
--
2.45.1.467.gbab1589fc0-googOn Wed, Jun 05, 2024 at 05:59:51PM +0000, Joy Chakraborty wrote:
> @@ -195,10 +195,11 @@ static struct attribute *sernum_attrs[] = {
> };
> ATTRIBUTE_GROUPS(sernum);
>
> -static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
> +static ssize_t at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
> {
> struct at25_data *at25 = priv;
> size_t maxsz = spi_max_transfer_size(at25->spi);
> + size_t bytes_written = count;
> const char *buf = val;
> int status = 0;
> unsigned buf_size;
> @@ -313,7 +314,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
> mutex_unlock(&at25->lock);
>
> kfree(bounce);
> - return status;
> + return status < 0 ? status : bytes_written;
> }
So the original bug was that rmem_read() is returning positive values
on success instead of zero[1]. That started a discussion about partial
reads which resulted in changing the API to support partial reads[2].
That patchset broke the build. This patchset is trying to fix the
build breakage.
[1] https://lore.kernel.org/all/20240206042408.224138-1-joychakr@google.com/
[2] https://lore.kernel.org/all/20240510082929.3792559-2-joychakr@google.com/
The bug in rmem_read() is still not fixed. That needs to be fixed as
a stand alone patch. We can discuss re-writing the API separately.
These functions are used internally and exported to the user through
sysfs via bin_attr_nvmem_read/write(). For internal users partial reads
should be treated as failure. What are we supposed to do with a partial
read? I don't think anyone has asked for partial reads to be supported
from sysfs either except Greg was wondering about it while reading the
code.
Currently, a lot of drivers return -EINVAL for partial read/writes but
some return success. It is a bit messy. But this patchset doesn't
really improve anything. In at24_read() we check if it's going to be a
partial read and return -EINVAL. Below we report a partial read as a
full read. It's just a more complicated way of doing exactly what we
were doing before.
drivers/misc/eeprom/at25.c
198 static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
199 {
200 struct at25_data *at25 = priv;
201 size_t maxsz = spi_max_transfer_size(at25->spi);
New: size_t bytes_written = count;
^^^^^^^^^^^^^^^^^^^^^
This is not the number of bytes written.
202 const char *buf = val;
203 int status = 0;
204 unsigned buf_size;
205 u8 *bounce;
206
207 if (unlikely(off >= at25->chip.byte_len))
208 return -EFBIG;
209 if ((off + count) > at25->chip.byte_len)
210 count = at25->chip.byte_len - off;
^^^^^
This is.
211 if (unlikely(!count))
212 return -EINVAL;
213
214 /* Temp buffer starts with command and address */
215 buf_size = at25->chip.page_size;
216 if (buf_size > io_limit)
217 buf_size = io_limit;
218 bounce = kmalloc(buf_size + at25->addrlen + 1, GFP_KERNEL);
219 if (!bounce)
220 return -ENOMEM;
221
regards,
dan carpenter
On 06/06/2024 09:41, Dan Carpenter wrote: > So the original bug was that rmem_read() is returning positive values > on success instead of zero[1]. That started a discussion about partial > reads which resulted in changing the API to support partial reads[2]. > That patchset broke the build. This patchset is trying to fix the > build breakage. > > [1]https://lore.kernel.org/all/20240206042408.224138-1-joychakr@google.com/ > [2]https://lore.kernel.org/all/20240510082929.3792559-2-joychakr@google.com/ > > The bug in rmem_read() is still not fixed. That needs to be fixed as > a stand alone patch. We can discuss re-writing the API separately. I agree with Dan, Lets fix the rmem_read and start working on the API rework in parallel. Am happy to pick the [1]. --srini
On Thu, Jun 6, 2024 at 2:11 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Wed, Jun 05, 2024 at 05:59:51PM +0000, Joy Chakraborty wrote:
> > @@ -195,10 +195,11 @@ static struct attribute *sernum_attrs[] = {
> > };
> > ATTRIBUTE_GROUPS(sernum);
> >
> > -static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
> > +static ssize_t at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
> > {
> > struct at25_data *at25 = priv;
> > size_t maxsz = spi_max_transfer_size(at25->spi);
> > + size_t bytes_written = count;
> > const char *buf = val;
> > int status = 0;
> > unsigned buf_size;
> > @@ -313,7 +314,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
> > mutex_unlock(&at25->lock);
> >
> > kfree(bounce);
> > - return status;
> > + return status < 0 ? status : bytes_written;
> > }
>
> So the original bug was that rmem_read() is returning positive values
> on success instead of zero[1]. That started a discussion about partial
> reads which resulted in changing the API to support partial reads[2].
> That patchset broke the build. This patchset is trying to fix the
> build breakage.
>
> [1] https://lore.kernel.org/all/20240206042408.224138-1-joychakr@google.com/
> [2] https://lore.kernel.org/all/20240510082929.3792559-2-joychakr@google.com/
>
> The bug in rmem_read() is still not fixed. That needs to be fixed as
> a stand alone patch. We can discuss re-writing the API separately.
>
True, fixing the return type would fix that as well is what I thought
but maybe yes we need to fix that separately as well.
> These functions are used internally and exported to the user through
> sysfs via bin_attr_nvmem_read/write(). For internal users partial reads
> should be treated as failure. What are we supposed to do with a partial
> read? I don't think anyone has asked for partial reads to be supported
> from sysfs either except Greg was wondering about it while reading the
> code.
>
> Currently, a lot of drivers return -EINVAL for partial read/writes but
> some return success. It is a bit messy. But this patchset doesn't
> really improve anything. In at24_read() we check if it's going to be a
> partial read and return -EINVAL. Below we report a partial read as a
> full read. It's just a more complicated way of doing exactly what we
> were doing before.
Currently what drivers return is up to their interpretation of int
return type, there are a few drivers which also return the number of
bytes written/read already like
drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c .
The objective of the patch was to handle partial reads and errors at
the nvmem core and instead of leaving it up to each nvmem provider by
providing a better return value to nvmem providers.
Regarding drivers/misc/eeprom/at25.c which you pointed below, that is
a problem in my code change. I missed that count was modified later on
and should initialize bytes_written to the new value of count, will
fix that when I come up with the new patch.
I agree that it does not improve anything for a lot of nvmem providers
for example the ones which call into other reg_map_read/write apis
which do not return the number of bytes read/written but it does help
us do better error handling at the nvmem core layer for nvmem
providers who can return the valid number of bytes read/written.
Please let me know if you have any other suggestions on how to handle
this better.
Thanks
Joy
>
> drivers/misc/eeprom/at25.c
> 198 static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
> 199 {
> 200 struct at25_data *at25 = priv;
> 201 size_t maxsz = spi_max_transfer_size(at25->spi);
> New: size_t bytes_written = count;
> ^^^^^^^^^^^^^^^^^^^^^
> This is not the number of bytes written.
>
> 202 const char *buf = val;
> 203 int status = 0;
> 204 unsigned buf_size;
> 205 u8 *bounce;
> 206
> 207 if (unlikely(off >= at25->chip.byte_len))
> 208 return -EFBIG;
> 209 if ((off + count) > at25->chip.byte_len)
> 210 count = at25->chip.byte_len - off;
> ^^^^^
> This is.
>
> 211 if (unlikely(!count))
> 212 return -EINVAL;
> 213
> 214 /* Temp buffer starts with command and address */
> 215 buf_size = at25->chip.page_size;
> 216 if (buf_size > io_limit)
> 217 buf_size = io_limit;
> 218 bounce = kmalloc(buf_size + at25->addrlen + 1, GFP_KERNEL);
> 219 if (!bounce)
> 220 return -ENOMEM;
> 221
>
> regards,
> dan carpenter
On Thu, Jun 06, 2024 at 03:12:03PM +0530, Joy Chakraborty wrote: > > These functions are used internally and exported to the user through > > sysfs via bin_attr_nvmem_read/write(). For internal users partial reads > > should be treated as failure. What are we supposed to do with a partial > > read? I don't think anyone has asked for partial reads to be supported > > from sysfs either except Greg was wondering about it while reading the > > code. > > > > Currently, a lot of drivers return -EINVAL for partial read/writes but > > some return success. It is a bit messy. But this patchset doesn't > > really improve anything. In at24_read() we check if it's going to be a > > partial read and return -EINVAL. Below we report a partial read as a > > full read. It's just a more complicated way of doing exactly what we > > were doing before. > > Currently what drivers return is up to their interpretation of int > return type, there are a few drivers which also return the number of > bytes written/read already like > drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c . Returning non-zero is a bug. It won't break bin_attr_nvmem_read/write() but it will break other places like nvmem_access_with_keepouts(), __nvmem_cell_read() and nvmem_cell_prepare_write_buffer() where all non-zero returns from nvmem_reg_read() are treated as an error. > The objective of the patch was to handle partial reads and errors at > the nvmem core and instead of leaving it up to each nvmem provider by > providing a better return value to nvmem providers. > > Regarding drivers/misc/eeprom/at25.c which you pointed below, that is > a problem in my code change. I missed that count was modified later on > and should initialize bytes_written to the new value of count, will > fix that when I come up with the new patch. > > I agree that it does not improve anything for a lot of nvmem providers > for example the ones which call into other reg_map_read/write apis > which do not return the number of bytes read/written but it does help > us do better error handling at the nvmem core layer for nvmem > providers who can return the valid number of bytes read/written. If we're going to support partial writes, then it needs to be done all the way. We need to audit functions like at24_read() and remove the -EINVAL lines. 440 if (off + count > at24->byte_len) 441 return -EINVAL; It should be: if (off + count > at24->byte_len) count = at24->byte_len - off; Some drivers handle writing zero bytes as -EINVAL and some return 0. Those changes could be done before we change the API. You updated nvmem_access_with_keepouts() to handle negative returns but not zero returns so it could lead to a forever loop. regards, dan carpenter
On Thu, Jun 6, 2024 at 3:41 PM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > On Thu, Jun 06, 2024 at 03:12:03PM +0530, Joy Chakraborty wrote: > > > These functions are used internally and exported to the user through > > > sysfs via bin_attr_nvmem_read/write(). For internal users partial reads > > > should be treated as failure. What are we supposed to do with a partial > > > read? I don't think anyone has asked for partial reads to be supported > > > from sysfs either except Greg was wondering about it while reading the > > > code. > > > > > > Currently, a lot of drivers return -EINVAL for partial read/writes but > > > some return success. It is a bit messy. But this patchset doesn't > > > really improve anything. In at24_read() we check if it's going to be a > > > partial read and return -EINVAL. Below we report a partial read as a > > > full read. It's just a more complicated way of doing exactly what we > > > were doing before. > > > > Currently what drivers return is up to their interpretation of int > > return type, there are a few drivers which also return the number of > > bytes written/read already like > > drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c . > > Returning non-zero is a bug. It won't break bin_attr_nvmem_read/write() > but it will break other places like nvmem_access_with_keepouts(), > __nvmem_cell_read() and nvmem_cell_prepare_write_buffer() where all > non-zero returns from nvmem_reg_read() are treated as an error. > Yes, I will resend the patch to fix that. > > The objective of the patch was to handle partial reads and errors at > > the nvmem core and instead of leaving it up to each nvmem provider by > > providing a better return value to nvmem providers. > > > > Regarding drivers/misc/eeprom/at25.c which you pointed below, that is > > a problem in my code change. I missed that count was modified later on > > and should initialize bytes_written to the new value of count, will > > fix that when I come up with the new patch. > > > > I agree that it does not improve anything for a lot of nvmem providers > > for example the ones which call into other reg_map_read/write apis > > which do not return the number of bytes read/written but it does help > > us do better error handling at the nvmem core layer for nvmem > > providers who can return the valid number of bytes read/written. > > If we're going to support partial writes, then it needs to be done all > the way. We need to audit functions like at24_read() and remove the > -EINVAL lines. > > 440 if (off + count > at24->byte_len) > 441 return -EINVAL; > > It should be: > > if (off + count > at24->byte_len) > count = at24->byte_len - off; > > Some drivers handle writing zero bytes as -EINVAL and some return 0. > Those changes could be done before we change the API. > Sure, we can do it in a phased manner like you suggested in another reply by creating new pointers and slowly moving each driver to the new pointer and then deprecating the old one. > You updated nvmem_access_with_keepouts() to handle negative returns but > not zero returns so it could lead to a forever loop. > Yes, that is a possible case. Will rework it. > regards, > dan carpenter > Thanks Joy
© 2016 - 2026 Red Hat, Inc.