[PATCH] Fix readw/writew warnings in drivers/net/wireless/hermes.h

[PATCH] Fix readw/writew warnings in drivers/net/wireless/hermes.h

[Cal Peake]

Hi,

This patch fixes several dozen warnings spit out when compiling the hermes 
wireless driver.

In file included from drivers/net/wireless/orinoco.c:448:
drivers/net/wireless/hermes.h: In function `hermes_present':
drivers/net/wireless/hermes.h:398: warning: passing arg 1 of `readw' makes pointer from integer without a cast
drivers/net/wireless/hermes.h: In function `hermes_set_irqmask':
drivers/net/wireless/hermes.h:404: warning: passing arg 2 of `writew' makes pointer from integer without a cast
...

thanks,

-- Cal


Signed-off-by: Cal Peake <cp@absolutedigital.net>


diff -Nru linux-2.6.9-rc4/drivers/net/wireless/hermes.h linux-2.6.9-rc4-1/drivers/net/wireless/hermes.h
--- linux-2.6.9-rc4/drivers/net/wireless/hermes.h	2004-10-11 02:38:38.000000000 -0400
+++ linux-2.6.9-rc4-1/drivers/net/wireless/hermes.h	2004-10-11 06:56:01.000000000 -0400
@@ -364,12 +364,12 @@
 /* Register access convenience macros */
 #define hermes_read_reg(hw, off) ((hw)->io_space ? \
 	inw((hw)->iobase + ( (off) << (hw)->reg_spacing )) : \
-	readw((hw)->iobase + ( (off) << (hw)->reg_spacing )))
+	readw((void __iomem *)(hw)->iobase + ( (off) << (hw)->reg_spacing )))
 #define hermes_write_reg(hw, off, val) do { \
 	if ((hw)->io_space) \
 		outw_p((val), (hw)->iobase + ((off) << (hw)->reg_spacing)); \
 	else \
-		writew((val), (hw)->iobase + ((off) << (hw)->reg_spacing)); \
+		writew((val), (void __iomem *)(hw)->iobase + ((off) << (hw)->reg_spacing)); \
 	} while (0)
 #define hermes_read_regn(hw, name) hermes_read_reg((hw), HERMES_##name)
 #define hermes_write_regn(hw, name, val) hermes_write_reg((hw), HERMES_##name, (val))
@@ -442,7 +442,7 @@
 		 * gcc is smart enough to fold away the two swaps on
 		 * big-endian platforms. */
 		for (i = 0, p = buf; i < count; i++) {
-			*p++ = cpu_to_le16(readw(hw->iobase + off));
+			*p++ = cpu_to_le16(readw((void __iomem *)hw->iobase + off));
 		}
 	}
 }
@@ -462,7 +462,7 @@
 		 * hope gcc is smart enough to fold away the two swaps
 		 * on big-endian platforms. */
 		for (i = 0, p = buf; i < count; i++) {
-			writew(le16_to_cpu(*p++), hw->iobase + off);
+			writew(le16_to_cpu(*p++), (void __iomem *)hw->iobase + off);
 		}
 	}
 }
@@ -478,7 +478,7 @@
 			outw(0, hw->iobase + off);
 	} else {
 		for (i = 0; i < count; i++)
-			writew(0, hw->iobase + off);
+			writew(0, (void __iomem *)hw->iobase + off);
 	}
 }
 

Re: [PATCH] Fix readw/writew warnings in drivers/net/wireless/hermes.h

[Jan Dittmer]

Cal Peake wrote:
> Hi,
> 
> This patch fixes several dozen warnings spit out when compiling the hermes 
> wireless driver.
> 
> In file included from drivers/net/wireless/orinoco.c:448:
> drivers/net/wireless/hermes.h: In function `hermes_present':
> drivers/net/wireless/hermes.h:398: warning: passing arg 1 of `readw' makes pointer from integer without a cast
> drivers/net/wireless/hermes.h: In function `hermes_set_irqmask':
> drivers/net/wireless/hermes.h:404: warning: passing arg 2 of `writew' makes pointer from integer without a cast
> ...

>  	inw((hw)->iobase + ( (off) << (hw)->reg_spacing )) : \
> -	readw((hw)->iobase + ( (off) << (hw)->reg_spacing )))
> +	readw((void __iomem *)(hw)->iobase + ( (off) << (hw)->reg_spacing )))
>  #define hermes_write_reg(hw, off, val) do { \

Isn't the correct fix to declare iobase as (void __iomem *) ?

Thanks,

Jank

Re: [PATCH] Fix readw/writew warnings in drivers/net/wireless/hermes.h

[Ricky lloyd]

> Isn't the correct fix to declare iobase as (void __iomem *) ?
> 

Earlier today i had posted a patch which mainly fixes this same
problem with lotsa scsi
drivers and tulip drivers. I wondered the same "shouldnt all the addrs
be declared as
void __iomem* ??". 

-- 
-> Ricky

Re: [PATCH] Fix readw/writew warnings in drivers/net/wireless/hermes.h

[Cal Peake]

On Mon, 11 Oct 2004, Jan Dittmer wrote:

> Cal Peake wrote:
> 
> >  	inw((hw)->iobase + ( (off) << (hw)->reg_spacing )) : \
> > -	readw((hw)->iobase + ( (off) << (hw)->reg_spacing )))
> > +	readw((void __iomem *)(hw)->iobase + ( (off) << (hw)->reg_spacing )))
> >  #define hermes_write_reg(hw, off, val) do { \
> 
> Isn't the correct fix to declare iobase as (void __iomem *) ?

iobase is an unsigned long, declaring it as a void pointer is prolly not 
what we want to do here. The typecast seems proper. A lot of other drivers 
do this as well thus it must be proper ;-)

-- Cal

Re: [PATCH] Fix readw/writew warnings in drivers/net/wireless/hermes.h

[Jan Dittmer]

Cal Peake wrote:
> On Mon, 11 Oct 2004, Jan Dittmer wrote:
> 
> 
>>Cal Peake wrote:
>>
>>
>>> 	inw((hw)->iobase + ( (off) << (hw)->reg_spacing )) : \
>>>-	readw((hw)->iobase + ( (off) << (hw)->reg_spacing )))
>>>+	readw((void __iomem *)(hw)->iobase + ( (off) << (hw)->reg_spacing )))
>>> #define hermes_write_reg(hw, off, val) do { \
>>
>>Isn't the correct fix to declare iobase as (void __iomem *) ?
> 
> 
> iobase is an unsigned long, declaring it as a void pointer is prolly not 
> what we want to do here. The typecast seems proper. A lot of other drivers 
> do this as well thus it must be proper ;-)

Why is iobase a unsigned long in the first place? Isn't this broken for 
64bit archs?

Thanks,

Jan

Re: [PATCH] Fix readw/writew warnings in drivers/net/wireless/hermes.h

[David Gibson]

On Mon, Oct 11, 2004 at 05:34:20PM +0530, Ricky lloyd wrote:
> > Isn't the correct fix to declare iobase as (void __iomem *) ?
> > 
> 
> Earlier today i had posted a patch which mainly fixes this same
> problem with lotsa scsi
> drivers and tulip drivers. I wondered the same "shouldnt all the addrs
> be declared as
> void __iomem* ??". 

The trouble with that is that for some versions of the orinoco card,
the iobase refers to a legacy ISA IO address, not a memory-mapped IO
address (that's the inw()/outw() path in the macro).  That needs an
integer, rather than a pointer.

It's not clear to me which way around the cast is less ugly.

-- 
David Gibson			| For every complex problem there is a
david AT gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.
http://www.ozlabs.org/people/dgibson

Re: [PATCH] Fix readw/writew warnings in drivers/net/wireless/hermes.h

[David Gibson]

On Mon, Oct 11, 2004 at 02:29:39PM +0200, Jan Dittmer wrote:
> Cal Peake wrote:
> >On Mon, 11 Oct 2004, Jan Dittmer wrote:
> >
> >
> >>Cal Peake wrote:
> >>
> >>
> >>>	inw((hw)->iobase + ( (off) << (hw)->reg_spacing )) : \
> >>>-	readw((hw)->iobase + ( (off) << (hw)->reg_spacing )))
> >>>+	readw((void __iomem *)(hw)->iobase + ( (off) << (hw)->reg_spacing )))
> >>>#define hermes_write_reg(hw, off, val) do { \
> >>
> >>Isn't the correct fix to declare iobase as (void __iomem *) ?
> >
> >
> >iobase is an unsigned long, declaring it as a void pointer is prolly not 
> >what we want to do here. The typecast seems proper. A lot of other drivers 
> >do this as well thus it must be proper ;-)
> 
> Why is iobase a unsigned long in the first place? Isn't this broken for 
> 64bit archs?

Um, no.

-- 
David Gibson			| For every complex problem there is a
david AT gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.
http://www.ozlabs.org/people/dgibson

Re: [PATCH] Fix readw/writew warnings in drivers/net/wireless/hermes.h

[Jan Dittmer]

David Gibson wrote:
> On Mon, Oct 11, 2004 at 02:29:39PM +0200, Jan Dittmer wrote:
> 
>>Cal Peake wrote:
>>
>>>On Mon, 11 Oct 2004, Jan Dittmer wrote:
>>>
>>>
>>>
>>>>Cal Peake wrote:
>>>>
>>>>
>>>>
>>>>>	inw((hw)->iobase + ( (off) << (hw)->reg_spacing )) : \
>>>>>-	readw((hw)->iobase + ( (off) << (hw)->reg_spacing )))
>>>>>+	readw((void __iomem *)(hw)->iobase + ( (off) << (hw)->reg_spacing )))
>>>>>#define hermes_write_reg(hw, off, val) do { \
>>>>
>>>>Isn't the correct fix to declare iobase as (void __iomem *) ?
>>>
>>>
>>>iobase is an unsigned long, declaring it as a void pointer is prolly not 
>>>what we want to do here. The typecast seems proper. A lot of other drivers 
>>>do this as well thus it must be proper ;-)
>>
>>Why is iobase a unsigned long in the first place? Isn't this broken for 
>>64bit archs?
> 
> 
> Um, no.
> 

Yeah, just rememberd when sending the mail ;-). Still, most drivers seem 
to use (void __iomem *) in the declaration of their iobase.

Jan

Re: [PATCH] Fix readw/writew warnings in drivers/net/wireless/hermes.h

[Cal Peake]

On Mon, 11 Oct 2004, David Gibson wrote:

> On Mon, Oct 11, 2004 at 05:34:20PM +0530, Ricky lloyd wrote:
> > > Isn't the correct fix to declare iobase as (void __iomem *) ?
> > > 
> > 
> > Earlier today i had posted a patch which mainly fixes this same
> > problem with lotsa scsi
> > drivers and tulip drivers. I wondered the same "shouldnt all the addrs
> > be declared as
> > void __iomem* ??". 
> 
> The trouble with that is that for some versions of the orinoco card,
> the iobase refers to a legacy ISA IO address, not a memory-mapped IO
> address (that's the inw()/outw() path in the macro).  That needs an
> integer, rather than a pointer.
> 
> It's not clear to me which way around the cast is less ugly.

I guess the cast is kludgy. I just wanted to shut the warnings up, it 
prints 68 of 'em I believe.

-- Cal

Re: [PATCH] Fix readw/writew warnings in drivers/net/wireless/hermes.h

[Ben Dooks]

On Mon, Oct 11, 2004 at 02:39:00PM +0200, Jan Dittmer wrote:
> David Gibson wrote:
> >On Mon, Oct 11, 2004 at 02:29:39PM +0200, Jan Dittmer wrote:
> >
> >>Cal Peake wrote:
> >>
> >>>On Mon, 11 Oct 2004, Jan Dittmer wrote:
> >>>
> >>>
> >>>
> >>>>Cal Peake wrote:
> >>>>
> >>>>
> >>>>
> >>>>>	inw((hw)->iobase + ( (off) << (hw)->reg_spacing )) : \
> >>>>>-	readw((hw)->iobase + ( (off) << (hw)->reg_spacing )))
> >>>>>+	readw((void __iomem *)(hw)->iobase + ( (off) << (hw)->reg_spacing )))
> >>>>>#define hermes_write_reg(hw, off, val) do { \
> >>>>
> >>>>Isn't the correct fix to declare iobase as (void __iomem *) ?
> >>>
> >>>
> >>>iobase is an unsigned long, declaring it as a void pointer is prolly not 
> >>>what we want to do here. The typecast seems proper. A lot of other 
> >>>drivers do this as well thus it must be proper ;-)
> >>
> >>Why is iobase a unsigned long in the first place? Isn't this broken for 
> >>64bit archs?
> >
> >
> >Um, no.
> >
> 
> Yeah, just rememberd when sending the mail ;-). Still, most drivers seem 
> to use (void __iomem *) in the declaration of their iobase.

IIRC, ioremap() and friends all return (void __iomem *) or at least
(void *)

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [PATCH] Fix readw/writew warnings in drivers/net/wireless/hermes.h

[Pavel Roskin]

On Mon, 11 Oct 2004, Cal Peake wrote:

> This patch fixes several dozen warnings spit out when compiling the hermes
> wireless driver.

I noticed them too.

By the way, it would be nice to move the discussion to the mailing list of 
the driver, orinoco-devel@lists.sourceforge.net.  Sorry that it wasn't 
mentioned in the MAINTAINERS file.  I've just submitted a patch to add the 
mailing lists to that file.

> @@ -364,12 +364,12 @@
> /* Register access convenience macros */
> #define hermes_read_reg(hw, off) ((hw)->io_space ? \
> 	inw((hw)->iobase + ( (off) << (hw)->reg_spacing )) : \
> -	readw((hw)->iobase + ( (off) << (hw)->reg_spacing )))
> +	readw((void __iomem *)(hw)->iobase + ( (off) << (hw)->reg_spacing )))

The HEAD version of the driver aims to support Linux starting with version 
2.4.10, so you need to add some magic in kcompat.h to define __iomem.

HostAP driver uses cast to (void *), which compiles without warnings.  I 
believe it's sufficient because the call to readw() would cast the 
argument to whatever readw() needs.

Another, more sophisticated solution would be to use union for iobase:

typedef struct hermes {
         union {
                 unsigned long io;
                 void *mem;
         } base;
         int io_space; /* 1 if we IO-mapped IO, 0 for memory-mapped IO? */
 	...
}

-- 
Regards,
Pavel Roskin

Re: [PATCH] Fix readw/writew warnings in drivers/net/wireless/hermes.h

[viro]

On Mon, Oct 11, 2004 at 08:23:35AM -0400, Cal Peake wrote:
> On Mon, 11 Oct 2004, Jan Dittmer wrote:
> 
> > Cal Peake wrote:
> > 
> > >  	inw((hw)->iobase + ( (off) << (hw)->reg_spacing )) : \
> > > -	readw((hw)->iobase + ( (off) << (hw)->reg_spacing )))
> > > +	readw((void __iomem *)(hw)->iobase + ( (off) << (hw)->reg_spacing )))
> > >  #define hermes_write_reg(hw, off, val) do { \
> > 
> > Isn't the correct fix to declare iobase as (void __iomem *) ?
> 
> iobase is an unsigned long, declaring it as a void pointer is prolly not 
> what we want to do here. The typecast seems proper. A lot of other drivers 
> do this as well thus it must be proper ;-)

Typecast is not a proper solution here.   Folks, there are cleanups underway
for all that mess, but it's not _that_ simple.

And adding casts to shut the warnings up is wrong in 99% of cases.

Re: [PATCH] Fix readw/writew warnings in drivers/net/wireless/hermes.h

[Borislav Petkov]

On Monday 11 October 2004 14:31, David Gibson wrote:
Hi all,
I'm having the same pointer casting warnings with my ymfpci soundchip...
> On Mon, Oct 11, 2004 at 05:34:20PM +0530, Ricky lloyd wrote:
> > > Isn't the correct fix to declare iobase as (void __iomem *) ?
> >
> > Earlier today i had posted a patch which mainly fixes this same
> > problem with lotsa scsi
> > drivers and tulip drivers. I wondered the same "shouldnt all the addrs
> > be declared as
> > void __iomem* ??".
>
> The trouble with that is that for some versions of the orinoco card,
> the iobase refers to a legacy ISA IO address, not a memory-mapped IO
> address (that's the inw()/outw() path in the macro).  That needs an
> integer, rather than a pointer.
however, 
I don't think there should be any casting of IO addresses for the simple reason that the void __iomem * 
is a part of an interface which does the right thing in both MMIO/PIO cases. The lkml 
thread "Being more anal about iospace accesses.." contains the answer to 
that in a great detail. As a result, the right thing to do here is, I think, to declare all addrs void __iomem*.
Which leaves a question: while compiling the following code fragment:

<sound/pci/ymfpci/ymfpci_main.c>
  static inline u8 snd_ymfpci_readb(ymfpci_t *chip, u32 offset)
  {
          return readb(chip->reg_area_virt + offset);
  }

gcc complains as so:

sound/pci/ymfpci/ymfpci_main.c: In function `snd_ymfpci_readb':
sound/pci/ymfpci/ymfpci_main.c:57: warning: passing arg 1 of `readb' makes pointer from integer without a cast

Do we have to cast here or use the new interface?

>
> It's not clear to me which way around the cast is less ugly.


Boris.

Re: [PATCH] Fix readw/writew warnings in drivers/net/wireless/hermes.h

[viro]

On Mon, Oct 11, 2004 at 03:18:38PM +0200, Borislav Petkov wrote:
> that in a great detail. As a result, the right thing to do here is, I think, to declare all addrs void __iomem*.
> Which leaves a question: while compiling the following code fragment:
> 
> <sound/pci/ymfpci/ymfpci_main.c>
>   static inline u8 snd_ymfpci_readb(ymfpci_t *chip, u32 offset)
>   {
>           return readb(chip->reg_area_virt + offset);
>   }
> 
> gcc complains as so:
> 
> sound/pci/ymfpci/ymfpci_main.c: In function `snd_ymfpci_readb':
> sound/pci/ymfpci/ymfpci_main.c:57: warning: passing arg 1 of `readb' makes pointer from integer without a cast
> 
> Do we have to cast here or use the new interface?

Make ->reg_area_virt void __iomem *.  *However*, ALSA folks said that they
have already done iomem annotations in their tree, so that's an area best
left alone until they merge.

Re: [PATCH] Fix readw/writew warnings in drivers/net/wireless/hermes.h

[viro]

On Mon, Oct 11, 2004 at 09:12:03AM -0400, Pavel Roskin wrote:
> Another, more sophisticated solution would be to use union for iobase:
> 
> typedef struct hermes {
>         union {
>                 unsigned long io;
>                 void *mem;
>         } base;
>         int io_space; /* 1 if we IO-mapped IO, 0 for memory-mapped IO? */
> 	...
> }

Not needed.  Use ioread*/iowrite* family; it does what you need.

Al, putting together a patchset and documention on that sort of cleanups...

Re: [PATCH] Fix readw/writew warnings in drivers/net/wireless/hermes.h

[Cal Peake]

On Mon, 11 Oct 2004 viro@parcelfarce.linux.theplanet.co.uk wrote:

> On Mon, Oct 11, 2004 at 08:23:35AM -0400, Cal Peake wrote:
> > On Mon, 11 Oct 2004, Jan Dittmer wrote:
> > 
> > > Cal Peake wrote:
> > > 
> > > >  	inw((hw)->iobase + ( (off) << (hw)->reg_spacing )) : \
> > > > -	readw((hw)->iobase + ( (off) << (hw)->reg_spacing )))
> > > > +	readw((void __iomem *)(hw)->iobase + ( (off) << (hw)->reg_spacing )))
> > > >  #define hermes_write_reg(hw, off, val) do { \
> > > 
> > > Isn't the correct fix to declare iobase as (void __iomem *) ?
> > 
> > iobase is an unsigned long, declaring it as a void pointer is prolly not 
> > what we want to do here. The typecast seems proper. A lot of other drivers 
> > do this as well thus it must be proper ;-)
> 
> Typecast is not a proper solution here.   Folks, there are cleanups underway
> for all that mess, but it's not _that_ simple.
> 
> And adding casts to shut the warnings up is wrong in 99% of cases.

ok, I'm retarded. I'll shut up for the moment and get a clue :^)

-- Cal