[2/3] syscon: add support for "syscon-smc" compatible

System controllers can be placed under secure monitor control when running
under them. In order to keep existing code which accesses such system
controllers using a syscon, add support for "syscon-smc" compatible.

When enable, the syscon will handle this new compatible and look for an
"arm,smc-id" property to execute the appropriate SMC. A SMC regmap is then
created to forward register access to the secure monitor.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/mfd/syscon.c | 170 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 145 insertions(+), 25 deletions(-)

On Fri, 23 Jul 2021, Clément Léger wrote:

> System controllers can be placed under secure monitor control when running
> under them. In order to keep existing code which accesses such system
> controllers using a syscon, add support for "syscon-smc" compatible.
> 
> When enable, the syscon will handle this new compatible and look for an
> "arm,smc-id" property to execute the appropriate SMC. A SMC regmap is then
> created to forward register access to the secure monitor.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>  drivers/mfd/syscon.c | 170 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 145 insertions(+), 25 deletions(-)

I'm going to let Arnd have at this, but just a couple of points.

> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index 765c0210cb52..eb727b146315 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -40,7 +40,15 @@ static const struct regmap_config syscon_regmap_config = {
>  	.reg_stride = 4,
>  };
>  
> -static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
> +static void syscon_add(struct syscon *syscon)
> +{
> +	spin_lock(&syscon_list_slock);
> +	list_add_tail(&syscon->list, &syscon_list);
> +	spin_unlock(&syscon_list_slock);
> +}
> +
> +static struct syscon *of_syscon_register_mmio(struct device_node *np,
> +					      bool check_clk)
>  {
>  	struct clk *clk;
>  	struct syscon *syscon;
> @@ -132,10 +140,6 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
>  	syscon->regmap = regmap;
>  	syscon->np = np;
>  
> -	spin_lock(&syscon_list_slock);
> -	list_add_tail(&syscon->list, &syscon_list);
> -	spin_unlock(&syscon_list_slock);
> -
>  	return syscon;
>  
>  err_attach:
> @@ -150,8 +154,49 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
>  	return ERR_PTR(ret);
>  }
>  
> +#ifdef CONFIG_REGMAP_SMCCC
> +static struct syscon *of_syscon_register_smccc(struct device_node *np)
> +{
> +	struct syscon *syscon;
> +	struct regmap *regmap;
> +	u32 reg_io_width = 4, smc_id;
> +	int ret;
> +	struct regmap_config syscon_config = syscon_regmap_config;
> +
> +	ret = of_property_read_u32(np, "arm,smc-id", &smc_id);

So this is Arm specific.

Not sure we want to be creating bespoke support for specific
architectures/platforms in the generic syscon implementation.

> +	if (ret)
> +		return ERR_PTR(-ENODEV);
> +
> +	syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
> +	if (!syscon)
> +		return ERR_PTR(-ENOMEM);
> +
> +	of_property_read_u32(np, "reg-io-width", &reg_io_width);
> +
> +	syscon_config.name = kasprintf(GFP_KERNEL, "%pOFn@smc%x", np, smc_id);
> +	syscon_config.val_bits = reg_io_width * 8;
> +
> +	regmap = regmap_init_smccc(NULL, smc_id, &syscon_config);
> +	if (IS_ERR(regmap)) {
> +		ret = PTR_ERR(regmap);
> +		goto err_regmap;
> +	}
> +
> +	syscon->regmap = regmap;
> +	syscon->np = np;
> +
> +	return syscon;
> +
> +err_regmap:
> +	kfree(syscon_config.name);
> +	kfree(syscon);
> +
> +	return ERR_PTR(ret);
> +}
> +#endif
> +
>  static struct regmap *device_node_get_regmap(struct device_node *np,
> -					     bool check_clk)
> +					     bool check_clk, bool use_smccc)
>  {
>  	struct syscon *entry, *syscon = NULL;
>  
> @@ -165,8 +210,19 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
>  
>  	spin_unlock(&syscon_list_slock);
>  
> -	if (!syscon)
> -		syscon = of_syscon_register(np, check_clk);
> +	if (!syscon) {
> +		if (use_smccc)
> +#ifdef CONFIG_REGMAP_SMCCC
> +			syscon = of_syscon_register_smccc(np);
> +#else
> +			syscon = NULL;
> +#endif

... and we certainly don't want to be doing so using #ifdefery.

Please find a better way to support this feature.

> +		else
> +			syscon = of_syscon_register_mmio(np, check_clk);
> +
> +		if (!IS_ERR(syscon))
> +			syscon_add(syscon);
> +	}
>  
>  	if (IS_ERR(syscon))
>  		return ERR_CAST(syscon);
> @@ -176,16 +232,19 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
>  
>  struct regmap *device_node_to_regmap(struct device_node *np)
>  {
> -	return device_node_get_regmap(np, false);
> +	return device_node_get_regmap(np, false, false);
>  }
>  EXPORT_SYMBOL_GPL(device_node_to_regmap);
>  
>  struct regmap *syscon_node_to_regmap(struct device_node *np)
>  {
> -	if (!of_device_is_compatible(np, "syscon"))
> -		return ERR_PTR(-EINVAL);
> +	if (of_device_is_compatible(np, "syscon"))
> +		return device_node_get_regmap(np, true, false);
> +
> +	if (of_device_is_compatible(np, "syscon-smc"))
> +		return device_node_get_regmap(np, true, true);
>  
> -	return device_node_get_regmap(np, true);
> +	return ERR_PTR(-EINVAL);
>  }
>  EXPORT_SYMBOL_GPL(syscon_node_to_regmap);
>  
> @@ -273,19 +332,19 @@ struct regmap *syscon_regmap_lookup_by_phandle_optional(struct device_node *np,
>  }
>  EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle_optional);
>  
> -static int syscon_probe(struct platform_device *pdev)
> +struct syscon_driver_data {
> +	int (*probe_func)(struct platform_device *pdev, struct device *dev,
> +			  struct syscon *syscon);
> +};
> +
> +static int syscon_probe_mmio(struct platform_device *pdev,
> +			     struct device *dev,
> +			     struct syscon *syscon)
>  {
> -	struct device *dev = &pdev->dev;
> -	struct syscon_platform_data *pdata = dev_get_platdata(dev);
> -	struct syscon *syscon;
>  	struct regmap_config syscon_config = syscon_regmap_config;
>  	struct resource *res;
>  	void __iomem *base;
>  
> -	syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL);
> -	if (!syscon)
> -		return -ENOMEM;
> -
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!res)
>  		return -ENOENT;
> @@ -295,23 +354,84 @@ static int syscon_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	syscon_config.max_register = resource_size(res) - 4;
> -	if (pdata)
> -		syscon_config.name = pdata->label;
> +
>  	syscon->regmap = devm_regmap_init_mmio(dev, base, &syscon_config);
>  	if (IS_ERR(syscon->regmap)) {
>  		dev_err(dev, "regmap init failed\n");
>  		return PTR_ERR(syscon->regmap);
>  	}
>  
> -	platform_set_drvdata(pdev, syscon);
> +	dev_dbg(dev, "regmap_mmio %pR registered\n", res);
> +
> +	return 0;
> +}
> +
> +static const struct syscon_driver_data syscon_mmio_data = {
> +	.probe_func = &syscon_probe_mmio,
> +};
> +
> +#ifdef CONFIG_REGMAP_SMCCC
> +
> +static int syscon_probe_smc(struct platform_device *pdev,
> +			    struct device *dev,
> +			    struct syscon *syscon)
> +{
> +	struct regmap_config syscon_config = syscon_regmap_config;
> +	int smc_id, ret;
> +
> +	ret = of_property_read_u32(dev->of_node, "arm,smc-id", &smc_id);
> +	if (!ret)
> +		return -ENODEV;
> +
> +	syscon->regmap = devm_regmap_init_smccc(dev, smc_id, &syscon_config);
> +	if (IS_ERR(syscon->regmap)) {
> +		dev_err(dev, "regmap init failed\n");
> +		return PTR_ERR(syscon->regmap);
> +	}
>  
> -	dev_dbg(dev, "regmap %pR registered\n", res);
> +	dev_dbg(dev, "regmap_smccc %x registered\n", smc_id);
> +
> +	return 0;
> +}
> +
> +static const struct syscon_driver_data syscon_smc_data = {
> +	.probe_func = &syscon_probe_smc,
> +};
> +#endif
> +
> +static int syscon_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +	struct syscon_platform_data *pdata = dev_get_platdata(dev);
> +	struct regmap_config syscon_config = syscon_regmap_config;
> +	struct syscon *syscon;
> +	const struct syscon_driver_data *driver_data;
> +
> +	if (pdata)
> +		syscon_config.name = pdata->label;
> +
> +	syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL);
> +	if (!syscon)
> +		return -ENOMEM;
> +
> +	driver_data = (const struct syscon_driver_data *)
> +				platform_get_device_id(pdev)->driver_data;
> +
> +	ret = driver_data->probe_func(pdev, dev, syscon);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, syscon);
>  
>  	return 0;
>  }
>  
>  static const struct platform_device_id syscon_ids[] = {
> -	{ "syscon", },
> +	{ .name = "syscon",	.driver_data = (kernel_ulong_t)&syscon_mmio_data},
> +#ifdef CONFIG_REGMAP_SMCCC
> +	{ .name = "syscon-smc",	.driver_data = (kernel_ulong_t)&syscon_smc_data},
> +#endif
>  	{ }
>  };
>

Le Fri, 23 Jul 2021 16:27:59 +0100,
Lee Jones <lee.jones@linaro.org> a écrit :

> On Fri, 23 Jul 2021, Clément Léger wrote:
> 
> > System controllers can be placed under secure monitor control when
> > running under them. In order to keep existing code which accesses
> > such system controllers using a syscon, add support for
> > "syscon-smc" compatible.
> > 
> > When enable, the syscon will handle this new compatible and look
> > for an "arm,smc-id" property to execute the appropriate SMC. A SMC
> > regmap is then created to forward register access to the secure
> > monitor.
> > 
> > Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> > ---
> >  drivers/mfd/syscon.c | 170
> > ++++++++++++++++++++++++++++++++++++------- 1 file changed, 145
> > insertions(+), 25 deletions(-)  
> 
> I'm going to let Arnd have at this, but just a couple of points.
> 
> > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> > index 765c0210cb52..eb727b146315 100644
> > --- a/drivers/mfd/syscon.c
> > +++ b/drivers/mfd/syscon.c
> > @@ -40,7 +40,15 @@ static const struct regmap_config
> > syscon_regmap_config = { .reg_stride = 4,
> >  };
> >  
> > -static struct syscon *of_syscon_register(struct device_node *np,
> > bool check_clk) +static void syscon_add(struct syscon *syscon)
> > +{
> > +	spin_lock(&syscon_list_slock);
> > +	list_add_tail(&syscon->list, &syscon_list);
> > +	spin_unlock(&syscon_list_slock);
> > +}
> > +
> > +static struct syscon *of_syscon_register_mmio(struct device_node
> > *np,
> > +					      bool check_clk)
> >  {
> >  	struct clk *clk;
> >  	struct syscon *syscon;
> > @@ -132,10 +140,6 @@ static struct syscon
> > *of_syscon_register(struct device_node *np, bool check_clk)
> > syscon->regmap = regmap; syscon->np = np;
> >  
> > -	spin_lock(&syscon_list_slock);
> > -	list_add_tail(&syscon->list, &syscon_list);
> > -	spin_unlock(&syscon_list_slock);
> > -
> >  	return syscon;
> >  
> >  err_attach:
> > @@ -150,8 +154,49 @@ static struct syscon
> > *of_syscon_register(struct device_node *np, bool check_clk) return
> > ERR_PTR(ret); }
> >  
> > +#ifdef CONFIG_REGMAP_SMCCC
> > +static struct syscon *of_syscon_register_smccc(struct device_node
> > *np) +{
> > +	struct syscon *syscon;
> > +	struct regmap *regmap;
> > +	u32 reg_io_width = 4, smc_id;
> > +	int ret;
> > +	struct regmap_config syscon_config = syscon_regmap_config;
> > +
> > +	ret = of_property_read_u32(np, "arm,smc-id", &smc_id);  
> 
> So this is Arm specific.
> 
> Not sure we want to be creating bespoke support for specific
> architectures/platforms in the generic syscon implementation.

Agreed, I wanted to keep an existing property name but having such
thing in the syscon is indeed a bad idea.

> 
> > +	if (ret)
> > +		return ERR_PTR(-ENODEV);
> > +
> > +	syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
> > +	if (!syscon)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	of_property_read_u32(np, "reg-io-width", &reg_io_width);
> > +
> > +	syscon_config.name = kasprintf(GFP_KERNEL, "%pOFn@smc%x",
> > np, smc_id);
> > +	syscon_config.val_bits = reg_io_width * 8;
> > +
> > +	regmap = regmap_init_smccc(NULL, smc_id, &syscon_config);
> > +	if (IS_ERR(regmap)) {
> > +		ret = PTR_ERR(regmap);
> > +		goto err_regmap;
> > +	}
> > +
> > +	syscon->regmap = regmap;
> > +	syscon->np = np;
> > +
> > +	return syscon;
> > +
> > +err_regmap:
> > +	kfree(syscon_config.name);
> > +	kfree(syscon);
> > +
> > +	return ERR_PTR(ret);
> > +}
> > +#endif
> > +
> >  static struct regmap *device_node_get_regmap(struct device_node
> > *np,
> > -					     bool check_clk)
> > +					     bool check_clk, bool
> > use_smccc) {
> >  	struct syscon *entry, *syscon = NULL;
> >  
> > @@ -165,8 +210,19 @@ static struct regmap
> > *device_node_get_regmap(struct device_node *np, 
> >  	spin_unlock(&syscon_list_slock);
> >  
> > -	if (!syscon)
> > -		syscon = of_syscon_register(np, check_clk);
> > +	if (!syscon) {
> > +		if (use_smccc)
> > +#ifdef CONFIG_REGMAP_SMCCC
> > +			syscon = of_syscon_register_smccc(np);
> > +#else
> > +			syscon = NULL;
> > +#endif  
> 
> ... and we certainly don't want to be doing so using #ifdefery.
> 
> Please find a better way to support this feature.

Agreed too, best solution would probably be to allow having multiple
syscon "backends" split in multiple files which would create the
correct regmap according to the devicetree.

Clément

> 
> > +		else
> > +			syscon = of_syscon_register_mmio(np,
> > check_clk); +
> > +		if (!IS_ERR(syscon))
> > +			syscon_add(syscon);
> > +	}
> >  
> >  	if (IS_ERR(syscon))
> >  		return ERR_CAST(syscon);
> > @@ -176,16 +232,19 @@ static struct regmap
> > *device_node_get_regmap(struct device_node *np, 
> >  struct regmap *device_node_to_regmap(struct device_node *np)
> >  {
> > -	return device_node_get_regmap(np, false);
> > +	return device_node_get_regmap(np, false, false);
> >  }
> >  EXPORT_SYMBOL_GPL(device_node_to_regmap);
> >  
> >  struct regmap *syscon_node_to_regmap(struct device_node *np)
> >  {
> > -	if (!of_device_is_compatible(np, "syscon"))
> > -		return ERR_PTR(-EINVAL);
> > +	if (of_device_is_compatible(np, "syscon"))
> > +		return device_node_get_regmap(np, true, false);
> > +
> > +	if (of_device_is_compatible(np, "syscon-smc"))
> > +		return device_node_get_regmap(np, true, true);
> >  
> > -	return device_node_get_regmap(np, true);
> > +	return ERR_PTR(-EINVAL);
> >  }
> >  EXPORT_SYMBOL_GPL(syscon_node_to_regmap);
> >  
> > @@ -273,19 +332,19 @@ struct regmap
> > *syscon_regmap_lookup_by_phandle_optional(struct device_node *np, }
> >  EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle_optional);
> >  
> > -static int syscon_probe(struct platform_device *pdev)
> > +struct syscon_driver_data {
> > +	int (*probe_func)(struct platform_device *pdev, struct
> > device *dev,
> > +			  struct syscon *syscon);
> > +};
> > +
> > +static int syscon_probe_mmio(struct platform_device *pdev,
> > +			     struct device *dev,
> > +			     struct syscon *syscon)
> >  {
> > -	struct device *dev = &pdev->dev;
> > -	struct syscon_platform_data *pdata = dev_get_platdata(dev);
> > -	struct syscon *syscon;
> >  	struct regmap_config syscon_config = syscon_regmap_config;
> >  	struct resource *res;
> >  	void __iomem *base;
> >  
> > -	syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL);
> > -	if (!syscon)
> > -		return -ENOMEM;
> > -
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	if (!res)
> >  		return -ENOENT;
> > @@ -295,23 +354,84 @@ static int syscon_probe(struct
> > platform_device *pdev) return -ENOMEM;
> >  
> >  	syscon_config.max_register = resource_size(res) - 4;
> > -	if (pdata)
> > -		syscon_config.name = pdata->label;
> > +
> >  	syscon->regmap = devm_regmap_init_mmio(dev, base,
> > &syscon_config); if (IS_ERR(syscon->regmap)) {
> >  		dev_err(dev, "regmap init failed\n");
> >  		return PTR_ERR(syscon->regmap);
> >  	}
> >  
> > -	platform_set_drvdata(pdev, syscon);
> > +	dev_dbg(dev, "regmap_mmio %pR registered\n", res);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct syscon_driver_data syscon_mmio_data = {
> > +	.probe_func = &syscon_probe_mmio,
> > +};
> > +
> > +#ifdef CONFIG_REGMAP_SMCCC
> > +
> > +static int syscon_probe_smc(struct platform_device *pdev,
> > +			    struct device *dev,
> > +			    struct syscon *syscon)
> > +{
> > +	struct regmap_config syscon_config = syscon_regmap_config;
> > +	int smc_id, ret;
> > +
> > +	ret = of_property_read_u32(dev->of_node, "arm,smc-id",
> > &smc_id);
> > +	if (!ret)
> > +		return -ENODEV;
> > +
> > +	syscon->regmap = devm_regmap_init_smccc(dev, smc_id,
> > &syscon_config);
> > +	if (IS_ERR(syscon->regmap)) {
> > +		dev_err(dev, "regmap init failed\n");
> > +		return PTR_ERR(syscon->regmap);
> > +	}
> >  
> > -	dev_dbg(dev, "regmap %pR registered\n", res);
> > +	dev_dbg(dev, "regmap_smccc %x registered\n", smc_id);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct syscon_driver_data syscon_smc_data = {
> > +	.probe_func = &syscon_probe_smc,
> > +};
> > +#endif
> > +
> > +static int syscon_probe(struct platform_device *pdev)
> > +{
> > +	int ret;
> > +	struct device *dev = &pdev->dev;
> > +	struct syscon_platform_data *pdata = dev_get_platdata(dev);
> > +	struct regmap_config syscon_config = syscon_regmap_config;
> > +	struct syscon *syscon;
> > +	const struct syscon_driver_data *driver_data;
> > +
> > +	if (pdata)
> > +		syscon_config.name = pdata->label;
> > +
> > +	syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL);
> > +	if (!syscon)
> > +		return -ENOMEM;
> > +
> > +	driver_data = (const struct syscon_driver_data *)
> > +
> > platform_get_device_id(pdev)->driver_data; +
> > +	ret = driver_data->probe_func(pdev, dev, syscon);
> > +	if (ret)
> > +		return ret;
> > +
> > +	platform_set_drvdata(pdev, syscon);
> >  
> >  	return 0;
> >  }
> >  
> >  static const struct platform_device_id syscon_ids[] = {
> > -	{ "syscon", },
> > +	{ .name = "syscon",	.driver_data =
> > (kernel_ulong_t)&syscon_mmio_data}, +#ifdef CONFIG_REGMAP_SMCCC
> > +	{ .name = "syscon-smc",	.driver_data =
> > (kernel_ulong_t)&syscon_smc_data}, +#endif
> >  	{ }
> >  };
> >    
>

On Fri, Jul 23, 2021 at 3:52 PM Clément Léger <clement.leger@bootlin.com> wrote:
>
> System controllers can be placed under secure monitor control when running
> under them. In order to keep existing code which accesses such system
> controllers using a syscon, add support for "syscon-smc" compatible.
>
> When enable, the syscon will handle this new compatible and look for an
> "arm,smc-id" property to execute the appropriate SMC. A SMC regmap is then
> created to forward register access to the secure monitor.
>
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>

I don't see anything wrong with the implementation, but this worries
me conceptually, because of the ways this might get abused:

- this creates one more way to keep device drivers hidden away
  behind firmware when they should be in the kernel. You can already
  do that with separate SMC calls, but adding an indirection makes it
  sneakier. If the 'registers' in here are purely

- This may be seen as an easy way out for firmware writers that just
   expose a bare register-level interface when the correct solution would
   be to create a high-level interface.

There is also a problem with locking: In the case that both firmware and
kernel have to access registers within a syscon area, you may need to
have a semaphore to protect an atomic sequence of accesses, but since
the interface only provides a single register load/store, there is no way for
a kernel driver to serialize against a firmware-internal driver.

        Arnd

On Fri, Jul 23, 2021 at 06:07:44PM +0200, Arnd Bergmann wrote:

> There is also a problem with locking: In the case that both firmware and
> kernel have to access registers within a syscon area, you may need to
> have a semaphore to protect an atomic sequence of accesses, but since
> the interface only provides a single register load/store, there is no way for
> a kernel driver to serialize against a firmware-internal driver.

The standard solution to this for the read/modify/write case would be to
expose an explicit update_bits() operation (some hardware does this for
concurrency and/or bus bandwidth/latency reasons), though that doesn't
help with larger or multi-register sequences (and to be clear as I've
been saying I don't think we should do this at all).

Hi "Clément,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on regmap/for-next]
[also build test WARNING on robh/for-next v5.14-rc2 next-20210723]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Cl-ment-L-ger/add-SMC-based-regmap-driver-for-secure-syscon-access/20210723-215708
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next
config: riscv-nommu_k210_defconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/5b8123662c54263f6fc86b6ef9e296739fe78353
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Cl-ment-L-ger/add-SMC-based-regmap-driver-for-secure-syscon-access/20210723-215708
        git checkout 5b8123662c54263f6fc86b6ef9e296739fe78353
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=riscv 

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

All warnings (new ones prefixed by >>):

   drivers/mfd/syscon.c: In function 'syscon_probe':
>> drivers/mfd/syscon.c:407:23: warning: variable 'syscon_config' set but not used [-Wunused-but-set-variable]

     407 |  struct regmap_config syscon_config = syscon_regmap_config;
         |                       ^~~~~~~~~~~~~


vim +/syscon_config +407 drivers/mfd/syscon.c

   401	
   402	static int syscon_probe(struct platform_device *pdev)
   403	{
   404		int ret;
   405		struct device *dev = &pdev->dev;
   406		struct syscon_platform_data *pdata = dev_get_platdata(dev);
 > 407		struct regmap_config syscon_config = syscon_regmap_config;

   408		struct syscon *syscon;
   409		const struct syscon_driver_data *driver_data;
   410	
   411		if (pdata)
   412			syscon_config.name = pdata->label;
   413	
   414		syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL);
   415		if (!syscon)
   416			return -ENOMEM;
   417	
   418		driver_data = (const struct syscon_driver_data *)
   419					platform_get_device_id(pdev)->driver_data;
   420	
   421		ret = driver_data->probe_func(pdev, dev, syscon);
   422		if (ret)
   423			return ret;
   424	
   425		platform_set_drvdata(pdev, syscon);
   426	
   427		return 0;
   428	}
   429	

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

> Subject: Re: [PATCH 2/3] syscon: add support for "syscon-smc" compatible

> 

> On Fri, Jul 23, 2021 at 3:52 PM Clément Léger <clement.leger@bootlin.com>

> wrote:

> >

> > System controllers can be placed under secure monitor control when

> > running under them. In order to keep existing code which accesses such

> > system controllers using a syscon, add support for "syscon-smc" compatible.

> >

> > When enable, the syscon will handle this new compatible and look for

> > an "arm,smc-id" property to execute the appropriate SMC. A SMC regmap

> > is then created to forward register access to the secure monitor.

> >

> > Signed-off-by: Clément Léger <clement.leger@bootlin.com>

> 

> I don't see anything wrong with the implementation,


I also vote for such an implementation. Such as we have a chip has a misc
register space, part as below:

44h USB Wake-up Control Register (DGO 10) (USB_WAKEUP) 
48h PTD Pads Compensation Cell Configuration Register
4Ch Lower CA35 TS Timer First Compare Value (TSTMR_CMP0_VAL_L)
50h Upper CA35 TS Timer First Compare Value
54h Lower CA35 TS Timer Second Compare value
58h Upper CA35 TS Timer Second Compare Value
5Ch CA35 Core0 Reset Vector Base Address (DGO 8) (RVBARADDR0) 
60h CA35 Core1 Reset Vector Base Address (DGO 9) (RVBARADDR1) 
64h Medium Quality Sound Configuration Register (MQS1_CF) 32 RW 0100_0000h

It contains several functions, we need protect 5Ch, 60h to avoid
Non-secure world modify it. Others could be directly used by Linux kernel.
But we could only hide the whole register space in secure world to make
5C/60h register not touch by linux.

We not find a good way to provide high-level interface for such
a misc register space, provide register level interface would make
it easy for various drivers to use.

Thanks,
Peng.


but this worries me
> conceptually, because of the ways this might get abused:

> 

> - this creates one more way to keep device drivers hidden away

>   behind firmware when they should be in the kernel. You can already

>   do that with separate SMC calls, but adding an indirection makes it

>   sneakier. If the 'registers' in here are purely

> 

> - This may be seen as an easy way out for firmware writers that just

>    expose a bare register-level interface when the correct solution would

>    be to create a high-level interface.

> 

> There is also a problem with locking: In the case that both firmware and

> kernel have to access registers within a syscon area, you may need to have a

> semaphore to protect an atomic sequence of accesses, but since the interface

> only provides a single register load/store, there is no way for a kernel driver to

> serialize against a firmware-internal driver.

> 

>         Arnd