unsigned long ioremap()?

unsigned long ioremap()?

[Geert Uytterhoeven]

	Hi,

Since you're not allowed to use direct memory dereferencing on ioremapped
areas, wouldn't it be more logical to let ioremap() return an unsigned long
instead of a void *?

Of course we then have to change readb() and friends to take a long as well,
but at least we'd get compiler warnings when someone tries to do a direct
dereference.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: unsigned long ioremap()?

[David S. Miller]

Geert Uytterhoeven writes:
 > Since you're not allowed to use direct memory dereferencing on ioremapped
 > areas, wouldn't it be more logical to let ioremap() return an unsigned long
 > instead of a void *?
 > 
 > Of course we then have to change readb() and friends to take a long as well,
 > but at least we'd get compiler warnings when someone tries to do a direct
 > dereference.

There is a school of thought which believes that:

struct xdev_regs {
	u32 reg1;
	u32 reg2;
};

	val = readl(&regs->reg2);

is cleaner than:

#define REG1 0x00
#define REG2 0x04

	val = readl(regs + REG2);

I'm personally ambivalent and believe that both cases should be allowed.

BTW, current {read,write}{b,w,l}() allow both pointer and unsigned
long arguments.  If your implementation isn't casting the port address
arg right now, perhaps you haven't tried to compile very many drivers
which use these interfaces or you're just ignoreing the warnings :-)

Later,
David S. Miller
davem@redhat.com

Re: unsigned long ioremap()?

[Jeff Garzik]

"David S. Miller" wrote:
> There is a school of thought which believes that:
> 
> struct xdev_regs {
>         u32 reg1;
>         u32 reg2;
> };
> 
>         val = readl(&regs->reg2);
> 
> is cleaner than:
> 
> #define REG1 0x00
> #define REG2 0x04
> 
>         val = readl(regs + REG2);
> 
> I'm personally ambivalent and believe that both cases should be allowed.

Agreed...  Tangent a bit, I wanted to plug using macros which IMHO make
code even more readable:

	val = RTL_R32(REG2);
	RTL_W32(REG2, val);

Since these are driver-private, if you are only dealing with one chip
you could even shorten things to "R32" and "W32", if that doesn't offend
any sensibilities :)

-- 
Jeff Garzik      | Game called on account of naked chick
Building 1024    |
MandrakeSoft     |

Re: unsigned long ioremap()?

[C.Praveen]

I apologize for ccing this to people not on the kernel list too, but i
hope  the more expert heads, the better ...

Can i do a

if (softirq_active(cpu) & softirq_mask(cpu))
    {
        do_softirq();
    }

at the end of smp_apic_timer_interrupt ? i mean

smp_apic_timer_interrupt ()
{
  irq_enter
  All it does normally.
  irq_exit
  if (softirq_active(cpu) & softirq_mask(cpu))
  {
        do_softirq();
  }
}

My understanding is that smp_apic_timer_interrupt is very similar to
do_IRQ but it knows which function to call already,
and since the do_IRQ does this at the end of its execution, it
should be ok here too. Am i ok in doing this ? basically the function
smp_apic_timer_interrupt activates a tasklet, that i would like done here
at this point, executed as a tasklet itself. If this is not ok, can
someone suggest something for acheiving this ?

Thanks for any help!

CP

Re: unsigned long ioremap()?

[Jonathan Lundell]

At 3:18 AM -0400 2001-05-03, Jeff Garzik wrote:
>"David S. Miller" wrote:
>>  There is a school of thought which believes that:
>>
>  > struct xdev_regs {
>>          u32 reg1;
>>          u32 reg2;
>>  };
>  >
>>          val = readl(&regs->reg2);
>>
>>  is cleaner than:
>>
>>  #define REG1 0x00
>>  #define REG2 0x04
>>
>>          val = readl(regs + REG2);
>>
>>  I'm personally ambivalent and believe that both cases should be allowed.
>
>Agreed...  Tangent a bit, I wanted to plug using macros which IMHO make
>code even more readable:
>
>	val = RTL_R32(REG2);
>	RTL_W32(REG2, val);
>
>Since these are driver-private, if you are only dealing with one chip
>you could even shorten things to "R32" and "W32", if that doesn't offend
>any sensibilities :)

With a little arithmetic behind the scenes and a NULL pointer to the 
struct xdev, you could have:

struct xdev_regs {
         u32 reg1;
         u32 reg2;
} *xdr = 0;

#define RTL_R32(REG) readl(cookie+(unsigned long)(&xdr->REG))

cookie = ioremap(blah, blah);

val = RTL_R32(reg2);

...and have the benefits of the R32 macro as well as the use of 
structure members.
-- 
/Jonathan Lundell.

Re: unsigned long ioremap()?

[Geert Uytterhoeven]

On Thu, 3 May 2001, Jonathan Lundell wrote:
> At 3:18 AM -0400 2001-05-03, Jeff Garzik wrote:
> >"David S. Miller" wrote:
> >>  There is a school of thought which believes that:
> >>
> >  > struct xdev_regs {
> >>          u32 reg1;
> >>          u32 reg2;
> >>  };
> >  >
> >>          val = readl(&regs->reg2);
> >>
> >>  is cleaner than:
> >>
> >>  #define REG1 0x00
> >>  #define REG2 0x04
> >>
> >>          val = readl(regs + REG2);
> >>
> >>  I'm personally ambivalent and believe that both cases should be allowed.
> >
> >Agreed...  Tangent a bit, I wanted to plug using macros which IMHO make
> >code even more readable:
> >
> >	val = RTL_R32(REG2);
> >	RTL_W32(REG2, val);
> >
> >Since these are driver-private, if you are only dealing with one chip
> >you could even shorten things to "R32" and "W32", if that doesn't offend
> >any sensibilities :)
> 
> With a little arithmetic behind the scenes and a NULL pointer to the 
> struct xdev, you could have:
> 
> struct xdev_regs {
>          u32 reg1;
>          u32 reg2;
> } *xdr = 0;
> 
> #define RTL_R32(REG) readl(cookie+(unsigned long)(&xdr->REG))

You can easily get rid of the xdr variable by s/xdr/((struct xdev_regs *)0)/.

> cookie = ioremap(blah, blah);
> 
> val = RTL_R32(reg2);
> 
> ...and have the benefits of the R32 macro as well as the use of 
> structure members.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: unsigned long ioremap()?

[Abramo Bagnara]

"David S. Miller" wrote:
> 
> Geert Uytterhoeven writes:
>  > Since you're not allowed to use direct memory dereferencing on ioremapped
>  > areas, wouldn't it be more logical to let ioremap() return an unsigned long
>  > instead of a void *?
>  >
>  > Of course we then have to change readb() and friends to take a long as well,
>  > but at least we'd get compiler warnings when someone tries to do a direct
>  > dereference.
> 
> There is a school of thought which believes that:
> 
> struct xdev_regs {
>         u32 reg1;
>         u32 reg2;
> };
> 
>         val = readl(&regs->reg2);
> 
> is cleaner than:
> 
> #define REG1 0x00
> #define REG2 0x04
> 
>         val = readl(regs + REG2);
> 
> I'm personally ambivalent and believe that both cases should be allowed.

The problem I see is that with the former solution nothing prevents from
to do:

	regs->reg2 = 13;

That's indeed the reason to change ioremap prototype for 2.5.

-- 
Abramo Bagnara                       mailto:abramo@alsa-project.org

Opera Unica                          Phone: +39.546.656023
Via Emilia Interna, 140
48014 Castel Bolognese (RA) - Italy

ALSA project               http://www.alsa-project.org
It sounds good!

Re: unsigned long ioremap()?

[Jeff Garzik]

Abramo Bagnara wrote:
> "David S. Miller" wrote:
> > There is a school of thought which believes that:
> >
> > struct xdev_regs {
> >         u32 reg1;
> >         u32 reg2;
> > };
> >
> >         val = readl(&regs->reg2);
> >
> > is cleaner than:
> >
> > #define REG1 0x00
> > #define REG2 0x04
> >
> >         val = readl(regs + REG2);

> The problem I see is that with the former solution nothing prevents from
> to do:
> 
>         regs->reg2 = 13;

Why should there be something to prevent that?

If a programmer does that to an ioremapped area, that is a bug.  Pure
and simple.

We do not need extra mechanisms simply to guard against programmers
doing the wrong thing all the time.


> That's indeed the reason to change ioremap prototype for 2.5.

Say what??

I have heard a good argument from rth about creating a pci_ioremap,
which takes a struct pci_dev argument.  But there is no reason to change
the ioremap prototype.

	Jeff


-- 
Jeff Garzik      | Game called on account of naked chick
Building 1024    |
MandrakeSoft     |

Re: unsigned long ioremap()?

[Geert Uytterhoeven]

On Thu, 3 May 2001, Jeff Garzik wrote:
> Abramo Bagnara wrote:
> > "David S. Miller" wrote:
> > > There is a school of thought which believes that:
> > >
> > > struct xdev_regs {
> > >         u32 reg1;
> > >         u32 reg2;
> > > };
> > >
> > >         val = readl(&regs->reg2);
> > >
> > > is cleaner than:
> > >
> > > #define REG1 0x00
> > > #define REG2 0x04
> > >
> > >         val = readl(regs + REG2);
> 
> > The problem I see is that with the former solution nothing prevents from
> > to do:
> > 
> >         regs->reg2 = 13;
> 
> Why should there be something to prevent that?

Because people can't seem to be teached to not do it?

> If a programmer does that to an ioremapped area, that is a bug.  Pure
> and simple.
> 
> We do not need extra mechanisms simply to guard against programmers
> doing the wrong thing all the time.
> 
> > That's indeed the reason to change ioremap prototype for 2.5.
> 
> Say what??
> 
> I have heard a good argument from rth about creating a pci_ioremap,
> which takes a struct pci_dev argument.  But there is no reason to change
> the ioremap prototype.

If ioremap() would return unsigned long, the programmer at least has to cast it
before he can do the buggy thing. But indeed bad programmers probably don't
know that casts are evil (except when used very carefully).

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: unsigned long ioremap()?

[Abramo Bagnara]

Jeff Garzik wrote:
> 
> Abramo Bagnara wrote:
> > "David S. Miller" wrote:
> > > There is a school of thought which believes that:
> > >
> > > struct xdev_regs {
> > >         u32 reg1;
> > >         u32 reg2;
> > > };
> > >
> > >         val = readl(&regs->reg2);
> > >
> > > is cleaner than:
> > >
> > > #define REG1 0x00
> > > #define REG2 0x04
> > >
> > >         val = readl(regs + REG2);
> 
> > The problem I see is that with the former solution nothing prevents from
> > to do:
> >
> >         regs->reg2 = 13;
> 
> Why should there be something to prevent that?
> 
> If a programmer does that to an ioremapped area, that is a bug.  Pure
> and simple.
> 
> We do not need extra mechanisms simply to guard against programmers
> doing the wrong thing all the time.
>
> > That's indeed the reason to change ioremap prototype for 2.5.
> 
> Say what??
> 

Please give a look
http://www.uwsg.indiana.edu/hypermail/linux/kernel/0008.1/0338.html
http://www.uwsg.indiana.edu/hypermail/linux/kernel/0008.1/0407.html

This was something that already got a wide consent.

-- 
Abramo Bagnara                       mailto:abramo@alsa-project.org

Opera Unica                          Phone: +39.546.656023
Via Emilia Interna, 140
48014 Castel Bolognese (RA) - Italy

ALSA project               http://www.alsa-project.org
It sounds good!

Re: unsigned long ioremap()?

[Jeff Garzik]

Abramo Bagnara wrote:
> > > That's indeed the reason to change ioremap prototype for 2.5.
> >
> > Say what??
> >
> 
> Please give a look
> http://www.uwsg.indiana.edu/hypermail/linux/kernel/0008.1/0338.html
> http://www.uwsg.indiana.edu/hypermail/linux/kernel/0008.1/0407.html
> 
> This was something that already got a wide consent.

Let's not return unsigned long -- DaveM's suggestion is far better. 
unsigned long is not opaque enough, IMHO.

-- 
Jeff Garzik      | Game called on account of naked chick
Building 1024    |
MandrakeSoft     |

Re: unsigned long ioremap()?

[Abramo Bagnara]

Jeff Garzik wrote:
> 
> Abramo Bagnara wrote:
> > > > That's indeed the reason to change ioremap prototype for 2.5.
> > >
> > > Say what??
> > >
> >
> > Please give a look
> > http://www.uwsg.indiana.edu/hypermail/linux/kernel/0008.1/0338.html
> > http://www.uwsg.indiana.edu/hypermail/linux/kernel/0008.1/0407.html
> >
> > This was something that already got a wide consent.
> 
> Let's not return unsigned long -- DaveM's suggestion is far better.
> unsigned long is not opaque enough, IMHO.

I'm not sure to understand what's the proposed way to do offsetting:

#define IO_ADDR(cookie, ofs)?
(maybe with #define IO_SADDR(cookie, struct_name, field))
Others?

-- 
Abramo Bagnara                       mailto:abramo@alsa-project.org

Opera Unica                          Phone: +39.546.656023
Via Emilia Interna, 140
48014 Castel Bolognese (RA) - Italy

ALSA project               http://www.alsa-project.org
It sounds good!

Re: unsigned long ioremap()?

[David Woodhouse]

abramo@alsa-project.org said:
>  The problem I see is that with the former solution nothing prevents
> from to do:

> 	regs->reg2 = 13;

> That's indeed the reason to change ioremap prototype for 2.5. 

An alternative is to add an fixed offset to the cookie before returning it, 
and subtract it again in {read,write}[bwl].

--
dwmw2

Re: unsigned long ioremap()?

[Abramo Bagnara]

David Woodhouse wrote:
> 
> abramo@alsa-project.org said:
> >  The problem I see is that with the former solution nothing prevents
> > from to do:
> 
> >       regs->reg2 = 13;
> 
> > That's indeed the reason to change ioremap prototype for 2.5.
> 
> An alternative is to add an fixed offset to the cookie before returning it,
> and subtract it again in {read,write}[bwl].

You understand that in this way you change a compile time warning in a
runtime error (conditioned to path reaching, not easy to interpret,
etc.)

IMO this is a far less effective debugging strategy.

-- 
Abramo Bagnara                       mailto:abramo@alsa-project.org

Opera Unica                          Phone: +39.546.656023
Via Emilia Interna, 140
48014 Castel Bolognese (RA) - Italy

ALSA project               http://www.alsa-project.org
It sounds good!

Re: unsigned long ioremap()?

[David S. Miller]

Abramo Bagnara writes:
 > IMO this is a far less effective debugging strategy.

I agree with you.

But guess what driver authors are going to do?  They are going to cast
the thing left and right.  And sure you can then search for that, but
it isn't likely to make people fix this from the start.

I suppose the point is that there is a fine line wrt. using APIs to
influence people to "do the right thing", and this has been
exemplified in several threads I've been involved in wrt. PCI dma
and other topics. :-)

One final point, I want to reiterate that I believe:

	foo = readl(&regs->bar);

is perfectly legal and should not be discouraged and in particular,
not made painful to do.

Later,
David S. Miller
davem@redhat.com

Re: unsigned long ioremap()?

[Abramo Bagnara]

"David S. Miller" wrote:
> 
> Abramo Bagnara writes:
>  > IMO this is a far less effective debugging strategy.
> 
> I agree with you.
> 
> But guess what driver authors are going to do?  They are going to cast
> the thing left and right.  And sure you can then search for that, but
> it isn't likely to make people fix this from the start.

We can easily find now the current misuses (I've already posted a
complete list some time ago) and ensure that authors will do the right
thing.

True benefits will come in future from to have a correct API (the only
point to remember is that ioremap returned value and readl arguments is
*not* a pointer, this is not questionable).

> 
> I suppose the point is that there is a fine line wrt. using APIs to
> influence people to "do the right thing", and this has been
> exemplified in several threads I've been involved in wrt. PCI dma
> and other topics. :-)
> 
> One final point, I want to reiterate that I believe:
> 
>         foo = readl(&regs->bar);
> 
> is perfectly legal and should not be discouraged and in particular,
> not made painful to do.

I disagree: regs it's not a dereferenceable thing and I think it's an
abuse of pointer type. You're keeping a pointer that need a big sign on
it saying "Don't dereference me", it's a mess.

However you might like to substitute this with:

#define fld_readl(cookie, str, fld) readl(cookie + (unsigned
long)&((struct str *) 0)->fld)

or without that, it's perfectly fine to have:

regs = (struct reg *) ioremap(addr, size);
foo = readl((unsigned long)&regs->bar);

It's a driver author matter of preference, that don't touch the fact
that API is correct.

However I've verified that often this lead to unexpected errors due to
different alignment on different architecture and this is why I
personally prefer constant offsets over structures fields.

-- 
Abramo Bagnara                       mailto:abramo@alsa-project.org

Opera Unica                          Phone: +39.546.656023
Via Emilia Interna, 140
48014 Castel Bolognese (RA) - Italy

ALSA project               http://www.alsa-project.org
It sounds good!

Re: unsigned long ioremap()?

[David S. Miller]

Abramo Bagnara writes:
 > it's perfectly fine to have:
 > 
 > regs = (struct reg *) ioremap(addr, size);
 > foo = readl((unsigned long)&regs->bar);
 > 

I don't see how one can find this valid compared to my preference of
just plain readl(&regs->bar); You're telling me it's nicer to have the
butt ugly cast there which serves no purpose?

One could argue btw that structure offsets are less error prone to
code than register offset defines out the wazoo.

I think your argument here is bogus.

Later,
David S. Miller
davem@redhat.com

Re: unsigned long ioremap()?

[Abramo Bagnara]

"David S. Miller" wrote:
> 
> Abramo Bagnara writes:
>  > it's perfectly fine to have:
>  >
>  > regs = (struct reg *) ioremap(addr, size);
>  > foo = readl((unsigned long)&regs->bar);
>  >
> 
> I don't see how one can find this valid compared to my preference of
> just plain readl(&regs->bar); You're telling me it's nicer to have the
> butt ugly cast there which serves no purpose?

It's right API a bit misused (to allow your request to use fields by
name)

i.e. foo = readl((unsigned long)&regs->bar);

vs a wrong API that need a cast to be used correctly

i.e. rme9652->iobase = (unsigned long) ioremap(rme9652->port,
RME9652_IO_EXTENT);

Taken in account that the main point is to not have fake pointers here
and there, my choice would be obvious.

> One could argue btw that structure offsets are less error prone to
> code than register offset defines out the wazoo.

offset defines are never correct on some architecture while being
incorrect on some other, that's the whole point: a wrong #define is
likely squashed during the very first phase of driver development.

-- 
Abramo Bagnara                       mailto:abramo@alsa-project.org

Opera Unica                          Phone: +39.546.656023
Via Emilia Interna, 140
48014 Castel Bolognese (RA) - Italy

ALSA project               http://www.alsa-project.org
It sounds good!

Re: unsigned long ioremap()?

[Jonathan Lundell]

At 12:30 AM -0700 2001-05-04, David S. Miller wrote:
>Abramo Bagnara writes:
>  > it's perfectly fine to have:
>  >
>  > regs = (struct reg *) ioremap(addr, size);
>  > foo = readl((unsigned long)&regs->bar);
>  >
>
>I don't see how one can find this valid compared to my preference of
>just plain readl(&regs->bar); You're telling me it's nicer to have the
>butt ugly cast there which serves no purpose?
>
>One could argue btw that structure offsets are less error prone to
>code than register offset defines out the wazoo.
>
>I think your argument here is bogus.

The proposed API change serves to avoid the worse-than-butt-ugly:

	foo = regs->bar;

which on the evidence of the current kernel source is in fact a real problem.

One could imagine a pointer-type-modifier in C that says "this 
pointer can't be dereferenced, but pointer arithmetic is OK, and any 
derived pointers inherit the property", with syntax similar to 
volatile, or some kind of C++ dereference overloading, but absent 
that, a correct API offsets the marginal burden of having to cast in 
order to treat non-pointers as pointers.

As Abramo points out, if you can't abide the above cast, you can 
create a relatively trivial macro to hide the dirty work.
-- 
/Jonathan Lundell.

Re: unsigned long ioremap()?

[Jes Sorensen]

>>>>> "Abramo" == Abramo Bagnara <abramo@alsa-project.org> writes:

Abramo> "David S. Miller" wrote:
>> One final point, I want to reiterate that I believe:
>> 
>> foo = readl(&regs->bar);
>> 
>> is perfectly legal and should not be discouraged and in particular,
>> not made painful to do.

Abramo> I disagree: regs it's not a dereferenceable thing and I think
Abramo> it's an abuse of pointer type. You're keeping a pointer that
Abramo> need a big sign on it saying "Don't dereference me", it's a
Abramo> mess.

Thats complete rubbish, in many cases the regs structure matches a
regs structure seen by another CPU on the other side of the PCI bus
(ie. the firmware case). There is nothing wrong with the above
approach as long as you keep in mind that you cannot dereference the
struct without using readl and you have to make sure to explicitly do
padding in the struct (not all CPUs guarantee the same natural
alignment).

Jes

Re: unsigned long ioremap()?

[Abramo Bagnara]

Jes Sorensen wrote:
> 
> >>>>> "Abramo" == Abramo Bagnara <abramo@alsa-project.org> writes:
> 
> Abramo> "David S. Miller" wrote:
> >> One final point, I want to reiterate that I believe:
> >>
> >> foo = readl(&regs->bar);
> >>
> >> is perfectly legal and should not be discouraged and in particular,
> >> not made painful to do.
> 
> Abramo> I disagree: regs it's not a dereferenceable thing and I think
> Abramo> it's an abuse of pointer type. You're keeping a pointer that
> Abramo> need a big sign on it saying "Don't dereference me", it's a
> Abramo> mess.
> 
> Thats complete rubbish, in many cases the regs structure matches a
> regs structure seen by another CPU on the other side of the PCI bus
> (ie. the firmware case). There is nothing wrong with the above
> approach as long as you keep in mind that you cannot dereference the
> struct without using readl and you have to make sure to explicitly do
> padding in the struct (not all CPUs guarantee the same natural
> alignment).

"As long as you handle with gloves thick enough such a shit, there's no
problems.."

-- 
Abramo Bagnara                       mailto:abramo@alsa-project.org

Opera Unica                          Phone: +39.546.656023
Via Emilia Interna, 140
48014 Castel Bolognese (RA) - Italy

ALSA project               http://www.alsa-project.org
It sounds good!

Re: unsigned long ioremap()?

[David Woodhouse]

abramo@alsa-project.org said:
>  You understand that in this way you change a compile time warning in
> a runtime error (conditioned to path reaching, not easy to interpret,
> etc.)

> IMO this is a far less effective debugging strategy. 

True. Perhaps we should make sure the Stanford checker can find these bugs?

--
dwmw2

Re: unsigned long ioremap()?

[Jonathan Lundell]

At 8:55 AM +0200 2001-05-03, Geert Uytterhoeven wrote:
>Since you're not allowed to use direct memory dereferencing on ioremapped
>areas, wouldn't it be more logical to let ioremap() return an unsigned long
>instead of a void *?
>
>Of course we then have to change readb() and friends to take a long as well,
>but at least we'd get compiler warnings when someone tries to do a direct
>dereference.

Better yet, seems to me, its own type. Say: typedef unsigned long io_ref_t;

It's already done for dma_addr_t, and this seems like an analogous case.

The bigger job would be to fix all the direct dereferences (a 
worthwhile thing, I guess; a quick scan shows at least a few), as 
well as to fix uncast assignments of ioremap(). Or ideally to get rid 
of the casts (most that I see are casts to unsigned long) and type 
the receiving buffer appropriately.

It'd be a big job. And Linus further suggests that ioremap's first 
argument is an architecture-specific object, not necessarily either a 
physical CPU address or a PCI address (though it's typically both in 
many (most?) i386 implementations). Now *there'd* be a cleanup.
-- 
/Jonathan Lundell.