resource_size_t and printk()

resource_size_t and printk()

[Pierre Ossman]

Hi there!

Your commit b60ba8343b78b182c03cf239d4342785376c1ad1 has been causing me
a bit of confusion and I thought I'd point out the problem so that you
can resolve it. :)

resource_size_t is not guaranteed to be a long long, but might be a u64
or u32 depending on your .config. So you need an explicit cast in the
printk:s or you get a lot of junk on the output.

There might be other commits with the same issue. I just noticed this one.

Rgds
Pierre

Re: resource_size_t and printk()

[Greg KH]

On Tue, Jul 04, 2006 at 10:54:54PM +0200, Pierre Ossman wrote:
> Hi there!
> 
> Your commit b60ba8343b78b182c03cf239d4342785376c1ad1 has been causing me
> a bit of confusion and I thought I'd point out the problem so that you
> can resolve it. :)
> 
> resource_size_t is not guaranteed to be a long long, but might be a u64
> or u32 depending on your .config. So you need an explicit cast in the
> printk:s or you get a lot of junk on the output.

That is exactly correct.  Is there somewhere in that patch that I forgot
to fix this up properly?

thanks,

greg k-h

Re: resource_size_t and printk()

[Pierre Ossman]

Greg KH wrote:
> On Tue, Jul 04, 2006 at 10:54:54PM +0200, Pierre Ossman wrote:
>   
>> Hi there!
>>
>> Your commit b60ba8343b78b182c03cf239d4342785376c1ad1 has been causing me
>> a bit of confusion and I thought I'd point out the problem so that you
>> can resolve it. :)
>>
>> resource_size_t is not guaranteed to be a long long, but might be a u64
>> or u32 depending on your .config. So you need an explicit cast in the
>> printk:s or you get a lot of junk on the output.
>>     
>
> That is exactly correct.  Is there somewhere in that patch that I forgot
> to fix this up properly?
>
>   

In drivers/pnp/interface.c, theres a couple of these:

@@ -264,7 +264,7 @@ static ssize_t pnp_show_current_resource
 			if (pnp_port_flags(dev, i) & IORESOURCE_DISABLED)
 				pnp_printf(buffer," disabled\n");
 			else
-				pnp_printf(buffer," 0x%lx-0x%lx\n",
+				pnp_printf(buffer," 0x%llx-0x%llx\n",
 						pnp_port_start(dev, i),
 						pnp_port_end(dev, i));
 		}


Rgds
Pierre

Re: resource_size_t and printk()

[Greg KH]

On Wed, Jul 05, 2006 at 06:20:07AM +0200, Pierre Ossman wrote:
> Greg KH wrote:
> > On Tue, Jul 04, 2006 at 10:54:54PM +0200, Pierre Ossman wrote:
> >   
> >> Hi there!
> >>
> >> Your commit b60ba8343b78b182c03cf239d4342785376c1ad1 has been causing me
> >> a bit of confusion and I thought I'd point out the problem so that you
> >> can resolve it. :)
> >>
> >> resource_size_t is not guaranteed to be a long long, but might be a u64
> >> or u32 depending on your .config. So you need an explicit cast in the
> >> printk:s or you get a lot of junk on the output.
> >>     
> >
> > That is exactly correct.  Is there somewhere in that patch that I forgot
> > to fix this up properly?
> >
> >   
> 
> In drivers/pnp/interface.c, theres a couple of these:
> 
> @@ -264,7 +264,7 @@ static ssize_t pnp_show_current_resource
>  			if (pnp_port_flags(dev, i) & IORESOURCE_DISABLED)
>  				pnp_printf(buffer," disabled\n");
>  			else
> -				pnp_printf(buffer," 0x%lx-0x%lx\n",
> +				pnp_printf(buffer," 0x%llx-0x%llx\n",
>  						pnp_port_start(dev, i),
>  						pnp_port_end(dev, i));
>  		}
> 

Good catch, care to create a patch to fix these?

thanks,

greg k-h

Re: resource_size_t and printk()

[Pierre Ossman]

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

Greg KH wrote:
> Good catch, care to create a patch to fix these?
>   

Included.

[-- Attachment #2: pnp-fixprintk.patch --]
[-- Type: text/x-patch, Size: 1655 bytes --]

[PNP] Add missing casts in printk() arguments

Some resource_size_t values are fed to printk() without handling the fact
that they can have different size depending on your .config.

Signed-off-by: Pierre Ossman <drzeus@drzeus.cx>
---

 drivers/pnp/interface.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pnp/interface.c b/drivers/pnp/interface.c
index 3163e3d..0c14f4f 100644
--- a/drivers/pnp/interface.c
+++ b/drivers/pnp/interface.c
@@ -265,8 +265,8 @@ static ssize_t pnp_show_current_resource
 				pnp_printf(buffer," disabled\n");
 			else
 				pnp_printf(buffer," 0x%llx-0x%llx\n",
-						pnp_port_start(dev, i),
-						pnp_port_end(dev, i));
+					(long long)pnp_port_start(dev, i),
+					(long long)pnp_port_end(dev, i));
 		}
 	}
 	for (i = 0; i < PNP_MAX_MEM; i++) {
@@ -276,8 +276,8 @@ static ssize_t pnp_show_current_resource
 				pnp_printf(buffer," disabled\n");
 			else
 				pnp_printf(buffer," 0x%llx-0x%llx\n",
-						pnp_mem_start(dev, i),
-						pnp_mem_end(dev, i));
+					(long long)pnp_mem_start(dev, i),
+					(long long)pnp_mem_end(dev, i));
 		}
 	}
 	for (i = 0; i < PNP_MAX_IRQ; i++) {
@@ -287,7 +287,7 @@ static ssize_t pnp_show_current_resource
 				pnp_printf(buffer," disabled\n");
 			else
 				pnp_printf(buffer," %lld\n",
-						pnp_irq(dev, i));
+					(long long)pnp_irq(dev, i));
 		}
 	}
 	for (i = 0; i < PNP_MAX_DMA; i++) {
@@ -297,7 +297,7 @@ static ssize_t pnp_show_current_resource
 				pnp_printf(buffer," disabled\n");
 			else
 				pnp_printf(buffer," %lld\n",
-						pnp_dma(dev, i));
+					(long long)pnp_dma(dev, i));
 		}
 	}
 	ret = (buffer->curr - buf);

Re: resource_size_t and printk()

[Randy.Dunlap]

On Wed, 12 Jul 2006 10:18:09 +0200 Pierre Ossman wrote:

> Greg KH wrote:
> > Good catch, care to create a patch to fix these?
> >   
> 
> Included.

ugh :(

I know it's more wordy, but we usually use
(unsigned long long), not just (long long).

Wish we had an abbreviation for that.

---
~Randy

Re: resource_size_t and printk()

[Kyle McMartin]

On Wed, Jul 12, 2006 at 08:15:35AM -0700, Randy.Dunlap wrote:
> I know it's more wordy, but we usually use
> (unsigned long long), not just (long long).
> 
> Wish we had an abbreviation for that.

We have a specific qualifier for ptrdiff_t... Why not add one (untested!)
for resource_size_t, and get rid of all these bloody annoying casts?

I'm more than slightly irritated to have to go and add ugly
(unsigned long long) casts to squelch compiler whining on parisc.

This patch will probably cause even more compiler warnings thought because
gcc will try to be too smart with the use of formats...

Signed-off-by: Kyle McMartin <kyle@parisc-linux.org>

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index bed7229..2d71ff6 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -343,7 +343,8 @@ int vsnprintf(char *buf, size_t size, co
 		/* get the conversion qualifier */
 		qualifier = -1;
 		if (*fmt == 'h' || *fmt == 'l' || *fmt == 'L' ||
-		    *fmt =='Z' || *fmt == 'z' || *fmt == 't') {
+		    *fmt == 'Z' || *fmt == 'z' || *fmt == 't' ||
+		    *fmt == 'r') {
 			qualifier = *fmt;
 			++fmt;
 			if (qualifier == 'l' && *fmt == 'l') {
@@ -477,6 +478,8 @@ int vsnprintf(char *buf, size_t size, co
 			num = (unsigned short) va_arg(args, int);
 			if (flags & SIGN)
 				num = (signed short) num;
+		} else if (qualifier == 'r') {
+			num = va_arg(args, resource_size_t);
 		} else {
 			num = va_arg(args, unsigned int);
 			if (flags & SIGN)

Re: resource_size_t and printk()

[Greg KH]

On Wed, Jul 12, 2006 at 10:18:09AM +0200, Pierre Ossman wrote:
> Greg KH wrote:
> > Good catch, care to create a patch to fix these?
> >   
> 
> Included.

> [PNP] Add missing casts in printk() arguments
> 
> Some resource_size_t values are fed to printk() without handling the fact
> that they can have different size depending on your .config.
> 
> Signed-off-by: Pierre Ossman <drzeus@drzeus.cx>
> ---
> 
>  drivers/pnp/interface.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pnp/interface.c b/drivers/pnp/interface.c
> index 3163e3d..0c14f4f 100644
> --- a/drivers/pnp/interface.c
> +++ b/drivers/pnp/interface.c
> @@ -265,8 +265,8 @@ static ssize_t pnp_show_current_resource
>  				pnp_printf(buffer," disabled\n");
>  			else
>  				pnp_printf(buffer," 0x%llx-0x%llx\n",
> -						pnp_port_start(dev, i),
> -						pnp_port_end(dev, i));
> +					(long long)pnp_port_start(dev, i),
> +					(long long)pnp_port_end(dev, i));
>  		}
>  	}
>  	for (i = 0; i < PNP_MAX_MEM; i++) {
> @@ -276,8 +276,8 @@ static ssize_t pnp_show_current_resource
>  				pnp_printf(buffer," disabled\n");
>  			else
>  				pnp_printf(buffer," 0x%llx-0x%llx\n",
> -						pnp_mem_start(dev, i),
> -						pnp_mem_end(dev, i));
> +					(long long)pnp_mem_start(dev, i),
> +					(long long)pnp_mem_end(dev, i));

Like Randy said, please use "unsigned long long".

Care to redo this?

thanks,

greg k-h

Re: resource_size_t and printk()

[Alan Cox]

Ar Mer, 2006-07-12 am 14:37 -0700, ysgrifennodd Greg KH:
> Like Randy said, please use "unsigned long long".
> 
> Care to redo this?


While you are at it someone did 8139cp and used "%l" not "%ll"

Signed-off-by: Alan Cox <alan@redhat.com>

diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.18-rc1/drivers/net/8139cp.c linux-2.6.18-rc1/drivers/net/8139cp.c
--- linux.vanilla-2.6.18-rc1/drivers/net/8139cp.c	2006-07-12 12:16:46.000000000 +0100
+++ linux-2.6.18-rc1/drivers/net/8139cp.c	2006-07-12 12:47:20.000000000 +0100
@@ -1916,7 +1916,7 @@
 	regs = ioremap(pciaddr, CP_REGS_SIZE);
 	if (!regs) {
 		rc = -EIO;
-		dev_err(&pdev->dev, "Cannot map PCI MMIO (%lx@%lx)\n",
+		dev_err(&pdev->dev, "Cannot map PCI MMIO (%llx@%llx)\n",
 		       (unsigned long long)pci_resource_len(pdev, 1),
 		       (unsigned long long)pciaddr);
 		goto err_out_res;

Re: resource_size_t and printk()

[Pierre Ossman]

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

Greg KH wrote:
> Like Randy said, please use "unsigned long long".
>
> Care to redo this?
>   

Here you go.

[-- Attachment #2: pnp-fixprintk.patch --]
[-- Type: text/x-patch, Size: 1709 bytes --]

[PNP] Add missing casts in printk() arguments

Some resource_size_t values are fed to printk() without handling the fact
that they can have different size depending on your .config.

Signed-off-by: Pierre Ossman <drzeus@drzeus.cx>
---

 drivers/pnp/interface.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pnp/interface.c b/drivers/pnp/interface.c
index 3163e3d..9d8b415 100644
--- a/drivers/pnp/interface.c
+++ b/drivers/pnp/interface.c
@@ -265,8 +265,8 @@ static ssize_t pnp_show_current_resource
 				pnp_printf(buffer," disabled\n");
 			else
 				pnp_printf(buffer," 0x%llx-0x%llx\n",
-						pnp_port_start(dev, i),
-						pnp_port_end(dev, i));
+					(unsigned long long)pnp_port_start(dev, i),
+					(unsigned long long)pnp_port_end(dev, i));
 		}
 	}
 	for (i = 0; i < PNP_MAX_MEM; i++) {
@@ -276,8 +276,8 @@ static ssize_t pnp_show_current_resource
 				pnp_printf(buffer," disabled\n");
 			else
 				pnp_printf(buffer," 0x%llx-0x%llx\n",
-						pnp_mem_start(dev, i),
-						pnp_mem_end(dev, i));
+					(unsigned long long)pnp_mem_start(dev, i),
+					(unsigned long long)pnp_mem_end(dev, i));
 		}
 	}
 	for (i = 0; i < PNP_MAX_IRQ; i++) {
@@ -287,7 +287,7 @@ static ssize_t pnp_show_current_resource
 				pnp_printf(buffer," disabled\n");
 			else
 				pnp_printf(buffer," %lld\n",
-						pnp_irq(dev, i));
+					(unsigned long long)pnp_irq(dev, i));
 		}
 	}
 	for (i = 0; i < PNP_MAX_DMA; i++) {
@@ -297,7 +297,7 @@ static ssize_t pnp_show_current_resource
 				pnp_printf(buffer," disabled\n");
 			else
 				pnp_printf(buffer," %lld\n",
-						pnp_dma(dev, i));
+					(unsigned long long)pnp_dma(dev, i));
 		}
 	}
 	ret = (buffer->curr - buf);