| Message ID | 20240605175953.2613260-1-joychakr@google.com |
|---|---|
| Headers | show |
| Series | nvmem: Handle change of return type in reg_read/write() definition | expand |
On 05/06/2024 18:59, Joy Chakraborty wrote: > Currently the nvmem core change is picked on > https://git.kernel.org/pub/scm/linux/kernel/git/srini/nvmem.git/log/?h=for-next This patch is reverted due to next build failures. Please resend the series with this included. --srini
On 6/5/24 10:59, Joy Chakraborty wrote: > Change nvmem read/write function definition return type to ssize_t. > > Signed-off-by: Joy Chakraborty <joychakr@google.com> > --- > drivers/misc/ds1682.c | 16 ++++++---------- > 1 file changed, 6 insertions(+), 10 deletions(-) > > diff --git a/drivers/misc/ds1682.c b/drivers/misc/ds1682.c > index 5f8dcd0e3848..953341666ddb 100644 > --- a/drivers/misc/ds1682.c > +++ b/drivers/misc/ds1682.c > @@ -198,26 +198,22 @@ static const struct bin_attribute ds1682_eeprom_attr = { > .write = ds1682_eeprom_write, > }; > > -static int ds1682_nvmem_read(void *priv, unsigned int offset, void *val, > - size_t bytes) > +static ssize_t ds1682_nvmem_read(void *priv, unsigned int offset, void *val, > + size_t bytes) > { > struct i2c_client *client = priv; > - int ret; > > - ret = i2c_smbus_read_i2c_block_data(client, DS1682_REG_EEPROM + offset, > + return i2c_smbus_read_i2c_block_data(client, DS1682_REG_EEPROM + offset, > bytes, val); > - return ret < 0 ? ret : 0; > } > > -static int ds1682_nvmem_write(void *priv, unsigned int offset, void *val, > - size_t bytes) > +static ssize_t ds1682_nvmem_write(void *priv, unsigned int offset, void *val, > + size_t bytes) > { > struct i2c_client *client = priv; > - int ret; > > - ret = i2c_smbus_write_i2c_block_data(client, DS1682_REG_EEPROM + offset, > + return i2c_smbus_write_i2c_block_data(client, DS1682_REG_EEPROM + offset, > bytes, val); i2c_smbus_write_i2c_block_data() does not return the number of bytes written. It returns either 0 or an error code. Guenter
On 6/5/24 10:59, Joy Chakraborty wrote: > Change nvmem read/write function definition return type to ssize_t. > > Signed-off-by: Joy Chakraborty <joychakr@google.com> > --- > drivers/hwmon/pmbus/adm1266.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c > index 2c4d94cc8729..7eaab5a7b04c 100644 > --- a/drivers/hwmon/pmbus/adm1266.c > +++ b/drivers/hwmon/pmbus/adm1266.c > @@ -375,7 +375,7 @@ static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff) > return 0; > } > > -static int adm1266_nvmem_read(void *priv, unsigned int offset, void *val, size_t bytes) > +static ssize_t adm1266_nvmem_read(void *priv, unsigned int offset, void *val, size_t bytes) > { > struct adm1266_data *data = priv; > int ret; > @@ -395,7 +395,7 @@ static int adm1266_nvmem_read(void *priv, unsigned int offset, void *val, size_t > > memcpy(val, data->dev_mem + offset, bytes); > > - return 0; > + return bytes; > } > > static int adm1266_config_nvmem(struct adm1266_data *data) The series doesn't explain what a driver is supposed to do if it only transfers part of the data but not all of it due to an error, or because the request exceeded the size of the media. For example, this driver still returns an error code if it successfully transferred some data but not all of it, or if more data was requested than is available. I didn't check other drivers, but I would assume that many of them have the same or a similar problem. Guenter
On Wed, Jun 05, 2024 at 05:59:45PM +0000, Joy Chakraborty wrote: > Change nvmem read/write function definition return type to ssize_t. > > Signed-off-by: Joy Chakraborty <joychakr@google.com> > --- > drivers/hwmon/pmbus/adm1266.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c > index 2c4d94cc8729..7eaab5a7b04c 100644 > --- a/drivers/hwmon/pmbus/adm1266.c > +++ b/drivers/hwmon/pmbus/adm1266.c > @@ -375,7 +375,7 @@ static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff) > return 0; > } > > -static int adm1266_nvmem_read(void *priv, unsigned int offset, void *val, size_t bytes) > +static ssize_t adm1266_nvmem_read(void *priv, unsigned int offset, void *val, size_t bytes) > { > struct adm1266_data *data = priv; > int ret; > @@ -395,7 +395,7 @@ static int adm1266_nvmem_read(void *priv, unsigned int offset, void *val, size_t > > memcpy(val, data->dev_mem + offset, bytes); > > - return 0; > + return bytes; > } This breaks the build so it's not allowed. The way to do it is to: 1) add a new pointer which takes a ssize_t 2) convert everything to the new pointer 3) Rename the new pointer to the old name regards, dan carpenter
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. 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 Thu, Jun 6, 2024 at 2:59 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On 6/5/24 10:59, Joy Chakraborty wrote: > > Change nvmem read/write function definition return type to ssize_t. > > > > Signed-off-by: Joy Chakraborty <joychakr@google.com> > > --- > > drivers/hwmon/pmbus/adm1266.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c > > index 2c4d94cc8729..7eaab5a7b04c 100644 > > --- a/drivers/hwmon/pmbus/adm1266.c > > +++ b/drivers/hwmon/pmbus/adm1266.c > > @@ -375,7 +375,7 @@ static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff) > > return 0; > > } > > > > -static int adm1266_nvmem_read(void *priv, unsigned int offset, void *val, size_t bytes) > > +static ssize_t adm1266_nvmem_read(void *priv, unsigned int offset, void *val, size_t bytes) > > { > > struct adm1266_data *data = priv; > > int ret; > > @@ -395,7 +395,7 @@ static int adm1266_nvmem_read(void *priv, unsigned int offset, void *val, size_t > > > > memcpy(val, data->dev_mem + offset, bytes); > > > > - return 0; > > + return bytes; > > } > > > > static int adm1266_config_nvmem(struct adm1266_data *data) > > The series doesn't explain what a driver is supposed to do if it > only transfers part of the data but not all of it due to an error, > or because the request exceeded the size of the media. > This patch series is actually a follow up on https://lore.kernel.org/all/20240206042408.224138-1-joychakr@google.com/ which has now been reverted . I shall try to collate it and send it again with a better explanation. > For example, this driver still returns an error code if it successfully > transferred some data but not all of it, or if more data was requested > than is available. > > I didn't check other drivers, but I would assume that many of them > have the same or a similar problem. > > Guenter >
On Thu, Jun 6, 2024 at 2:48 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On 6/5/24 10:59, Joy Chakraborty wrote: > > Change nvmem read/write function definition return type to ssize_t. > > > > Signed-off-by: Joy Chakraborty <joychakr@google.com> > > --- > > drivers/misc/ds1682.c | 16 ++++++---------- > > 1 file changed, 6 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/misc/ds1682.c b/drivers/misc/ds1682.c > > index 5f8dcd0e3848..953341666ddb 100644 > > --- a/drivers/misc/ds1682.c > > +++ b/drivers/misc/ds1682.c > > @@ -198,26 +198,22 @@ static const struct bin_attribute ds1682_eeprom_attr = { > > .write = ds1682_eeprom_write, > > }; > > > > -static int ds1682_nvmem_read(void *priv, unsigned int offset, void *val, > > - size_t bytes) > > +static ssize_t ds1682_nvmem_read(void *priv, unsigned int offset, void *val, > > + size_t bytes) > > { > > struct i2c_client *client = priv; > > - int ret; > > > > - ret = i2c_smbus_read_i2c_block_data(client, DS1682_REG_EEPROM + offset, > > + return i2c_smbus_read_i2c_block_data(client, DS1682_REG_EEPROM + offset, > > bytes, val); > > - return ret < 0 ? ret : 0; > > } > > > > -static int ds1682_nvmem_write(void *priv, unsigned int offset, void *val, > > - size_t bytes) > > +static ssize_t ds1682_nvmem_write(void *priv, unsigned int offset, void *val, > > + size_t bytes) > > { > > struct i2c_client *client = priv; > > - int ret; > > > > - ret = i2c_smbus_write_i2c_block_data(client, DS1682_REG_EEPROM + offset, > > + return i2c_smbus_write_i2c_block_data(client, DS1682_REG_EEPROM + offset, > > bytes, val); > > i2c_smbus_write_i2c_block_data() does not return the number of bytes written. > It returns either 0 or an error code. > Ack, I see only i2c_smbus_read_i2c_block_data() returns the number of bytes read . Will fix it next revision. > Guenter >
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
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