Re: [PATCH 1/3] net: add a new NETDEV_CHANGEROOM event type

Tue, Aug 20, 2013 at 05:30:48PM CEST, f.fainelli@gmail.com wrote:

2013/8/20 Johannes Berg [off-list ref]:

quoted

On Tue, 2013-08-20 at 13:45 +0100, Florian Fainelli wrote:

quoted

 /**
+ *   dev_set_headroom - Change device needed headroom
+ *   @dev: device
+ *   @new_headroom: new headroom size
+ *
+ *   Change the network device headroom space.
+ */
+int dev_set_headroom(struct net_device *dev, unsigned short new_headroom)

It seems that you need to invoke these under RTNL, might be worth
documenting that.

Good point, yes.

quoted

Also, maybe it would be worth doing it in one call? If you need to
change both, then you'd end up calling the notifier twice, which is less
efficient?

I have mixed feelings about this. I do not expect changing the
headroom/tailroom to be in a hot-path, and we would need to have a
name such as dev_set_head_and_tailroom() or something that clearly
states that it operates on both quantities. Looking at the subsystems
and drivers, there are quite a lot of users which only set one or the
other, occasionaly both before registration.

quoted

I suppose you could make them 'int' arguments and reserve -1

Ugh, -1, I don't like this. I think that they should be set separate. Not
real need to do it in one function.

quoted

for no changes, or just require both new values to be given (if doing
this at all.)

What I like about keeping them separate is that we can use the
"native" storage type that is used in struct net_device, and have
compile-time checking of this.
-- 
Florian