> +static int rmnet_unregister_real_device(struct net_device *dev)
> +{
> +  int config_id = RMNET_LOCAL_LOGICAL_ENDPOINT;
> +  struct rmnet_logical_ep_conf_s *epconfig_l;
> +  struct rmnet_phys_ep_conf_s *config;
> +
> +  ASSERT_RTNL();
> +
> +  netdev_info(dev, "Removing device %s\n", dev->name);
> +
> +  if (!rmnet_is_real_dev_registered(dev))
> +          return -EINVAL;
> +
> +  for (; config_id < RMNET_MAX_LOGICAL_EP; config_id++) {

This loop is so much harder to understand because you initialize
the loop index several lines above the for() statement.  Just
initialize it here at the beginning of the for() loop and deal
with the fact that this will have to therefore be a multi-line
for() statement the best you can.
...

One way to split the multi-line for() is to put the initialiser
on the immediately preceding line:
        config_id = RMNET_LOCAL_LOGICAL_ENDPOINT;
        for (; config_id < RMNET_MAX_LOGICAL_EP; config_id++) {

        David

Hi DaveM / David

I'll move the initialization as above.


+static int rmnet_set_egress_data_format(struct net_device *dev, u32 edf,
+                                       u16 agg_size, u16 agg_count)

Use a space instead of a TAB character before the "u32 edf," argument.


I'll fix this.

+static int
+__rmnet_set_logical_endpoint_config(struct net_device *dev,
+                                   int config_id,
+                                   struct rmnet_logical_ep_conf_s *epconfig)
+{
+       struct rmnet_logical_ep_conf_s *epconfig_l;
+
+       ASSERT_RTNL();
+
+       epconfig_l = rmnet_get_logical_ep(dev, config_id);
+
+       if (!epconfig_l || epconfig_l->refcount)
+               return -EINVAL;
+
+ memcpy(epconfig_l, epconfig, sizeof(struct rmnet_logical_ep_conf_s));
+       if (config_id == RMNET_LOCAL_LOGICAL_ENDPOINT)
+               epconfig_l->mux_id = 0;
+       else
+               epconfig_l->mux_id = config_id;
+
+       dev_hold(epconfig_l->egress_dev);
+       return 0;
+}

This looks really messy.

One invariant that must hold is that if I try to take down netdev
"X", all resources holding a reference to X will be immediately
(or quickly) dropped when that request comes in via notifiers.

Will this happen properly for this egress_dev reference counting?

+static int rmnet_config_notify_cb(struct notifier_block *nb,
+                                 unsigned long event, void *data)
+{
+       struct net_device *dev = netdev_notifier_info_to_dev(data);
+
+       if (!dev)
+               return NOTIFY_DONE;
+
+       switch (event) {
+       case NETDEV_UNREGISTER_FINAL:
+       case NETDEV_UNREGISTER:
+               netdev_info(dev, "Kernel unregister\n");
+               rmnet_force_unassociate_device(dev);
+               break;

So I guess it happens here?

Yes, all the rmnet devices are removed from the real_dev
in the notifier callback.

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Reply via email to