| Message ID | 1566327686-8996-1-git-send-email-uli+renesas@fpond.eu (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [v3] ravb: implement MTU change while device is up | expand |
Hi Uli, thanks for the patch. On Tue, Aug 20, 2019 at 09:01:26PM +0200, Ulrich Hecht wrote: > Uses the same method as various other drivers: shut the device down, > change the MTU, then bring it back up again. > > Tested on Renesas D3 Draak and M3-W Salvator-X boards. > > Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Are these reviews left from v2? If so, I'd prefer to see them given again because the logic was changed in v3. > --- > > Hi! > > This revision reverts the MTU change if re-opening the device fails. > > CU > Uli > > > drivers/net/ethernet/renesas/ravb_main.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index ef8f089..402bcec 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -1810,12 +1810,24 @@ static int ravb_do_ioctl(struct net_device *ndev, struct ifreq *req, int cmd) > > static int ravb_change_mtu(struct net_device *ndev, int new_mtu) > { > + unsigned int old_mtu = ndev->mtu; > + > if (netif_running(ndev)) > - return -EBUSY; > + ravb_close(ndev); > > ndev->mtu = new_mtu; > netdev_update_features(ndev); > > + if (netif_running(ndev)) { > + int err = ravb_open(ndev); > + > + if (err) { > + ndev->mtu = old_mtu; > + netdev_update_features(ndev); > + return err; > + } > + } > + > return 0; > } > > -- > 2.7.4 >
From: Ulrich Hecht <uli+renesas@fpond.eu> Date: Tue, 20 Aug 2019 21:01:26 +0200 > This revision reverts the MTU change if re-opening the device fails. But you leave the device closed if this happens. You have to implement this properly, with a full prepare/commit sequence. First allocate all of the necessary resources, such that you can guarantee success. If you cannot allocate these resources you must fail the operation and leave the device _UP_ with the original MTU value. If you can allocate the resource, you can fully commit to the MTU change and return success. You must not fail the operation in such a way that the device is left inoperable. But that is precisely what your patch currently does.
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index ef8f089..402bcec 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -1810,12 +1810,24 @@ static int ravb_do_ioctl(struct net_device *ndev, struct ifreq *req, int cmd) static int ravb_change_mtu(struct net_device *ndev, int new_mtu) { + unsigned int old_mtu = ndev->mtu; + if (netif_running(ndev)) - return -EBUSY; + ravb_close(ndev); ndev->mtu = new_mtu; netdev_update_features(ndev); + if (netif_running(ndev)) { + int err = ravb_open(ndev); + + if (err) { + ndev->mtu = old_mtu; + netdev_update_features(ndev); + return err; + } + } + return 0; }