Re: [PATCH net-next 1/1 v3] drivers: net: rmnet: Initial implementation

Subash Abhinov Kasiviswanathan
2017-04-14T23:02:56Z

>> +https://source.codeaurora.org/quic/la/platform/vendor/qcom-opensource/\
>> +dataservices/tree/rmnetctl
>
> Don't split URL better to have long line.
>> diff --git a/drivers/net/rmnet/Kconfig b/drivers/net/rmnet/Kconfig
>
> Since this is Qualcomm and Ethernet specific, maybe better to put
> in drivers/net/ethernet/qualcom/rmnet
>
>> +config RMNET_DEBUG
>
> Please use network device standard debug mechanism.
> netif_msg_XXX
>
>> + buffloc += snprintf(&buffer[buffloc], sizeof(buffer) - buffloc,
>> + " %02x", skb->data[i]);
>
> If you really have to do this. Use hex_dump_bytes API.
>
>> + skb->mac_header = 0;
>> + skb->mac_len = 0;
>> +}
>
> Why not use sbk_set_mac_header(skb, 0)?
>
>> +static inline struct rmnet_phys_ep_conf_s *_rmnet_get_phys_ep_config
>> + (struct net_device *dev)
> awkward line break.
> dev could be const
>
>> + config = (struct rmnet_phys_ep_conf_s *)
>> + rcu_dereference(dev->rx_handler_data);
>
> Please don't directly reference rx_handler. There is already functions
> like netdev_is_rx_handler_busy() to abstract that API.
>
>> + * @epconfig: endpoint configuration structure to set
>> + */
>
> You are using docbook format here, but this is not a docbook comment.
> ie.
> /**
> * function - This is a docbook comment
> * @dev: this is a param
> */
>
> Plus these are static functions so there is no point in documentating
> internal API with docbook.
>
>> + ASSERT_RTNL();
>> +
>> + if (!dev || config_id < RMNET_LOCAL_LOGICAL_ENDPOINT ||
>> + config_id >= RMNET_MAX_LOGICAL_EP)
>> + return -EINVAL;
>
> For internal API's you should validate parmeters at the external
> entry point not in each call. Otherwise you have a multitude of
> impossible error checks and dead code paths.
>
>> + .next = 0,
>> + .priority = 0
>> +};
> Don't initialize fields that are not used or should be zero.
>

Hi Stephen

I'll make these changes.

>
> Could just be:
>
> static inline int _rmnet_is_physical_endpoint_associated(const struct
> net_device *dev)
> {
> rx_handler_func_t *rx_handler
> = rcu_dereference(dev->rx_handler);
>
> return rx_handler == rmet_rx_handler;
> }
>
> But standard practice is to use ndo_ops to identify self in network
> drivers.
> I.e
> return dev->netdev_ops == &rmnet_device_ops;
>

The netdevice which is associated is the physical (real_dev). This
device
can vary based on hardware and will have its own netdev_ops associated
with it.
rmnet_device_ops applies to the rmnet devices only. I'll use the first
option you have suggested.

>> + if (!data[IFLA_RMNET_MUX_ID])
>> + return -EINVAL;
>
> So you are inventing private link netlink attributes.
> Why? Why can't you use device switch, bridge, or other master/slave
> model.
>

I would like to eventually add more configuration options for the
ingress & egress data formats as well as the endpoint configuration
(VND mode vs BRIDGE mode). I was able to re-use IFLA_VLAN_ID for
IFLA_RMNET_MUX_ID and test but it wasn't sufficient for the additional
parameters. I'll check the bridge attributes and try to reuse it.