EV-64260-BP & GT64260 bi_recs

I like this idea (a lot), but I'm not sure you were serious. The problem with this method starts when you have a lot of information to pass - it gets unwieldy and becomes almost as unreadable as a hex dump (anyone ever had to read the output from a make of the gcc compiler to decipher what went wrong? with all those command line options passed to each invocation of the compiler, the real information gets lost).

I don't think the method matters a lot, as long as there is only one way to do it (that is unlimited, flexible, extensible), not the three different methods we have today (command line, bd_t struct and bi_recs).

Oh, and for the record - I vote for b). The stuff posted by benh is a very good start for this - I really like the idea of bi_recs within bi_recs - this gives you effectively unlimited flexibility, but handlers at the top level can just skip/pass the entire record without worrying about its content. Cheers!

								Murray...

--
Murray Jensen, CSIRO Manufacturing Sci & Tech, Phone: +61 3 9662 7763
Locked Bag No. 9, Preston, Vic, 3072, Australia. Fax: +61 3 9662 7853
Internet: [email protected]

Hymod project: http://www.msa.cmst.csiro.au/projects/Hymod/

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

I'm always serious :-).

I'm going to (constantly :-) argue that if you are passing lots of information then something isn't designed correctly. It's one thing to be passing a few hardware hints or small configuration values, but I think it's quite wrong to design a bunch of dynamic software that requires a small data base to be passed into to the kernel. Please don't be confused by this comment and start aruging OF device trees on Macs. The OF tree is really just a bunch of small configuration items that allows lots of generic software to run on a variety of Mac platforms.

	-- Dan

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Then I guess that I am going to (constantly) disagree with you :-)

The more Linux can learn from the boot loader, the more generic the kernel and the wider the set of hardware a particular kernel will run on without recompilation.

I disagree with your contention (in another recent message) that this is bloat and is bad for embedded systems. It actually reduces bloat because code in the kernel that duplicates things that are done by the boot loader goes away - the kernel is smaller and more generic, and the code is cleaner (fewer #ifdefs etc)
- and a more generic kernel is more user friendly (i.e. easier for someone without 20 years Comp. Sci. experience to port).

And I think this is what is needed. Just because we have always done it one way, does not mean it is the right way, or that we have to continue to do it that way.

It is clear that (many?) others think along the same lines as I do. Evidence is the many boot loaders that implement some sort of device tree - the OpenBoot on my Ultra-60 is a good example. They are all just bunches of small configuration items, but there might be a lot of them, and/or they might have complex structure.

However, your position is the default position and requires little work. My position requires a lot of work and coordination - I don't think it is going to happen soon, if ever (although benh's posted code was the closest I've seen yet and looks very promising). Cheers!

								Murray...

--
Murray Jensen, CSIRO Manufacturing Sci & Tech, Phone: +61 3 9662 7763
Locked Bag No. 9, Preston, Vic, 3072, Australia. Fax: +61 3 9662 7853
Internet: [email protected]

Hymod project: http://www.msa.cmst.csiro.au/projects/Hymod/

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

It is quite clearly broken - a quick perusal of the code will convince you of this. The problem will be triggered whenever a character with decimal value 10 is transmitted via my_console_write(), and the buffer address resides in DPRAM. Either this is a bug, or the DPRAM test is redundant.

It appears that the code originally didn't take into account buffer addresses that may be in DPRAM, and when it was updated to do so, whoever did it, updated the first instance of the code, but not the second (which is intended to convert <linefeed> into <linefeed><carriage-return> - which I reckon is back to front anyway :-).

It may be that no consequences arise from possibly writing the number 13 into a random memory location, but I for one would like to see this bug fixed. In fact, I have a vague recollection that the same bug also existed in the 8260 version of this code, and that I submitted a fix for it among the 8260 uart patches I posted some time ago.

Having just checked my version of both linuxppc_2_4_devel and linuxppc-2.5 I see this bug still exists everywhere (both 2.4-devel and 2.5, and both 8xx and 8260) - and it is indeed fixed in my local version of 2.4_devel (which means I did post this fix).

This serves to highlight the difficulty of getting stuff into the "official" linuxppc kernel sources, although it is only a minor case. Old-timers such as myself tend to post our patches so they exist on the public record, and then basically go back to our work when the patches are ignored, usually for the most lame reasons, simply because we don't have the time to debate the issue.

However, I believe linuxppc suffers because of this - especially since we are usually talking about the "development" version of the kernel which people should expect to be broken occasionally. Another thing to note here, is that certain people and/or companies have no problem getting their stuff in.

I don't know what to do about this, which is probably why I haven't bothered to say anything up until now, I just thought this comment followed on logically from this case. I guess it simply means that if you want the best linuxppc kernel, you can't simply clone the linuxppc trees - you have to then hunt around the mailing lists and other places for fixes for various things. Some people have been so frustrated in the past, that they have set up their own source trees. I don't like this sort of thing, but then again maybe my apathy is worse. Food for thought. Cheers!

								Murray...

--
Murray Jensen, CSIRO Manufacturing & Infra. Tech. Phone: +61 3 9662 7763
Locked Bag No. 9, Preston, Vic, 3072, Australia. Fax: +61 3 9662 7853
Internet: [email protected]

Hymod project: http://www.msa.cmst.csiro.au/projects/Hymod/

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Pay attention....:-)

You can also read this over and over again in the archives........

First, these functions are used by more than kernel printk. They are shared with xmon and kgdb debugger functions.

Second, the kernel printk doesn't call this driver until the driver has been initialized, which means it has allocated all of the "normal" memory buffers for fifo and the CPM BD's.

The ONLY time these functions will find a buffer in DPRAM is when xmon or kgdb is used. If you are using kgdb or xmon, and find something wrong with the buffer management, then there is a bug.......xmon and kgdb don't generate buffer contents that necessitates this test. If you add this code it will never get executed.

No it won't. Find the case where this happens and then I'll be convinced :-)

I did it. It was updated to support xmon and kgdb. Only the modifications necessary to support these functions were added. In addition to these driver changes, there are assumptions and interactions with boot roms/loaders so we could start kgdb very early in the debugging phase, long before the driver was formally initialized.

Prove it's a bug, it will get fixed.

I will mildly apoligize, but you have to understand I get lots of patches from people claiming "bugs", when it isn't one and they don't take the time to understand why something was done or even how it works. I'll add the code, it won't have any effect, it that makes you happy.

I took the time to look for something from you in the past and didn't find anything. If it showed up, you would have received the same discussion. The 8260 driver was just a copy of this one with only the necessary changes to make it work on that processor.

The real difficulty arises from the time many of us have to spend proving patches work correctly or that a real bug exists. In this case, no one took the time to prove it was a bug, which it isn't, or that it was even fixed. The reason we have only a few people with the ability to actually update the trees is we take the time to understand how all of this stuff works and do our best to ensure we aren't creating more problems than we are solving.

For example, was someone supposed to blindly accept Jocke's patch to make the serial port fifos so much larger? I had to take the time (over and over) :-) to explain why this wasn't a bug. It would be nice if some of you went through the learning curve to understand how things are supposed to work before you just declare bugs and submit patches.

The thing to do about it is for _you_ to invest the time in some engineering practices and show that you really found a bug, you can reproduce the bug, you implemented some feature enhancement, your modifications didn't introduce other problems, the code considers effects on other platforms, and that your update does what it claims. When I have to do this, the priority of these updates falls well below my own work.

As a final example, Jocke keeps bugging me about his ethernet updates. I explained (yet again, it was in the archives) that this had been attempted in the past, and it caused problems with packet routing/forwarding. Why do I have to take the time to set up the testing environment and once again prove this is still a problem? I hoped to find someone with a test environment to prove this (as they mentioned the bug in the first place), but they are also busy with their own projects. Someday I may have time to set this up, make some changes to the patch so it works better, and do some testing to ensure the upstream alignment changes are done properly. If someone else can take the time to ensure it works (or make it work) in this condition where it was known to fail in the past, I would be more likely to check it in without taking the time to do the testing myself.

It's easy to hack up code, throw it over the fence, and complain when someone else won't take care of it. Make that job easier for those of us trying to maintain the source trees, it would be appreciated.

Thanks.

	-- Dan

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Correct, it won't fail since the first instance is redundant. The info ptr is set to &consinfo and consinfo has "good" values for tx_va_base and tx_bd_base.

However if my_console_write is called BEFORE consinfo has been setup(in serial_console_setup) then both is needed. Will KGDB/XMON write this early and/or is KGDB/XMON guaranteed not to write a LF? It sure seems fragile.

Well, the patch itself did not contain the FIFO part, but the mail contained a request to make them bigger. I would had included the FIFO part in the patch if I had been sure it was the right thing to do. Still think it's a good idea to expand the # of TX BD and the FIFO length, but possibly one should keep the lower length during early access to keep the usage of DPRAM low.

Come on, the last constructive mail I got from you regarding my Ethernet patch was http://lists.linuxppc.org/linuxppc-embedded/200211/msg00123.html
where you told me you were looking at the patch and would integrate "something that hopefully works". You had some concerns about L1 cache alignment and I explained that I carefully checked that the buffer were L1 cache aligned.

The buffer is kmalloc'ed and kmalloc always returns L1 cache aligned buffers. Then __dev_alloc_skb()/dev_alloc_skb() reserves 16 bytes in the beginning of the buffer and since the L1 cache line is 16 bytes it is still L1 cache aligned.

If this is your only concern, I can make a new patch that allocates 2*L1_CACHE_BYTES extra space and place the data buffer in between.

Since then you told me that you had sent the patch to some guys that would be able to trigger the problem you are worried about and I have waited several months to get the results(and sent reminder to you) and this the first I hear from you about the outcome.

You reefer to the archives and I have tried to find it, but I didn't find anything. Please supply search keys or a link when you reefer to the archives since the search function does not work very well(you must known this already since you could not locate the mail from Murray Jensen).

You think I just hacked this up and throwed it over the fence? I beg to differ, I have been monitoring the success/failure of this patch all along, posted updates and answered questions. The patch so far has been a success and I think it's more than ready to go in.

Jocke

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Yes, yes. Making these work, especially kgdb, was a real PITA. Both format their own packets/strings and expect behavior of reading/writing directly to a uart FIFO. Like I said, if you really understand how xmon/kgdb work, you would understand why these functions are written exactly as they are.

The printk never calls a serial driver before it is fully initialized. Debugger serial interfaces, especially kgdb, expect a simple uart fifo immediately available within a few hundred lines of kernel code execution.

It's the data at the end of the buffer that's the problem. The IP stack (used to) stores information in dynamically allocated memory that would get corrupted when buffers were invalidated. There was lots of discussion about this on kernel and other mailing lists (like mips and arm). After we found it, there was some discussion about how to fix it, but I didn't follow up on all of the details. To ensure this works, you have to configure a forwarding/routing multiple ethernet environment and get the 8xx to forward packets across the links. That was the failure condition, there are people using 8xx in these configurations, and we can't introduce a "solution" that has been known to be a problem in the past.

No, but you could have saved yourself some time by reading the archives. You could have started with:

	http://lists.linuxppc.org/linuxppc-embedded/200008/msg00105.html
and then:
	http://lists.linuxppc.org/linuxppc-embedded/200106/msg00078.html

After the problems with the 405gp and tulip driver I started looking around at the other lists and asking network folks for solutions. They didn't have any and suggested non coherent processors not dma into skbuffers. We are fortunate on 8xx that the driver is only used on this processor, so if we can find some solution it doesn't have to work generically for others. The networking folks suggested the only solution was to give skbuf allocators knowledge of cache line sizes, but that was never pursued as far as I know.

	-- Dan

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

I think I know why the are as they are now. KGDB/XMON stuff is messy and if you are sure that XMON/KGDB never outputs a LF all is fine.

printk's will go out after serial_console_setup() has executed which I don't consider "fully initialized" but it's initialized enough for printk's to get out safely.

Like I said, if you had given me a better hint then "the archives" we could had resolved this long ago.

I have checked the links above it looks like it's solved. As far as I can tell alternative:
0. change alloc_skb()/skb_add_mtu() to force the allocated size to be a cache line multiple.
has been impl. in the kernel:
- skb_add_mtu() does not exist anymore.
- alloc_skb() does L1 cache align the size:

       size = requested_size + 16;
       size = SKB_DATA_ALIGN(size); /* this does L1 cache alignment */
       data = kmalloc(size + sizeof(struct skb_shared_info), gfp_mask);

This makes the skb_shared_info to always start on a new cache line. There is no risk that it is invalidated by dma_cache_inv() call.

Jocke

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Well I can tell you for a fact that xmon will output \n.

Dan, the code looks fragile as it is - why shouldn't the second part (inside the if (*s == 10) body) do the same test as the first part? Relying on xmon/kgdb never outputting \n sounds like a bad idea to me.

Paul.

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Dan, any doubts still?

Jocke

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

I'm in the midst of getting 2.5 working for 8xx. I'll check in all of these patches when I can get them into both trees. It's going well, should have it done shortly.

Once 8xx is running in 2.5, I'm going to ask for all 8xx/82xx patches to be tested and will be applied to both 2.4 and 2.5 trees.

Thanks.

	-- Dan

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

If you simply looked at the code you would see the problem we are talking about. What are you suggesting? That kgdb and xmon will _NEVER_ send a character with decimal value 10 via my_console_write()?

I really don't care either way - I gave up worrying about this sort of thing a long time ago. I do think you have the wrong attitude for the position you have assumed for yourself in the linuxppc community - you are unnecessarily aggressive, assuming that everyone is a moron until proven otherwise.

However, having said this, I very much appreciate the effort you (and others) put into it - the positives outweigh the negatives. This is why I keep my mouth shut (usually), and will keep my mouth shut in the future (after this message). If I don't like it, I will put my effort where my mouth is - or go use a kernel tree set up by someone else.

http://lists.linuxppc.org/linuxppc-embedded/200205/msg00268.html

You probably didn't find it because I compressed the patch. I guess this is a good example of why one shouldn't compress patches sent to the list.

That much is clear to all who are working on linuxppc on the 8260, I'm sure.

It didn't need proving by testing - it was a simple logic exercise. Either the first test against IMAP_ADDR is redundant, or the same test is needed in the second instance of the code in the case where the character being transmitted is equal to 10 (decimal). Please can't you just view the piece of code under discussion and see what we are talking about?

Like I said, I think you have the wrong attitude - are you suggesting that I am incapable of understanding this stuff? The question under discussion is quite trivial and if you'd actually look at it instead of assuming I was a moron, you'd see.

You are mis-representing what I said. I wasn't suggesting blind acceptance. What I am suggesting is that the list be used to discuss patches, and if after a certain amount of time and discussion there are not any *objections* to a patch, it should make its way into the *development* tree. If it then breaks something, it can be backed out (easy with bk), and the person who sent it can resubmit it via the list at a later stage when they have fixed the reason it was breaking things. The development tree is supposed to be used this way. When after time there is no obvious problem with a patch, it can make its way into the stable tree - this process must be handled more carefully, of course - the stable tree *must* be stable.

And in fact, the system worked well in the case you cited - the discussion in the list led Jocke to look elsewhere for the cause of his problem and the result was a patch to drivers/char/n_tty.c (which must be submitted via the linux kernel mailing list, since it is a patch to a non-ppc file). I am sure Jocke appreciated your input, and the issue was resolved satisfactorily.

I actually have done so. Dan Malek is not the only person in the world who understands this stuff. Like I say, you have a bad attitude - but again, I am not in a position to put in the effort required to displace you, so I will just put up with it.

This was a logic (i.e. thinking) exercise - not an engineering problem. For less trivial examples, I am very careful about any code I submit - but of course, no-one is perfect - including yourself.

You are not god's gift to linuxppc - other people have worthy contributions to make and understand the way it all works just as well as (if not better than) you. Most of your message was informing us how much effort you put in and for us to put up or shut up, then you come out with this - which basically says that you don't have the time to put in all this effort.

I admit I don't have the time. I will contribute as much as I can, when I can, but if my patches are not accepted, I am not going to lose any sleep over that. The patches are out there on the public record for anyone to use if they so wish - they just need to put in a bit of effort to find them.

Please don't let my (hopefully constructive) criticism affect the amount of effort you put in. I know you contribute a lot and you shouldn't say "well bugger them, I'm not doing any more for the ungrateful bastards". I am grateful. To you, and all the others who participate in this list. I have fixed quite a few problems on our board from patches in the list that have not made it into the "official" linuxppc tree.

A good example of this is the 8260 FCC ethernet driver - in fcc_restart(). If you want to set Full Duplex, then both the Full Duplex Enable (FDE) bit *and* the Local Protect (LPB) bit need to be set - if the LPB bit is not set, the receiver is blocked while transmitting, thus negating true full duplex operation. The symptom is poor throughput on fcc ethernet reception. A number of people, including myself (although I didn't "discover" it, I just use it in my local tree), have posted this fix, and it still has not made it into the "official" linuxppc source. There is a lot of other good stuff out there - the Denx site has heaps of good code that hasn't been accepted.

I will restate my position, then shut up. It needs to be made easier to get code into the *development* kernel tree. The level of testing required is by definition less than required for the "stable" tree. The current system is not optimum. If there is a lack of time on the part of the current people with write permission to the linuxppc *development* tree, then there needs to be more "writers". Doing this will inevitably lead to problems, but the gains will outweigh them, I'm sure. BitKeeper allows easy management of the problems. Someone needs to be appointed the judge, or there needs to be a panel of "judges", so that writers will not have patch backout wars. All this has been done quite successfully in other projects. And finally, at the very least, it needs to be seen to be fair, so that certain people and/or companies must not appear to receive favourable treatment.

This discussion is wasting everyone's time (well, mine at least). Lets just agree to disagree and get on with our work. While the current situation is not optimal, I am getting by with it as it is. So unless you are going to say "ahh, I see the problem, I will try to change the way I do things", there really isn't any point in responding. Cheers!

								Murray...

--
Murray Jensen, CSIRO Manufacturing & Infra. Tech. Phone: +61 3 9662 7763
Locked Bag No. 9, Preston, Vic, 3072, Australia. Fax: +61 3 9662 7853
Internet: [email protected]

Hymod project: http://www.msa.cmst.csiro.au/projects/Hymod/

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

I've looked at it. I know it well. I wrote it. I know how its used. xmon and kgdb format their own strings/packets and expect read/write of a simple uart fifo.

Well, ahhh, there isn't anything mentioned there about fixing bugs. From your description it's all about yet another scc uart configuration method. I have lots of those on the shelf, all conflicting, that always seem to be useful to the person/platform/application. The easiest way to keep everyone (un)happy is to leave things alone. I used to apply those, a new one every other week, and the only person happy about it was the last one that had the patch applied. :-)

You can pull the uart "bugfix" from the 2_4_devel tree now, if you wish.

	-- Dan

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

The point was, and always has been, can it be guaranteed that kgdb/xmon will never output a character with decimal value 10? As a simple logic exercise, it cannot be guaranteed - of course, in practice it is unlikely to happen, because they encode everything as two hex digits.

But someone in the future may decide to make everything scroll nicely on output by adding a (redundant) linefeed to the end of every packet - then the kernel starts crashing in early boot when KGDB or XMON is enabled and they will have no idea why.

It wasn't mentioned because it is trivial - the code as it stood was logically inconsistent - I fixed it, as have (many?) others.

This illustrates my point(s) - linuxppc on the 8260 is stuck with the amazingly bad uart configuration/management arrangement it has had for years (and which was spawned back in the 8xx days) because I don't work for the right company.

Like I said - I will shut up now - it isn't that important to me. I made my points in the previous message, and it is on the public record (such as it is), but now I don't want to distract either of us from our work. Cheers!

								Murray...

--
Murray Jensen, CSIRO Manufacturing & Infra. Tech. Phone: +61 3 9662 7763
Locked Bag No. 9, Preston, Vic, 3072, Australia. Fax: +61 3 9662 7853
Internet: [email protected]

Hymod project: http://www.msa.cmst.csiro.au/projects/Hymod/

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Whoops, forgot to ask whether you had also examined the need to set the Local Protect Bit for full duplex mode in fcc_restart()? Cheers!

								Murray...

--
Murray Jensen, CSIRO Manufacturing & Infra. Tech. Phone: +61 3 9662 7763
Locked Bag No. 9, Preston, Vic, 3072, Australia. Fax: +61 3 9662 7853
Internet: [email protected]

Hymod project: http://www.msa.cmst.csiro.au/projects/Hymod/

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

On Tue, Feb 11, 2003 at 12:13:49PM +1100, Murray Jensen wrote:

[snip]

I'm not going to say too much on this branch of the thread, but this is also a _very_ good example of why I'm always asking for 1 logical change per patch.

--
Tom Rini
http://gate.crashing.org/~trini/

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/