On Thu, Jun 6, 2024 at 3:41 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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
