[API-NEXT,v1,1/1] api:pktio: add MAC address and MTU set functions.

Message ID 1501573094-950-2-git-send-email-vattunuru@cavium.com
State New
Headers show
Series
  • api:pktio: add MAC address and MTU set functions
Related show

Commit Message

vamsi a Aug. 1, 2017, 7:38 a.m.
Signed-off-by: Vamsi Attunuru <vattunuru@cavium.com>

Signed-off-by: Shally Verma   <sverma@cavium.com>

Signed-off-by: Mahipal Challa <mchalla@cavium.com>


---
 include/odp/api/spec/packet_io.h | 167 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 167 insertions(+)

-- 
1.9.3

Comments

Bill Fischofer Aug. 1, 2017, 12:02 p.m. | #1
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.cavium@gmail.com>
wrote:

> Signed-off-by: Vamsi Attunuru <vattunuru@cavium.com>

> Signed-off-by: Shally Verma   <sverma@cavium.com>

> Signed-off-by: Mahipal Challa <mchalla@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;

> +

> +/**

> + * 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;

> +

> +/**

> + * 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;

> +

> +       /** Pointer to buffer containing MAC address */

> +       uint8_t *mac_addr;

> +

> +       /** Length of mac_addr buffer */

> +       uint32_t mac_addr_len;

> +

> +       /** 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;

> +} 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;

> +

> +       /** Maximum number of MAC addresses supported on this specific

> interface */

> +       uint32_t max_mac_addresses;

>  } 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

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

> + *

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

> + *

> + * @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

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

> + */

> +int odp_pktio_mac_addr_add(odp_pktio_t pktio,

> +                                odp_pktio_mac_addr_t *mac_addr_tbl[], 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.

> + * MAC addresses are not removed on failure.

> + *

> + * @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

> + * @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);

> +

> +/**

>   * Setup per-port default class-of-service.

>   *

>   * @param[in]  pktio           Ingress port pktio handle.

> --

> 1.9.3

>

>
Dmitry Eremin-Solenikov Aug. 1, 2017, 12:53 p.m. | #2
On 01/08/17 10:38, Vamsi Attunuru wrote:
> Signed-off-by: Vamsi Attunuru <vattunuru@cavium.com>

> Signed-off-by: Shally Verma   <sverma@cavium.com>

> Signed-off-by: Mahipal Challa <mchalla@cavium.com>


I'd suggest to split this into two separate patches: one for MTU (seems
simpler and will go in w/o any issues) and one for MAC addresses, which
will require some tender.

> 

> ---

>  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;

> +

> +/**

> + * 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,


What is the reason for MAC address being invalid (other than it having
wrong length)?

> +

> +	/** 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;

> +

> +/**

> + * 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;

> +

> +	/** Pointer to buffer containing MAC address */

> +	uint8_t *mac_addr;

> +

> +	/** Length of mac_addr buffer */

> +	uint32_t mac_addr_len;

> +

> +	/** 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;

> +} odp_pktio_mac_addr_t;


This structure mixes external params (like mac address), device-specific
index and return status. Could you please refactor it, so that it won't
contain a mixture of different ingredients? At least status should be
removed from this structure, in my opinion.

> +

> +/**

>   * 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;

> +

> +	/** Maximum number of MAC addresses supported on this specific interface */

> +	uint32_t max_mac_addresses;

>  } 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

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

> + *

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

> + *

> + * @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

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

> + */

> +int odp_pktio_mac_addr_add(odp_pktio_t pktio,

> +				 odp_pktio_mac_addr_t *mac_addr_tbl[], 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.

> + * MAC addresses are not removed on failure.

> + *

> + * @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

> + * @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);



What happens with indices, if an application removes several addresses
from the middle? E.g. device has address #0, I add #1 and #2, then I
remove #1. Will #2 shift to #1 or will there be a hole: #0, #2?

-- 
With best wishes
Dmitry
Bill Fischofer Aug. 1, 2017, 6:42 p.m. | #3
We discussed this patch during today's ODP public call. Notes from that
discussion:

1. Ethernet MAC addresses are not of arbitrary length. They are always 48
bits. Some pktios (not currently supported in ODP) may support 64-bit MAC
addresses, so it's OK to make provision for future support, but these two
are the only cases of interest.

2. A standard odph_ethaddr_t type is already defined (in
helper/include/odp/include/eth.h). This is more useful than an array of
uint8_ts, so the suggestion is to define odp_ethaddr_t and odp_macaddr64_t
types for use with this patch and then have parameter to
odp_pktio_mac_addr_set/add/remove() include this as a union. Since each
pktio knows the size of the mac_addr it uses, that determines which variant
is used in that particular call.

3. I agree with Dmitry that separating the simpler MTU part from the MAC
addr part will make reviews eaiser.

Other comments inline:



On Tue, Aug 1, 2017 at 7:53 AM, Dmitry Eremin-Solenikov <
dmitry.ereminsolenikov@linaro.org> wrote:

> On 01/08/17 10:38, Vamsi Attunuru wrote:

> > Signed-off-by: Vamsi Attunuru <vattunuru@cavium.com>

> > Signed-off-by: Shally Verma   <sverma@cavium.com>

> > Signed-off-by: Mahipal Challa <mchalla@cavium.com>

>

> I'd suggest to split this into two separate patches: one for MTU (seems

> simpler and will go in w/o any issues) and one for MAC addresses, which

> will require some tender.

>

> >

> > ---

> >  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 this really necessary? The mac addr type (unicast, broadcast, multicast)
is encoded in the Mac addr itself. If this is specified separately we have
a whole other set of corner cases to consider where this says one thing but
the mac addr value says something different.


> > +

> > +/**

> > + * 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,

>

> What is the reason for MAC address being invalid (other than it having

> wrong length)?

>


When we get rid of the extraneous errors, this is a general "call failed"
error. ODP_MAC_ADDR_INVALID simply means the pktio for whatever reason did
not accept this MAC_ADDR. An example might be if it were a duplicate of one
that already exists, but that's up to each pktio to say.


> > +

> > +     /** 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,

>


These two can be eliminated by using defined odp_ethaddr_t /
odp_macaddr64_t types.


> > +

> > +     /** MAC address size mismatch

> > +      * odp_pktio_mac_addr_t:mac_addr_len

> > +      * != odp_pktio_capability_t:mac_addr_len */

> > +     ODP_MAC_ADDR_SIZE_ERR,

>


Again this can be eliminated by removing explicit lengths from the API.


> > +

> > +     /** 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;

> > +

> > +/**

> > + * 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;

> > +

> > +     /** Pointer to buffer containing MAC address */

> > +     uint8_t *mac_addr;

> > +

> > +     /** Length of mac_addr buffer */

> > +     uint32_t mac_addr_len;

> > +

> > +     /** 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;

> > +} odp_pktio_mac_addr_t;

>


This should be a struct, not a union. If it's a union then all fields are
mapped on top of each other, which is clearly not what you intend here.
However, based on today's discussion, a union would be contained in the
struct:

typedef struct odp_pktio_mac_addr_t {
        uint32_t index;
        odp_mac_ops_status_t status;
        union {
                   odp_macaddr64_t mac_addr64;
                   odp_ethaddr_t mac_addr48;
        };
} odp_pktio_mac_addr_t;

The caller sets either mac_addr48 or ma_addr64 as appropriate to the
pktio_capability.mac_addr_len. The pktio always uses the one appropriate to
it so there's no need for an internal discriminator here.


> This structure mixes external params (like mac address), device-specific

> index and return status. Could you please refactor it, so that it won't

> contain a mixture of different ingredients? At least status should be

> removed from this structure, in my opinion.

>

> > +

> > +/**

> >   * 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;

>


This should be specified in bits and noted to be either 48 or 64. Or we can
make this an enum for the supported sizes rather than a uint32_t.


> > +

> > +     /** Maximum number of MAC addresses supported on this specific

> interface */

> > +     uint32_t max_mac_addresses;

> >  } 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

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

> > + *

> > + * @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);

>


While the existing odp_pktio_mac_addr() API returns a byte array, should
use mac_addr types for this to eliminate the need for a length. Similarly,
we need an odp_pktio_mac_addr_list() to query/return all of the mac_addrs
associated with this pktio.


> > +

> > +/**

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

> > + *

> > + * @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

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

> > + */

> > +int odp_pktio_mac_addr_add(odp_pktio_t pktio,

> > +                              odp_pktio_mac_addr_t *mac_addr_tbl[], 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.

> > + * MAC addresses are not removed on failure.

> > + *

> > + * @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

> > + * @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);

>

>

> What happens with indices, if an application removes several addresses

> from the middle? E.g. device has address #0, I add #1 and #2, then I

> remove #1. Will #2 shift to #1 or will there be a hole: #0, #2?

>


I'd imagine these are simply slots that are either "filled" or "empty" but
agree this should be documented.


>

> --

> With best wishes

> Dmitry

>
Dmitry Eremin-Solenikov Aug. 1, 2017, 7:05 p.m. | #4
On 01/08/17 21:42, Bill Fischofer wrote:
> We discussed this patch during today's ODP public call. Notes from that

> discussion:


[skipped]

> This should be a struct, not a union. If it's a union then all fields

> are mapped on top of each other, which is clearly not what you intend

> here. However, based on today's discussion, a union would be contained

> in the struct:

> 

> typedef struct odp_pktio_mac_addr_t {

>         uint32_t index;

>         odp_mac_ops_status_t status;

>         union {

>                    odp_macaddr64_t mac_addr64;

>                    odp_ethaddr_t mac_addr48;

>         };

> } odp_pktio_mac_addr_t;

>  

> The caller sets either mac_addr48 or ma_addr64 as appropriate to the

> pktio_capability.mac_addr_len. The pktio always uses the one appropriate

> to it so there's no need for an internal discriminator here.


status field should be removed from this struct. It is a result of the
operation, not the parameter.

-- 
With best wishes
Dmitry
Maxim Uvarov Aug. 1, 2017, 8:09 p.m. | #5
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.cavium@gmail.com>

> wrote:

> 

>> Signed-off-by: Vamsi Attunuru <vattunuru@cavium.com>

>> Signed-off-by: Shally Verma   <sverma@cavium.com>

>> Signed-off-by: Mahipal Challa <mchalla@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.


>> +/**

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

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

>> + */

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

>>

>>
shally verma Aug. 2, 2017, 6:03 a.m. | #6
On Wed, Aug 2, 2017 at 1:39 AM, Maxim Uvarov <maxim.uvarov@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.cavium@gmail.com>

>> wrote:

>>

>>> Signed-off-by: Vamsi Attunuru <vattunuru@cavium.com>

>>> Signed-off-by: Shally Verma   <sverma@cavium.com>

>>> Signed-off-by: Mahipal Challa <mchalla@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


>>> + */

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

>>>

>>>

>
Maxim Uvarov Aug. 2, 2017, 7:03 a.m. | #7
On 2 August 2017 at 09:03, shally verma <shallyvermacavium@gmail.com> wrote:

> On Wed, Aug 2, 2017 at 1:39 AM, Maxim Uvarov <maxim.uvarov@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.cavium@gmail.com>

> >> wrote:

> >>

> >>> Signed-off-by: Vamsi Attunuru <vattunuru@cavium.com>

> >>> Signed-off-by: Shally Verma   <sverma@cavium.com>

> >>> Signed-off-by: Mahipal Challa <mchalla@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

> >>>

> >>>

> >

>
Dmitry Eremin-Solenikov Aug. 2, 2017, 9:29 a.m. | #8
On 02/08/17 09:03, shally verma wrote:
> On Wed, Aug 2, 2017 at 1:39 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

>> On 08/01/17 15:02, Bill Fischofer wrote:


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


This looks like overloading of return result. I see three viable
alternatives:

- Try to do as much as possible. Return the array of statuses/errors
telling which addresses got propagated to hw and which did not.

- Stop on first error. Return amount of addresses processed successfully.

- Transaction-like. Either all or none addresses get set. Return 0 on
success, -1 on failure.

I'd settle for either option 2 or option 3.

>>

>>>> + *

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



You can have an array of statuses as output parameter.

-- 
With best wishes
Dmitry
Dmitry Eremin-Solenikov Aug. 2, 2017, 9:30 a.m. | #9
On 02/08/17 10:03, Maxim Uvarov wrote:
> On 2 August 2017 at 09:03, shally verma <shallyvermacavium@gmail.com> wrote:

> 

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

Seconding this idea, if it fits application requirements.

-- 
With best wishes
Dmitry

Patch hide | download patch | download mbox

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;
+
+/**
+ * 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;
+
+/**
+ * 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;
+
+	/** Pointer to buffer containing MAC address */
+	uint8_t *mac_addr;
+
+	/** Length of mac_addr buffer */
+	uint32_t mac_addr_len;
+
+	/** 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;
+} 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;
+
+	/** Maximum number of MAC addresses supported on this specific interface */
+	uint32_t max_mac_addresses;
 } 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
+ * 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.
+ *
+ * @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.
+ *
+ * @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
+ * @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.
+ */
+int odp_pktio_mac_addr_add(odp_pktio_t pktio,
+				 odp_pktio_mac_addr_t *mac_addr_tbl[], 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.
+ * MAC addresses are not removed on failure.
+ *
+ * @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
+ * @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);
+
+/**
  * Setup per-port default class-of-service.
  *
  * @param[in]	pktio		Ingress port pktio handle.