Re: [PATCH v1 0/3] nvmem: Add support for write-only instances, and clean-up

[Srinivas Kandagatla]

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(-)