allow rb_hardconfig to set ethernet mac-addresses

[john-tho]

No description provided.

[john-tho]

Hi @f00b4r0,

I cobbled together a rough function as an extension to rb_hardconfig, to dynamically add hardcfg based mac addresses to device tree ethernet nodes. I though I would run it by you first, given that it is your driver.
My reason for introducing this is that now, Mikrotik device ethernet interfaces/ports removed from an OpenWrt bridge get a random MAC address at each reboot. More details in the commit messages. If you think this is unnecessary, overcomplicated, or you have a different solution; I will not send it to OpenWrt. Any feedback would be appreciated.
Adrian did not like the idea of this (that I would be further complicating a subsystem that very few people are familiar with) here: openwrt#4215

Separately, I was considering modifying the partition parser and sysfs generator; when parsing the cfg tag partitions, after finding the soft or hard magic, build an array of cfg tag id, tag len, and offset. Then we could extend the hard_config to after the last tag (or the next 4K boundary), and also avoid re-scanning memory when looking for each tag to expose in sysfs. Only another rough idea.

Cheers,
Thanks for your time.

[f00b4r0]

Hi @john-tho,

Hi @f00b4r0,

I cobbled together a rough function as an extension to rb_hardconfig, to dynamically add hardcfg based mac addresses to device tree ethernet nodes. I though I would run it by you first, given that it is your driver.
My reason for introducing this is that now, Mikrotik device ethernet interfaces/ports removed from an OpenWrt bridge get a random MAC address at each reboot. More details in the commit messages. If you think this is unnecessary, overcomplicated, or you have a different solution; I will not send it to OpenWrt. Any feedback would be appreciated.
Adrian did not like the idea of this (that I would be further complicating a subsystem that very few people are familiar with) here: openwrt#4215

I need some time to think about this. There's a number of problems with your current proposal, including the fact that you make rb_hardconfig depend on OF, that the hard-coded linkage will likely reduce the chances to get this upstream if we ever wanted to, or that you're adding more special cases in the init() routine, which is something I'd rather avoid. I'm not sure all this is the correct approach yet, but I'm not saying it isn't either :)

Separately, I was considering modifying the partition parser and sysfs generator; when parsing the cfg tag partitions, after finding the soft or hard magic, build an array of cfg tag id, tag len, and offset. Then we could extend the hard_config to after the last tag (or the next 4K boundary), and also avoid re-scanning memory when looking for each tag to expose in sysfs. Only another rough idea.

I don't follow what you're suggesting here. If what you're suggesting is to combine the work of the mtd parser with the work of the sysfs interface, that's a bad idea. Linux wants drivers that are independent units as much as possible. There is no extra work being done the way things are done now: the mtd parser scans at the block level for a signature, it doesn't look at the content; the sysfs interface parses the content from an identified mtd partition, without scanning the whole mtd for this.

Cheers

[john-tho]

I need some time to think about this. There's a number of problems with your current proposal, including the fact that you make rb_hardconfig depend on OF, that the hard-coded linkage will likely reduce the chances to get this upstream if we ever wanted to, or that you're adding more special cases in the init() routine, which is something I'd rather avoid. I'm not sure all this is the correct approach yet, but I'm not saying it isn't either :)

Yes, thank you. I appreciate it.
Upstreaming your routerboot work would be great.
This is only proof of concept, to check if I could get it to work, so I have no strong opinions about it. I only built it into rb_hardconfig because I knew your code worked, rather than I duplicate parts of it somewhere else (I don't even know where would suit) for testing.

I don't follow what you're suggesting here. If what you're suggesting is to combine the work of the mtd parser with the work of the sysfs interface, that's a bad idea. Linux wants drivers that are independent units as much as possible. There is no extra work being done the way things are done now: the mtd parser scans at the block level for a signature, it doesn't look at the content; the sysfs interface parses the content from an identified mtd partition, without scanning the whole mtd for this.

Not wanting to combine parser and sysfs interpreter.
For the parser: similar to how the parser looks into the DTB, to find the DTB partition size.
Step through the hard config tags (only interpreting tag length), to check hard_config is not cut short, to avoid these: https://lists.openwrt.org/pipermail/openwrt-devel/2021-April/034911.html
or maybe check that the data at the end of the hard_config partition is zero or FF.
I do not like "auto-extend up to the next defined partition", because my MIPS devices have partitions that follow that are not automatically detected (bootloader2 on one, bios on the other).
Otherwise, as a rule, do we manually set hard_config size for new Mikrotik devices where there is possibly space for a longer hard_config?

Also, whether is is done in the parser, or somewhere else, I would like a means to access the raw data for a tag_id. Example: storing and exposing an array of tag_id, tag_len, offset. Then I could easily access the raw tag data for a tag id from a different driver. As a bonus, the sysfs interpreter would not need to rescan hardcfg for each tag.

These are only nice-to-have ideas.

Thank you

[f00b4r0]

I don't follow what you're suggesting here. If what you're suggesting is to combine the work of the mtd parser with the work of the sysfs interface, that's a bad idea. Linux wants drivers that are independent units as much as possible. There is no extra work being done the way things are done now: the mtd parser scans at the block level for a signature, it doesn't look at the content; the sysfs interface parses the content from an identified mtd partition, without scanning the whole mtd for this.

Not wanting to combine parser and sysfs interpreter.
For the parser: similar to how the parser looks into the DTB,

it doesn't. It' only reads the header. It doesn't look into the DTB, it doesn't parse it, it doesn't do anything about it. hard/soft config have no header giving the size of the data.

to find the DTB partition size.
Step through the hard config tags (only interpreting tag length),

Right here you're already pulling hardconfig-specific bits into a pure MTD parser. NACK.

to check hard_config is not cut short, to avoid these: https://lists.openwrt.org/pipermail/openwrt-devel/2021-April/034911.html

This has been addressed. There's only so much "automation" the parser can do. The autosizing was initially implemented as a convenience on the assumption that all hard config partitions where 4K. That assumption turned out to be incorrect with newer hardware and that's the reason why the driver also provides an option to override that assumption, which is now used.

or maybe check that the data at the end of the hard_config partition is zero or FF.

That's completely unreliable. Some tags are padded.

I do not like "auto-extend up to the next defined partition", because my MIPS devices have partitions that follow that are not automatically detected (bootloader2 on one, bios on the other).

You're supposed to declare those partitions in the DTB. The auto extend is only valid if all the partitions are correctly defined and contiguous. I thought this was plenty clear enough from the code documentation. You cannot expect the code to magically work around incorrect declarations. I'm already feeling regrets implementing this "convenience" code :P

Otherwise, as a rule, do we manually set hard_config size for new Mikrotik devices where there is possibly space for a longer hard_config?

Yes.

Also, whether is is done in the parser, or somewhere else, I would like a means to access the raw data for a tag_id. Example: storing and exposing an array of tag_id, tag_len, offset. Then I could easily access the raw tag data for a tag id from a different driver.

That structure already exists internally to the sysfs driver (see hc_attrs[]). However it seems like a bad idea to expose this though, since the data is precisely raw and unformatted and the content of the array may change as the driver is updated with new tags.

As a bonus, the sysfs interpreter would not need to rescan hardcfg for each tag.

It doesn't.

Cheers