Modify

Opened 9 years ago

Closed 9 years ago

Last modified 4 years ago

#4909 closed enhancement (fixed)

imagetag improvement (profile for alice, deac0de, bs alignement)

Reported by: anselmo@… Owned by: developers
Priority: high Milestone: Barrier Breaker 14.07
Component: toolchain Version: Trunk
Keywords: imagetag AGPF brcm Cc:

Description

Hi,

i made some improvements to imagetag:

1 - Added option -d, if true imagetag will add the final alignement to erase block size and deac0de.

2 - Added option -p, actually the only profile is 'alice' to compute only the kernel crc

3 - Corrected imagelen to real lenght of the image with aligned rootfsoff (should be done?) Easy to revert.

So image/Makefile should not call prepare generic squash for AGPF, as imagetag as already aligned the image and added the deac0de.

The function object_crc32 could be used to compute any crc of the binfile given start address and lenght of the data to be included.
Useful for different profile.

I have added a copyright line with my data, i'm not sure that's correct behaviour, feel free the erase the line if not.

the attached diff contain both imagetag and image/Mafile changes

Attachments (6)

imagetag_improvement.diff (9.9 KB) - added by anselmo@… 9 years ago.
imagetag AGPF brcm
imagetag_improvement.2.diff (9.8 KB) - added by anselmo@… 9 years ago.
imagetag AGPF brcm
imagetag_improvement.3.diff (9.7 KB) - added by anselmo@… 9 years ago.
small change to object_crc32 calculation function
4909+4943_imagetag_improvement.diff (13.4 KB) - added by anselmo@… 9 years ago.
some clean up, and include 4943 to allow flashing via web or tftp
4909+4943+imagetag_improvement+deadcode_fix.diff (14.0 KB) - added by cshore@… 9 years ago.
fix so that deadcode is fully used, not just one or two bytes of it
4909+4943+imagetag_improvement+deadcode_fix.patch (13.2 KB) - added by anselmo@… 9 years ago.
I've just deleted the Copyright line i've hadded initially

Download all attachments as: .zip

Change History (21)

Changed 9 years ago by anselmo@…

imagetag AGPF brcm

Changed 9 years ago by anselmo@…

imagetag AGPF brcm

comment:1 Changed 9 years ago by anselmo@…

there is a little error in the first attachment

comment:2 follow-ups: Changed 9 years ago by cshore@…

Most of those changes should be unnecessary:

read will equal the actual number of read bytes doing the last < 1024 bytes differently from the rest isn't need (fread returns actual bytes read).

If all you are calculating is the crc of the kernel you don't need to read back the file. If you were wanting it to also work for non-alice gate boards there were other things that needed doing to and you will need to look at the patch I'll submit in the wee hours of the morning (as another ticket).

If this isn't already working for you I suggest you look at my patch; I'm not sure how different the Alice Gate is from standard broadcom code but I have looked at the source from Broadcom and for the ones that use the standard broadcom code, what you have won't work.

If this is working for you then let me know and I will merge what should be the necessary changes from the patch with my patch and let you try it on the Alice Gate.

comment:3 in reply to: ↑ 2 ; follow-ups: Changed 9 years ago by anonymous

Hi,

Replying to cshore@…:

read will equal the actual number of read bytes doing the last < 1024 bytes differently from the rest isn't need (fread returns actual bytes read).

Yes i know it, but as you should read only the number of bytes specified in object_len, you should change the minimun number of bytes read, as if you keep reading using sizeof(readbuffer) it will take too much byte during the last reading.

If all you are calculating is the crc of the kernel you don't need to read back the file. If you were wanting it to also work for non-alice gate boards there were other things that needed doing to and you will need to look at the patch I'll submit in the wee hours of the morning (as another ticket).

The object_crc32 just gived the possibility to compute the crc32 in any moment of any data already writed, and not sequentially as at present.

It should work for others boards too, the only changes (except the image lenght) specific for alice is the crc computation in the alice profile.


If this is working for you then let me know and I will merge what should be the necessary changes from the patch with my patch and let you try it on the Alice Gate.

My patch work perfectly for the alice, i'll try yours and keep you informed on the results

comment:4 in reply to: ↑ 3 Changed 9 years ago by anonymous

Replying to anonymous:

Hi,

Replying to cshore@…:

read will equal the actual number of read bytes doing the last < 1024 bytes differently from the rest isn't need (fread returns actual bytes read).

Yes i know it, but as you should read only the number of bytes specified in object_len, you should change the minimun number of bytes read, as if you keep reading using sizeof(readbuffer) it will take too much byte during the last reading.

Probably the last read could be done with only one operation

read = fread(readbuf, sizeof(uint8_t), object_len, binfile);

comment:5 follow-up: Changed 9 years ago by anselmo@…

Profiles or something similar could be used to use the reserve filed too.

For example, the alice gate..... divide the first reserved field in three part and print them before booting:

28 char : Distribution
28 char : Version
18 char : Buil Date

That isn't necessary but could be nice to use.

Changed 9 years ago by anselmo@…

small change to object_crc32 calculation function

comment:6 in reply to: ↑ 2 Changed 9 years ago by cshore@…

The patch is at ticket 4943

comment:7 in reply to: ↑ 3 ; follow-up: Changed 9 years ago by anonymous

Replying to anonymous:

Hi,

Replying to cshore@…:

read will equal the actual number of read bytes doing the last < 1024 bytes differently from the rest isn't need (fread returns actual bytes read).

Yes i know it, but as you should read only the number of bytes specified in object_len, you should change the minimun number of bytes read, as if you keep reading using sizeof(readbuffer) it will take too much byte during the last reading.

Are you sure? According the the C standard fread stops read at end of file, so reading sizeof(readbuffer) is the maximum to read, not what is actually read.

If all you are calculating is the crc of the kernel you don't need to read back the file. If you were wanting it to also work for non-alice gate boards there were other things that needed doing to and you will need to look at the patch I'll submit in the wee hours of the morning (as another ticket).

The object_crc32 just gived the possibility to compute the crc32 in any moment of any data already writed, and not sequentially as at present.

I guess I decided I preferred to seek back and forth within the file; that seems messy to me.

It should work for others boards too, the only changes (except the image lenght) specific for alice is the crc computation in the alice profile.


If this is working for you then let me know and I will merge what should be the necessary changes from the patch with my patch and let you try it on the Alice Gate.

My patch work perfectly for the alice, i'll try yours and keep you informed on the results

Ok, I'll see if I can to this sooner instead of later.

comment:8 in reply to: ↑ 5 Changed 9 years ago by anonymous

Replying to anselmo@…:

Profiles or something similar could be used to use the reserve filed too.

For example, the alice gate..... divide the first reserved field in three part and print them before booting:

28 char : Distribution
28 char : Version
18 char : Buil Date

That isn't necessary but could be nice to use.

Hmmm....this will require moving the reserved fields I used (which are required to get flashing to work for stock firmware for generic broadcom stuff).

comment:9 in reply to: ↑ 7 ; follow-up: Changed 9 years ago by anselmo@…

Are you sure? According the the C standard fread stops read at end of file, so reading sizeof(readbuffer) is the maximum to read, not what is actually read.

Yes, I understand that, but that's the problem, i don't want to get the end of the file, I don't know wich is compute_len (the lenght of data that I should read) so I could compute the crc of only part of the file, as for the alice. compute_len could be the end of the file or not, and if not, the lenght could be no aligned to 1024 bytes.

I guess I decided I preferred to seek back and forth within the file; that seems messy to me.

I can understand that but, for the alice I need to stop the CRC32 before the end of the file; maybe others need could emerge in future. So I think keeping separate the growing of the image and the computation of the CRC32 give more adaptability.

To stop the computation of the CRC32 I could (that's what came to my mind):

  • Use and intermediate variable. I need in extra variable and a control that select wich CRC32 insert in the tag.
  • Check everytime (padding kernel, rootfs, padding rootfs, deadc0de) if I should compute the CRC32 or not.

In both case I think the change are really specific to alice but inserted in general code, and they are distributed in the several place in the code.

While computing the CRC32 with a call to a function and when you have finished the growing of the image, keep clearly separated the two procedure. You build the image and you clearly see where and how each piece is placed. When you have to compute the CRC32, you have to check only one call to a function and clearly see where and wich are the data included on the computation.

I think that the little mess of seeking back and forth (that just changig a pointer) is a good deal in exchange of some more clarity about wich piece fall in the CRC32.

That give the ability to made other changes to the tag for specific router too.

Changed 9 years ago by anselmo@…

some clean up, and include 4943 to allow flashing via web or tftp

comment:10 follow-up: Changed 9 years ago by anselmo@…

At present, the last patch use reserved2 and reserved3 to store wrtrootfsaddr and wrtrootfslen.

We have to wait that someone (cshore? :-) ) could try the patch on generic hardware, naturally any volunteer is welcome.

comment:11 in reply to: ↑ 9 Changed 9 years ago by cshore@…

Replying to anselmo@…:

While computing the CRC32 with a call to a function and when you have finished the growing of the image, keep clearly separated the two procedure. You build the image and you clearly see where and how each piece is placed. When you have to compute the CRC32, you have to check only one call to a function and clearly see where and wich are the data included on the computation.

I think that the little mess of seeking back and forth (that just changig a pointer) is a good deal in exchange of some more clarity about wich piece fall in the CRC32.

That give the ability to made other changes to the tag for specific router too.

I've decided I agree with you. Now that I think about it, calculating the CRC throughout the code is messier in terms of separating things out. Good call :-)

I will test the new patch soon.

comment:12 in reply to: ↑ 10 Changed 9 years ago by cshore@…

It doesn't work. The crc is right, but the deadcode is wrong (I think). (A least the initial erasing of blocks fails because wrt doesn't find the magic code for end of filesystem). I'll work on it now and see if I can figure out what is wrong.

Changed 9 years ago by cshore@…

fix so that deadcode is fully used, not just one or two bytes of it

comment:13 Changed 9 years ago by cshore@…

sizeof(DEADCODE) was the problem. The sizeof a #define is the size of the default type of the literal (in this case int I believe), not the type we assign to it. To get a 4-byte literal as literal it would need L for long int appended to it. (sizeof returns the sizeof a type not the size of an object; char[10] is considered to a different type than char[20], but char *smallmem and char *bigmem would return the same value for sizeof even if they were malloc'ed different sizes).

Changing sizeof(DEADCODE) to sizeof(deadcode) works and the patch it attached above.

Changed 9 years ago by anselmo@…

I've just deleted the Copyright line i've hadded initially

comment:14 Changed 9 years ago by florian

  • Resolution set to fixed
  • Status changed from new to closed

Applied in [15253], huge thanks for this patch ! Tested on a 96348GW with success.

comment:15 Changed 4 years ago by jow

  • Milestone changed from Attitude Adjustment 12.09 to Barrier Breaker 14.07

Milestone Attitude Adjustment 12.09 deleted

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.