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

Subash Abhinov Kasiviswanathan
2017-04-14T21:57:24Z

> No, please use standard facilities in order to do debug logging.
>
>> +static struct rmnet_logical_ep_conf_s *_rmnet_get_logical_ep
>
> Dont use "_" at the start of a name without purpose
>
>> +static int rmnet_associate_network_device(struct net_device *dev)
>
> I would expect to see here you making connection between real_dev and
> rmnet dev. I don't see such thing. Name of the function is misleading.
>
>> +
>> + config = kmalloc(sizeof(*config), GFP_ATOMIC);
>
> kzalloc, and you don't have to zero the memory.
>
>> +static struct notifier_block rmnet_dev_notifier = {
>
> You should add "__read_mostly"
>
>> + .notifier_call = rmnet_config_notify_cb,
>> + .next = 0,
>> + .priority = 0
>
> This initialization of 0s is not needed.
>
>> +/* rmnet_vnd_is_vnd() gives mux_id + 1, so subtract 1 to get the
>> correct mux_id
>> + */
>
> Fix this comment format.
>> +void rmnet_print_packet(const struct sk_buff *skb, const char *dev,
>> char dir)
>
> No reason to have this function. One can use P_ALL tap to get skbs to
> userspace.
>
>> +/* rmnet_bridge_handler() - Bridge related functionality
>> + */
>
> Fix the comment format (you have it on multiple places)
>
>> +static rx_handler_result_t rmnet_bridge_handler
>> + (struct sk_buff *skb, struct rmnet_logical_ep_conf_s *ep)
>
> The formatting is incorrect:
>
> static rx_handler_result_t
> rmnet_bridge_handler(struct sk_buff *skb, struct
> rmnet_logical_ep_conf_s *ep)
>
>
>> + config = (struct rmnet_phys_ep_conf_s *)
>> + rcu_dereference(skb->dev->rx_handler_data);
>> +
>> + if (!config) {
>
> Cannot happen. Please remove this.
>
>> + .ndo_set_mac_address = 0,
>> + .ndo_validate_addr = 0,
>
> These are NULL by default. No need to init.
>
>> + if ((id < 0) || (id >= RMNET_MAX_VND) || !rmnet_devices[id])
>
> Unneeded inner "()"s. I see you have it on multiple places.
>
>> +
>> + dev_conf = (struct rmnet_vnd_private_s *)netdev_priv(dev);
>
> The typecast is not needed since netdev_priv is void*. You have it all
> over the code.
>
>> +#define ETH_P_MAP 0xDA1A /* Multiplexing and Aggregation Protocol
>> + * NOT AN OFFICIALLY REGISTERED ID ]
>
> Please push this and ARPHRD_RAWIP as separate patches, to increase the
> visibility.
>
>> +enum {
>> + IFLA_RMNET_UNSPEC,
>> + IFLA_RMNET_MUX_ID,
>> + __IFLA_RMNET_MAX,
>> +};
>
> This belongs to include/uapi/linux/if_link.h
> Please see IFLA_BOND_* as example
>> +#define RMNET_INIT_OK 0
>> +#define RMNET_INIT_ERROR 1
>
> Please use common error codes (0/-ENOMEM/-EINVAL/...)
>
>> + static u32 rmnet_mod_mask = X
>
> Don't use this custom helpers. Use existing loggign facilities.
>
>> + pr_notice("[RMNET:LOW] %s(): " fmt "\n", __func__, \
>> + ##__VA_ARGS__); \
>> + } while (0)
>
> These look scarry. Please use netdev_err, dev_err and others instead.
>
>> +unsigned int rmnet_log_module_mask;
>> +module_param(rmnet_log_module_mask, uint, 0644);
>> +MODULE_PARM_DESC(rmnet_log_module_mask, "Logging module mask");
>
> No module options please.
>
>> + config = (struct rmnet_phys_ep_conf_s *)
>> + rcu_dereference(skb->dev->rx_handler_data);
>
> This is certainly a misuse of dev->rx_handler_data. Dev private of a
> function arg to carry the pointer around.
>
>> +struct net_device *rmnet_devices[RMNET_MAX_VND];
>
> Avoid this global variable.
>

Hi Jiri

I'll make these changes.

>> + if (!data[IFLA_RMNET_MUX_ID])
>> + return -EINVAL;
>> +
>> + mux_id = nla_get_u16(data[IFLA_VLAN_ID]);
>
> This is a bug I believe... ^^^^^^^
> I'm pretty sure that you did not test this code.
>

Since both IFLA_VLAN_ID and IFLA_RMNET_MUX_ID are 1 from enum, this
actually
works. I was using VLAN as a reference, so I missed to change this to
IFLA_RMNET_MUX_ID.

>
>> + rmnet_kfree_skb
>> + (skb,
>> + RMNET_STATS_SKBFREE_INGRESS_NOT_EXPECT_MAPD);
>
> very odd formatting. Please fix.
>
Checkpatch complains if it is over 80 chars in a line, so I had to do
this.
I'll change to a single line.