[PATCH 0/5] nvmem: patches (set 2) for 5.7

[PATCH 0/5] nvmem: patches (set 2) for 5.7

[Srinivas Kandagatla]

Hi Greg,

Here are some nvmem patches for 5.7 which includes
- sprd nvmem provider fixes 
- mxs-ocotp driver cleanup
- add proper checks for read/write callbacks and support root-write only

If its not too late, Can you please queue them up for 5.7.

thanks for you help,
srini

Anson Huang (1):
  nvmem: mxs-ocotp: Use devm_add_action_or_reset() for cleanup

Baolin Wang (1):
  nvmem: sprd: Determine double data programming from device data

Freeman Liu (2):
  nvmem: sprd: Fix the block lock operation
  nvmem: sprd: Optimize the block lock operation

Nicholas Johnson (1):
  nvmem: Add support for write-only instances

 drivers/nvmem/core.c        | 10 +++++--
 drivers/nvmem/mxs-ocotp.c   | 30 ++++++++------------
 drivers/nvmem/nvmem-sysfs.c | 56 +++++++++++++++++++++++++++++++------
 drivers/nvmem/sprd-efuse.c  | 27 ++++++++++++++----
 4 files changed, 89 insertions(+), 34 deletions(-)

-- 
2.21.0

[PATCH 1/5] nvmem: sprd: Fix the block lock operation

[Srinivas Kandagatla]

From: Freeman Liu <freeman.liu@unisoc.com>

According to the Spreadtrum eFuse specification, we should write 0 to
the block to trigger the lock operation.

Fixes: 096030e7f449 ("nvmem: sprd: Add Spreadtrum SoCs eFuse support")
Signed-off-by: Freeman Liu <freeman.liu@unisoc.com>
Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/nvmem/sprd-efuse.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvmem/sprd-efuse.c b/drivers/nvmem/sprd-efuse.c
index 2f1e0fbd1901..7a189ef52333 100644
--- a/drivers/nvmem/sprd-efuse.c
+++ b/drivers/nvmem/sprd-efuse.c
@@ -239,7 +239,7 @@ static int sprd_efuse_raw_prog(struct sprd_efuse *efuse, u32 blk, bool doub,
 		ret = -EBUSY;
 	} else {
 		sprd_efuse_set_prog_lock(efuse, lock);
-		writel(*data, efuse->base + SPRD_EFUSE_MEM(blk));
+		writel(0, efuse->base + SPRD_EFUSE_MEM(blk));
 		sprd_efuse_set_prog_lock(efuse, false);
 	}
 
-- 
2.21.0

[PATCH 2/5] nvmem: sprd: Optimize the block lock operation

[Srinivas Kandagatla]

From: Freeman Liu <freeman.liu@unisoc.com>

We have some cases that will programme the eFuse block partially multiple
times, so we should allow the block to be programmed again if it was
programmed partially. But we should lock the block if the whole block
was programmed. Thus add a condition to validate if we need lock the
block or not.

Moreover we only enable the auto-check function when locking the block.

Signed-off-by: Freeman Liu <freeman.liu@unisoc.com>
Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/nvmem/sprd-efuse.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/nvmem/sprd-efuse.c b/drivers/nvmem/sprd-efuse.c
index 7a189ef52333..43b3f6ef8c20 100644
--- a/drivers/nvmem/sprd-efuse.c
+++ b/drivers/nvmem/sprd-efuse.c
@@ -217,12 +217,14 @@ static int sprd_efuse_raw_prog(struct sprd_efuse *efuse, u32 blk, bool doub,
 	 * Enable the auto-check function to validate if the programming is
 	 * successful.
 	 */
-	sprd_efuse_set_auto_check(efuse, true);
+	if (lock)
+		sprd_efuse_set_auto_check(efuse, true);
 
 	writel(*data, efuse->base + SPRD_EFUSE_MEM(blk));
 
 	/* Disable auto-check and data double after programming */
-	sprd_efuse_set_auto_check(efuse, false);
+	if (lock)
+		sprd_efuse_set_auto_check(efuse, false);
 	sprd_efuse_set_data_double(efuse, false);
 
 	/*
@@ -237,7 +239,7 @@ static int sprd_efuse_raw_prog(struct sprd_efuse *efuse, u32 blk, bool doub,
 		writel(SPRD_EFUSE_ERR_CLR_MASK,
 		       efuse->base + SPRD_EFUSE_ERR_CLR);
 		ret = -EBUSY;
-	} else {
+	} else if (lock) {
 		sprd_efuse_set_prog_lock(efuse, lock);
 		writel(0, efuse->base + SPRD_EFUSE_MEM(blk));
 		sprd_efuse_set_prog_lock(efuse, false);
@@ -322,6 +324,7 @@ static int sprd_efuse_read(void *context, u32 offset, void *val, size_t bytes)
 static int sprd_efuse_write(void *context, u32 offset, void *val, size_t bytes)
 {
 	struct sprd_efuse *efuse = context;
+	bool lock;
 	int ret;
 
 	ret = sprd_efuse_lock(efuse);
@@ -332,7 +335,20 @@ static int sprd_efuse_write(void *context, u32 offset, void *val, size_t bytes)
 	if (ret)
 		goto unlock;
 
-	ret = sprd_efuse_raw_prog(efuse, offset, false, false, val);
+	/*
+	 * If the writing bytes are equal with the block width, which means the
+	 * whole block will be programmed. For this case, we should not allow
+	 * this block to be programmed again by locking this block.
+	 *
+	 * If the block was programmed partially, we should allow this block to
+	 * be programmed again.
+	 */
+	if (bytes < SPRD_EFUSE_BLOCK_WIDTH)
+		lock = false;
+	else
+		lock = true;
+
+	ret = sprd_efuse_raw_prog(efuse, offset, false, lock, val);
 
 	clk_disable_unprepare(efuse->clk);
 
-- 
2.21.0

[PATCH 3/5] nvmem: sprd: Determine double data programming from device data

[Srinivas Kandagatla]

From: Baolin Wang <baolin.wang7@gmail.com>

We've saved the double data flag in the device data, so we should
use it when programming a block.

Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/nvmem/sprd-efuse.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvmem/sprd-efuse.c b/drivers/nvmem/sprd-efuse.c
index 43b3f6ef8c20..925feb21d5ad 100644
--- a/drivers/nvmem/sprd-efuse.c
+++ b/drivers/nvmem/sprd-efuse.c
@@ -324,6 +324,7 @@ static int sprd_efuse_read(void *context, u32 offset, void *val, size_t bytes)
 static int sprd_efuse_write(void *context, u32 offset, void *val, size_t bytes)
 {
 	struct sprd_efuse *efuse = context;
+	bool blk_double = efuse->data->blk_double;
 	bool lock;
 	int ret;
 
@@ -348,7 +349,7 @@ static int sprd_efuse_write(void *context, u32 offset, void *val, size_t bytes)
 	else
 		lock = true;
 
-	ret = sprd_efuse_raw_prog(efuse, offset, false, lock, val);
+	ret = sprd_efuse_raw_prog(efuse, offset, blk_double, lock, val);
 
 	clk_disable_unprepare(efuse->clk);
 
-- 
2.21.0

[PATCH 4/5] nvmem: mxs-ocotp: Use devm_add_action_or_reset() for cleanup

[Srinivas Kandagatla]

From: Anson Huang <Anson.Huang@nxp.com>

Use devm_add_action_or_reset() for cleanup to call clk_unprepare(),
which can simplify the error handling in .probe, and .remove callback
can be dropped.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/nvmem/mxs-ocotp.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/nvmem/mxs-ocotp.c b/drivers/nvmem/mxs-ocotp.c
index 8e4898dec002..588ab56d75b7 100644
--- a/drivers/nvmem/mxs-ocotp.c
+++ b/drivers/nvmem/mxs-ocotp.c
@@ -130,6 +130,11 @@ static const struct of_device_id mxs_ocotp_match[] = {
 };
 MODULE_DEVICE_TABLE(of, mxs_ocotp_match);
 
+static void mxs_ocotp_action(void *data)
+{
+	clk_unprepare(data);
+}
+
 static int mxs_ocotp_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -160,39 +165,26 @@ static int mxs_ocotp_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = devm_add_action_or_reset(&pdev->dev, mxs_ocotp_action, otp->clk);
+	if (ret)
+		return ret;
+
 	data = match->data;
 
 	ocotp_config.size = data->size;
 	ocotp_config.priv = otp;
 	ocotp_config.dev = dev;
 	otp->nvmem = devm_nvmem_register(dev, &ocotp_config);
-	if (IS_ERR(otp->nvmem)) {
-		ret = PTR_ERR(otp->nvmem);
-		goto err_clk;
-	}
+	if (IS_ERR(otp->nvmem))
+		return PTR_ERR(otp->nvmem);
 
 	platform_set_drvdata(pdev, otp);
 
-	return 0;
-
-err_clk:
-	clk_unprepare(otp->clk);
-
-	return ret;
-}
-
-static int mxs_ocotp_remove(struct platform_device *pdev)
-{
-	struct mxs_ocotp *otp = platform_get_drvdata(pdev);
-
-	clk_unprepare(otp->clk);
-
 	return 0;
 }
 
 static struct platform_driver mxs_ocotp_driver = {
 	.probe = mxs_ocotp_probe,
-	.remove = mxs_ocotp_remove,
 	.driver = {
 		.name = "mxs-ocotp",
 		.of_match_table = mxs_ocotp_match,
-- 
2.21.0

[PATCH 5/5] nvmem: Add support for write-only instances

[Srinivas Kandagatla]

From: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>

There is at least one real-world use-case for write-only nvmem
instances. Refer to 03cd45d2e219 ("thunderbolt: Prevent crash if
non-active NVMem file is read").

Add support for write-only nvmem instances by adding attrs for 0200.

Change nvmem_register() to abort if NULL group is returned from
nvmem_sysfs_get_groups().

Return NULL from nvmem_sysfs_get_groups() in invalid cases.

Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/nvmem/core.c        | 10 +++++--
 drivers/nvmem/nvmem-sysfs.c | 56 +++++++++++++++++++++++++++++++------
 2 files changed, 56 insertions(+), 10 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 77d890d3623d..ddc7be5149d5 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -381,6 +381,14 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	nvmem->type = config->type;
 	nvmem->reg_read = config->reg_read;
 	nvmem->reg_write = config->reg_write;
+	nvmem->dev.groups = nvmem_sysfs_get_groups(nvmem, config);
+	if (!nvmem->dev.groups) {
+		ida_simple_remove(&nvmem_ida, nvmem->id);
+		gpiod_put(nvmem->wp_gpio);
+		kfree(nvmem);
+		return ERR_PTR(-EINVAL);
+	}
+
 	if (!config->no_of_node)
 		nvmem->dev.of_node = config->dev->of_node;
 
@@ -395,8 +403,6 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	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);
 
 	dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
index 8759c4470012..9728da948988 100644
--- a/drivers/nvmem/nvmem-sysfs.c
+++ b/drivers/nvmem/nvmem-sysfs.c
@@ -202,16 +202,49 @@ 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;
+	/* Read-only */
+	if (nvmem->reg_read && (!nvmem->reg_write || nvmem->read_only))
+		return config->root_only ?
+			nvmem_ro_root_dev_groups : nvmem_ro_dev_groups;
+
+	/* Read-write */
+	if (nvmem->reg_read && nvmem->reg_write && !nvmem->read_only)
+		return config->root_only ?
+			nvmem_rw_root_dev_groups : nvmem_rw_dev_groups;
+
+	/* Write-only, do not honour request for global writable entry */
+	if (!nvmem->reg_read && nvmem->reg_write && !nvmem->read_only)
+		return config->root_only ? nvmem_wo_root_dev_groups : NULL;
 
-	return nvmem->read_only ? nvmem_ro_dev_groups : nvmem_rw_dev_groups;
+	return NULL;
 }
 
 /*
@@ -230,17 +263,24 @@ 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 || nvmem->read_only)) {
 		if (config->root_only)
 			nvmem->eeprom = bin_attr_ro_root_nvmem;
 		else
 			nvmem->eeprom = bin_attr_ro_nvmem;
-	} else {
+	} else if (!nvmem->reg_read && nvmem->reg_write && !nvmem->read_only) {
+		if (config->root_only)
+			nvmem->eeprom = bin_attr_wo_root_nvmem;
+		else
+			return -EINVAL;
+	} else if (nvmem->reg_read && nvmem->reg_write && !nvmem->read_only) {
 		if (config->root_only)
 			nvmem->eeprom = bin_attr_rw_root_nvmem;
 		else
 			nvmem->eeprom = bin_attr_rw_nvmem;
-	}
+	} else
+		return -EINVAL;
+
 	nvmem->eeprom.attr.name = "eeprom";
 	nvmem->eeprom.size = nvmem->size;
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
-- 
2.21.0

Re: [PATCH 1/5] nvmem: sprd: Fix the block lock operation

[Greg KH]

On Mon, Mar 23, 2020 at 03:00:03PM +0000, Srinivas Kandagatla wrote:
> From: Freeman Liu <freeman.liu@unisoc.com>
> 
> According to the Spreadtrum eFuse specification, we should write 0 to
> the block to trigger the lock operation.
> 
> Fixes: 096030e7f449 ("nvmem: sprd: Add Spreadtrum SoCs eFuse support")
> Signed-off-by: Freeman Liu <freeman.liu@unisoc.com>
> Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/nvmem/sprd-efuse.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This should go to stable, I'll add the tag...

Re: [PATCH 5/5] nvmem: Add support for write-only instances

[Greg KH]

On Mon, Mar 23, 2020 at 03:00:07PM +0000, Srinivas Kandagatla wrote:
> From: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> 
> There is at least one real-world use-case for write-only nvmem
> instances. Refer to 03cd45d2e219 ("thunderbolt: Prevent crash if
> non-active NVMem file is read").
> 
> Add support for write-only nvmem instances by adding attrs for 0200.
> 
> Change nvmem_register() to abort if NULL group is returned from
> nvmem_sysfs_get_groups().
> 
> Return NULL from nvmem_sysfs_get_groups() in invalid cases.
> 
> Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/nvmem/core.c        | 10 +++++--
>  drivers/nvmem/nvmem-sysfs.c | 56 +++++++++++++++++++++++++++++++------
>  2 files changed, 56 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 77d890d3623d..ddc7be5149d5 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -381,6 +381,14 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>  	nvmem->type = config->type;
>  	nvmem->reg_read = config->reg_read;
>  	nvmem->reg_write = config->reg_write;
> +	nvmem->dev.groups = nvmem_sysfs_get_groups(nvmem, config);
> +	if (!nvmem->dev.groups) {
> +		ida_simple_remove(&nvmem_ida, nvmem->id);
> +		gpiod_put(nvmem->wp_gpio);
> +		kfree(nvmem);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>  	if (!config->no_of_node)
>  		nvmem->dev.of_node = config->dev->of_node;
>  
> @@ -395,8 +403,6 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>  	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);
>  
>  	dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
> diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
> index 8759c4470012..9728da948988 100644
> --- a/drivers/nvmem/nvmem-sysfs.c
> +++ b/drivers/nvmem/nvmem-sysfs.c
> @@ -202,16 +202,49 @@ 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,
> +};

You are adding a new sysfs file without a Documentation/ABI/ new entry
as well?


> +
> +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;
> +	/* Read-only */
> +	if (nvmem->reg_read && (!nvmem->reg_write || nvmem->read_only))
> +		return config->root_only ?
> +			nvmem_ro_root_dev_groups : nvmem_ro_dev_groups;
> +
> +	/* Read-write */
> +	if (nvmem->reg_read && nvmem->reg_write && !nvmem->read_only)
> +		return config->root_only ?
> +			nvmem_rw_root_dev_groups : nvmem_rw_dev_groups;
> +
> +	/* Write-only, do not honour request for global writable entry */
> +	if (!nvmem->reg_read && nvmem->reg_write && !nvmem->read_only)
> +		return config->root_only ? nvmem_wo_root_dev_groups : NULL;


What is this supposed to be doing, I am totally lost.

greg k-h

Re: [PATCH 0/5] nvmem: patches (set 2) for 5.7

[Greg KH]

On Mon, Mar 23, 2020 at 03:00:02PM +0000, Srinivas Kandagatla wrote:
> Hi Greg,
> 
> Here are some nvmem patches for 5.7 which includes
> - sprd nvmem provider fixes 
> - mxs-ocotp driver cleanup
> - add proper checks for read/write callbacks and support root-write only
> 
> If its not too late, Can you please queue them up for 5.7.

I've applied the first 4 patches, and provided review comments on the
last.

thanks,

greg k-h

Re: [PATCH 5/5] nvmem: Add support for write-only instances

[Nicholas Johnson]

On Mon, Mar 23, 2020 at 08:05:05PM +0100, Greg KH wrote:
> On Mon, Mar 23, 2020 at 03:00:07PM +0000, Srinivas Kandagatla wrote:
> > From: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> > 
> > There is at least one real-world use-case for write-only nvmem
> > instances. Refer to 03cd45d2e219 ("thunderbolt: Prevent crash if
> > non-active NVMem file is read").
> > 
> > Add support for write-only nvmem instances by adding attrs for 0200.
> > 
> > Change nvmem_register() to abort if NULL group is returned from
> > nvmem_sysfs_get_groups().
> > 
> > Return NULL from nvmem_sysfs_get_groups() in invalid cases.
> > 
> > Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > ---
> >  drivers/nvmem/core.c        | 10 +++++--
> >  drivers/nvmem/nvmem-sysfs.c | 56 +++++++++++++++++++++++++++++++------
> >  2 files changed, 56 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index 77d890d3623d..ddc7be5149d5 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -381,6 +381,14 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> >  	nvmem->type = config->type;
> >  	nvmem->reg_read = config->reg_read;
> >  	nvmem->reg_write = config->reg_write;
> > +	nvmem->dev.groups = nvmem_sysfs_get_groups(nvmem, config);
> > +	if (!nvmem->dev.groups) {
> > +		ida_simple_remove(&nvmem_ida, nvmem->id);
> > +		gpiod_put(nvmem->wp_gpio);
> > +		kfree(nvmem);
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> >  	if (!config->no_of_node)
> >  		nvmem->dev.of_node = config->dev->of_node;
> >  
> > @@ -395,8 +403,6 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> >  	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);
> >  
> >  	dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
> > diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
> > index 8759c4470012..9728da948988 100644
> > --- a/drivers/nvmem/nvmem-sysfs.c
> > +++ b/drivers/nvmem/nvmem-sysfs.c
> > @@ -202,16 +202,49 @@ 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,
> > +};
> 
> You are adding a new sysfs file without a Documentation/ABI/ new entry
> as well?
It is the same existing sysfs file, but adding the ability for that 
existing sysfs file to have write-only (0200) permissions if the driver 
requires it. The perms / groups for the nvmem that is getting registered 
in nvmem_register() in drivers/nvmem/core.c are chosen by 
nvmem_sysfs_get_groups() in drivers/nvmem/nvmem-sysfs.c, and this new 
0200 will get selected if reg_read is NULL and reg_write is provided 
(and nvmem->read_only override is not set).

> 
> 
> > +
> > +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;
> > +	/* Read-only */
> > +	if (nvmem->reg_read && (!nvmem->reg_write || nvmem->read_only))
> > +		return config->root_only ?
> > +			nvmem_ro_root_dev_groups : nvmem_ro_dev_groups;
> > +
> > +	/* Read-write */
> > +	if (nvmem->reg_read && nvmem->reg_write && !nvmem->read_only)
> > +		return config->root_only ?
> > +			nvmem_rw_root_dev_groups : nvmem_rw_dev_groups;
> > +
> > +	/* Write-only, do not honour request for global writable entry */
> > +	if (!nvmem->reg_read && nvmem->reg_write && !nvmem->read_only)
> > +		return config->root_only ? nvmem_wo_root_dev_groups : NULL;
> 
> 
> What is this supposed to be doing, I am totally lost.
This nvmem_sysfs_get_groups() is called by nvmem_register() when the 
nvmem is registered by a driver. This returns the filesystem perms for 
the nvmem based on the inputs (or NULL to abort). There are four things 
we care about in the function arguments:

1. nvmem->reg_read which is a function pointer used when reading from 
the nvmem

2. nvmem->reg_write which is a function pointer used when writing to the 
nvmem

3. nvmem->read_only which overrides if nvmem is read only

4. config->root_only which indicates if world perms are used

We use these to decide which perms to return. We can determine whether 
read-only or write-only or read-write by whether the reg_read and 
reg_write are provided, using nvmem->read_only as an override (sometimes 
from dtb, both reg_read and reg_write are provided but it can override 
to read only with this flag).

Once that is decided, we use config->root_only to determine if 
world-accessible. We return the appropriate group, or NULL if 
world-writable requested.

Before this patch, we could not return NULL (always had to return a 
group, no matter what). This had to be fixed, hence the changes to the 
caller (nvmem_register() function in drivers/nvmem/core.c) to check for 
NULL group and abort. Before this patch, any (unsupported) request for 
write-only was interpreted as read-write, instead of returning an error 
and failing cleanly. As of this patch, the above 0200 which I am 
introducing is returned.

Please let me know if I have failed to clarify this for you and I will 
try again. :)
> 
> greg k-h

Kind regards,
Nicholas Johnson

Re: [PATCH 0/5] nvmem: patches (set 2) for 5.7

[Srinivas Kandagatla]

On 23/03/2020 19:06, Greg KH wrote:
> On Mon, Mar 23, 2020 at 03:00:02PM +0000, Srinivas Kandagatla wrote:
>> Hi Greg,
>>
>> Here are some nvmem patches for 5.7 which includes
>> - sprd nvmem provider fixes
>> - mxs-ocotp driver cleanup
>> - add proper checks for read/write callbacks and support root-write only
>>
>> If its not too late, Can you please queue them up for 5.7.
> 
> I've applied the first 4 patches, and provided review comments on the
> last.
> 
Thanks Greg for dropping the last patch and adding stable tag, I did 
find a 2 new issues with the last patch. Will discuss with Nicholas and 
provide a proper patch once rc1 is out or in next cycle!

thanks,
srini

> thanks,
> 
> greg k-h
> 

Re: [PATCH 5/5] nvmem: Add support for write-only instances

[Srinivas Kandagatla]

On 23/03/2020 19:05, Greg KH wrote:
> On Mon, Mar 23, 2020 at 03:00:07PM +0000, Srinivas Kandagatla wrote:
>> From: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
>>
>> There is at least one real-world use-case for write-only nvmem
>> instances. Refer to 03cd45d2e219 ("thunderbolt: Prevent crash if
>> non-active NVMem file is read").
>>
>> Add support for write-only nvmem instances by adding attrs for 0200.
>>
>> Change nvmem_register() to abort if NULL group is returned from
>> nvmem_sysfs_get_groups().
>>
>> Return NULL from nvmem_sysfs_get_groups() in invalid cases.
>>
>> Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/nvmem/core.c        | 10 +++++--
>>   drivers/nvmem/nvmem-sysfs.c | 56 +++++++++++++++++++++++++++++++------
>>   2 files changed, 56 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>> index 77d890d3623d..ddc7be5149d5 100644
>> --- a/drivers/nvmem/core.c
>> +++ b/drivers/nvmem/core.c
>> @@ -381,6 +381,14 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>>   	nvmem->type = config->type;
>>   	nvmem->reg_read = config->reg_read;
>>   	nvmem->reg_write = config->reg_write;
>> +	nvmem->dev.groups = nvmem_sysfs_get_groups(nvmem, config);
>> +	if (!nvmem->dev.groups) {
>> +		ida_simple_remove(&nvmem_ida, nvmem->id);
>> +		gpiod_put(nvmem->wp_gpio);
>> +		kfree(nvmem);
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>>   	if (!config->no_of_node)
>>   		nvmem->dev.of_node = config->dev->of_node;
>>   
>> @@ -395,8 +403,6 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>>   	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);
>>   
>>   	dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
>> diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
>> index 8759c4470012..9728da948988 100644
>> --- a/drivers/nvmem/nvmem-sysfs.c
>> +++ b/drivers/nvmem/nvmem-sysfs.c
>> @@ -202,16 +202,49 @@ 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,
>> +};
> 
> You are adding a new sysfs file without a Documentation/ABI/ new entry
> as well?

This is not a new file, we already have documentation for this file at
./Documentation/ABI/stable/sysfs-bus-nvmem

Thanks for dropping this patch, I did endup finding some issues with 
this patch.

replied to original patch on the list with CC.

> 
> 
>> +
>> +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;
>> +	/* Read-only */
>> +	if (nvmem->reg_read && (!nvmem->reg_write || nvmem->read_only))
>> +		return config->root_only ?
>> +			nvmem_ro_root_dev_groups : nvmem_ro_dev_groups;
>> +
>> +	/* Read-write */
>> +	if (nvmem->reg_read && nvmem->reg_write && !nvmem->read_only)
>> +		return config->root_only ?
>> +			nvmem_rw_root_dev_groups : nvmem_rw_dev_groups;
>> +
>> +	/* Write-only, do not honour request for global writable entry */
>> +	if (!nvmem->reg_read && nvmem->reg_write && !nvmem->read_only)
>> +		return config->root_only ? nvmem_wo_root_dev_groups : NULL;
> 
> 
> What is this supposed to be doing, I am totally lost.

I agree, its bit confusing to the reader, but it does work.

But the Idea here is :
We ended up with providing different options like read-only,root-only to 
nvmem providers combined with read/write callbacks.
With that, there are some cases which are totally invalid, existing code 
does very minimal check to ensure that before populating with correct 
attributes to sysfs file. One of such case is with thunderbolt provider 
which supports only write callback.

With this new checks in place these flags and callbacks are correctly 
validated, would result in correct file attributes.


thanks,
srini

> 
> greg k-h
> 

Re: [PATCH 5/5] nvmem: Add support for write-only instances

[Greg KH]

On Tue, Mar 24, 2020 at 12:24:00PM +0000, Srinivas Kandagatla wrote:
> 
> 
> On 23/03/2020 19:05, Greg KH wrote:
> > On Mon, Mar 23, 2020 at 03:00:07PM +0000, Srinivas Kandagatla wrote:
> > > From: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> > > 
> > > There is at least one real-world use-case for write-only nvmem
> > > instances. Refer to 03cd45d2e219 ("thunderbolt: Prevent crash if
> > > non-active NVMem file is read").
> > > 
> > > Add support for write-only nvmem instances by adding attrs for 0200.
> > > 
> > > Change nvmem_register() to abort if NULL group is returned from
> > > nvmem_sysfs_get_groups().
> > > 
> > > Return NULL from nvmem_sysfs_get_groups() in invalid cases.
> > > 
> > > Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > > ---
> > >   drivers/nvmem/core.c        | 10 +++++--
> > >   drivers/nvmem/nvmem-sysfs.c | 56 +++++++++++++++++++++++++++++++------
> > >   2 files changed, 56 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > > index 77d890d3623d..ddc7be5149d5 100644
> > > --- a/drivers/nvmem/core.c
> > > +++ b/drivers/nvmem/core.c
> > > @@ -381,6 +381,14 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> > >   	nvmem->type = config->type;
> > >   	nvmem->reg_read = config->reg_read;
> > >   	nvmem->reg_write = config->reg_write;
> > > +	nvmem->dev.groups = nvmem_sysfs_get_groups(nvmem, config);
> > > +	if (!nvmem->dev.groups) {
> > > +		ida_simple_remove(&nvmem_ida, nvmem->id);
> > > +		gpiod_put(nvmem->wp_gpio);
> > > +		kfree(nvmem);
> > > +		return ERR_PTR(-EINVAL);
> > > +	}
> > > +
> > >   	if (!config->no_of_node)
> > >   		nvmem->dev.of_node = config->dev->of_node;
> > > @@ -395,8 +403,6 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> > >   	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);
> > >   	dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
> > > diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
> > > index 8759c4470012..9728da948988 100644
> > > --- a/drivers/nvmem/nvmem-sysfs.c
> > > +++ b/drivers/nvmem/nvmem-sysfs.c
> > > @@ -202,16 +202,49 @@ 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,
> > > +};
> > 
> > You are adding a new sysfs file without a Documentation/ABI/ new entry
> > as well?
> 
> This is not a new file, we already have documentation for this file at
> ./Documentation/ABI/stable/sysfs-bus-nvmem
> 
> Thanks for dropping this patch, I did endup finding some issues with this
> patch.
> 
> replied to original patch on the list with CC.
> 
> > 
> > 
> > > +
> > > +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;
> > > +	/* Read-only */
> > > +	if (nvmem->reg_read && (!nvmem->reg_write || nvmem->read_only))
> > > +		return config->root_only ?
> > > +			nvmem_ro_root_dev_groups : nvmem_ro_dev_groups;
> > > +
> > > +	/* Read-write */
> > > +	if (nvmem->reg_read && nvmem->reg_write && !nvmem->read_only)
> > > +		return config->root_only ?
> > > +			nvmem_rw_root_dev_groups : nvmem_rw_dev_groups;
> > > +
> > > +	/* Write-only, do not honour request for global writable entry */
> > > +	if (!nvmem->reg_read && nvmem->reg_write && !nvmem->read_only)
> > > +		return config->root_only ? nvmem_wo_root_dev_groups : NULL;
> > 
> > 
> > What is this supposed to be doing, I am totally lost.
> 
> I agree, its bit confusing to the reader, but it does work.
> 
> But the Idea here is :
> We ended up with providing different options like read-only,root-only to
> nvmem providers combined with read/write callbacks.
> With that, there are some cases which are totally invalid, existing code
> does very minimal check to ensure that before populating with correct
> attributes to sysfs file. One of such case is with thunderbolt provider
> which supports only write callback.
> 
> With this new checks in place these flags and callbacks are correctly
> validated, would result in correct file attributes.

Why this crazy set of different groups?  You can set the mode of a sysfs
file in the callback for when the file is about to be created, that's so
much simpler and is what it is for.  This feels really hacky and almost
impossible to follow :(

thanks,

greg k-h

Re: [PATCH 5/5] nvmem: Add support for write-only instances

[Srinivas Kandagatla]

On 24/03/2020 12:29, Greg KH wrote:
>> But the Idea here is :
>> We ended up with providing different options like read-only,root-only to
>> nvmem providers combined with read/write callbacks.
>> With that, there are some cases which are totally invalid, existing code
>> does very minimal check to ensure that before populating with correct
>> attributes to sysfs file. One of such case is with thunderbolt provider
>> which supports only write callback.
>>
>> With this new checks in place these flags and callbacks are correctly
>> validated, would result in correct file attributes.
> Why this crazy set of different groups?  You can set the mode of a sysfs
> file in the callback for when the file is about to be created, that's so
> much simpler and is what it is for.  This feels really hacky and almost
> impossible to follow:(
Thanks for the inputs, That definitely sounds much simpler to deal with.

Am guessing you are referring to is_bin_visible callback?

I will try to clean this up!

thanks,
srini
> 
> thanks,
> 
> greg k-h

Re: [PATCH 5/5] nvmem: Add support for write-only instances

[Greg KH]

On Tue, Mar 24, 2020 at 01:25:46PM +0000, Srinivas Kandagatla wrote:
> 
> 
> On 24/03/2020 12:29, Greg KH wrote:
> > > But the Idea here is :
> > > We ended up with providing different options like read-only,root-only to
> > > nvmem providers combined with read/write callbacks.
> > > With that, there are some cases which are totally invalid, existing code
> > > does very minimal check to ensure that before populating with correct
> > > attributes to sysfs file. One of such case is with thunderbolt provider
> > > which supports only write callback.
> > > 
> > > With this new checks in place these flags and callbacks are correctly
> > > validated, would result in correct file attributes.
> > Why this crazy set of different groups?  You can set the mode of a sysfs
> > file in the callback for when the file is about to be created, that's so
> > much simpler and is what it is for.  This feels really hacky and almost
> > impossible to follow:(
> Thanks for the inputs, That definitely sounds much simpler to deal with.
> 
> Am guessing you are referring to is_bin_visible callback?

Yes.

Re: [PATCH 5/5] nvmem: Add support for write-only instances

[Nicholas Johnson]

On Tue, Mar 24, 2020 at 01:25:46PM +0000, Srinivas Kandagatla wrote:
> 
> 
> On 24/03/2020 12:29, Greg KH wrote:
> > > But the Idea here is :
> > > We ended up with providing different options like read-only,root-only to
> > > nvmem providers combined with read/write callbacks.
> > > With that, there are some cases which are totally invalid, existing code
> > > does very minimal check to ensure that before populating with correct
> > > attributes to sysfs file. One of such case is with thunderbolt provider
> > > which supports only write callback.
> > > 
> > > With this new checks in place these flags and callbacks are correctly
> > > validated, would result in correct file attributes.
> > Why this crazy set of different groups?  You can set the mode of a sysfs
> > file in the callback for when the file is about to be created, that's so
> > much simpler and is what it is for.  This feels really hacky and almost
> > impossible to follow:(
> Thanks for the inputs, That definitely sounds much simpler to deal with.
> 
> Am guessing you are referring to is_bin_visible callback?
> 
> I will try to clean this up!
I am still onboard and willing do the work, but we may need to discuss
to be on the same page with new plans. How do you wish to do this?

Does this new approach still allow us to abort if we receive an invalid
configuration? Or do we still need to have something in nvmem_register()
to abort in invalid case?

The documentation of is_bin_visible says only read/write permissions are 
accepted. Does this mean that it will not take read-only or write-only? 
That is one way of interpreting it.

I am further studying up on what was said in this email chain.

Regards,
Nicholas

> 
> thanks,
> srini
> > 
> > thanks,
> > 
> > greg k-h

Re: [PATCH 5/5] nvmem: Add support for write-only instances

[Greg KH]

On Tue, Mar 24, 2020 at 02:24:21PM +0000, Nicholas Johnson wrote:
> On Tue, Mar 24, 2020 at 01:25:46PM +0000, Srinivas Kandagatla wrote:
> > 
> > 
> > On 24/03/2020 12:29, Greg KH wrote:
> > > > But the Idea here is :
> > > > We ended up with providing different options like read-only,root-only to
> > > > nvmem providers combined with read/write callbacks.
> > > > With that, there are some cases which are totally invalid, existing code
> > > > does very minimal check to ensure that before populating with correct
> > > > attributes to sysfs file. One of such case is with thunderbolt provider
> > > > which supports only write callback.
> > > > 
> > > > With this new checks in place these flags and callbacks are correctly
> > > > validated, would result in correct file attributes.
> > > Why this crazy set of different groups?  You can set the mode of a sysfs
> > > file in the callback for when the file is about to be created, that's so
> > > much simpler and is what it is for.  This feels really hacky and almost
> > > impossible to follow:(
> > Thanks for the inputs, That definitely sounds much simpler to deal with.
> > 
> > Am guessing you are referring to is_bin_visible callback?
> > 
> > I will try to clean this up!
> I am still onboard and willing do the work, but we may need to discuss
> to be on the same page with new plans. How do you wish to do this?
> 
> Does this new approach still allow us to abort if we receive an invalid
> configuration? Or do we still need to have something in nvmem_register()
> to abort in invalid case?
> 
> The documentation of is_bin_visible says only read/write permissions are 
> accepted. Does this mean that it will not take read-only or write-only? 
> That is one way of interpreting it.

That's a funny way of interpreting it :)

Please be sane, you pass back the permissions of the file, look at all
of the places in the kernel is it used for examples...

thanks,

greg k-h

Re: [PATCH 5/5] nvmem: Add support for write-only instances

[Nicholas Johnson]

On Tue, Mar 24, 2020 at 04:18:31PM +0100, Greg KH wrote:
> On Tue, Mar 24, 2020 at 02:24:21PM +0000, Nicholas Johnson wrote:
> > On Tue, Mar 24, 2020 at 01:25:46PM +0000, Srinivas Kandagatla wrote:
> > > 
> > > 
> > > On 24/03/2020 12:29, Greg KH wrote:
> > > > > But the Idea here is :
> > > > > We ended up with providing different options like read-only,root-only to
> > > > > nvmem providers combined with read/write callbacks.
> > > > > With that, there are some cases which are totally invalid, existing code
> > > > > does very minimal check to ensure that before populating with correct
> > > > > attributes to sysfs file. One of such case is with thunderbolt provider
> > > > > which supports only write callback.
> > > > > 
> > > > > With this new checks in place these flags and callbacks are correctly
> > > > > validated, would result in correct file attributes.
> > > > Why this crazy set of different groups?  You can set the mode of a sysfs
> > > > file in the callback for when the file is about to be created, that's so
> > > > much simpler and is what it is for.  This feels really hacky and almost
> > > > impossible to follow:(
> > > Thanks for the inputs, That definitely sounds much simpler to deal with.
> > > 
> > > Am guessing you are referring to is_bin_visible callback?
> > > 
> > > I will try to clean this up!
> > I am still onboard and willing do the work, but we may need to discuss
> > to be on the same page with new plans. How do you wish to do this?
> > 
> > Does this new approach still allow us to abort if we receive an invalid
> > configuration? Or do we still need to have something in nvmem_register()
> > to abort in invalid case?
> > 
> > The documentation of is_bin_visible says only read/write permissions are 
> > accepted. Does this mean that it will not take read-only or write-only? 
> > That is one way of interpreting it.
> 
> That's a funny way of interpreting it :)
Now that I look back, yes.

> 
> Please be sane, you pass back the permissions of the file, look at all
> of the places in the kernel is it used for examples...
It's more inexperience and sleep deprivation than insanity. I am working 
on those. :)

There is only one use of is_bin_visible but a lot for is_visible, so I 
will go off those.

Regards,
Nicholas

> 
> thanks,
> 
> greg k-h

Re: [PATCH 5/5] nvmem: Add support for write-only instances

[Srinivas Kandagatla]

On 24/03/2020 14:24, Nicholas Johnson wrote:
>> Am guessing you are referring to is_bin_visible callback?
>>
>> I will try to clean this up!
> I am still onboard and willing do the work, but we may need to discuss
> to be on the same page with new plans. How do you wish to do this?
I have done some cleanup of old code to use is_bin_visible.

https://git.linaro.org/people/srinivas.kandagatla/linux.git/log/?h=topic/is_bin_visible

Can you try these two patches along with your new patch?

--srini