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

[Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>]

>> +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.