Re: unsigned long ioremap()?
Jes Sorensen wrote: > > > "Abramo" == Abramo Bagnara <[EMAIL PROTECTED]> writes: > > Abramo> "David S. Miller" wrote: > >> One final point, I want to reiterate that I believe: > >> > >> foo = readl(®s->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:[EMAIL PROTECTED] 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! - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: unsigned long ioremap()?
> "Abramo" == Abramo Bagnara <[EMAIL PROTECTED]> writes: Abramo> "David S. Miller" wrote: >> One final point, I want to reiterate that I believe: >> >> foo = readl(®s->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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: unsigned long ioremap()?
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)®s->bar); > > > >I don't see how one can find this valid compared to my preference of >just plain readl(®s->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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: unsigned long ioremap()?
"David S. Miller" wrote: > > Abramo Bagnara writes: > > it's perfectly fine to have: > > > > regs = (struct reg *) ioremap(addr, size); > > foo = readl((unsigned long)®s->bar); > > > > I don't see how one can find this valid compared to my preference of > just plain readl(®s->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)®s->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:[EMAIL PROTECTED] 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! - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: unsigned long ioremap()?
Abramo Bagnara writes: > it's perfectly fine to have: > > regs = (struct reg *) ioremap(addr, size); > foo = readl((unsigned long)®s->bar); > I don't see how one can find this valid compared to my preference of just plain readl(®s->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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: unsigned long ioremap()?
"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(®s->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)®s->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:[EMAIL PROTECTED] 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! - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: unsigned long ioremap()?
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(®s->bar); is perfectly legal and should not be discouraged and in particular, not made painful to do. Later, David S. Miller [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: unsigned long ioremap()?
David Woodhouse wrote:
>
> [EMAIL PROTECTED] 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:[EMAIL PROTECTED]
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!
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: unsigned long ioremap()?
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(®s->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 |
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: unsigned long ioremap()?
"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(®s->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:[EMAIL PROTECTED]
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!
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: unsigned long ioremap()?
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(®s->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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: unsigned long ioremap()?
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: unsigned long ioremap()?
"David S. Miller" wrote:
> There is a school of thought which believes that:
>
> struct xdev_regs {
> u32 reg1;
> u32 reg2;
> };
>
> val = readl(®s->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 |
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: unsigned long ioremap()?
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(®s->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
[EMAIL PROTECTED]
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
unsigned long ioremap()?
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 -- [EMAIL PROTECTED]
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
