Re: [PATCH net-next 3/3 v5] drivers: net: ethernet: qualcomm: rmnet: Initial implementation

I'm probably forgetting a bit of the original context.  Say you have
one of these "network devices in IP mode".  What happens with the
device *before* you attach an rmnet child?  Can it pass traffic before
that point at all, or does the modem expect MAP already?

Hi Dan

All traffic needs to be in MAP format only.

quoted

+	dev_hold(real_dev);

I could be entirely wrong, but isn't this mostly handled for you if you
use netdev_upper_dev_link() like ipvlan and macvlan do?  That looks
like it provides a couple things: (a) handles the dev_hold() for you
and (b) provides mechanisms to get the upper device if you need it.
I'm not actually familiar with the "adjacent device" functionality, but
it looked similar in purpose.

Does this API modify the data path as well or is it only for establishing
a hierarchy between nodes (which I assume should help for easier cleanup).
Currently, I register with the real_dev and use the rx_handler to intercept
the incoming MAP packets. If netdev_upper_dev_link only modifies the device
refcounting, I should be able to easily modify to use it.

quoted

+	return 0;
+}
+
+static int __rmnet_set_endpoint_config(struct net_device *dev, int
config_id,
+				       struct rmnet_endpoint *ep)
+{
+	struct rmnet_endpoint *dev_ep;
+
+	ASSERT_RTNL();
+
+	dev_ep = rmnet_get_endpoint(dev, config_id);
+
+	if (!dev_ep || dev_ep->refcount)
+		return -EINVAL;
+
+	memcpy(dev_ep, ep, sizeof(struct rmnet_endpoint));
+	if (config_id == RMNET_LOCAL_LOGICAL_ENDPOINT)
+		dev_ep->mux_id = 0;
+	else
+		dev_ep->mux_id = config_id;

So... if config_id is LOGICAL_ENDPOINT (-1) this sets mux_id to 0.

But if config_id is 0, it *also* sets mux_id to 0?

Can you clarify what mux_id = 0 actually means here?  Can you also talk
a bit about the difference between local_ep and muxed_ep?

mux_id 0 is for the pass through mode (for the testing scenario).
The MAP packets arriving maybe shipped as is to a different net device
(say one exposed by USB).

While transmitting packets from rmnet dev to real_dev, the local_ep
has the information about the rmnet dev. Based on that, the MAP
header is stamped and packet is transmitted.

muxed_ep is for receiving the packets where the MAP header is
stripped off and the packets is passed to the appropriate rmnet dev.

quoted

+	case NETDEV_UNREGISTER_FINAL:

I don't see anywhere that RMNET_INGRESS_FIX_ETHERNET can get set?
Also, can't that be autodetected?


Just use ETH_HLEN instead of RMNET_ETHERNET_HEADER_LENGTH.

But really, I can't see where FIX_ETHERNET ever gets set.  And as
above, can't this be automatically detected?  Can you describe what the
issue is here in more detail?

I know for qmi_wwan.c we had to fix up a number of firmware bugs, but
all that is done automatically.

The ethernet mode was earlier for some experimental configurations.

quoted

+	int egress_format = RMNET_EGRESS_FORMAT_MUXING |
+			    RMNET_EGRESS_FORMAT_MAP;
+	struct net_device *real_dev;
+	int mode = RMNET_EPMODE_VND;
+	u16 mux_id;
+
+	real_dev = __dev_get_by_index(src_net,
nla_get_u32(tb[IFLA_LINK]));
+	if (!real_dev || !dev)
+		return -ENODEV;
+
+	if (!data[IFLA_VLAN_ID])

Also, I wasn't thinking to actually *use* IFLA_VLAN_ID, but I'll let
others weigh in.  It does fit in this case, I think, but maybe creating
an RMNET-specific attribute would be better?

I have implemented a single message for setting up the device based on mux
in this patchset so this suffices for me :) . Stephen had suggested to reuse
existing structs as much as possible.

quoted

+struct rmnet_map_control_command {
+	u8  command_name;
+	u8  cmd_type:2;
+	u8  reserved:6;
+	u16 reserved2;
+	u32 transaction_id;
+	union {
+		u8  data[65528];

Um....  that seems really, really odd.  Typically this would go below
the flow_control struct, and actually be:

u8 data[0];

To indicate that the struct member should exist and that you can use
it, but that it has no specific size (since the size will be determined
by the skb size or by a protocol field instead).

Thats all for now...

Dan

I will change this to u8 data[0];