On Wed, Sep 23, 2020 at 04:51:06PM +0100, Srinivas Kandagatla wrote: > > > On 23/09/2020 15:51, Vadym Kochan wrote: > > > - return nvmem_cell_write(&cell, buf, cell.bytes); > > > + rc = nvmem_cell_write(&cell, buf, cell.bytes); > > > + if (rc) > > > + kfree_const(cell->name); > > > + > > > + return rc; > > > } > > > EXPORT_SYMBOL_GPL(nvmem_device_cell_write); > > > ------------------------>cut<--------------------------- > > > > > > --srini > > > > > But is it really needed to kstrdup(cell->name) for > > nvmem_device_cell_{read,write} ? > This boils down to if we want to use same api to parse nvmem_cell_info or > not! > > If we want to keep this simple, we can either explicitly add free for > successful caller to nvmem_cell_info_to_nvmem_cell()! >
I think that such additional kfree_const(cell->name) handling adds more complexity for error handling, also to my understanding usually resource allocation should be done in the called func in case of error was returned. > Or > > use something like what you did, but new api needs more clarity! > May be renaming __nvmem_cell_info_to_nvmem_cell to > nvmem_cell_info_to_nvmem_cell_no_alloc would clarify that a bit! > Yes, I agree that naming should be better, actually "__" already points to it's unsafety (no kstrdup() is used), but of course additional suffix would be better. > Also can you make sure that linewrapping on function names be inline with > existing code. You mean do not do such func attributes breaking as I did (moved them line upper) ? > > Please send v3 with that changes! > > > --srini > > It is used only for log error in case the unaligned access did not > > pass the check