>
> > + goto failed;
> > + }
>
> I think it is better to merge this ioctl and the socket creation with
> sk_get_ts_info().
> No reason for duplication.
> You can use a static function or inline one.
>
> > +
> > + if (ecmd.req.link_mode_masks_nwords >= 0 ||
> > + ecmd.req.cmd != ETHTOOL_GLINKSETTINGS) {
> > + return 1;
> > + }
>
[Devasish]: This can be done.
> + /* clear data and ensure it is not marked valid */
> + memset(if_info, 0, sizeof(struct sk_if_info));
> + return -1;
You need to close the socket!
[Devasish]: Noted. Will be updated in the next patch.
> +struct sk_if_info {
> + bool valid;
It is better not to use bool in a structure, as the size is usually
int, i.e. 64 bits to hold 1 bit.
[Devasish]: Earlier we had it as an integer. We can update it to uint8_t as
well. But Richard suggested updating it to Boolean.
So, leaving this to Richard.
> + int speed;
What are the units here?
Old systems use 32 bits, if you measure in bits per second, the maximum
is 4 GB.
It is better to use uint64_t and use 1 mbps as units.
[Devasish]: It is the same as the speed in ethtool_link_settings which is a
uint32_t variable and it is in Mbps.
Will change it to uint32_t and update the comments.
Thanks,
Devasish.
On Fri, 9 Dec 2022 at 17:29, Geva, Erez <erez.geva....@siemens.com> wrote:
> On Wed, 2022-12-07 at 17:34 +0530, SyncMonk Technologies wrote:
> > When master and slave instance interacting with each other operating
> > at different interface speed, delay assymetry needs to be compensated
> > as described in G.8271 appendix V.
> >
> > In this patch we are adding changes to get the interface speed using
> > ethtool.
> >
> > v1: initial commit.
> > v2: updating comments and data types.
> > v3: updating Boolean data type to bool from <stdbool.h>
> > ---
> > sk.c | 71
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > sk.h | 19 ++++++++++++++++
> > 2 files changed, 90 insertions(+)
> >
> > diff --git a/sk.c b/sk.c
> > index d27abff..1d1a656 100644
> > --- a/sk.c
> > +++ b/sk.c
> > @@ -205,6 +205,77 @@ failed:
> > return -1;
> > }
> >
> > +int sk_get_if_info(const char *name, struct sk_if_info *if_info)
> > +{
> > +#ifdef ETHTOOL_GLINKSETTINGS
> > + struct ifreq ifr;
> > + int fd, err;
> > +
> > + struct {
> > + struct ethtool_link_settings req;
> > + /*
> > + * link_mode_data consists of supported[],
> > advertising[],
> > + * lp_advertising[] with size up to 127 each.
> > + * The actual size is provided by the kernel.
> > + */
> > + __u32 link_mode_data[3 * 127];
> > + } ecmd;
> > +
> > + memset(&ifr, 0, sizeof(ifr));
> > + memset(&ecmd, 0, sizeof(ecmd));
> > +
> > + fd = socket(AF_INET, SOCK_DGRAM, 0);
> > + if (fd < 0) {
> > + goto failed;
> > + }
> > +
> > + ecmd.req.cmd = ETHTOOL_GLINKSETTINGS;
> > +
> > + strncpy(ifr.ifr_name, name, IFNAMSIZ - 1);
> > + ifr.ifr_data = (char *) &ecmd;
> > +
> > + /* Handshake with kernel to determine number of words for
> > link
> > + * mode bitmaps. When requested number of bitmap words is not
> > + * the one expected by kernel, the latter returns the integer
> > + * opposite of what it is expecting. We request length 0
> > below
> > + * (aka. invalid bitmap length) to get this info.
> > + */
> > + err = ioctl(fd, SIOCETHTOOL, &ifr);
> > + if (err < 0) {
> > + pr_err("ioctl SIOCETHTOOL failed: %m");
> > + close(fd);
> > + goto failed;
> > + }
>
> I think it is better to merge this ioctl and the socket creation with
> sk_get_ts_info().
> No reason for duplication.
> You can use a static function or inline one.
>
> > +
> > + if (ecmd.req.link_mode_masks_nwords >= 0 ||
> > + ecmd.req.cmd != ETHTOOL_GLINKSETTINGS) {
> > + return 1;
> > + }
> > + ecmd.req.link_mode_masks_nwords = -
> > ecmd.req.link_mode_masks_nwords;
> > +
> > + err = ioctl(fd, SIOCETHTOOL, &ifr);
> > + if (err < 0) {
> > + pr_err("ioctl SIOCETHTOOL failed: %m");
> > + close(fd);
> > + goto failed;
> > + }
> > +
> > + close(fd);
> > +
> > + /* copy the necessary data to sk_info */
> > + memset(if_info, 0, sizeof(struct sk_if_info));
> > + if_info->valid = 1;
> > + if_info->speed = ecmd.req.speed;
> > +
> > + return 0;
> > +failed:
> > +#endif
> > + /* clear data and ensure it is not marked valid */
> > + memset(if_info, 0, sizeof(struct sk_if_info));
> > + return -1;
>
> You need to close the socket!
>
> > +}
> > +
> > +
> > static int sk_interface_guidaddr(const char *name, unsigned char
> > *guid)
> > {
> > char file_name[64], buf[64], addr[8];
> > diff --git a/sk.h b/sk.h
> > index 486dbc4..4cee3d7 100644
> > --- a/sk.h
> > +++ b/sk.h
> > @@ -20,6 +20,7 @@
> > #ifndef HAVE_SK_H
> > #define HAVE_SK_H
> >
> > +#include <stdbool.h>
> > #include "address.h"
> > #include "transport.h"
> >
> > @@ -49,6 +50,16 @@ struct sk_ts_info {
> > unsigned int rx_filters;
> > };
> >
> > +/**
> > + * Contains interface information returned by theGLINKSETTINGS
> > ioctl.
> > + * @valid: set to non-zero when the info struct contains
> > valid data.
> > + * @speed: interface speed.
> > + */
> > +struct sk_if_info {
> > + bool valid;
> It is better not to use bool in a structure, as the size is usually
> int, i.e. 64 bits to hold 1 bit.
>
> > + int speed;
> What are the units here?
> Old systems use 32 bits, if you measure in bits per second, the maximum
> is 4 GB.
> It is better to use uint64_t and use 1 mbps as units.
>
> > +};
> > +
> > /**
> > * Obtains a socket suitable for use with sk_interface_index().
> > * @return An open socket on success, -1 otherwise.
> > @@ -78,6 +89,14 @@ int sk_general_init(int fd);
> > */
> > int sk_get_ts_info(const char *name, struct sk_ts_info *sk_info);
> >
> > +/**
> > + * Obtain supporte interface information
> > + * @param name The name of the interface
> > + * @param info Struct containing obtained interface
> > information.
> > + * @return zero on success, negative on failure.
> > + */
> > +int sk_get_if_info(const char *name, struct sk_if_info *sk_info);
> > +
> > /**
> > * Obtain the MAC address of a network interface.
> > * @param name The name of the interface
>
>
> Erez
>
>
> _______________________________________________
> Linuxptp-devel mailing list
> Linuxptp-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
>
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
