[PATCH] staging: fieldbus: anybuss: force address space conversion

[PATCH] staging: fieldbus: anybuss: force address space conversion

[Sven Van Asbroeck]

The regmap's context (stored as 'void *') consists of a pointer to
__iomem. Mixing __iomem and non-__iomem addresses generates
sparse warnings.

Fix by using __force when converting to/from 'void __iomem *' and
the regmap's context.

Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
 drivers/staging/fieldbus/anybuss/arcx-anybus.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/fieldbus/anybuss/arcx-anybus.c b/drivers/staging/fieldbus/anybuss/arcx-anybus.c
index a167fb68e355..8126e5535ada 100644
--- a/drivers/staging/fieldbus/anybuss/arcx-anybus.c
+++ b/drivers/staging/fieldbus/anybuss/arcx-anybus.c
@@ -114,7 +114,7 @@ static void export_reset_1(struct device *dev, bool assert)
 static int read_reg_bus(void *context, unsigned int reg,
 			unsigned int *val)
 {
-	void __iomem *base = context;
+	void __iomem *base = (__force void __iomem *)context;
 
 	*val = readb(base + reg);
 	return 0;
@@ -123,7 +123,7 @@ static int read_reg_bus(void *context, unsigned int reg,
 static int write_reg_bus(void *context, unsigned int reg,
 			 unsigned int val)
 {
-	void __iomem *base = context;
+	void __iomem *base = (__force void __iomem *)context;
 
 	writeb(val, base + reg);
 	return 0;
@@ -153,7 +153,7 @@ static struct regmap *create_parallel_regmap(struct platform_device *pdev,
 	base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(base))
 		return ERR_CAST(base);
-	return devm_regmap_init(dev, NULL, base, &regmap_cfg);
+	return devm_regmap_init(dev, NULL, (__force void *)base, &regmap_cfg);
 }
 
 static struct anybuss_host *
-- 
2.17.1

Re: [PATCH] staging: fieldbus: anybuss: force address space conversion

[Dan Carpenter]

On Tue, May 21, 2019 at 10:51:16AM -0400, Sven Van Asbroeck wrote:
> The regmap's context (stored as 'void *') consists of a pointer to
> __iomem. Mixing __iomem and non-__iomem addresses generates
> sparse warnings.
> 
> Fix by using __force when converting to/from 'void __iomem *' and
> the regmap's context.
> 
> Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
> ---
>  drivers/staging/fieldbus/anybuss/arcx-anybus.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/fieldbus/anybuss/arcx-anybus.c b/drivers/staging/fieldbus/anybuss/arcx-anybus.c
> index a167fb68e355..8126e5535ada 100644
> --- a/drivers/staging/fieldbus/anybuss/arcx-anybus.c
> +++ b/drivers/staging/fieldbus/anybuss/arcx-anybus.c
> @@ -114,7 +114,7 @@ static void export_reset_1(struct device *dev, bool assert)
>  static int read_reg_bus(void *context, unsigned int reg,
>  			unsigned int *val)
>  {
> -	void __iomem *base = context;
> +	void __iomem *base = (__force void __iomem *)context;


There is no need to use __force.  Just:

	void __iomem *base = (void __iomem *)context;

For the rest as well.

regards,
dan carpenter

Re: [PATCH] staging: fieldbus: anybuss: force address space conversion

[Sven Van Asbroeck]

On Tue, May 21, 2019 at 11:13 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> There is no need to use __force.  Just:
>
>         void __iomem *base = (void __iomem *)context;
>
> For the rest as well.

Yes, that appears to work for 'void *' -> __iomem, but not the other way around:

+       return devm_regmap_init(dev, NULL, (void *)base, &regmap_cfg);

sparse generates:
drivers/staging/fieldbus/anybuss/arcx-anybus.c:156:16: warning: cast
removes address space of expression

Would it be a ok solution to use __force in this instance only?

Re: [PATCH] staging: fieldbus: anybuss: force address space conversion

[Greg KH]

On Tue, May 21, 2019 at 11:19:59AM -0400, Sven Van Asbroeck wrote:
> On Tue, May 21, 2019 at 11:13 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > There is no need to use __force.  Just:
> >
> >         void __iomem *base = (void __iomem *)context;
> >
> > For the rest as well.
> 
> Yes, that appears to work for 'void *' -> __iomem, but not the other way around:
> 
> +       return devm_regmap_init(dev, NULL, (void *)base, &regmap_cfg);
> 
> sparse generates:
> drivers/staging/fieldbus/anybuss/arcx-anybus.c:156:16: warning: cast
> removes address space of expression
> 
> Would it be a ok solution to use __force in this instance only?

Ick, if you are using __force, almost always something is wrong.

thanks

greg k-h

Re: [PATCH] staging: fieldbus: anybuss: force address space conversion

[Sven Van Asbroeck]

On Tue, May 21, 2019 at 11:42 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> Ick, if you are using __force, almost always something is wrong.
>

What if I create a separate structure for the regmap context ?

struct anybus_regmap_context {
        void __iomem *base;
};

Then just store the base pointer inside the struct, and pass the struct
as the regmap context:

ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
ctx->base = base;
devm_regmap_init(..., ctx);

static int write_reg_bus(void *context, unsigned int reg,
                  unsigned int val)
{
        struct anybus_regmap_context *ctx = context;
        <now access ctx->base>
}

Penalty is an additional dynamic pointer-size
allocation. Pro: it'll be formally correct ?

Re: [PATCH] staging: fieldbus: anybuss: force address space conversion

[Greg KH]

On Tue, May 21, 2019 at 11:53:15AM -0400, Sven Van Asbroeck wrote:
> On Tue, May 21, 2019 at 11:42 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > Ick, if you are using __force, almost always something is wrong.
> >
> 
> What if I create a separate structure for the regmap context ?
> 
> struct anybus_regmap_context {
>         void __iomem *base;
> };
> 
> Then just store the base pointer inside the struct, and pass the struct
> as the regmap context:
> 
> ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> ctx->base = base;
> devm_regmap_init(..., ctx);
> 
> static int write_reg_bus(void *context, unsigned int reg,
>                   unsigned int val)
> {
>         struct anybus_regmap_context *ctx = context;
>         <now access ctx->base>
> }

Ick, no.

> Penalty is an additional dynamic pointer-size
> allocation. Pro: it'll be formally correct ?

what is so odd about this code that makes you have to jump through
strange hoops that no other driver has to?

Just set your pointer types up properly to start with, and all should be
fine.  Why are you trying to cast anything here?

thanks,

greg k-h

Re: [PATCH] staging: fieldbus: anybuss: force address space conversion

[Sven Van Asbroeck]

On Tue, May 21, 2019 at 12:24 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> what is so odd about this code that makes you have to jump through
> strange hoops that no other driver has to?
>

Basically because it creates a regmap which accesses __iomem memory,
instead of i2c/spi.

This was done because future hardware in the company's pipeline will access
device register space through spi, instead of through a parallel memory bus.

The lower driver just has to create the appropriate regmap, __iomem or
spi, and pass it to the
upper driver, which does not have to know about the exact way the h/w
gets accessed.
So regmap is used as a hw abstraction layer.

The issue here is that a regmap context is a 'void *' yet the parallel
memory base pointer
is 'void __iomem *'. And so the two are incompatible.

Re: [PATCH] staging: fieldbus: anybuss: force address space conversion

[Sven Van Asbroeck]

On Tue, May 21, 2019 at 12:53 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
>
> On Tue, May 21, 2019 at 12:24 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > what is so odd about this code that makes you have to jump through
> > strange hoops that no other driver has to?
> >
>
> Basically because it creates a regmap which accesses __iomem memory,
> instead of i2c/spi.
>

Wait a second... doesn't devm_regmap_init_mmio() do exactly that ?!
How could I overlook this :( :(