Fix finding appropriate BGX node in DTB and move it to a separate function

[zbb]

Summary

Search for BGX node in DTS in two ways:

1. Try to find it uder root node first
2. If not found under root, find the top level PCI bridge node and search all nodes below it until appropriate BGX node is found.

Move search code to another function to make the code more clear.
Remove unused variable by the way.

[wma]

sys/dev/vnic/thunder_bgx_fdt.c

136 ↗

(On Diff #12685)

Can we check that len fits into sizeof(*node_name)*FDT_NAME_MAXLEN and does not overflow the buffer?
What about string null-terminating? I know it's not used now, but someday one might add printf here and get into troubles.

[andrew]

sys/dev/vnic/thunder_bgx_fdt.c

157 ↗

(On Diff #12720)

Why not OF_getprop_alloc?

217 ↗

(On Diff #12720)

What if, in a future dtb, there were more than 10 bgx nodes?

219 ↗

(On Diff #12720)

sizeof(char) == 1 by definition

234 ↗

(On Diff #12720)

Why is this needed?

247 ↗

(On Diff #12720)

When could this be negative?

[zbb]

sys/dev/vnic/thunder_bgx_fdt.c

157 ↗

(On Diff #12720)

Standard defines name property to be shorter than 32 bytes so I thought to skip malloc/free here.

217 ↗

(On Diff #12720)

There are up to 2 BGX per node, up to 4 nodes in the system (currently 2) so this gives and architectural limitation to 8 BGX per system.

219 ↗

(On Diff #12720)

Sure, but I though that keeping the size of the element of an array that I am going to operate on will be safer.
That is why there is no sizeof(char) but sizeof(*bgx_sel). Is that a problem here?

234 ↗

(On Diff #12720)

I would say "yes" because first you try to find "/bgx<X>" and then bgx<X> (not on root).
So I clean up the buffer before another snprintf (don't want to have problems with \0)
Although according to the previous comment I should either remove sizeof(*bgx_sel) when calling malloc() or add it here.

247 ↗

(On Diff #12720)

Right

[andrew]

sys/dev/vnic/thunder_bgx_fdt.c

234 ↗

(On Diff #12720)

It it's not null terminating you might have the length incorrectly set.

[zbb]

sys/dev/vnic/thunder_bgx_fdt.c

234 ↗

(On Diff #12720)

?
snprintf is null terminating the string.
I just don't want to have any garbage in the buffer even though in this particular example it does not break anything. The length to zero the buffer is not wrong here.

I will have:

/bgx0\0
bgx0\0

instead of

/bgx0\0
bgx0\0\0

[andrew]

sys/dev/vnic/thunder_bgx_fdt.c

219 ↗

(On Diff #12785)

Are you expecting the type would ever change? The sizeof makes the code more difficult to read.

226 ↗

(On Diff #12785)

Is the cast needed?

249 ↗

(On Diff #12785)

Unneeded, node is already 0

234 ↗

(On Diff #12720)

The memset is a needles write to a buffer that will be written to one line later.

[zbb]

sys/dev/vnic/thunder_bgx_fdt.c

226 ↗

(On Diff #12785)

Yes. (-1) in uint32_t is > 0

249 ↗

(On Diff #12785)

OK

[zbb]

sys/dev/vnic/thunder_bgx_fdt.c

219 ↗

(On Diff #12785)

No. I do not expect the type to change. I find it a good programming practice to do so.

[zbb]

sys/dev/vnic/thunder_bgx_fdt.c

234 ↗

(On Diff #12785)

I agree in the context of strings used here, because each string happens to be null terminated by snprintf() and the strings comparison happens to stop when encountered \0 (so the garbage does not harm anything). Nevertheless it is not on a critical path so is it a problem of any kind?