I wonder why these question were risen now. In patch review, not in arch
review :)
But summarizing everything:
1. Patches are checkpatch clean.
2. Minor style check for comment - agree will correct it.
3. Requested change 'odp_pktio_t id' to 'odp_pktio_t hdl' is not
correct. That is subject for other patch.
Code has to look same for all function. Everywhere we use 'odp_pktio_t id'.
Run: fgrep -r odp_pktio_t ./platform/linux-generic
4. Return codes. I followed return codes as API proposed. Please note
that I used ioctl, but I could do it in other way.
So it is only implementation. Inside implementation function I added
ODP_ERR. If you need specific reason
run strace on your application, that is very common for linux apps.
If we decided to add more error codes to odp, then we can add. Like
ERROR_MTU_TOO_SMALL, ERROR_MTU_TOO_BIG.
But that has to go to arch first. And I would like to have it in
separate patch. To discuss with vendors if that returns codes
are valuable and if they can really return them.
Maxim.
On 11/11/2014 11:41 PM, Bill Fischofer wrote:
On Tue, Nov 11, 2014 at 2:28 PM, Anders Roxell
<[email protected] <mailto:[email protected]>> wrote:
Drop linux-generic because we only have linux-generic and say
something
like:
"pktio: add MTU manipulation functions"
On 2014-11-11 21:14, Maxim Uvarov wrote:
> Implement pktio mtu functions:
> odp_pktio_mtu() to get mtu value;
> odp_pktio_set_mtu() to set mtu value.
>
> Signed-off-by: Maxim Uvarov <[email protected]
<mailto:[email protected]>>
> ---
> platform/linux-generic/include/api/odp_packet_io.h | 20 +++++++++
> .../linux-generic/include/odp_packet_io_internal.h | 4 ++
> platform/linux-generic/odp_packet_io.c | 50 ++++++++++++++++++++++
> 3 files changed, 74 insertions(+)
>
> diff --git a/platform/linux-generic/include/api/odp_packet_io.h
b/platform/linux-generic/include/api/odp_packet_io.h
> index 360636d..b784705 100644
> --- a/platform/linux-generic/include/api/odp_packet_io.h
> +++ b/platform/linux-generic/include/api/odp_packet_io.h
> @@ -135,6 +135,26 @@ void odp_pktio_set_input(odp_packet_t pkt,
odp_pktio_t id);
> */
> odp_pktio_t odp_pktio_get_input(odp_packet_t pkt);
>
> +/*
> + * Configure the MTU for a packet IO interface.
Is this description enough for a new user to understand
what will occur?
I'd think so. MTU is a pretty standard bit of interface metadata.
> + *
> + * @param[in] id ODP packet IO handle.
hdl should be the standard way for ODP to specify the handle.
> + * @param[in] mtu The MTU to be applied.
The value of MTU that the interface will be configured to use.
> + *
> + * @return 0 on success, -1 on error.
@retval 0 <reason>
@retval -1 <reason(s)>
Elaborate on what causes an error, if it's not possible to provide a
good reason for -1 should this be a void return?
The ioctl may fail. Wouldn't the application want to know that? For
example the range of valid MTUs will vary by device and ODP can't be
expected to know that. It just calls the ioctl and Linux tells
whether that's OK. Since linux will set errno on ioctl failure, -1 is
just following the standard ODP error return protocol.
> + */
> +int odp_pktio_set_mtu(odp_pktio_t id, unsigned int mtu);
> +
> +/*
> + * Get the currently configured MTU of a packet IO interface.
Is this description enough for a new user to understand
what will occur?
Again, yes.
> + *
> + * @param[in] id ODP packet IO handle.
Does not line up
hdl should be the standard way for ODP to specify the handle.
> + * @param[out] mtu Pointer to location in which to store the
MTU value.
> + *
> + * @return 0 on success, -1 on error.
> + */
> +int odp_pktio_mtu(odp_pktio_t id, unsigned int *mtu);
unsigned int *odp_pktio_mtu(odp_pktio_t id);
@retval mtu on succes
@retval NULL on error <list all the errors>
No, this routine has an output parameter. This is just following
standard ODP RC conventions for reporting success/failure as an int
function return value. NULL can only be assigned to a pointer, not an
int. Regarding the errors, again the ioctl will set errno so if the
caller want's to know why it failed it can read that. It's
system-dependent since each interface may be slightly different.
> +
> /**
> * @}
> */
> diff --git
a/platform/linux-generic/include/odp_packet_io_internal.h
b/platform/linux-generic/include/odp_packet_io_internal.h
> index 23633ed..ff73970 100644
> --- a/platform/linux-generic/include/odp_packet_io_internal.h
> +++ b/platform/linux-generic/include/odp_packet_io_internal.h
> @@ -21,6 +21,8 @@ extern "C" {
> #include <odp_spinlock.h>
> #include <odp_packet_socket.h>
>
> +#include <linux/if.h>
> +
> /**
> * Packet IO types
> */
> @@ -38,6 +40,8 @@ struct pktio_entry {
> odp_pktio_type_t type; /**< pktio type */
> pkt_sock_t pkt_sock; /**< using socket API for
IO */
> pkt_sock_mmap_t pkt_sock_mmap; /**< using socket mmap API
for IO */
> + char name[IFNAMSIZ]; /**< name of pktio provided to
> + pktio_open() */
Does not line up.
Doesn't checkpatch catch actual style errors? I'm assuming this is
checkpatch clean. If it is that should be sufficient. If it's not,
then it should be made checkpatch-clean before posting it.
Cheers,
Anders
> };
>
> typedef union {
> diff --git a/platform/linux-generic/odp_packet_io.c
b/platform/linux-generic/odp_packet_io.c
> index f35193f..20fe10a 100644
> --- a/platform/linux-generic/odp_packet_io.c
> +++ b/platform/linux-generic/odp_packet_io.c
> @@ -20,6 +20,7 @@
> #include <odp_debug.h>
>
> #include <string.h>
> +#include <sys/ioctl.h>
>
> typedef struct {
> pktio_entry_t entries[ODP_CONFIG_PKTIO_ENTRIES];
> @@ -203,6 +204,7 @@ odp_pktio_t odp_pktio_open(const char *dev,
odp_buffer_pool_t pool)
> return ODP_PKTIO_INVALID;
>
> done:
> + strncpy(pktio_entry->s.name <http://s.name>, dev, IFNAMSIZ);
> unlock_entry(pktio_entry);
> return id;
> }
> @@ -476,3 +478,51 @@ int pktin_deq_multi(queue_entry_t *qentry,
odp_buffer_hdr_t *buf_hdr[], int num)
>
> return nbr;
> }
> +
> +int odp_pktio_set_mtu(odp_pktio_t id, unsigned int mtu)
> +{
> + pktio_entry_t *pktio_entry = get_entry(id);
> + int sockfd;
> + struct ifreq ifr;
> + int ret;
> +
> + if (pktio_entry->s.pkt_sock_mmap.sockfd)
> + sockfd = pktio_entry->s.pkt_sock_mmap.sockfd;
> + else
> + sockfd = pktio_entry->s.pkt_sock.sockfd;
> +
> + strncpy(ifr.ifr_name, pktio_entry->s.name <http://s.name>,
IFNAMSIZ);
> + ifr.ifr_mtu = mtu;
> +
> + ret = ioctl(sockfd, SIOCSIFMTU, (caddr_t)&ifr);
> + if (ret != 0) {
> + ODP_ERR("ioctl SIOCSIFMTU error\n");
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +int odp_pktio_mtu(odp_pktio_t id, unsigned int *mtu)
> +{
> + pktio_entry_t *pktio_entry = get_entry(id);
> + int sockfd;
> + struct ifreq ifr;
> + int ret;
> +
> + if (pktio_entry->s.pkt_sock_mmap.sockfd)
> + sockfd = pktio_entry->s.pkt_sock_mmap.sockfd;
> + else
> + sockfd = pktio_entry->s.pkt_sock.sockfd;
> +
> + strncpy(ifr.ifr_name, pktio_entry->s.name <http://s.name>,
IFNAMSIZ);
> +
> + ret = ioctl(sockfd, SIOCGIFMTU, &ifr);
> + if (ret != 0) {
> + ODP_ERR("ioctl SIOCGIFMTU error\n");
> + return -1;
> + }
> +
> + *mtu = ifr.ifr_mtu;
> + return 0;
> +}
> --
> 1.8.5.1.163.gd7aced9
>
>
> _______________________________________________
> lng-odp mailing list
> [email protected] <mailto:[email protected]>
> http://lists.linaro.org/mailman/listinfo/lng-odp
--
Anders Roxell
[email protected] <mailto:[email protected]>
M: +46 709 71 42 85 <tel:%2B46%20709%2071%2042%2085> | IRC: roxell
_______________________________________________
lng-odp mailing list
[email protected] <mailto:[email protected]>
http://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp
