On 2 August 2017 at 09:03, shally verma <shallyvermacav...@gmail.com> wrote:
> On Wed, Aug 2, 2017 at 1:39 AM, Maxim Uvarov <maxim.uva...@linaro.org>
> wrote:
> > On 08/01/17 15:02, Bill Fischofer wrote:
> >> Correct tag for this should be --subject-prefix="API-NEXT PATCHv2"
> >>
> >> Every time a revision of a patch is submitted, increment the version.
> This
> >> is done automatically if you use a GitHub pull request, but must be done
> >> manually if you submit the patch via e-mail.
> >>
> >> Thanks.
> >>
> >> On Tue, Aug 1, 2017 at 2:38 AM, Vamsi Attunuru <vamsi.cav...@gmail.com>
> >> wrote:
> >>
> >>> Signed-off-by: Vamsi Attunuru <vattun...@cavium.com>
> >>> Signed-off-by: Shally Verma <sve...@cavium.com>
> >>> Signed-off-by: Mahipal Challa <mcha...@cavium.com>
> >>>
> >>> ---
> >>> include/odp/api/spec/packet_io.h | 167 ++++++++++++++++++++++++++++++
> >>> +++++++++
> >>> 1 file changed, 167 insertions(+)
> >>>
> >>> diff --git a/include/odp/api/spec/packet_io.h
> >>> b/include/odp/api/spec/packet_io.h
> >>> index 8802089..7174c0f 100644
> >>> --- a/include/odp/api/spec/packet_io.h
> >>> +++ b/include/odp/api/spec/packet_io.h
> >>> @@ -451,6 +451,16 @@ typedef union odp_pktio_set_op_t {
> >>> struct {
> >>> /** Promiscuous mode */
> >>> uint32_t promisc_mode : 1;
> >>> + /** Allow default MAC address to be set */
> >>> + uint32_t mac_addr_set : 1;
> >>> + /** Allow multiple addresses to be added
> >>> + * other than default. When enabled, app
> >>> + * can call odp_mac_addr_add API to set
> >>> + * mac addresses upto the limit indicated by
> >>> + * Odp_pktio_capability_t:max_mac_addresses */
> >>> + uint32_t mac_addr_add : 1;
> >>> + /** Allow app to set MTU size */
> >>> + uint32_t mtu_set : 1;
> >>> } op;
> >>> /** All bits of the bit field structure.
> >>> * This field can be used to set/clear all flags, or bitwise
> >>> @@ -459,6 +469,71 @@ typedef union odp_pktio_set_op_t {
> >>> } odp_pktio_set_op_t;
> >>>
> >>> /**
> >>> + * MAC address type
> >>> + */
> >>> +typedef enum odp_mac_addr_type_t {
> >>> + /** Unicast MAC address type */
> >>> + ODP_MAC_ADDR_TYPE_UCAST = 0,
> >>> +
> >>> + /** Multicast MAC address type */
> >>> + ODP_MAC_ADDR_TYPE_BCAST
> >>> +} odp_mac_addr_type_t;
> >>> +
> >
> > is that needed? broadcast/multicast macs are known. I think we don't
> > need to flag them in api.
>
> Shally - This is not really necessary to add. We added this keeping in
> mind the devices that may internally have separate tables for storing
> multi and unicast addresses. Alternative is to detect which type is it
> (as in example mentioned below) and use appropriate place to store
> them. For now, we can get rid of them. It was added to have more views
> on it.
>
> >
> >
> >>> +/**
> >>> + * MAC address add/remove operation status types
> >>> + *
> >>> + * These status types denote various statuses set by
> >>> + * odp_pktio_mac_addr_add/remove APIs in odp_pktio_mac_addr_t:status.
> >>> + */
> >>> +typedef enum odp_mac_ops_status_t {
> >>> + /** MAC address add/remove is successful */
> >>> + ODP_MAC_ADDR_OP_SUCCESS = 0,
> >>> +
> >>> + /** Invalid mac address passed in
> odp_pktio_mac_addr_t:mac_addr */
> >>> + ODP_MAC_ADDR_INVALID,
> >>> +
> >>> + /** odp_pktio_mac_addr_t:mac_addr points to NULL */
> >>> + ODP_MAC_ADDR_PTR_NULL,
> >>> +
> >>> + /** Entry in mac_addr_tbl[] is NULL */
> >>> + ODP_MAC_ADDR_ENTRY_NULL,
> >>> +
> >>> + /** MAC address size mismatch
> >>> + * odp_pktio_mac_addr_t:mac_addr_len
> >>> + * != odp_pktio_capability_t:mac_addr_len */
> >>> + ODP_MAC_ADDR_SIZE_ERR,
> >>> +
> >>> + /** Index is invalid,
> >>> + * odp_pktio_mac_addr_t:index >= odp_pktio_capability_t:max_
> mac_addresses
> >>> */
> >>> + ODP_MAC_ADDR_INVALID_INDEX
> >>> +} odp_mac_ops_status_t;
> >>> +
> >
> > Then simple then better. return -1 and odp_errno set if needed. Any
> > initialization time errors can be easily found while debugging. For
> > runtime errors there is more reason for enum. For current case it is
> > not needed.
> >
> >
> >>> +/**
> >>> + * Packet IO MAC address structure
> >>> + *
> >>> + * These parameters are used for adding/removing MAC addresses.
> >>> + * "status" parameter of each index indicates the result after
> >>> + * the odp_pktio_mac_addr_add/remove operation.
> >>> + */
> >>> +typedef union odp_pktio_mac_addr_t {
> >>> + /** Type of MAC address (ucast/mcast) */
> >>> + odp_mac_addr_type_t mac_type;
> >>> +
> >
> > type not needed. Refer to kernel functions how detect type:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/
> linux.git/tree/include/linux/etherdevice.h?h=v4.13-rc3
> >
> > static inline bool is_multicast_ether_addr(const u8 *addr)
> > {
> > #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
> > u32 a = *(const u32 *)addr;
> > #else
> > u16 a = *(const u16 *)addr;
> > #endif
> > #ifdef __BIG_ENDIAN
> > return 0x01 & (a >> ((sizeof(a) * 8) - 8));
> > #else
> > return 0x01 & a;
> > #endif
> >
> > is_multicast_ether_addr_64bits(const u8 addr[6+2]);
> >
> > static inline bool is_local_ether_addr(const u8 *addr)
> > {
> > return 0x02 & addr[0];
> > }
> >
> > static inline bool is_broadcast_ether_addr(const u8 *addr)
> >
> > and etc.
> >
> >>> + /** Pointer to buffer containing MAC address */
> >>> + uint8_t *mac_addr;
> >>> +
> >
> > mac_addr[ETH_ALEN]
> >
> >>> + /** Length of mac_addr buffer */
> >>> + uint32_t mac_addr_len;
> >>> +
> >
> > 6+2 is max.
> >
> >>> + /** Index value associated to this MAC address.
> >>> + * Should be <= odp_pktio_capability_t:max_mac_addresses */
> >>> + uint32_t index;
> >>> +
> >>> + /** Status flag of the mac_addr_add/remove operation */
> >>> + odp_mac_ops_status_t status;
> >
> > status is not needed here.
> >
> Shally -
> >>> +} odp_pktio_mac_addr_t;
> >>> +
> >>> +/**
> >>> * Packet IO capabilities
> >>> */
> >>> typedef struct odp_pktio_capability_t {
> >>> @@ -482,6 +557,17 @@ typedef struct odp_pktio_capability_t {
> >>> * A boolean to denote whether loop back mode is supported on
> this
> >>> * specific interface. */
> >>> odp_bool_t loop_supported;
> >>> +
> >>> + /** Maximum MTU size supported */
> >>> + uint32_t max_mtu_size;
> >>> +
> >>> + /** Length of MAC addresses supported on this specific
> interface
> >>> + * All of the mac addresses supported by this pktio carry
> ,fixed
> >>> size
> >>> + * length as indicated by this capability param */
> >>> + uint32_t mac_addr_len;
> >
> > not needed.
> >
> >>> +
> >>> + /** Maximum number of MAC addresses supported on this specific
> >>> interface */
> >>> + uint32_t max_mac_addresses;
> >
> > num_macs;
> >
> >>> } odp_pktio_capability_t;
> >>>
> >>> /**
> >>> @@ -912,6 +998,21 @@ int odp_pktout_send(odp_pktout_queue_t queue,
> const
> >>> odp_packet_t packets[],
> >>> uint32_t odp_pktio_mtu(odp_pktio_t pktio);
> >>>
> >>> /**
> >>> + * Set MTU value of a packet IO interface.
> >>> + *
> >>> + * Application should pass value upto max_mtu_size as indicated by
> >
> > up to - add white space.
> >
> >>> + * odp_pktio_capability_t:max_mtu_size. Any value beyond max_mtu_size
> >>> + * limit will result in failure. mtu value == 0 also results in
> failure.
> >
> > less then 64 I think.
> >
> >>> + *
> >>> + * @param pktio Packet IO handle.
> >>> + * @param mtu MTU value to be set.
> >>> + *
> >>> + * @return 0 on success
> >>> + * @retval <0 on failure
> >>> + */
> >>> +int odp_pktio_mtu_set(odp_pktio_t pktio, uint32_t mtu);
> >>> +
> >>> +/**
> >>> * Enable/Disable promiscuous mode on a packet IO interface.
> >>> *
> >>> * @param[in] pktio Packet IO handle.
> >>> @@ -946,6 +1047,72 @@ int odp_pktio_promisc_mode(odp_pktio_t pktio);
> >>> int odp_pktio_mac_addr(odp_pktio_t pktio, void *mac_addr, int size);
> >>>
> >>> /**
> >>> + * Set the default MAC address of a packet IO interface.
> >>> + *
> >>> + * Changes interface default MAC address to the address pointed by
> >>> 'mac_addr'.
> >>> + * Value of 'size' must be equal to the interface mac address size,
> which
> >>> is
> >>> + * specified by 'mac_addr_len' capability. Operation returns failure
> on
> >>> other
> >>> + * values of 'size'. MAC address is not changed on failure.
> >>> + *
> >>> + * @param pktio Packet IO handle
> >>> + * @param mac_addr Pointer to MAC address to be set
> >>> + * @param size Size of MAC address buffer
> >>> + *
> >>> + * @return 0 on success
> >>> + * @retval <0 on failure
> >>> + *
> >>> + * @note This API only modifies default MAC address. It doesn’t impact
> >>> + * addresses added via call to odp_mac_add_add().
> >>> + */
> >>> +int odp_pktio_mac_addr_set(odp_pktio_t pktio, const uint8_t
> *mac_addr,
> >>> + int size);
> >>> +
> >>> +/**
> >>> + * Add MAC address to a packet IO interface.
> >>> + *
> >>> + * Adds one or more number of MAC addresses to the given packet IO
> >>> interface.
> >>> + * Operation returns failure for num > odp_pktio_capablity_t:max_mac_
> >>> addresses.
> >>> + * Else return number of mac addresses actually set.
> >>> + * MAC addresses are not added on failure.
> >
> > I think that on falure for set of mac address result is undefined. The
> > reason for that you loaded some addresses to chip, some not. So on
> > failure you need to get addresses and check what was exactly loaded.
> >Even might return -index (negative index) of first failed to load mac.
> >
> >>> + *
> >>> + * @param pktio Packet IO handle
> >>> + * @param mac_addr_tbl Points to an array of MAC addresses to be added
> >>> + * @param num Number of MAC addresses to be added
> >>> + *
> >>> + * @return Number of MAC addresses actually set
> >
> > Nope. 0 on success. That is silly to expect partial load.
> >
> >>> + * @retval <0 on failure
> >>> + *
> >>> + * @note If number returns < number originally passed, application
> needs
> >>> + * to verify odp_pktio_mac_addr_t:status parameter of each MAC
> address to
> >>> + * confirm whether the corresponding MAC address is added
> successfully or
> >>> not.
> >
> > load should be stopped on first error.
> >
>
> Shally - One summary to address all responses around output parameter
> "status".
>
> Consider following use case:
> user added mac addresses @ index 0, 1, 2, 3, 4
> user removed #1 and #3 --> now we have hole at #1 and #3
> user try to add #1 #2 and #3 (though he shouldn't pass #2 but still
> passes) --> here only #1 would be set but not #2 (as its already
> occupied with valid entry). So what is the expectations? should API
> return at #2 Or continue to #3(as 3 is available to set)?
>
> if we want to enforce a rule that all indexes up to 'requested' should
> be empty / removed, then we can get rid of "status" as output param.
> Else, we can allow this flexibility to implementation to go ahead and
> consume as much as possible
> and update "status" w.r.t each entry so that app could know which of
> 'n' returned entries have been set.
>
> I think we can debate further of this based on above example. Any
> further responses to the following feedback will be more clear once we
> understand expectations to scenario above.
>
> Thanks
> Shally
>
That is why I suggested to not add indexes and just go with add() and
remove() by value.
Different hw or implementation might have it's own representation of
storage and maintain
that indexes might be nightmare. Even from application side - there is no
need to know something
about indexes and exact place in the table.
Maxim.
>
>
> >>> + */
> >>> +int odp_pktio_mac_addr_add(odp_pktio_t pktio,
> >>> + odp_pktio_mac_addr_t *mac_addr_tbl[],
> int
> >>> num);
> >
> > be careful with types. In capacity you return uint32_t, there you pass
> > int num.
> >
> >>> +
> >>> +/**
> >>> + * Remove MAC address of a packet IO interface.
> >>> + *
> >>> + * Removes one or more number of MAC addresses of the given packet IO
> >>> interface.
> >>> + * Operation returns failure for num > odp_pktio_capablity_t:max_mac_
> >>> addresses.
> >>> + * Else return number of mac addresses actually removed.
> >
> > Nope. ODP call has to exactly what is requested. No any partial remove.
> >
> >>> + * MAC addresses are not removed on failure.
> >>> + *
> >
> > Undefined state. You can not force app to load to hw what was removed on
> > error.
> >
> >>> + * @param pktio Packet IO handle
> >>> + * @param mac_addr_tbl Points to an array of MAC address to be removed
> >>> + * @param num Number of MAC addresses to be removed
> >>> + *
> >>> + * @return Number of MAC addresses actually removed
> > 0 on success.
> >
> >>> + * @retval <0 on failure
> >>> + *
> >>> + * @note If number returns < number originally passed, application
> needs
> >>> + * to verify odp_pktio_mac_addr_t:status parameter of each MAC
> address to
> >>> + * confirm whether the corresponding MAC address is removed
> successfully
> >>> + * or not.
> >>> + */
> >>> +int odp_pktio_mac_addr_remove(odp_pktio_t pktio,
> >>> + odp_pktio_mac_addr_t *mac_addr_tbl[], int num);
> >>> +
> >
> > uint32_t.
> >
> > Call is better to write as:
> >
> > int odp_pktio_mac_addr_remove(odp_pktio_t pktio, uint32_t start_idx,
> > uint32_t num);
> >
> >
> > Best regards,
> > Maxim.
> >
> >>> +/**
> >>> * Setup per-port default class-of-service.
> >>> *
> >>> * @param[in] pktio Ingress port pktio handle.
> >>> --
> >>> 1.9.3
> >>>
> >>>
> >
>
