Re: [PATCH net-next v2 2/3] ethtool: Add support for DMA buffer settings
From: David Miller <davem@davemloft.net>
Date: 2014-07-31 00:53:07
From: Govindarajulu Varadarajan <redacted> Date: Tue, 29 Jul 2014 17:10:38 +0530
quoted hunk ↗ jump to hunk
@@ -440,6 +440,20 @@ struct ethtool_ringparam { }; /** + * struct ethtool_buffparam - DMA buffer parameters + * @rx_copybreak_cur: current receive DMA buff rx_copybreak. + * @rx_copybreak_min: min rx_copybreak set by driver. + * @rx_copybreak_max: Max rx_copybreak set by driver. + * @reserved: reserve room for future use. + */ +struct ethtool_buffparam { + __u32 cmd; + __u32 rx_copybreak_cur; + __u32 rx_copybreak_max; + __u8 reserved[84]; +};
I don't understand the reasoning behind this reserved field. You can't use it to add more fields later, because right now we'll let the user put any garbage there and thus if you add more fields that garbage from older apps would be interpreted as one of the new values. Largely we have not been adding reserved fields to new ethtool structures, and this is the primary reason I guess. It's a shame that when we want to add a new 32-bit knob we have to add an entire new struct. We have ethtool_value, but that's only good for one knob at a time and we have to allocate an entire new ethtool command value for each such knob. I really don't know what the recommend here. However I wonder what value that "max" thing has, I think setting the value to infinity is just fine, it just means every packet will be copied. So if we remove the 'max', we just have the copybreak itself, and you can therefore use ethtool_value and allocate the ethtool command numbers. What do you think?