On Thu, Jun 6, 2024 at 2:11 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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@xxxxxxxxxx/ > [2] https://lore.kernel.org/all/20240510082929.3792559-2-joychakr@xxxxxxxxxx/ > > 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