On Mon, Apr 23, 2018 at 07:57:36AM +0000, Shahaf Shuler wrote: > Monday, April 23, 2018 10:33 AM, Nélio Laranjeiro: > [...] > > > > +/** > > > > + * DPDK callback to set multicast addresses list. > > > > + * > > > > + * @param dev > > > > + * Pointer to Ethernet device structure. > > > > + * @param mac_addr_set > > > > + * Multicast MAC address pointer array. > > > > + * @param nb_mac_addr > > > > + * Number of entries in the array. > > > > + * > > > > + * @return > > > > + * 0 on success, a negative errno value otherwise and rte_errno is > > > > set. > > > > + */ > > > > +int > > > > +mlx5_set_mc_addr_list(struct rte_eth_dev *dev, > > > > + struct ether_addr *mc_addr_set, uint32_t > > > > nb_mc_addr) { > > > > + uint32_t i; > > > > + int ret; > > > > + > > > > > > We should check nb_mc_addr < MLX5_MAX_MC_MAC_ADDRESSES > > before we start > > > operate. > > > > This verification is done in the sub function. > > I see only assert. Did I missed anything?
No. > > Considering an application calling such API wants to remove/replace the old > > list with new entries. > > That this new one can be just an addition or totally different list or even > > empty. > > This new list can be larger than the amount of MAC addresses the PMD can > > support. > > > > There are two possibilities: > > > > 1. The list is too large: the application will enable the all multicast > > mode to > > receive the extra mac addresses it needs. > > How can application know the size of the MC list? > only the UC size is being reported: > info->max_mac_addrs = MLX5_MAX_UC_MAC_ADDRESSES Such information is not reported at all. The application has to guess. > > 2. The list fits (or empty): no issues. > > > > At the end the application can also call this API with an empty list to > > clear it > > before/after enabling the "all multicast" mode. > > The final result being the same, does it worse to add a duplicated > > verification? > > At the current code if the list is too large and the PMD was compiled > w/o debug mode it will results in seg fault. Right it needs a verification. > > Note: if an error happens the new list is not committed yet i.e. the traffic > > remains untouched. > > > > > > + for (i = MLX5_MAX_UC_MAC_ADDRESSES; i != > > > > MLX5_MAX_MAC_ADDRESSES; ++i) > > > > + mlx5_internal_mac_addr_remove(dev, i); > > > > + i = MLX5_MAX_UC_MAC_ADDRESSES; > > > > + while (nb_mc_addr--) { > > > > > > Maybe worth checking is_multicast_ether_addr(mc_addr_set) and to skip > > > + warn if it is not. > > > > Such verification should be done in the public API i.e. ethdev. > > I don't understand. > In the first patch of the series you add extra verification to check > the mac address validity. It only verify the MAC address is not zero, it does not verify the MAC address is valid in the function context (e.g. unicast in mlx5_mac_addr_add()). > But for the MC you claim it should be done on ethdev layer. Dito. > It should be consistant. Either ethdev verify the MAC address or the > PMD. If the first one, then there is no need to add the > is_zero_ether_addr check on the first patch. It is consistent, the PMD only verify the MAC address is not zero and this in both API. > > > > + ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++, > > > > i++); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > + if (!dev->data->promiscuous) > > > > + return mlx5_traffic_restart(dev); > > > > + return 0; > > > > +} > > > > -- > > > > 2.17.0 Regards, -- Nélio Laranjeiro 6WIND