Messages in this thread | | | Subject | Re: [PATCH v1 0/3] nvmem: Add support for write-only instances, and clean-up | From | Srinivas Kandagatla <> | Date | Tue, 25 Feb 2020 14:59:46 +0000 |
| |
On 24/02/2020 17:41, Nicholas Johnson wrote: > [Based on Linux v5.6-rc3, does not apply successfully to Linux v5.6-rc2] > > Hello all, > > I offer the first patch in this series to support write-only instances > of nvmem. The use-case is the Thunderbolt driver, for which Mika > Westerberg needs write-only nvmem. Refer to 03cd45d2e219 ("thunderbolt: > Prevent crash if non-active NVMem file is read"). >
Had a look at the crash trace from the mentioned patch.
Why can not we add a check for reg_read in bin_attr_nvmem_read() before dereferencing it?
The reason I ask this is because removing read_only is not that simple as you think. Firstly because a there is no way to derive this flag by just looking at read/write callbacks. Providers are much more generic drivers ex: at24 which can have read/write interfaces implemented, however read only flag is enforced at board/platform level config either via device tree property bindings or a write protection gpio. Removing this is also going to break the device tree bindings.
only alternative I can see ATM is the mentioned check.
--srini
> The second patch in this series reverts the workaround 03cd45d2e219 > ("thunderbolt: Prevent crash if non-active NVMem file is read") which > Mika applied in the mean time to prevent a kernel-mode NULL dereference. > If Mika wants to do this himself or there is some reason not to apply > this, that is fine, but in my mind, it is a logical progression to apply > one after the other in the same series. > > The third patch in this series removes the .read_only field, because we > do not have a .write_only field. It only makes sense to have both or > neither. Having either of them makes it hard to be consistent - what > happens if a driver were to set both .read_only and .write_only? And > there is the question of deciding if the nvmem is read-only because of > the .read_only, or based on if the .reg_read is not NULL. What if they > disagree? This patch does touch a lot of files, and I will understand if > you do not wish to apply it. It is optional and does not need to be > applied with the first two. > > Thank you in advance for reviewing these. > > Kind regards, > > Nicholas Johnson (3): > nvmem: Add support for write-only instances > Revert "thunderbolt: Prevent crash if non-active NVMem file is read" > nvmem: Remove .read_only field from nvmem_config > > drivers/misc/eeprom/at24.c | 4 +- > drivers/misc/eeprom/at25.c | 4 +- > drivers/misc/eeprom/eeprom_93xx46.c | 4 +- > drivers/mtd/mtdcore.c | 1 - > drivers/nvmem/bcm-ocotp.c | 1 - > drivers/nvmem/core.c | 5 +- > drivers/nvmem/imx-iim.c | 1 - > drivers/nvmem/imx-ocotp-scu.c | 1 - > drivers/nvmem/imx-ocotp.c | 1 - > drivers/nvmem/lpc18xx_otp.c | 1 - > drivers/nvmem/meson-mx-efuse.c | 1 - > drivers/nvmem/nvmem-sysfs.c | 77 ++++++++++++++++++++++++++--- > drivers/nvmem/nvmem.h | 1 - > drivers/nvmem/rockchip-efuse.c | 1 - > drivers/nvmem/rockchip-otp.c | 1 - > drivers/nvmem/sc27xx-efuse.c | 1 - > drivers/nvmem/sprd-efuse.c | 1 - > drivers/nvmem/stm32-romem.c | 1 - > drivers/nvmem/sunxi_sid.c | 1 - > drivers/nvmem/uniphier-efuse.c | 1 - > drivers/nvmem/zynqmp_nvmem.c | 1 - > drivers/soc/tegra/fuse/fuse-tegra.c | 1 - > drivers/thunderbolt/switch.c | 8 --- > drivers/w1/slaves/w1_ds250x.c | 1 - > include/linux/nvmem-provider.h | 2 - > 25 files changed, 77 insertions(+), 45 deletions(-) >
| |