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

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

[Nicholas Johnson]

[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").

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

-- 
2.25.1

[PATCH v1 1/3] nvmem: Add support for write-only instances

[Nicholas Johnson]

Mika Westerberg requires write-only nvmem for the Thunderbolt driver.
Refer to 03cd45d2e219 ("thunderbolt: Prevent crash if non-active NVMem
file is read"). Hence, there is at least one real-world use for
write-only nvmem instances.

Add support for write-only nvmem instances by changing the nvmem attrs
to 0222 if the .reg_read callback is not populated.

Add a WARN_ON in case a driver populates neither .reg_read nor
.reg_write because this behaviour should clearly never occur.

Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
---
 drivers/nvmem/nvmem-sysfs.c | 77 +++++++++++++++++++++++++++++++++----
 1 file changed, 70 insertions(+), 7 deletions(-)

diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
index 9e0c429cd..be3b94f0b 100644
--- a/drivers/nvmem/nvmem-sysfs.c
+++ b/drivers/nvmem/nvmem-sysfs.c
@@ -147,6 +147,30 @@ static const struct attribute_group *nvmem_ro_dev_groups[] = {
 	NULL,
 };
 
+/* write only permission */
+static struct bin_attribute bin_attr_wo_nvmem = {
+	.attr	= {
+		.name	= "nvmem",
+		.mode	= 0222,
+	},
+	.write	= bin_attr_nvmem_write,
+};
+
+static struct bin_attribute *nvmem_bin_wo_attributes[] = {
+	&bin_attr_wo_nvmem,
+	NULL,
+};
+
+static const struct attribute_group nvmem_bin_wo_group = {
+	.bin_attrs	= nvmem_bin_wo_attributes,
+	.attrs		= nvmem_attrs,
+};
+
+static const struct attribute_group *nvmem_wo_dev_groups[] = {
+	&nvmem_bin_wo_group,
+	NULL,
+};
+
 /* default read/write permissions, root only */
 static struct bin_attribute bin_attr_rw_root_nvmem = {
 	.attr	= {
@@ -196,16 +220,50 @@ static const struct attribute_group *nvmem_ro_root_dev_groups[] = {
 	NULL,
 };
 
+/* write only permission, root only */
+static struct bin_attribute bin_attr_wo_root_nvmem = {
+	.attr	= {
+		.name	= "nvmem",
+		.mode	= 0200,
+	},
+	.write	= bin_attr_nvmem_write,
+};
+
+static struct bin_attribute *nvmem_bin_wo_root_attributes[] = {
+	&bin_attr_wo_root_nvmem,
+	NULL,
+};
+
+static const struct attribute_group nvmem_bin_wo_root_group = {
+	.bin_attrs	= nvmem_bin_wo_root_attributes,
+	.attrs		= nvmem_attrs,
+};
+
+static const struct attribute_group *nvmem_wo_root_dev_groups[] = {
+	&nvmem_bin_wo_root_group,
+	NULL,
+};
+
 const struct attribute_group **nvmem_sysfs_get_groups(
 					struct nvmem_device *nvmem,
 					const struct nvmem_config *config)
 {
-	if (config->root_only)
-		return nvmem->read_only ?
-			nvmem_ro_root_dev_groups :
-			nvmem_rw_root_dev_groups;
-
-	return nvmem->read_only ? nvmem_ro_dev_groups : nvmem_rw_dev_groups;
+	/*
+	 * If neither reg_read nor reg_write are provided, we cannot use this
+	 * nvmem entry, as any operation will cause kernel mode NULL reference.
+	 */
+	WARN_ON(!nvmem->reg_read && !nvmem->reg_write);
+
+	if (nvmem->reg_read && nvmem->reg_write)
+		return config->root_only ?
+			nvmem_rw_root_dev_groups : nvmem_rw_dev_groups;
+
+	if (nvmem->reg_read && !nvmem->reg_write)
+		return config->root_only ?
+			nvmem_ro_root_dev_groups : nvmem_ro_dev_groups;
+
+	return config->root_only ?
+		nvmem_wo_root_dev_groups : nvmem_wo_dev_groups;
 }
 
 /*
@@ -224,11 +282,16 @@ int nvmem_sysfs_setup_compat(struct nvmem_device *nvmem,
 	if (!config->base_dev)
 		return -EINVAL;
 
-	if (nvmem->read_only) {
+	if (nvmem->reg_read && !nvmem->reg_write) {
 		if (config->root_only)
 			nvmem->eeprom = bin_attr_ro_root_nvmem;
 		else
 			nvmem->eeprom = bin_attr_ro_nvmem;
+	} else if (!nvmem->reg_read && nvmem->reg_write) {
+		if (config->root_only)
+			nvmem->eeprom = bin_attr_wo_root_nvmem;
+		else
+			nvmem->eeprom = bin_attr_wo_nvmem;
 	} else {
 		if (config->root_only)
 			nvmem->eeprom = bin_attr_rw_root_nvmem;
-- 
2.25.1

[PATCH v1 2/3] Revert "thunderbolt: Prevent crash if non-active NVMem file is read"

[Nicholas Johnson]

This reverts commit 03cd45d2e219301880cabc357e3cf478a500080f.

Since commit cd76ed9e5913 ("nvmem: add support for write-only
instances"), this work around is no longer required, so drop it.

Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
---
 drivers/thunderbolt/switch.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 7d6ecc342..ad5479f21 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -348,12 +348,6 @@ static int tb_switch_nvm_read(void *priv, unsigned int offset, void *val,
 	return ret;
 }
 
-static int tb_switch_nvm_no_read(void *priv, unsigned int offset, void *val,
-				 size_t bytes)
-{
-	return -EPERM;
-}
-
 static int tb_switch_nvm_write(void *priv, unsigned int offset, void *val,
 			       size_t bytes)
 {
@@ -399,7 +393,6 @@ static struct nvmem_device *register_nvmem(struct tb_switch *sw, int id,
 		config.read_only = true;
 	} else {
 		config.name = "nvm_non_active";
-		config.reg_read = tb_switch_nvm_no_read;
 		config.reg_write = tb_switch_nvm_write;
 		config.root_only = true;
 	}
-- 
2.25.1

[PATCH v1 3/3] nvmem: Remove .read_only field from nvmem_config

[Nicholas Johnson]

There is no .write_only field in nvmem_config, so having .read_only
makes no sense. We can determine the attrs based on whether .reg_read
and .reg_write are provided. Using only .reg_read and .reg_write means
that there can no longer be contradictions (for instance, if .read_only
is set but .reg_read is NULL).

Remove .read_only field from nvmem_config.

Remove all references to .read_only field from drivers.

Update drivers to only supply nvmem->reg_write if write attrs are
desired.

Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
---
 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.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        | 1 -
 drivers/w1/slaves/w1_ds250x.c       | 1 -
 include/linux/nvmem-provider.h      | 2 --
 24 files changed, 7 insertions(+), 31 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 031eb6454..000a78f0f 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -679,13 +679,13 @@ static int at24_probe(struct i2c_client *client)
 
 	nvmem_config.name = dev_name(dev);
 	nvmem_config.dev = dev;
-	nvmem_config.read_only = !writable;
 	nvmem_config.root_only = !(flags & AT24_FLAG_IRUGO);
 	nvmem_config.owner = THIS_MODULE;
 	nvmem_config.compat = true;
 	nvmem_config.base_dev = dev;
 	nvmem_config.reg_read = at24_read;
-	nvmem_config.reg_write = at24_write;
+	if (writable)
+		nvmem_config.reg_write = at24_write;
 	nvmem_config.priv = at24;
 	nvmem_config.stride = 1;
 	nvmem_config.word_size = 1;
diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
index cde9a2fc1..57e26ca2c 100644
--- a/drivers/misc/eeprom/at25.c
+++ b/drivers/misc/eeprom/at25.c
@@ -350,13 +350,13 @@ static int at25_probe(struct spi_device *spi)
 
 	at25->nvmem_config.name = dev_name(&spi->dev);
 	at25->nvmem_config.dev = &spi->dev;
-	at25->nvmem_config.read_only = chip.flags & EE_READONLY;
 	at25->nvmem_config.root_only = true;
 	at25->nvmem_config.owner = THIS_MODULE;
 	at25->nvmem_config.compat = true;
 	at25->nvmem_config.base_dev = &spi->dev;
 	at25->nvmem_config.reg_read = at25_ee_read;
-	at25->nvmem_config.reg_write = at25_ee_write;
+	if (!(chip.flags && EE_READONLY))
+		at25->nvmem_config.reg_write = at25_ee_write;
 	at25->nvmem_config.priv = at25;
 	at25->nvmem_config.stride = 4;
 	at25->nvmem_config.word_size = 1;
diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
index 94cfb675f..29c05ea5f 100644
--- a/drivers/misc/eeprom/eeprom_93xx46.c
+++ b/drivers/misc/eeprom/eeprom_93xx46.c
@@ -457,13 +457,13 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
 	edev->size = 128;
 	edev->nvmem_config.name = dev_name(&spi->dev);
 	edev->nvmem_config.dev = &spi->dev;
-	edev->nvmem_config.read_only = pd->flags & EE_READONLY;
 	edev->nvmem_config.root_only = true;
 	edev->nvmem_config.owner = THIS_MODULE;
 	edev->nvmem_config.compat = true;
 	edev->nvmem_config.base_dev = &spi->dev;
 	edev->nvmem_config.reg_read = eeprom_93xx46_read;
-	edev->nvmem_config.reg_write = eeprom_93xx46_write;
+	if (!(pd->flags & EE_READONLY))
+		edev->nvmem_config.reg_write = eeprom_93xx46_write;
 	edev->nvmem_config.priv = edev;
 	edev->nvmem_config.stride = 4;
 	edev->nvmem_config.word_size = 1;
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 5fac4355b..660871f7b 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -557,7 +557,6 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
 	config.size = mtd->size;
 	config.word_size = 1;
 	config.stride = 1;
-	config.read_only = true;
 	config.root_only = true;
 	config.no_of_node = true;
 	config.priv = mtd;
diff --git a/drivers/nvmem/bcm-ocotp.c b/drivers/nvmem/bcm-ocotp.c
index a80975115..e692143d2 100644
--- a/drivers/nvmem/bcm-ocotp.c
+++ b/drivers/nvmem/bcm-ocotp.c
@@ -230,7 +230,6 @@ static int bcm_otpc_write(void *context, unsigned int offset, void *val,
 
 static struct nvmem_config bcm_otpc_nvmem_config = {
 	.name = "bcm-ocotp",
-	.read_only = false,
 	.word_size = 4,
 	.stride = 4,
 	.reg_read = bcm_otpc_read,
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index ef326f243..2b4df8c42 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -384,9 +384,6 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 			     config->name ? config->id : nvmem->id);
 	}
 
-	nvmem->read_only = device_property_present(config->dev, "read-only") ||
-			   config->read_only || !nvmem->reg_write;
-
 	nvmem->dev.groups = nvmem_sysfs_get_groups(nvmem, config);
 
 	device_initialize(&nvmem->dev);
@@ -1065,7 +1062,7 @@ int nvmem_cell_write(struct nvmem_cell *cell, void *buf, size_t len)
 	struct nvmem_device *nvmem = cell->nvmem;
 	int rc;
 
-	if (!nvmem || nvmem->read_only ||
+	if (!nvmem || (nvmem->reg_read && !nvmem->reg_write) ||
 	    (cell->bit_offset == 0 && len != cell->bytes))
 		return -EINVAL;
 
diff --git a/drivers/nvmem/imx-iim.c b/drivers/nvmem/imx-iim.c
index 701704b87..4778f44b3 100644
--- a/drivers/nvmem/imx-iim.c
+++ b/drivers/nvmem/imx-iim.c
@@ -122,7 +122,6 @@ static int imx_iim_probe(struct platform_device *pdev)
 		return PTR_ERR(iim->clk);
 
 	cfg.name = "imx-iim",
-	cfg.read_only = true,
 	cfg.word_size = 1,
 	cfg.stride = 1,
 	cfg.reg_read = imx_iim_read,
diff --git a/drivers/nvmem/imx-ocotp-scu.c b/drivers/nvmem/imx-ocotp-scu.c
index 399e1eb8b..ed21256fd 100644
--- a/drivers/nvmem/imx-ocotp-scu.c
+++ b/drivers/nvmem/imx-ocotp-scu.c
@@ -220,7 +220,6 @@ static int imx_scu_ocotp_write(void *context, unsigned int offset,
 
 static struct nvmem_config imx_scu_ocotp_nvmem_config = {
 	.name = "imx-scu-ocotp",
-	.read_only = false,
 	.word_size = 4,
 	.stride = 1,
 	.owner = THIS_MODULE,
diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c
index 4ba9cc8f7..2c4b5d9be 100644
--- a/drivers/nvmem/imx-ocotp.c
+++ b/drivers/nvmem/imx-ocotp.c
@@ -437,7 +437,6 @@ static int imx_ocotp_write(void *context, unsigned int offset, void *val,
 
 static struct nvmem_config imx_ocotp_nvmem_config = {
 	.name = "imx-ocotp",
-	.read_only = false,
 	.word_size = 4,
 	.stride = 4,
 	.reg_read = imx_ocotp_read,
diff --git a/drivers/nvmem/lpc18xx_otp.c b/drivers/nvmem/lpc18xx_otp.c
index 16c92ea85..27a6aa1dc 100644
--- a/drivers/nvmem/lpc18xx_otp.c
+++ b/drivers/nvmem/lpc18xx_otp.c
@@ -58,7 +58,6 @@ static int lpc18xx_otp_read(void *context, unsigned int offset,
 
 static struct nvmem_config lpc18xx_otp_nvmem_config = {
 	.name = "lpc18xx-otp",
-	.read_only = true,
 	.word_size = LPC18XX_OTP_WORD_SIZE,
 	.stride = LPC18XX_OTP_WORD_SIZE,
 	.reg_read = lpc18xx_otp_read,
diff --git a/drivers/nvmem/meson-mx-efuse.c b/drivers/nvmem/meson-mx-efuse.c
index 07c9f38c1..9ecda17de 100644
--- a/drivers/nvmem/meson-mx-efuse.c
+++ b/drivers/nvmem/meson-mx-efuse.c
@@ -217,7 +217,6 @@ static int meson_mx_efuse_probe(struct platform_device *pdev)
 	efuse->config.stride = drvdata->word_size;
 	efuse->config.word_size = drvdata->word_size;
 	efuse->config.size = SZ_512;
-	efuse->config.read_only = true;
 	efuse->config.reg_read = meson_mx_efuse_read;
 
 	efuse->core_clk = devm_clk_get(&pdev->dev, "core");
diff --git a/drivers/nvmem/nvmem.h b/drivers/nvmem/nvmem.h
index be0d66d75..1d6c76ef4 100644
--- a/drivers/nvmem/nvmem.h
+++ b/drivers/nvmem/nvmem.h
@@ -19,7 +19,6 @@ struct nvmem_device {
 	int			id;
 	struct kref		refcnt;
 	size_t			size;
-	bool			read_only;
 	int			flags;
 	enum nvmem_type		type;
 	struct bin_attribute	eeprom;
diff --git a/drivers/nvmem/rockchip-efuse.c b/drivers/nvmem/rockchip-efuse.c
index e4579de5d..6bd18511d 100644
--- a/drivers/nvmem/rockchip-efuse.c
+++ b/drivers/nvmem/rockchip-efuse.c
@@ -207,7 +207,6 @@ static struct nvmem_config econfig = {
 	.name = "rockchip-efuse",
 	.stride = 1,
 	.word_size = 1,
-	.read_only = true,
 };
 
 static const struct of_device_id rockchip_efuse_match[] = {
diff --git a/drivers/nvmem/rockchip-otp.c b/drivers/nvmem/rockchip-otp.c
index 9f53bcce2..38ae30363 100644
--- a/drivers/nvmem/rockchip-otp.c
+++ b/drivers/nvmem/rockchip-otp.c
@@ -183,7 +183,6 @@ static int rockchip_otp_read(void *context, unsigned int offset,
 static struct nvmem_config otp_config = {
 	.name = "rockchip-otp",
 	.owner = THIS_MODULE,
-	.read_only = true,
 	.stride = 1,
 	.word_size = 1,
 	.reg_read = rockchip_otp_read,
diff --git a/drivers/nvmem/sc27xx-efuse.c b/drivers/nvmem/sc27xx-efuse.c
index ab5e7e0bc..92183db62 100644
--- a/drivers/nvmem/sc27xx-efuse.c
+++ b/drivers/nvmem/sc27xx-efuse.c
@@ -222,7 +222,6 @@ static int sc27xx_efuse_probe(struct platform_device *pdev)
 
 	econfig.stride = 1;
 	econfig.word_size = 1;
-	econfig.read_only = true;
 	econfig.name = "sc27xx-efuse";
 	econfig.size = SC27XX_EFUSE_BLOCK_MAX * SC27XX_EFUSE_BLOCK_WIDTH;
 	econfig.reg_read = sc27xx_efuse_read;
diff --git a/drivers/nvmem/sprd-efuse.c b/drivers/nvmem/sprd-efuse.c
index 2f1e0fbd1..f2580d692 100644
--- a/drivers/nvmem/sprd-efuse.c
+++ b/drivers/nvmem/sprd-efuse.c
@@ -388,7 +388,6 @@ static int sprd_efuse_probe(struct platform_device *pdev)
 
 	econfig.stride = 1;
 	econfig.word_size = 1;
-	econfig.read_only = false;
 	econfig.name = "sprd-efuse";
 	econfig.size = efuse->data->blk_nums * SPRD_EFUSE_BLOCK_WIDTH;
 	econfig.reg_read = sprd_efuse_read;
diff --git a/drivers/nvmem/stm32-romem.c b/drivers/nvmem/stm32-romem.c
index 354be5268..c71c1b8e0 100644
--- a/drivers/nvmem/stm32-romem.c
+++ b/drivers/nvmem/stm32-romem.c
@@ -162,7 +162,6 @@ static int stm32_romem_probe(struct platform_device *pdev)
 	cfg = (const struct stm32_romem_cfg *)
 		of_match_device(dev->driver->of_match_table, dev)->data;
 	if (!cfg) {
-		priv->cfg.read_only = true;
 		priv->cfg.size = resource_size(res);
 		priv->cfg.reg_read = stm32_romem_read;
 	} else {
diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
index e26ef1bbf..8bf2d66f1 100644
--- a/drivers/nvmem/sunxi_sid.c
+++ b/drivers/nvmem/sunxi_sid.c
@@ -142,7 +142,6 @@ static int sunxi_sid_probe(struct platform_device *pdev)
 
 	nvmem_cfg->dev = dev;
 	nvmem_cfg->name = "sunxi-sid";
-	nvmem_cfg->read_only = true;
 	nvmem_cfg->size = cfg->size;
 	nvmem_cfg->word_size = 1;
 	nvmem_cfg->stride = 4;
diff --git a/drivers/nvmem/uniphier-efuse.c b/drivers/nvmem/uniphier-efuse.c
index aca910b3b..312a3a99d 100644
--- a/drivers/nvmem/uniphier-efuse.c
+++ b/drivers/nvmem/uniphier-efuse.c
@@ -48,7 +48,6 @@ static int uniphier_efuse_probe(struct platform_device *pdev)
 
 	econfig.stride = 1;
 	econfig.word_size = 1;
-	econfig.read_only = true;
 	econfig.reg_read = uniphier_reg_read;
 	econfig.size = resource_size(res);
 	econfig.priv = priv;
diff --git a/drivers/nvmem/zynqmp_nvmem.c b/drivers/nvmem/zynqmp_nvmem.c
index 589354391..0336aa056 100644
--- a/drivers/nvmem/zynqmp_nvmem.c
+++ b/drivers/nvmem/zynqmp_nvmem.c
@@ -43,7 +43,6 @@ static struct nvmem_config econfig = {
 	.owner = THIS_MODULE,
 	.word_size = 1,
 	.size = 1,
-	.read_only = true,
 };
 
 static const struct of_device_id zynqmp_nvmem_match[] = {
diff --git a/drivers/soc/tegra/fuse/fuse-tegra.c b/drivers/soc/tegra/fuse/fuse-tegra.c
index 802717b9f..38663a389 100644
--- a/drivers/soc/tegra/fuse/fuse-tegra.c
+++ b/drivers/soc/tegra/fuse/fuse-tegra.c
@@ -221,7 +221,6 @@ static int tegra_fuse_probe(struct platform_device *pdev)
 	nvmem.cells = tegra_fuse_cells;
 	nvmem.ncells = ARRAY_SIZE(tegra_fuse_cells);
 	nvmem.type = NVMEM_TYPE_OTP;
-	nvmem.read_only = true;
 	nvmem.root_only = true;
 	nvmem.reg_read = tegra_fuse_read;
 	nvmem.size = fuse->soc->info->size;
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index ad5479f21..a0f05a47e 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -390,7 +390,6 @@ static struct nvmem_device *register_nvmem(struct tb_switch *sw, int id,
 	if (active) {
 		config.name = "nvm_active";
 		config.reg_read = tb_switch_nvm_read;
-		config.read_only = true;
 	} else {
 		config.name = "nvm_non_active";
 		config.reg_write = tb_switch_nvm_write;
diff --git a/drivers/w1/slaves/w1_ds250x.c b/drivers/w1/slaves/w1_ds250x.c
index e50711744..35b92e791 100644
--- a/drivers/w1/slaves/w1_ds250x.c
+++ b/drivers/w1/slaves/w1_ds250x.c
@@ -170,7 +170,6 @@ static int w1_eprom_add_slave(struct w1_slave *sl)
 		.dev = &sl->dev,
 		.reg_read = w1_nvmem_read,
 		.type = NVMEM_TYPE_OTP,
-		.read_only = true,
 		.word_size = 1,
 		.priv = sl,
 		.id = -1
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index 6d6f8e5d2..69b63f4c2 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -37,7 +37,6 @@ enum nvmem_type {
  * @cells:	Optional array of pre-defined NVMEM cells.
  * @ncells:	Number of elements in cells.
  * @type:	Type of the nvmem storage
- * @read_only:	Device is read-only.
  * @root_only:	Device is accessibly to root only.
  * @no_of_node:	Device should not use the parent's of_node even if it's !NULL.
  * @reg_read:	Callback to read data.
@@ -64,7 +63,6 @@ struct nvmem_config {
 	const struct nvmem_cell_info	*cells;
 	int			ncells;
 	enum nvmem_type		type;
-	bool			read_only;
 	bool			root_only;
 	bool			no_of_node;
 	nvmem_reg_read_t	reg_read;
-- 
2.25.1

Re: [PATCH v1 3/3] nvmem: Remove .read_only field from nvmem_config

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2455 bytes --]

Hi Nicholas,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on linus/master v5.6-rc3 next-20200224]
[cannot apply to shawnguo/for-next rockchip/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Nicholas-Johnson/nvmem-Add-support-for-write-only-instances-and-clean-up/20200225-033100
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 1f836f5b10f2524d33efde47d2b694b861ecf319
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/soc/atmel/sfr.c:35:3: error: 'struct nvmem_config' has no member named 'read_only'; did you mean 'root_only'?
     .read_only = true,
      ^~~~~~~~~
      root_only

vim +35 drivers/soc/atmel/sfr.c

c3277f8ee8cdadf Kamel Bouhara 2019-10-04  32  
c3277f8ee8cdadf Kamel Bouhara 2019-10-04  33  static struct nvmem_config atmel_sfr_nvmem_config = {
c3277f8ee8cdadf Kamel Bouhara 2019-10-04  34  	.name = "atmel-sfr",
c3277f8ee8cdadf Kamel Bouhara 2019-10-04 @35  	.read_only = true,
c3277f8ee8cdadf Kamel Bouhara 2019-10-04  36  	.word_size = 4,
c3277f8ee8cdadf Kamel Bouhara 2019-10-04  37  	.stride = 4,
c3277f8ee8cdadf Kamel Bouhara 2019-10-04  38  	.size = SFR_SN_SIZE,
c3277f8ee8cdadf Kamel Bouhara 2019-10-04  39  	.reg_read = atmel_sfr_read,
c3277f8ee8cdadf Kamel Bouhara 2019-10-04  40  };
c3277f8ee8cdadf Kamel Bouhara 2019-10-04  41  

:::::: The code at line 35 was first introduced by commit
:::::: c3277f8ee8cdadf011b8390dfdb4c44ecfaa1a7a soc: at91: Add Atmel SFR SN (Serial Number) support

:::::: TO: Kamel Bouhara <kamel.bouhara@bootlin.com>
:::::: CC: Alexandre Belloni <alexandre.belloni@bootlin.com>

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 73494 bytes --]

Re: [PATCH v1 3/3] nvmem: Remove .read_only field from nvmem_config

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2510 bytes --]

Hi Nicholas,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on linus/master v5.6-rc3 next-20200224]
[cannot apply to shawnguo/for-next rockchip/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Nicholas-Johnson/nvmem-Add-support-for-write-only-instances-and-clean-up/20200225-033100
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 1f836f5b10f2524d33efde47d2b694b861ecf319
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/soc/atmel/sfr.c:35:3: error: 'struct nvmem_config' has no member named 'read_only'; did you mean 'root_only'?
     .read_only = true,
      ^~~~~~~~~
      root_only

vim +35 drivers/soc/atmel/sfr.c

c3277f8ee8cdadf Kamel Bouhara 2019-10-04  32  
c3277f8ee8cdadf Kamel Bouhara 2019-10-04  33  static struct nvmem_config atmel_sfr_nvmem_config = {
c3277f8ee8cdadf Kamel Bouhara 2019-10-04  34  	.name = "atmel-sfr",
c3277f8ee8cdadf Kamel Bouhara 2019-10-04 @35  	.read_only = true,
c3277f8ee8cdadf Kamel Bouhara 2019-10-04  36  	.word_size = 4,
c3277f8ee8cdadf Kamel Bouhara 2019-10-04  37  	.stride = 4,
c3277f8ee8cdadf Kamel Bouhara 2019-10-04  38  	.size = SFR_SN_SIZE,
c3277f8ee8cdadf Kamel Bouhara 2019-10-04  39  	.reg_read = atmel_sfr_read,
c3277f8ee8cdadf Kamel Bouhara 2019-10-04  40  };
c3277f8ee8cdadf Kamel Bouhara 2019-10-04  41  

:::::: The code at line 35 was first introduced by commit
:::::: c3277f8ee8cdadf011b8390dfdb4c44ecfaa1a7a soc: at91: Add Atmel SFR SN (Serial Number) support

:::::: TO: Kamel Bouhara <kamel.bouhara@bootlin.com>
:::::: CC: Alexandre Belloni <alexandre.belloni@bootlin.com>

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 73494 bytes --]

Re: [PATCH v1 3/3] nvmem: Remove .read_only field from nvmem_config

[Nicholas Johnson]

On Mon, Feb 24, 2020 at 05:43:29PM +0000, Nicholas Johnson wrote:
> There is no .write_only field in nvmem_config, so having .read_only
> makes no sense. We can determine the attrs based on whether .reg_read
> and .reg_write are provided. Using only .reg_read and .reg_write means
> that there can no longer be contradictions (for instance, if .read_only
> is set but .reg_read is NULL).
> 
> Remove .read_only field from nvmem_config.
> 
> Remove all references to .read_only field from drivers.
> 
> Update drivers to only supply nvmem->reg_write if write attrs are
> desired.
> 
> Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> ---

Sorry all, I got this build bot [0] telling me that this patch 3/3 fails 
to compile for ARM architecture. It worked for x86-64. So please ignore 
3/3 for now while I investigate. 1/3 and 2/3 are all good.

I will find a better way to ensure that I have all of the references and 
post a PATCH v2 sometime.

Kind regards,
Nicholas

[0]
https://lore.kernel.org/lkml/202002250920.gc0wDekv%25lkp@intel.com/

Re: [PATCH v1 1/3] nvmem: Add support for write-only instances

[Mika Westerberg]

On Mon, Feb 24, 2020 at 05:42:33PM +0000, Nicholas Johnson wrote:
> Mika Westerberg requires write-only nvmem for the Thunderbolt driver.
> Refer to 03cd45d2e219 ("thunderbolt: Prevent crash if non-active NVMem
> file is read"). Hence, there is at least one real-world use for
> write-only nvmem instances.

Well, I don't require anything ;-) It is the thunderbolt driver that has
one nvmem device that is write-only and it may take advantage of this.

> Add support for write-only nvmem instances by changing the nvmem attrs
> to 0222 if the .reg_read callback is not populated.
> 
> Add a WARN_ON in case a driver populates neither .reg_read nor
> .reg_write because this behaviour should clearly never occur.
> 
> Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> ---
>  drivers/nvmem/nvmem-sysfs.c | 77 +++++++++++++++++++++++++++++++++----
>  1 file changed, 70 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
> index 9e0c429cd..be3b94f0b 100644
> --- a/drivers/nvmem/nvmem-sysfs.c
> +++ b/drivers/nvmem/nvmem-sysfs.c
> @@ -147,6 +147,30 @@ static const struct attribute_group *nvmem_ro_dev_groups[] = {
>  	NULL,
>  };
>  
> +/* write only permission */
> +static struct bin_attribute bin_attr_wo_nvmem = {
> +	.attr	= {
> +		.name	= "nvmem",
> +		.mode	= 0222,

I would say no sysfs attribute should ever be writable by the world.

Actually I think maybe we make this one only writeable by root, in other
words it would always require ->root_only to be set.

> +	},
> +	.write	= bin_attr_nvmem_write,
> +};
> +
> +static struct bin_attribute *nvmem_bin_wo_attributes[] = {
> +	&bin_attr_wo_nvmem,
> +	NULL,
> +};
> +
> +static const struct attribute_group nvmem_bin_wo_group = {
> +	.bin_attrs	= nvmem_bin_wo_attributes,
> +	.attrs		= nvmem_attrs,
> +};
> +
> +static const struct attribute_group *nvmem_wo_dev_groups[] = {
> +	&nvmem_bin_wo_group,
> +	NULL,
> +};
> +
>  /* default read/write permissions, root only */
>  static struct bin_attribute bin_attr_rw_root_nvmem = {
>  	.attr	= {
> @@ -196,16 +220,50 @@ static const struct attribute_group *nvmem_ro_root_dev_groups[] = {
>  	NULL,
>  };
>  
> +/* write only permission, root only */
> +static struct bin_attribute bin_attr_wo_root_nvmem = {
> +	.attr	= {
> +		.name	= "nvmem",
> +		.mode	= 0200,
> +	},
> +	.write	= bin_attr_nvmem_write,
> +};
> +
> +static struct bin_attribute *nvmem_bin_wo_root_attributes[] = {
> +	&bin_attr_wo_root_nvmem,
> +	NULL,
> +};
> +
> +static const struct attribute_group nvmem_bin_wo_root_group = {
> +	.bin_attrs	= nvmem_bin_wo_root_attributes,
> +	.attrs		= nvmem_attrs,
> +};
> +
> +static const struct attribute_group *nvmem_wo_root_dev_groups[] = {
> +	&nvmem_bin_wo_root_group,
> +	NULL,
> +};
> +
>  const struct attribute_group **nvmem_sysfs_get_groups(
>  					struct nvmem_device *nvmem,
>  					const struct nvmem_config *config)
>  {
> -	if (config->root_only)
> -		return nvmem->read_only ?
> -			nvmem_ro_root_dev_groups :
> -			nvmem_rw_root_dev_groups;
> -
> -	return nvmem->read_only ? nvmem_ro_dev_groups : nvmem_rw_dev_groups;
> +	/*
> +	 * If neither reg_read nor reg_write are provided, we cannot use this
> +	 * nvmem entry, as any operation will cause kernel mode NULL reference.
> +	 */
> +	WARN_ON(!nvmem->reg_read && !nvmem->reg_write);

This should also be documented in kernel-doc of struct nvmem_config.

> +
> +	if (nvmem->reg_read && nvmem->reg_write)
> +		return config->root_only ?
> +			nvmem_rw_root_dev_groups : nvmem_rw_dev_groups;
> +
> +	if (nvmem->reg_read && !nvmem->reg_write)
> +		return config->root_only ?
> +			nvmem_ro_root_dev_groups : nvmem_ro_dev_groups;
> +
> +	return config->root_only ?
> +		nvmem_wo_root_dev_groups : nvmem_wo_dev_groups;
>  }
>  
>  /*
> @@ -224,11 +282,16 @@ int nvmem_sysfs_setup_compat(struct nvmem_device *nvmem,
>  	if (!config->base_dev)
>  		return -EINVAL;
>  
> -	if (nvmem->read_only) {
> +	if (nvmem->reg_read && !nvmem->reg_write) {
>  		if (config->root_only)
>  			nvmem->eeprom = bin_attr_ro_root_nvmem;
>  		else
>  			nvmem->eeprom = bin_attr_ro_nvmem;
> +	} else if (!nvmem->reg_read && nvmem->reg_write) {
> +		if (config->root_only)
> +			nvmem->eeprom = bin_attr_wo_root_nvmem;
> +		else
> +			nvmem->eeprom = bin_attr_wo_nvmem;
>  	} else {
>  		if (config->root_only)
>  			nvmem->eeprom = bin_attr_rw_root_nvmem;
> -- 
> 2.25.1

Re: [PATCH v1 2/3] Revert "thunderbolt: Prevent crash if non-active NVMem file is read"

[Mika Westerberg]

On Mon, Feb 24, 2020 at 05:43:05PM +0000, Nicholas Johnson wrote:
> This reverts commit 03cd45d2e219301880cabc357e3cf478a500080f.
> 
> Since commit cd76ed9e5913 ("nvmem: add support for write-only
> instances"), this work around is no longer required, so drop it.

I don't think you can refer commits that only exists in your local tree.
I would just say that "since NVMem subsystem gained support for
write-only instances this workaround is not needed anymore" or somesuch.

Otherwise this looks good to me, no objections and thanks for doing this :)

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

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

[Nicholas Johnson]

On Tue, Feb 25, 2020 at 02:59:46PM +0000, Srinivas Kandagatla wrote:
> 
> 
> 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?
That can be easily done in PATCH v2. What error code should be returned?

> 
> 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
Noted. However, the .read_only flag is only removed in the third patch, 
which can be discarded if you feel that is the best plan of action.

The write-only will not have a flag added, which should not be a 
problem, as nothing relies on there being one yet.

Regards,
Nicholas

Re: [PATCH v1 1/3] nvmem: Add support for write-only instances

[Nicholas Johnson]

On Tue, Feb 25, 2020 at 02:51:41PM +0200, Mika Westerberg wrote:
> On Mon, Feb 24, 2020 at 05:42:33PM +0000, Nicholas Johnson wrote:
> > Mika Westerberg requires write-only nvmem for the Thunderbolt driver.
> > Refer to 03cd45d2e219 ("thunderbolt: Prevent crash if non-active NVMem
> > file is read"). Hence, there is at least one real-world use for
> > write-only nvmem instances.
> 
> Well, I don't require anything ;-) It is the thunderbolt driver that has
> one nvmem device that is write-only and it may take advantage of this.
Sorry, I will re-word it to be more accurate. I need to work on saying 
what I actually mean.

> 
> > Add support for write-only nvmem instances by changing the nvmem attrs
> > to 0222 if the .reg_read callback is not populated.
> > 
> > Add a WARN_ON in case a driver populates neither .reg_read nor
> > .reg_write because this behaviour should clearly never occur.
> > 
> > Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> > ---
> >  drivers/nvmem/nvmem-sysfs.c | 77 +++++++++++++++++++++++++++++++++----
> >  1 file changed, 70 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
> > index 9e0c429cd..be3b94f0b 100644
> > --- a/drivers/nvmem/nvmem-sysfs.c
> > +++ b/drivers/nvmem/nvmem-sysfs.c
> > @@ -147,6 +147,30 @@ static const struct attribute_group *nvmem_ro_dev_groups[] = {
> >  	NULL,
> >  };
> >  
> > +/* write only permission */
> > +static struct bin_attribute bin_attr_wo_nvmem = {
> > +	.attr	= {
> > +		.name	= "nvmem",
> > +		.mode	= 0222,
> 
> I would say no sysfs attribute should ever be writable by the world.
I cannot think of an argument against this, nor can I rule out one 
existing. But I would be inclined to agree.

> 
> Actually I think maybe we make this one only writeable by root, in other
> words it would always require ->root_only to be set.
There is a world-accessible rw entry already, which would, if anything, 
be even more dangerous than a world writable entry. However, there could 
be a hypothetical use case. I agree it is unlikely to be required, but 
who knows?

Based on your statement that no sysfs should ever be world-writable, 
should I be trying to remove the world-accessible rw as well?
> 
> > +	},
> > +	.write	= bin_attr_nvmem_write,
> > +};
> > +
> > +static struct bin_attribute *nvmem_bin_wo_attributes[] = {
> > +	&bin_attr_wo_nvmem,
> > +	NULL,
> > +};
> > +
> > +static const struct attribute_group nvmem_bin_wo_group = {
> > +	.bin_attrs	= nvmem_bin_wo_attributes,
> > +	.attrs		= nvmem_attrs,
> > +};
> > +
> > +static const struct attribute_group *nvmem_wo_dev_groups[] = {
> > +	&nvmem_bin_wo_group,
> > +	NULL,
> > +};
> > +
> >  /* default read/write permissions, root only */
> >  static struct bin_attribute bin_attr_rw_root_nvmem = {
> >  	.attr	= {
> > @@ -196,16 +220,50 @@ static const struct attribute_group *nvmem_ro_root_dev_groups[] = {
> >  	NULL,
> >  };
> >  
> > +/* write only permission, root only */
> > +static struct bin_attribute bin_attr_wo_root_nvmem = {
> > +	.attr	= {
> > +		.name	= "nvmem",
> > +		.mode	= 0200,
> > +	},
> > +	.write	= bin_attr_nvmem_write,
> > +};
> > +
> > +static struct bin_attribute *nvmem_bin_wo_root_attributes[] = {
> > +	&bin_attr_wo_root_nvmem,
> > +	NULL,
> > +};
> > +
> > +static const struct attribute_group nvmem_bin_wo_root_group = {
> > +	.bin_attrs	= nvmem_bin_wo_root_attributes,
> > +	.attrs		= nvmem_attrs,
> > +};
> > +
> > +static const struct attribute_group *nvmem_wo_root_dev_groups[] = {
> > +	&nvmem_bin_wo_root_group,
> > +	NULL,
> > +};
> > +
> >  const struct attribute_group **nvmem_sysfs_get_groups(
> >  					struct nvmem_device *nvmem,
> >  					const struct nvmem_config *config)
> >  {
> > -	if (config->root_only)
> > -		return nvmem->read_only ?
> > -			nvmem_ro_root_dev_groups :
> > -			nvmem_rw_root_dev_groups;
> > -
> > -	return nvmem->read_only ? nvmem_ro_dev_groups : nvmem_rw_dev_groups;
> > +	/*
> > +	 * If neither reg_read nor reg_write are provided, we cannot use this
> > +	 * nvmem entry, as any operation will cause kernel mode NULL reference.
> > +	 */
> > +	WARN_ON(!nvmem->reg_read && !nvmem->reg_write);
> 
> This should also be documented in kernel-doc of struct nvmem_config.
Roger.

> 
> > +
> > +	if (nvmem->reg_read && nvmem->reg_write)
> > +		return config->root_only ?
> > +			nvmem_rw_root_dev_groups : nvmem_rw_dev_groups;
> > +
> > +	if (nvmem->reg_read && !nvmem->reg_write)
> > +		return config->root_only ?
> > +			nvmem_ro_root_dev_groups : nvmem_ro_dev_groups;
> > +
> > +	return config->root_only ?
> > +		nvmem_wo_root_dev_groups : nvmem_wo_dev_groups;
> >  }
> >  
> >  /*
> > @@ -224,11 +282,16 @@ int nvmem_sysfs_setup_compat(struct nvmem_device *nvmem,
> >  	if (!config->base_dev)
> >  		return -EINVAL;
> >  
> > -	if (nvmem->read_only) {
> > +	if (nvmem->reg_read && !nvmem->reg_write) {
> >  		if (config->root_only)
> >  			nvmem->eeprom = bin_attr_ro_root_nvmem;
> >  		else
> >  			nvmem->eeprom = bin_attr_ro_nvmem;
> > +	} else if (!nvmem->reg_read && nvmem->reg_write) {
> > +		if (config->root_only)
> > +			nvmem->eeprom = bin_attr_wo_root_nvmem;
> > +		else
> > +			nvmem->eeprom = bin_attr_wo_nvmem;
> >  	} else {
> >  		if (config->root_only)
> >  			nvmem->eeprom = bin_attr_rw_root_nvmem;
> > -- 
> > 2.25.1

Re: [PATCH v1 2/3] Revert "thunderbolt: Prevent crash if non-active NVMem file is read"

[Nicholas Johnson]

On Tue, Feb 25, 2020 at 02:56:29PM +0200, Mika Westerberg wrote:
> On Mon, Feb 24, 2020 at 05:43:05PM +0000, Nicholas Johnson wrote:
> > This reverts commit 03cd45d2e219301880cabc357e3cf478a500080f.
> > 
> > Since commit cd76ed9e5913 ("nvmem: add support for write-only
> > instances"), this work around is no longer required, so drop it.
> 
> I don't think you can refer commits that only exists in your local tree.
> I would just say that "since NVMem subsystem gained support for
> write-only instances this workaround is not needed anymore" or somesuch.
Are the commit hashes changed when applied to the kernel? If so, oops!

I will remove it in PATCH v2.
> 
> Otherwise this looks good to me, no objections and thanks for doing this :)
Glad I can be of help. :)

Re: [PATCH v1 1/3] nvmem: Add support for write-only instances

[Mika Westerberg]

On Tue, Feb 25, 2020 at 03:30:22PM +0000, Nicholas Johnson wrote:
> > Actually I think maybe we make this one only writeable by root, in other
> > words it would always require ->root_only to be set.
> There is a world-accessible rw entry already, which would, if anything, 
> be even more dangerous than a world writable entry. However, there could 
> be a hypothetical use case. I agree it is unlikely to be required, but 
> who knows?

You mean 0644 entry? That should be fine as it is not writable by anyone
else than the owner (root in this case).

> Based on your statement that no sysfs should ever be world-writable, 
> should I be trying to remove the world-accessible rw as well?

No I don't think it is necesary. Just let's not add attributes that
anyone can write without good reasoning ;-)

Re: [PATCH v1 2/3] Revert "thunderbolt: Prevent crash if non-active NVMem file is read"

[Mika Westerberg]

On Tue, Feb 25, 2020 at 03:33:00PM +0000, Nicholas Johnson wrote:
> On Tue, Feb 25, 2020 at 02:56:29PM +0200, Mika Westerberg wrote:
> > On Mon, Feb 24, 2020 at 05:43:05PM +0000, Nicholas Johnson wrote:
> > > This reverts commit 03cd45d2e219301880cabc357e3cf478a500080f.
> > > 
> > > Since commit cd76ed9e5913 ("nvmem: add support for write-only
> > > instances"), this work around is no longer required, so drop it.
> > 
> > I don't think you can refer commits that only exists in your local tree.
> > I would just say that "since NVMem subsystem gained support for
> > write-only instances this workaround is not needed anymore" or somesuch.
> Are the commit hashes changed when applied to the kernel? If so, oops!

Yes, they will change once the patch gets committed to another git tree.

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

[Srinivas Kandagatla]

On 25/02/2020 15:23, Nicholas Johnson wrote:
>> Why can not we add a check for reg_read in bin_attr_nvmem_read() before
>> dereferencing it?
> That can be easily done in PATCH v2. What error code should be returned?
> 
-EPERM should be good in this case indicating Operation not permitted 
for implementation reasons!

--srini

Re: [PATCH v1 1/3] nvmem: Add support for write-only instances

[Nicholas Johnson]

On Tue, Feb 25, 2020 at 05:43:43PM +0200, Mika Westerberg wrote:
> On Tue, Feb 25, 2020 at 03:30:22PM +0000, Nicholas Johnson wrote:
> > > Actually I think maybe we make this one only writeable by root, in other
> > > words it would always require ->root_only to be set.
> > There is a world-accessible rw entry already, which would, if anything, 
> > be even more dangerous than a world writable entry. However, there could 
> > be a hypothetical use case. I agree it is unlikely to be required, but 
> > who knows?
> 
> You mean 0644 entry? That should be fine as it is not writable by anyone
> else than the owner (root in this case).
Oops, you are right. I glossed over this and in my head thought it was 
0666 for some reason, and that is why mine was 0222. Sorry for the 
confusion. :(

My 0222 would have to become 0200 which would be the same as the 
root-only one, because 0244 would be utter nonsense.

> 
> > Based on your statement that no sysfs should ever be world-writable, 
> > should I be trying to remove the world-accessible rw as well?
> 
> No I don't think it is necesary. Just let's not add attributes that
> anyone can write without good reasoning ;-)
I can change nvmem_register() to return NULL if nvmem_sysfs_get_groups() 
returns NULL and that way I can return NULL from 
nvmem_sysfs_get_groups() in the instances we do not want to honour. This 
will also remove the need for me to WARN_ON when neither reg_read nor 
reg_write are provided - I can just return NULL.

I could also change the "root_only" flag to be named "world_readable" 
and invert the logic. That way I can deny world writable and still be in 
the clear. This would make me happy about denying world-writable 
requests, because the variable being false would no longer imply 
world-writable privileges. I feel like "world_readable" is a more 
accurate description of what the variable is intended for. This can be a 
single commit with no functional changes (easy sign-off) at the start of 
the series.

Srinivas, please offer your opinion on the above proposals, if you have 
one. :)

I will aim for 2020-03-02 (Monday) for PATCH v2, to give myself adequate 
time to reflect on feedback and to try to get it right.

Thanks!

Kind regards,
Nicholas