Minor: fix bridge implementation

[faizana-nvidia]

This PR addresses minor fixes for bridge implementation.

[faizana-nvidia]

I'm a bit confused here, while I tested the intended gateway route creation in test framework, that seem to be working as expected if ifindex = 0 as arg in mctp_nl_route_add().

But same fails in my machine.

Aug 29 07:56:49 hgxb300 mctpd[3669]: Error from kernel: Invalid argument (-22)
Aug 29 07:56:49 hgxb300 mctpd[3669]: incorrect format
Aug 29 07:56:49 hgxb300 mctpd[3669]: mctpd: Failed to add gateway route for EID 10: Invalid argument

though per my understanding of implementation func: fill_rtalter_args() in mctpd we do need to pass ifindex zero

	if (ifindex) {
		msg->nh.nlmsg_len += mctp_put_rtnlmsg_attr(
			&rta, &rta_len, RTA_OIF, &ifindex, sizeof(ifindex));
	} else {
		msg->nh.nlmsg_len += mctp_put_rtnlmsg_attr(
			&rta, &rta_len, RTA_GATEWAY, gw, sizeof(*gw));
	}

on further inspecting looks like RTA_GATEWAY rta policy is not supported yet in the latest kernel ?? I remember testing this with your branch you gave last time to test from https://github.com/CodeConstruct/linux/tree/dev/forwarding

[jk-ozlabs]

The forwarding patches are upstream, but not in a released version as yet. 6.17 should have the merged gateway support.

[faizana-nvidia]

The forwarding patches are upstream, but not in a released version as yet. 6.17 should have the merged gateway support.

Thanks for the infor, will this be a blocker in merge till release happens?

[jk-ozlabs]

No, that shouldn't block us here, as long as you're able to test with a pre-release kernel.

[faizana-nvidia]

No, that shouldn't block us here, as long as you're able to test with a pre-release kernel.

Ack, will update soon on testing.

[faizana-nvidia]

I was able to test this.

mctp route show
eid min 11 max 25 net 1 gw 10 mtu 68
eid min 10 max 10 net 1 dev mctpusb0 mtu 0

[jk-ozlabs]

On cc7ea36, you mention:

If endpoint responds with some invalid endpoint id range, assumed
bridge pool space needs to reset too, this is needed as it misleads
remove_peer() to delete gateway routes too which only be created
after bridge pool allocation

I think we also have an issue that the pool route is not created until after the allocate endpoint IDs call. This gives us the following race:

BO	bridge	EP
Sends Allocate Endpoint IDs
	Allocates downstream endpoint ids
	Sends Set Endpoint ID to EP
		now has an EID, sends a message to BO
Receives message, cannot reply
Adds pool route

which is essentially the same thing fixed in 9711f0b, but with bridges.

[faizana-nvidia]

On cc7ea36, you mention:

If endpoint responds with some invalid endpoint id range, assumed
bridge pool space needs to reset too, this is needed as it misleads
remove_peer() to delete gateway routes too which only be created
after bridge pool allocation

I think we also have an issue that the pool route is not created until after the allocate endpoint IDs call. This gives us the following race:

BO bridge EP
Sends Allocate Endpoint IDs
Allocates downstream endpoint ids
Sends Set Endpoint ID to EP
now has an EID, sends a message to BO
Receives message, cannot reply
Adds pool route
which is essentially the same thing fixed in 9711f0b, but with bridges.

If i understand the original problem statement correctly which got fixed in 9711f0b,, the issue is right after the moment EID is assigned to an endpoint, we were spending few moments during publishing the peer and then setting up the routes, right at this window, endpoint could very well begin communicating to BusOwner which BusOwner might not be able to respond to since routes doesn't exist yet.

Going by same implication, we should setup routes for downstream eid just before AllocateEndpoint ID and may have delete these routes if Allocation fails.

[jk-ozlabs]

Going by same implication, we should setup routes for downstream eid just before AllocateEndpoint ID and may have delete these routes if Allocation fails.

yes, that's correct.

However, no specific need to have that fixed in this PR; more a note that we'll need to address it at some point.

[jk-ozlabs]

However, no specific need to have that fixed in this PR; more a note that we'll need to address it at some point.

Ah, you have already added it here. All good!

[jk-ozlabs]

Couple of minor things, plus a typo (birdge) in the commit message for patch 3.

Otherwise, all looks good.

[faizana-nvidia]

Addressed the comments, once this merges, will rebase other PR 85 to ease the review

[jk-ozlabs]

Merged, thanks!