[U-Boot] [PATCH] EXT4

[U-Boot] [PATCH] EXT4

[Jorgen Lundman]

Slowly getting the hang of github and u-boot. I took a fork of u-boot 
20120520, and merged in the EXT4 patched.

In addition to Samsung's work;

  * Fix minor string ext2->ext4
  * 'unsigned' sectors for volumes over 1TB
  * changed to fit with current u-boot compile env

https://github.com/lundman/u-boot/compare/upstream...upstream-ext4



There is a new limit of 2TB with 512 byte blocks. To go beyond that would 
require "disk_partition_t" to go bigger than 32bit. I think that is outside 
of my access permissions.





-- 
Jorgen Lundman       | <lundman@lundman.net>
Unix Administrator   | +81 (0)3 -5456-2687 ext 1017 (work)
Shibuya-ku, Tokyo    | +81 (0)90-5578-8500          (cell)
Japan                | +81 (0)3 -3375-1767          (home)

[U-Boot] [PATCH] EXT4

[Graeme Russ]

Hi Jorgen

On Tue, May 22, 2012 at 1:31 PM, Jorgen Lundman <lundman@lundman.net> wrote:
>
> Slowly getting the hang of github and u-boot. I took a fork of u-boot
> 20120520, and merged in the EXT4 patched.
>
> In addition to Samsung's work;
>
> ?* Fix minor string ext2->ext4
> ?* 'unsigned' sectors for volumes over 1TB
> ?* changed to fit with current u-boot compile env
>
> https://github.com/lundman/u-boot/compare/upstream...upstream-ext4

These patches will likely not get reviewed and definitely will not get
merged unless that are posted to the mailing list

> There is a new limit of 2TB with 512 byte blocks. To go beyond that would
> require "disk_partition_t" to go bigger than 32bit. I think that is outside
> of my access permissions.

What do you mean by this?

Regards,

Graeme

[U-Boot] [PATCH] EXT4

[Jorgen Lundman]

> These patches will likely not get reviewed and definitely will not get
> merged unless that are posted to the mailing list

Ok, is it better if I use "git format-patch" and send them on the list?


>
>> There is a new limit of 2TB with 512 byte blocks. To go beyond that would
>> require "disk_partition_t" to go bigger than 32bit. I think that is outside
>> of my access permissions.
>
> What do you mean by this?
>

I just assumed that I, new to the use of u-boot, should not tinker with 
core structs, and core API functionality. I merely wish to mention the 
issue in case it has already been addressed by the gurus.

It could very well be that u-boot developers don't want to go larger. 
(presumably, majority of u-boot devices aren't booting from > 2TB devices..)

Or, it has already been discussed and I just missed it.


-- 
Jorgen Lundman       | <lundman@lundman.net>
Unix Administrator   | +81 (0)3 -5456-2687 ext 1017 (work)
Shibuya-ku, Tokyo    | +81 (0)90-5578-8500          (cell)
Japan                | +81 (0)3 -3375-1767          (home)

[U-Boot] [PATCH] EXT4

[Graeme Russ]

Hi Jorgen,

On Tue, May 22, 2012 at 1:45 PM, Jorgen Lundman <lundman@lundman.net> wrote:
>> These patches will likely not get reviewed and definitely will not get
>> merged unless that are posted to the mailing list
>
>
> Ok, is it better if I use "git format-patch" and send them on the list?

Yes, that is exactly what you should do.

But before you post them, make sure you run them through checkpatch.pl
first and resolve/explain any errors or warnings

>>> There is a new limit of 2TB with 512 byte blocks. To go beyond that would
>>> require "disk_partition_t" to go bigger than 32bit. I think that is
>>> outside
>>> of my access permissions.
>>
>>
>> What do you mean by this?
>>
>
> I just assumed that I, new to the use of u-boot, should not tinker with core
> structs, and core API functionality. I merely wish to mention the issue in
> case it has already been addressed by the gurus.

Anyone can post patches to the mailing list that tinker with core
structs, APIs, functionality etc. There are no 'permission' levels
here :)

Whatever you post will get reviewed by the community at large and if
the feeling is that you proposed 'tinkering' is not suitable for
inclusion in U-Boot then comments to that effect will be made. You
will then have the opportunity to either a) argue your position or b)
submit revised patches which address the comments made

> It could very well be that u-boot developers don't want to go larger.
> (presumably, majority of u-boot devices aren't booting from > 2TB devices..)

Well you could port U-Boot to a NAS with >> 2TB storage :)

> Or, it has already been discussed and I just missed it.

Not that I am aware

Regards,

Graeme

[U-Boot] [PATCH] EXT4

[Jorgen Lundman]

> Yes, that is exactly what you should do.
>
> But before you post them, make sure you run them through checkpatch.pl
> first and resolve/explain any errors or warnings

Wow, ohhweee this will take a little while.

How set in stone is the output of checkpatch.pl ? Specifically;

ERROR: do not initialise globals to 0 or NULL
#596: FILE: fs/zfs/zfs.c:33:
+block_dev_desc_t *zfs_dev_desc = NULL;

That strikes me as dangerous. One lets you fail gracefully (Sorry, X has 
not been initialised) and the other is just a plain crash. I find crashes 
to be very ugly, even if it is only reachable by other developers.




WARNING: do not add new typedefs
#728: FILE: fs/zfs/zfs.c:165:
+typedef struct decomp_entry

I'm seriously not allowed to make new typedefs? ouch.


So yeah, should it always pass without a single problem, or may I employ 
some measure of moderation?




-- 
Jorgen Lundman       | <lundman@lundman.net>
Unix Administrator   | +81 (0)3 -5456-2687 ext 1017 (work)
Shibuya-ku, Tokyo    | +81 (0)90-5578-8500          (cell)
Japan                | +81 (0)3 -3375-1767          (home)

[U-Boot] [PATCH] EXT4

[Stefan Kristiansson]

On Tue, May 22, 2012 at 02:55:57PM +0900, Jorgen Lundman wrote:
> 
> ERROR: do not initialise globals to 0 or NULL
> #596: FILE: fs/zfs/zfs.c:33:
> +block_dev_desc_t *zfs_dev_desc = NULL;
> 
> That strikes me as dangerous. One lets you fail gracefully (Sorry, X
> has not been initialised) and the other is just a plain crash. I
> find crashes to be very ugly, even if it is only reachable by other
> developers.
> 

Globals are per se initialised to 0,
so there is no need to explicitly initialise them.
As a consequence, it is neither dangerous to omit the initialisation.

Stefan

[U-Boot] [PATCH] EXT4

[Graeme Russ]

Hi Jorgen,

On Tue, May 22, 2012 at 3:55 PM, Jorgen Lundman <lundman@lundman.net> wrote:
>> Yes, that is exactly what you should do.
>>
>> But before you post them, make sure you run them through checkpatch.pl
>> first and resolve/explain any errors or warnings
>
>
> Wow, ohhweee this will take a little while.
>
> How set in stone is the output of checkpatch.pl ? Specifically;
>
> ERROR: do not initialise globals to 0 or NULL
> #596: FILE: fs/zfs/zfs.c:33:
> +block_dev_desc_t *zfs_dev_desc = NULL;
>
> That strikes me as dangerous. One lets you fail gracefully (Sorry, X has not
> been initialised) and the other is just a plain crash. I find crashes to be
> very ugly, even if it is only reachable by other developers.

Uninitialized global and static variables reside in .bss and are set
to zero during relocation. Initialised globals and static variables go
into .data

Does a global/static initialised to zero belong in .bss or .data?

By not initialising them, the compiler/linker is forced into putting
them into .bss where they are guaranteed to be zero when first
accessed

> WARNING: do not add new typedefs
> #728: FILE: fs/zfs/zfs.c:165:
> +typedef struct decomp_entry
>
> I'm seriously not allowed to make new typedefs? ouch.

If the code is copied verbatim from an existing external repository,
the rules can be relaxed - Just make sure you provide a clear and
concise reference (like a git commit ID or tag)

Oh and remember, just because you can find a prior art in the U-Boot
code does not mean it will be allowed to be used as a backing argument
;)

> So yeah, should it always pass without a single problem, or may I employ
> some measure of moderation?

Only one way to find out ;) But try to make it as clean as you believe
reasonable and explain what's left

Regards,

Graeme

[U-Boot] [PATCH] EXT4

[Jorgen Lundman]

> Uninitialized global and static variables reside in .bss and are set
> to zero during relocation. Initialised globals and static variables go
> into .data

Alas, I know from experience that Microsoft's C compiler does not 
initialise global variables (to make it faster one assumes) which has led 
to hours of debugging.

If u-boot has decided that going without Microsoft compiling support is 
A-OK, then that is A-OK with me too. :)


 > Oh and remember, just because you can find a prior art in the U-Boot
 > code does not mean it will be allowed to be used as a backing argument
 > ;)

This I understand. Even if what came in from legacy has yet to be cleaned, 
there is no reason to allow more filth in :)

>
> Only one way to find out ;) But try to make it as clean as you believe
> reasonable and explain what's left
>

Understood, clean enough to eat of is the goal.

Lund

-- 
Jorgen Lundman       | <lundman@lundman.net>
Unix Administrator   | +81 (0)3 -5456-2687 ext 1017 (work)
Shibuya-ku, Tokyo    | +81 (0)90-5578-8500          (cell)
Japan                | +81 (0)3 -3375-1767          (home)

[U-Boot] [PATCH] EXT4

[Graeme Russ]

Hi Jorgen,

On Tue, May 22, 2012 at 4:17 PM, Jorgen Lundman <lundman@lundman.net> wrote:
>
>
>> Uninitialized global and static variables reside in .bss and are set
>> to zero during relocation. Initialised globals and static variables go
>> into .data
>
>
> Alas, I know from experience that Microsoft's C compiler does not initialise
> global variables (to make it faster one assumes) which has led to hours of
> debugging.

Well actually, it's not the compiler/linker that initialised .bss -
it's the dynamic loader. In the case of U-Boot, it's the Flash->RAM
relocation code.

> If u-boot has decided that going without Microsoft compiling support is
> A-OK, then that is A-OK with me too. :)

I don't know of anyone that has even tried, let alone succeeded in
compiling U-Boot with a Microsoft compiler. I've heard of cygwin
attempts (that from memory have also failed miserably)

Generally speaking, gnu based tools are your best bet. Speaking of
which, we really should have a list of 'supported tools' which are
known to successfully build U-Boot...

Regards,

Graeme

[U-Boot] [PATCH] EXT4

[Wolfgang Denk]

Dear Jorgen Lundman,

In message <4FBB2F6F.5000907@lundman.net> you wrote:
> 
> Alas, I know from experience that Microsoft's C compiler does not 
> initialise global variables (to make it faster one assumes) which has led 
> to hours of debugging.

That would be a violation of existing C standards.

> If u-boot has decided that going without Microsoft compiling support is 
> A-OK, then that is A-OK with me too. :)

We really don't care about broken, standard-violating crap.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Honest error is to be pitied, not ridiculed.
                                       -- Philip Earl of Chesterfield

[U-Boot] [PATCH] EXT4

[Wolfgang Denk]

Dear Jorgen Lundman,

In message <4FBB0BBE.6050103@lundman.net> you wrote:
> > These patches will likely not get reviewed and definitely will not get
> > merged unless that are posted to the mailing list
> 
> Ok, is it better if I use "git format-patch" and send them on the list?

Please read the docs.  Start at http://www.denx.de/wiki/U-Boot/Patches

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Out of register space (ugh)"
- vi