From: Andrew Lunn <andrew@lunn.ch>
To: Murali Krishna Policharla <murali.policharla@broadcom.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>,
davem@davemloft.net, amritha.nambiar@intel.com,
ecree@solarflare.com, ktkhai@virtuozzo.com,
alexander.h.duyck@intel.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net: core: Fix to store new mtu setting in netdevice.
Date: Wed, 2 Jan 2019 14:16:57 +0100 [thread overview]
Message-ID: <20190102131657.GE22737@lunn.ch> (raw)
In-Reply-To: <b6ff97415cd415614393e35793972ccc@mail.gmail.com>
> Hi Andrew/Heiner
>
> Thanks for the feedback. This patch
> fixes a case where ndo_change_mtu function is provided but the callback
> function is not storing mtu to netdevice structure.
Hi Murali
At the moment, any driver which implements ndo_change_mtu MUST set
ndev->mtu. It is a nice clean definition, easy for any driver write to
understand.
What you are proposing is that the core will set ndev->mtu. That is
fine, but again we need a nice clean definition. The drivers MUST NOT
set ndev->mtu.
If you plan to make this core change, please also change all the
drivers as well, so we have very clear semantics of what is expected.
It would also be good to extend the comment in include/linux/netdevice.h:
* int (*ndo_change_mtu)(struct net_device *dev, int new_mtu);
* Called when a user wants to change the Maximum Transfer Unit
* of a device.
Adding something like: Should only program the hardware with the new MTU.
To give the hint that the core is doing all the validation and
modifying ndev->mtu.
I had one other thought. Please also take a look at how stacked
devices work. I've not looked, but i expect there are cases where a
lower device calls into an upper device to inform it the MTU has
changed. When does this call happen? Does the MTU of the lower device
need to of already changed before this call is made? Are there similar
cases of an upper device calling down to the lower device? You need to
be careful here, you are changing exactly when the ndev->mtu value
changes, and so could be introducing bugs if you don't do a proper
code review.
Andrew
next prev parent reply other threads:[~2019-01-02 13:16 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-01 6:42 [PATCH] net: core: Fix to store new mtu setting in netdevice Murali Krishna Policharla
2019-01-01 7:54 ` Kirill Tkhai
2019-01-01 9:40 ` Heiner Kallweit
2019-01-01 23:36 ` Andrew Lunn
2019-01-02 6:27 ` Heiner Kallweit
2019-01-01 8:34 ` Andrew Lunn
2019-01-01 9:48 ` Murali Krishna Policharla
2019-01-01 23:24 ` Andrew Lunn
2019-01-02 9:54 ` Murali Krishna Policharla
2019-01-02 13:16 ` Andrew Lunn [this message]
2019-01-03 9:54 ` Murali Krishna Policharla
2019-01-01 9:35 ` Heiner Kallweit
2019-01-01 21:44 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190102131657.GE22737@lunn.ch \
--to=andrew@lunn.ch \
--cc=alexander.h.duyck@intel.com \
--cc=amritha.nambiar@intel.com \
--cc=davem@davemloft.net \
--cc=ecree@solarflare.com \
--cc=hkallweit1@gmail.com \
--cc=ktkhai@virtuozzo.com \
--cc=linux-kernel@vger.kernel.org \
--cc=murali.policharla@broadcom.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).