[PATCH 6/9] virtio_mmio: Cast &resources[1].start to ‘unsigned int *’ to rid compiler warning

[PATCH 6/9] virtio_mmio: Cast &resources[1].start to ‘unsigned int *’ to rid compiler warning

[Lee Jones]

drivers/virtio/virtio_mmio.c: In function ‘vm_cmdline_set’:
drivers/virtio/virtio_mmio.c:535:4: warning: format ‘%u’ expects argument of type ‘unsigned int *’, but argument 4 has type ‘resource_size_t *’ [-Wformat]

Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: virtualization@lists.linux-foundation.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/virtio/virtio_mmio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 6b1b7e1..077e9ca 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -531,7 +531,7 @@ static int vm_cmdline_set(const char *device,
 	resources[0].end = memparse(device, &str) - 1;
 
 	processed = sscanf(str, "@%lli:%u%n:%d%n",
-			&base, &resources[1].start, &consumed,
+			&base, (unsigned int *)&resources[1].start, &consumed,
 			&vm_cmdline_id, &consumed);
 
 	if (processed < 2 || processed > 3 || str[consumed])
-- 
1.7.9.5

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 6/9] virtio_mmio: Cast &resources[1].start to ‘unsigned int *’ to rid compiler warning

[Rusty Russell]

Lee Jones <lee.jones@linaro.org> writes:
> drivers/virtio/virtio_mmio.c: In function ‘vm_cmdline_set’:
> drivers/virtio/virtio_mmio.c:535:4: warning: format ‘%u’ expects argument of type ‘unsigned int *’, but argument 4 has type ‘resource_size_t *’ [-Wformat]
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: virtualization@lists.linux-foundation.org
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/virtio/virtio_mmio.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 6b1b7e1..077e9ca 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -531,7 +531,7 @@ static int vm_cmdline_set(const char *device,
>  	resources[0].end = memparse(device, &str) - 1;
>  
>  	processed = sscanf(str, "@%lli:%u%n:%d%n",
> -			&base, &resources[1].start, &consumed,
> +			&base, (unsigned int *)&resources[1].start, &consumed,
>  			&vm_cmdline_id, &consumed);

This suppresses a valid warning, leaving our code no less wrong than
before.  But now it's silently wrong!

Do you want to try again?

Cheers,
Rusty.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 6/9] virtio_mmio: Cast &resources[1].start to ‘unsigned int *’ to rid compiler warning

[Lee Jones]

On Mon, 05 Nov 2012, Rusty Russell wrote:

> Lee Jones <lee.jones@linaro.org> writes:
> > drivers/virtio/virtio_mmio.c: In function ‘vm_cmdline_set’:
> > drivers/virtio/virtio_mmio.c:535:4: warning: format ‘%u’ expects argument of type ‘unsigned int *’, but argument 4 has type ‘resource_size_t *’ [-Wformat]
> >
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Cc: virtualization@lists.linux-foundation.org
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/virtio/virtio_mmio.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> > index 6b1b7e1..077e9ca 100644
> > --- a/drivers/virtio/virtio_mmio.c
> > +++ b/drivers/virtio/virtio_mmio.c
> > @@ -531,7 +531,7 @@ static int vm_cmdline_set(const char *device,
> >  	resources[0].end = memparse(device, &str) - 1;
> >  
> >  	processed = sscanf(str, "@%lli:%u%n:%d%n",
> > -			&base, &resources[1].start, &consumed,
> > +			&base, (unsigned int *)&resources[1].start, &consumed,
> >  			&vm_cmdline_id, &consumed);
> 
> This suppresses a valid warning, leaving our code no less wrong than
> before.  But now it's silently wrong!
> 
> Do you want to try again?

For sure I'll try again.

How does `s/%u/%p/` suit?

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 6/9] virtio_mmio: Cast &resources[1].start to ‘unsigned int *’ to rid compiler warning

[Pawel Moll]

On Mon, 2012-11-05 at 08:21 +0000, Lee Jones wrote:
> > >  	processed = sscanf(str, "@%lli:%u%n:%d%n",
> > > -			&base, &resources[1].start, &consumed,
> > > +			&base, (unsigned int *)&resources[1].start, &consumed,
> > >  			&vm_cmdline_id, &consumed);
> > 
> > This suppresses a valid warning, leaving our code no less wrong than
> > before.  But now it's silently wrong!
> > 
> > Do you want to try again?
> 
> For sure I'll try again.
> 
> How does `s/%u/%p/` suit?

sscanf doesn't do "%p"... The only reasonable way of fixing this is a
intermediate local variable - will post a patch in a second.

Thanks for spotting this!

Paweł


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH] virtio-mmio: Fix irq parsing in command line parameter

[Pawel Moll]

On 64-bit machines resource_size_t is a 64-bit value, while
sscanf() format for this argument was defined as "%u". Fixed
by using an intermediate local value of a known length.

Also added cleaned up the resource creation and adde extra
comments to make the parameters parsing easier to follow.

Reported-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 drivers/virtio/virtio_mmio.c |   26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 6b1b7e1..0d08843 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -521,25 +521,35 @@ static int vm_cmdline_set(const char *device,
 	int err;
 	struct resource resources[2] = {};
 	char *str;
-	long long int base;
+	long long int base, size;
+	unsigned int irq;
 	int processed, consumed = 0;
 	struct platform_device *pdev;
 
-	resources[0].flags = IORESOURCE_MEM;
-	resources[1].flags = IORESOURCE_IRQ;
-
-	resources[0].end = memparse(device, &str) - 1;
+	/* Get "size" part of the command line parameter */
+	size = memparse(device, &str) - 1;
 
+	/* Get "@<base>:<irq>[:<id>]" chunks */
 	processed = sscanf(str, "@%lli:%u%n:%d%n",
-			&base, &resources[1].start, &consumed,
+			&base, &irq, &consumed,
 			&vm_cmdline_id, &consumed);
 
+	/*
+	 * sscanf() processes 3 chunks if "<id>" is given, 2 if not;
+	 * also there must be no extra characters after the last
+	 * chunk, so str[consumed] should be '\0'
+	 */
 	if (processed < 2 || processed > 3 || str[consumed])
 		return -EINVAL;
 
+	/* Memory resource */
+	resources[0].flags = IORESOURCE_MEM;
 	resources[0].start = base;
-	resources[0].end += base;
-	resources[1].end = resources[1].start;
+	resources[0].end = base + size;
+
+	/* Interrupt resource */
+	resources[1].flags = IORESOURCE_IRQ;
+	resources[1].start = resources[1].end = irq;
 
 	if (!vm_cmdline_parent_registered) {
 		err = device_register(&vm_cmdline_parent);
-- 
1.7.10.4

Re: [PATCH] virtio-mmio: Fix irq parsing in command line parameter

[Lee Jones]

On Mon, 05 Nov 2012, Pawel Moll wrote:

> On 64-bit machines resource_size_t is a 64-bit value, while
> sscanf() format for this argument was defined as "%u". Fixed
> by using an intermediate local value of a known length.
> 
> Also added cleaned up the resource creation and adde extra
> comments to make the parameters parsing easier to follow.
> 
> Reported-by: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> ---
>  drivers/virtio/virtio_mmio.c |   26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)

Resorted to poaching now have we Pawel? ;)

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-mmio: Fix irq parsing in command line parameter

[Pawel Moll]

On Mon, 2012-11-05 at 13:44 +0000, Lee Jones wrote:
> On Mon, 05 Nov 2012, Pawel Moll wrote:
> 
> > On 64-bit machines resource_size_t is a 64-bit value, while
> > sscanf() format for this argument was defined as "%u". Fixed
> > by using an intermediate local value of a known length.
> > 
> > Also added cleaned up the resource creation and adde extra
> > comments to make the parameters parsing easier to follow.
> > 
> > Reported-by: Lee Jones <lee.jones@linaro.org>
> > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> > ---
> >  drivers/virtio/virtio_mmio.c |   26 ++++++++++++++++++--------
> >  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> Resorted to poaching now have we Pawel? ;)

Sure thing - the poached eggs are my firm favourite :-P ;-)

But seriously speaking - it's an old patch (all the comments and
resources creation, the IRQ stuff is a new thing :-), but never had
enough motivation to push it out...

Cheers!

Paweł



_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-mmio: Fix irq parsing in command line parameter

[Lee Jones]

On Mon, 05 Nov 2012, Pawel Moll wrote:

> On Mon, 2012-11-05 at 13:44 +0000, Lee Jones wrote:
> > On Mon, 05 Nov 2012, Pawel Moll wrote:
> > 
> > > On 64-bit machines resource_size_t is a 64-bit value, while
> > > sscanf() format for this argument was defined as "%u". Fixed
> > > by using an intermediate local value of a known length.
> > > 
> > > Also added cleaned up the resource creation and adde extra
> > > comments to make the parameters parsing easier to follow.
> > > 
> > > Reported-by: Lee Jones <lee.jones@linaro.org>
> > > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> > > ---
> > >  drivers/virtio/virtio_mmio.c |   26 ++++++++++++++++++--------
> > >  1 file changed, 18 insertions(+), 8 deletions(-)
> > 
> > Resorted to poaching now have we Pawel? ;)
> 
> Sure thing - the poached eggs are my firm favourite :-P ;-)
> 
> But seriously speaking - it's an old patch (all the comments and
> resources creation, the IRQ stuff is a new thing :-), but never had
> enough motivation to push it out...

I believe you, many would not. ;)

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-mmio: Fix irq parsing in command line parameter

[Rusty Russell]

Lee Jones <lee.jones@linaro.org> writes:
> On Mon, 05 Nov 2012, Pawel Moll wrote:
>
>> On 64-bit machines resource_size_t is a 64-bit value, while
>> sscanf() format for this argument was defined as "%u". Fixed
>> by using an intermediate local value of a known length.
>> 
>> Also added cleaned up the resource creation and adde extra
>> comments to make the parameters parsing easier to follow.
>> 
>> Reported-by: Lee Jones <lee.jones@linaro.org>
>> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
>> ---
>>  drivers/virtio/virtio_mmio.c |   26 ++++++++++++++++++--------
>>  1 file changed, 18 insertions(+), 8 deletions(-)
>
> Resorted to poaching now have we Pawel? ;)

I hope you were joking!  Doing your work for you isn't poaching.

Your correct response was "thanks" followed by 'Tested-by:'.

Cheers,
Rusty.

Re: [PATCH] virtio-mmio: Fix irq parsing in command line parameter

[Lee Jones]

[-- Attachment #1.1: Type: text/plain, Size: 676 bytes --]

> > Resorted to poaching now have we Pawel?

> I hope you were joking!

Yes, of course. I thought that was clearly indicated by the jovial winking
smiley. :)

I realise it wasn't obvious soley by this exchange, but Pawel and I are
actually ol' friends.

> Doing your work for you isn't poaching.

This isn't related to my work, as I have no professional interest in
virtio. In this particular instance I was trying to help out by fixing bugs
uncovered by the use of randconfig, which I'm doing in my own time, as a
hobby. I guess the use my work address clouds this fact, so I apologise for
that.

Sorry for any confusion or offence caused though Rusty. :)

Kind regards,
Lee

[-- Attachment #1.2: Type: text/html, Size: 780 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-mmio: Fix irq parsing in command line parameter

[Rusty Russell]

Lee Jones <lee.jones@linaro.org> writes:

>> > Resorted to poaching now have we Pawel?
>
>> I hope you were joking!
>
> Yes, of course. I thought that was clearly indicated by the jovial winking
> smiley. :)
>
> I realise it wasn't obvious soley by this exchange, but Pawel and I are
> actually ol' friends.
>
>> Doing your work for you isn't poaching.
>
> This isn't related to my work, as I have no professional interest in
> virtio. In this particular instance I was trying to help out by fixing bugs
> uncovered by the use of randconfig, which I'm doing in my own time, as a
> hobby. I guess the use my work address clouds this fact, so I apologise for
> that.
>
> Sorry for any confusion or offence caused though Rusty. :)
>
> Kind regards,
> Lee

No offence for me.  I just don't want bystanders to believe that there
is some etiquette other than "best patch wins".

Thanks for the clarification!
Rusty.

[PATCH v2] virtio-mmio: Fix irq parsing in the command line

[Pawel Moll]

On 64-bit machines resource_size_t is a 64-bit value, while
sscanf() format for this argument was defined as "%u". Fixed
by using an intermediate local value of a known length.

Also added cleaned up the resource creation and adde extra
comments to make the parameters parsing easier to follow.

Reported-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 drivers/virtio/virtio_mmio.c |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

Comparing to v1 I've simply "polished" the size assignment
(moved -1 from one place to another) and removed two of the
new comments, as they weren't really useful any more.

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 6b1b7e1..e807c2a 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -521,25 +521,33 @@ static int vm_cmdline_set(const char *device,
 	int err;
 	struct resource resources[2] = {};
 	char *str;
-	long long int base;
+	long long int base, size;
+	unsigned int irq;
 	int processed, consumed = 0;
 	struct platform_device *pdev;
 
-	resources[0].flags = IORESOURCE_MEM;
-	resources[1].flags = IORESOURCE_IRQ;
-
-	resources[0].end = memparse(device, &str) - 1;
+	/* Consume "size" part of the command line parameter */
+	size = memparse(device, &str);
 
+	/* Get "@<base>:<irq>[:<id>]" chunks */
 	processed = sscanf(str, "@%lli:%u%n:%d%n",
-			&base, &resources[1].start, &consumed,
+			&base, &irq, &consumed,
 			&vm_cmdline_id, &consumed);
 
+	/*
+	 * sscanf() processes 3 chunks if "<id>" is given, 2 if not;
+	 * also there must be no extra characters after the last
+	 * chunk, so str[consumed] should be '\0'
+	 */
 	if (processed < 2 || processed > 3 || str[consumed])
 		return -EINVAL;
 
+	resources[0].flags = IORESOURCE_MEM;
 	resources[0].start = base;
-	resources[0].end += base;
-	resources[1].end = resources[1].start;
+	resources[0].end = base + size - 1;
+
+	resources[1].flags = IORESOURCE_IRQ;
+	resources[1].start = resources[1].end = irq;
 
 	if (!vm_cmdline_parent_registered) {
 		err = device_register(&vm_cmdline_parent);
-- 
1.7.10.4

Re: [PATCH v2] virtio-mmio: Fix irq parsing in the command line

[Rusty Russell]

Pawel Moll <pawel.moll@arm.com> writes:

> On 64-bit machines resource_size_t is a 64-bit value, while
> sscanf() format for this argument was defined as "%u". Fixed
> by using an intermediate local value of a known length.

Actually, on 32-bit machines, too (eg. x86 with PAE).  Otherwise we'd
just change it to %lu.

> +	/* Get "@<base>:<irq>[:<id>]" chunks */
>  	processed = sscanf(str, "@%lli:%u%n:%d%n",
> -			&base, &resources[1].start, &consumed,
> +			&base, &irq, &consumed,
>  			&vm_cmdline_id, &consumed);
>  
> +	/*
> +	 * sscanf() processes 3 chunks if "<id>" is given, 2 if not;
> +	 * also there must be no extra characters after the last
> +	 * chunk, so str[consumed] should be '\0'
> +	 */
>  	if (processed < 2 || processed > 3 || str[consumed])
>  		return -EINVAL;

I would drop the > 3 case.  It's unnecessary, and you're assuming
consumed doesn't add to the count, which may be true but is a documented
sscanf weirdness so I don't like to rely on it.

Thanks,
Rusty.

[PATCH v3] virtio-mmio: Fix irq parsing in command line parameter

[Pawel Moll]

When the resource_size_t is 64-bit long, the sscanf() on
the virtio device command line paramter string may return
wrong value because its format was defined as "%u". Fixed
by using an intermediate local value of a known length.

Also added cleaned up the resource creation and added extra
comments to make the parameters parsing easier to follow.

Reported-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 drivers/virtio/virtio_mmio.c |   26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 6b1b7e1..9df4578 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -521,25 +521,33 @@ static int vm_cmdline_set(const char *device,
 	int err;
 	struct resource resources[2] = {};
 	char *str;
-	long long int base;
+	long long int base, size;
+	unsigned int irq;
 	int processed, consumed = 0;
 	struct platform_device *pdev;
 
-	resources[0].flags = IORESOURCE_MEM;
-	resources[1].flags = IORESOURCE_IRQ;
-
-	resources[0].end = memparse(device, &str) - 1;
+	/* Consume "size" part of the command line parameter */
+	size = memparse(device, &str);
 
+	/* Get "@<base>:<irq>[:<id>]" chunks */
 	processed = sscanf(str, "@%lli:%u%n:%d%n",
-			&base, &resources[1].start, &consumed,
+			&base, &irq, &consumed,
 			&vm_cmdline_id, &consumed);
 
-	if (processed < 2 || processed > 3 || str[consumed])
+	/*
+	 * sscanf() must processes at least 2 chunks; also there
+	 * must be no extra characters after the last chunk, so
+	 * str[consumed] must be '\0'
+	 */
+	if (processed < 2 || str[consumed])
 		return -EINVAL;
 
+	resources[0].flags = IORESOURCE_MEM;
 	resources[0].start = base;
-	resources[0].end += base;
-	resources[1].end = resources[1].start;
+	resources[0].end = base + size - 1;
+
+	resources[1].flags = IORESOURCE_IRQ;
+	resources[1].start = resources[1].end = irq;
 
 	if (!vm_cmdline_parent_registered) {
 		err = device_register(&vm_cmdline_parent);
-- 
1.7.10.4

Re: [PATCH v3] virtio-mmio: Fix irq parsing in command line parameter

[Rusty Russell]

Pawel Moll <pawel.moll@arm.com> writes:
> When the resource_size_t is 64-bit long, the sscanf() on
> the virtio device command line paramter string may return
> wrong value because its format was defined as "%u". Fixed
> by using an intermediate local value of a known length.
>
> Also added cleaned up the resource creation and added extra
> comments to make the parameters parsing easier to follow.

Applied.

Thanks,
Rusty.