Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v1 0/3] nvmem: Add support for write-only instances, and
 clean-up
To:     Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc:     Mika Westerberg <mika.westerberg@linux.intel.com>
References: <PSXP216MB043899D4B8F693E1E5C3ECCE80EC0@PSXP216MB0438.KORP216.PROD.OUTLOOK.COM>
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Message-ID: <abdbeaf4-487d-8921-facc-b979421e97e7@linaro.org>
Date:   Tue, 25 Feb 2020 14:59:46 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <PSXP216MB043899D4B8F693E1E5C3ECCE80EC0@PSXP216MB0438.KORP216.PROD.OUTLOOK.COM>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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