search

Hurricos / realtek-poe

5 stars 10 forks source link

realtek-poe: Do not abandon status for disabled ports #17

Closed mrnuke closed 2 years ago

mrnuke commented 2 years ago

A port may be disabled, but it is still part of the family. It exists, it consumes space, and it might even carry network traffic.

ubus call poe info should reflect that. Instead, it ignores ports configured as disabled. In many jurisdictions, this is considered port abuse, and users might get a visit from port protection services.

To resolve this, introduce the concept of a valid port config, and keep all valid ports in the "poe info" packet.

Use a bitfield for the "valid" and "enable" flags to make sure the port_config struct does not increase in size.

Should resolve #16

Hurricos commented 2 years ago

I like it! I haven't read this closely, but this should also solve the question of reading status off of ports not explicitly noted in the configuration, right?

If so, we could later re-enable command 0x06 and provide a less intensive configuration pathway.

mrnuke commented 2 years ago

If it's not noted in the config, it shouldn't show up in the output.

Hurricos commented 2 years ago

If it's not noted in the config, it shouldn't show up in the output.

OK, got it. No problem; gathering info from all ports the MCU asserts to exist could fold in nicely to a later configuration streamlining effort.

mrnuke commented 2 years ago

I presume you could MAX(config.port_count, state.num_detected_ports) to make sure you query all ports. I had considered that when writing #8, but you wouldn't get the output with poe info anyway. The other option, pushing status for all detected ports would break the config model.

I guess you could export num_detected_port via `poe info, but what is the use case?

walterav1984 commented 2 years ago

This seems fixed for CI36 but not CI40 and CI41...

mrnuke commented 2 years ago

This seems fixed for CI36 but not CI40 and CI41...

That is expected. CI 36 is the correct build for this PR. Each PR gets its own CI build. A higher CI number doesn't necessarily mean more better.

walterav1984 commented 2 years ago

Thanks, I was in the assumption that every CI build accumulated the previous patches.

Hurricos commented 2 years ago

Tests OK on my hardware.

Simple one to merge. Done.