On Tue, 2022-12-13 at 10:39 +0530, Devasish Dey wrote:
> + 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.
Good point, we can define a Boolean type using uint8_t for structures use.
But we can leave this decision 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.
We do not have to follow the kernel ioctl type for speed, they tend to change
them from time to time.
Probably, someone in the past choose 'int' as he did not predict future speed
will pass 31 bits (int is signed).
And also the use of uintNN_t was not so common than.
But as sk.c is our internal layer, it can translate from kernel units and types
to our application units and types.
You do provide internal structure and do not use the kernel one :-)
So if in the future the kernel will use a new ioctl for speed as int will be
too small, we can retain out internal interface.
This is my opinion, but you can defer.
Thanks,
Devasish.
Erez
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
