| Message ID | 20210506050658.9624-1-qiangqing.zhang@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V4,net] net: stmmac: Fix MAC WoL not working if PHY does not support WoL | expand |
On Thu, 6 May 2021 13:06:58 +0800 Joakim Zhang wrote: > Both get and set WoL will check device_can_wakeup(), if MAC supports PMT, > it will set device wakeup capability. After commit 1d8e5b0f3f2c ("net: > stmmac: Support WOL with phy"), device wakeup capability will be overwrite > in stmmac_init_phy() according to phy's Wol feature. If phy doesn't support > WoL, then MAC will lose wakeup capability. Let's take a step back. Can we get a minimal fix for losing the config in stmmac_init_phy(), and then extend the support for WoL for devices which do support wake up themselves? > This patch combines WoL capabilities both MAC and PHY from stmmac_get_wol(), > set wakeup capability and give WoL priority to the PHY in stmmac_set_wol() > when enable WoL. What PHYs do implement is WAKE_MAGIC, WAKE_UCAST, WAKE_MAGICSECURE > and WAKE_BCAST. > > Fixes: commit 1d8e5b0f3f2c ("net: stmmac: Support WOL with phy") Please remove "commit" from the fixes tag. > Suggested-by: Andrew Lunn <andrew@lunn.ch> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > index c5642985ef95..6d09908dec1f 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > @@ -629,35 +629,49 @@ static void stmmac_get_strings(struct net_device *dev, u32 stringset, u8 *data) > /* Currently only support WOL through Magic packet. */ > static void stmmac_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol) > { > + struct ethtool_wolinfo wol_phy = { .cmd = ETHTOOL_GWOL }; > struct stmmac_priv *priv = netdev_priv(dev); > > - if (!priv->plat->pmt) > - return phylink_ethtool_get_wol(priv->phylink, wol); > - > mutex_lock(&priv->lock); > - if (device_can_wakeup(priv->device)) { Why remove the device_can_wakeup() check? > + if (priv->plat->pmt) { > wol->supported = WAKE_MAGIC | WAKE_UCAST; > if (priv->hw_cap_support && !priv->dma_cap.pmt_magic_frame) > wol->supported &= ~WAKE_MAGIC; > - wol->wolopts = priv->wolopts; > } > + > + phylink_ethtool_get_wol(priv->phylink, &wol_phy); > + > + /* Combine WoL capabilities both PHY and MAC */ > + wol->supported |= wol_phy.supported; > + wol->wolopts = priv->wolopts; > + > mutex_unlock(&priv->lock); > } > > static int stmmac_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) > { > + u32 support = WAKE_MAGIC | WAKE_UCAST | WAKE_MAGICSECURE | WAKE_BCAST; Why this list? > + struct ethtool_wolinfo wol_phy = { .cmd = ETHTOOL_GWOL }; > struct stmmac_priv *priv = netdev_priv(dev); > - u32 support = WAKE_MAGIC | WAKE_UCAST; This list was the list of what the MAC supported, right? > + int ret; > > - if (!device_can_wakeup(priv->device)) Why remove this check? > + if (wol->wolopts & ~support) > return -EOPNOTSUPP; > > - if (!priv->plat->pmt) { > - int ret = phylink_ethtool_set_wol(priv->phylink, wol); > - > - if (!ret) > - device_set_wakeup_enable(priv->device, !!wol->wolopts); > - return ret; > + /* First check if can WoL from PHY */ > + phylink_ethtool_get_wol(priv->phylink, &wol_phy); > + if (wol->wolopts & wol_phy.supported) { So if _any_ requests match PHY capabilities we'd use PHY? I think the check should be: if ((wol->woltops & wol_phy.supported) == wol->woltops) That said I'm not 100% sure what the semantics for WoL are. > + wol->wolopts &= wol_phy.supported; > + ret = phylink_ethtool_set_wol(priv->phylink, wol); > + > + if (!ret) { > + pr_info("stmmac: phy wakeup enable\n"); > + device_set_wakeup_capable(priv->device, 1); > + device_set_wakeup_enable(priv->device, 1); > + goto wolopts_update; > + } else { > + return ret; > + } > } > > /* By default almost all GMAC devices support the WoL via > @@ -666,18 +680,21 @@ static int stmmac_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) > if ((priv->hw_cap_support) && (!priv->dma_cap.pmt_magic_frame)) > wol->wolopts &= ~WAKE_MAGIC; > > - if (wol->wolopts & ~support) > - return -EINVAL; Now you seem to not validate against MAC capabilities anywhere. > - if (wol->wolopts) { > - pr_info("stmmac: wakeup enable\n"); > + if (priv->plat->pmt && wol->wolopts) { > + pr_info("stmmac: mac wakeup enable\n"); > + device_set_wakeup_capable(priv->device, 1); > device_set_wakeup_enable(priv->device, 1); > enable_irq_wake(priv->wol_irq); > - } else { > + goto wolopts_update; > + } > + > + if (!wol->wolopts) { > + device_set_wakeup_capable(priv->device, 0); > device_set_wakeup_enable(priv->device, 0); > disable_irq_wake(priv->wol_irq); > } > > +wolopts_update: > mutex_lock(&priv->lock); > priv->wolopts = wol->wolopts; > mutex_unlock(&priv->lock);
Hi Jakub, > -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: 2021年5月7日 8:55 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com; > joabreu@synopsys.com; davem@davemloft.net; f.fainelli@gmail.com; > andrew@lunn.ch; Jisheng.Zhang@synaptics.com; netdev@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH V4 net] net: stmmac: Fix MAC WoL not working if PHY does > not support WoL > > On Thu, 6 May 2021 13:06:58 +0800 Joakim Zhang wrote: > > Both get and set WoL will check device_can_wakeup(), if MAC supports > > PMT, it will set device wakeup capability. After commit 1d8e5b0f3f2c ("net: > > stmmac: Support WOL with phy"), device wakeup capability will be > > overwrite in stmmac_init_phy() according to phy's Wol feature. If phy > > doesn't support WoL, then MAC will lose wakeup capability. > > Let's take a step back. Can we get a minimal fix for losing the config in > stmmac_init_phy(), and then extend the support for WoL for devices which do > support wake up themselves? Sure, please review the V1, I think this is a minimal fix, then we can extend this as a new feature. https://www.spinics.net/lists/netdev/msg733531.html > > This patch combines WoL capabilities both MAC and PHY from > > stmmac_get_wol(), set wakeup capability and give WoL priority to the > > PHY in stmmac_set_wol() when enable WoL. What PHYs do implement is > > WAKE_MAGIC, WAKE_UCAST, WAKE_MAGICSECURE and WAKE_BCAST. > > > > Fixes: commit 1d8e5b0f3f2c ("net: stmmac: Support WOL with phy") > > Please remove "commit" from the fixes tag. Yes. > > Suggested-by: Andrew Lunn <andrew@lunn.ch> > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > > b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > > index c5642985ef95..6d09908dec1f 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > > @@ -629,35 +629,49 @@ static void stmmac_get_strings(struct net_device > > *dev, u32 stringset, u8 *data) > > /* Currently only support WOL through Magic packet. */ static void > > stmmac_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol) { > > + struct ethtool_wolinfo wol_phy = { .cmd = ETHTOOL_GWOL }; > > struct stmmac_priv *priv = netdev_priv(dev); > > > > - if (!priv->plat->pmt) > > - return phylink_ethtool_get_wol(priv->phylink, wol); > > - > > mutex_lock(&priv->lock); > > - if (device_can_wakeup(priv->device)) { > > Why remove the device_can_wakeup() check? The original code logic is setting wakeup capability when check it supports PMT in stmmac_hw_init () at probe stage. After this patch, we set wakeup capability when configure WoL in stmmac_set_wol(), so we change to check " priv->plat->pmt ", rather than " device_can_wakeup()". > > + if (priv->plat->pmt) { > > wol->supported = WAKE_MAGIC | WAKE_UCAST; > > if (priv->hw_cap_support && !priv->dma_cap.pmt_magic_frame) > > wol->supported &= ~WAKE_MAGIC; > > - wol->wolopts = priv->wolopts; > > } > > + > > + phylink_ethtool_get_wol(priv->phylink, &wol_phy); > > + > > + /* Combine WoL capabilities both PHY and MAC */ > > + wol->supported |= wol_phy.supported; > > + wol->wolopts = priv->wolopts; > > + > > mutex_unlock(&priv->lock); > > } > > > > static int stmmac_set_wol(struct net_device *dev, struct > > ethtool_wolinfo *wol) { > > + u32 support = WAKE_MAGIC | WAKE_UCAST | WAKE_MAGICSECURE | > > +WAKE_BCAST; > > Why this list? Please see comments from Andrew: https://lore.kernel.org/netdev/YIgBJQi1H+f2VGWf@lunn.ch/T/#m00f11a84c1c43b3b4047dffcdfce57d534565a96 "What PHYs do implement is WAKE_MAGIC, WAKE_MAGICSEC, WAKE_UCAST, and WAKE_BCAST. So there is a clear overlap with what the MAC can do." So this list is cover all the WoL sources both PHY and STMMAC. > > + struct ethtool_wolinfo wol_phy = { .cmd = ETHTOOL_GWOL }; > > struct stmmac_priv *priv = netdev_priv(dev); > > - u32 support = WAKE_MAGIC | WAKE_UCAST; > > This list was the list of what the MAC supported, right? Right. > > + int ret; > > > > - if (!device_can_wakeup(priv->device)) > > Why remove this check? Explain above. > > + if (wol->wolopts & ~support) > > return -EOPNOTSUPP; > > > > - if (!priv->plat->pmt) { > > - int ret = phylink_ethtool_set_wol(priv->phylink, wol); > > - > > - if (!ret) > > - device_set_wakeup_enable(priv->device, !!wol->wolopts); > > - return ret; > > + /* First check if can WoL from PHY */ > > + phylink_ethtool_get_wol(priv->phylink, &wol_phy); > > + if (wol->wolopts & wol_phy.supported) { > > So if _any_ requests match PHY capabilities we'd use PHY? > I think the check should be: > > if ((wol->woltops & wol_phy.supported) == wol->woltops) > > That said I'm not 100% sure what the semantics for WoL are. Yes, you are right. Multiple wakeup sources can be enabled at the same time, from PHY or MAC, we give priority to PHY. As Andrew said: "If you are trying to save power, it is better if the PHY provides the WoL sources. If the PHY can provide all the required WoL sources, you can turn the MAC off and save more power. So give priority to the PHY." > > + wol->wolopts &= wol_phy.supported; > > + ret = phylink_ethtool_set_wol(priv->phylink, wol); > > + > > + if (!ret) { > > + pr_info("stmmac: phy wakeup enable\n"); > > + device_set_wakeup_capable(priv->device, 1); > > + device_set_wakeup_enable(priv->device, 1); > > + goto wolopts_update; > > + } else { > > + return ret; > > + } > > } > > > > /* By default almost all GMAC devices support the WoL via @@ -666,18 > > +680,21 @@ static int stmmac_set_wol(struct net_device *dev, struct > ethtool_wolinfo *wol) > > if ((priv->hw_cap_support) && (!priv->dma_cap.pmt_magic_frame)) > > wol->wolopts &= ~WAKE_MAGIC; > > > > - if (wol->wolopts & ~support) > > - return -EINVAL; > > Now you seem to not validate against MAC capabilities anywhere. Yes, since we have combined WoL capabilities both PHY and MAC in stmmac_get_wol(). Best Regards, Joakim Zhang > > - if (wol->wolopts) { > > - pr_info("stmmac: wakeup enable\n"); > > + if (priv->plat->pmt && wol->wolopts) { > > + pr_info("stmmac: mac wakeup enable\n"); > > + device_set_wakeup_capable(priv->device, 1); > > device_set_wakeup_enable(priv->device, 1); > > enable_irq_wake(priv->wol_irq); > > - } else { > > + goto wolopts_update; > > + } > > + > > + if (!wol->wolopts) { > > + device_set_wakeup_capable(priv->device, 0); > > device_set_wakeup_enable(priv->device, 0); > > disable_irq_wake(priv->wol_irq); > > } > > > > +wolopts_update: > > mutex_lock(&priv->lock); > > priv->wolopts = wol->wolopts; > > mutex_unlock(&priv->lock);
On Fri, 7 May 2021 10:59:12 +0000 Joakim Zhang wrote: > > On Thu, 6 May 2021 13:06:58 +0800 Joakim Zhang wrote: > > > Both get and set WoL will check device_can_wakeup(), if MAC supports > > > PMT, it will set device wakeup capability. After commit 1d8e5b0f3f2c ("net: > > > stmmac: Support WOL with phy"), device wakeup capability will be > > > overwrite in stmmac_init_phy() according to phy's Wol feature. If phy > > > doesn't support WoL, then MAC will lose wakeup capability. > > > > Let's take a step back. Can we get a minimal fix for losing the config in > > stmmac_init_phy(), and then extend the support for WoL for devices which do > > support wake up themselves? > > Sure, please review the V1, I think this is a minimal fix, then we > can extend this as a new feature. > https://www.spinics.net/lists/netdev/msg733531.html Something like that, yes (you can pull the get_wol call into the same if block). Andrew, would that be acceptable to you? As limited as the either/or approach is it should not break any existing users, and the fix needs to go to longterm 5.10. We could make the improvements in net-next? > > > static int stmmac_set_wol(struct net_device *dev, struct > > > ethtool_wolinfo *wol) { > > > + u32 support = WAKE_MAGIC | WAKE_UCAST | WAKE_MAGICSECURE | > > > +WAKE_BCAST; > > > > Why this list? > > Please see comments from Andrew: https://lore.kernel.org/netdev/YIgBJQi1H+f2VGWf@lunn.ch/T/#m00f11a84c1c43b3b4047dffcdfce57d534565a96 > "What PHYs do implement is WAKE_MAGIC, WAKE_MAGICSEC, WAKE_UCAST, and WAKE_BCAST. So there is a clear overlap with what the MAC can do." > > So this list is cover all the WoL sources both PHY and STMMAC. I don't think that's what Andrew meant, although again, I'm not 100% sure of expected WoL semantics.
On Fri, 7 May 2021 15:46:24 -0700 Jakub Kicinski wrote: > > > On Fri, 7 May 2021 10:59:12 +0000 Joakim Zhang wrote: > > > On Thu, 6 May 2021 13:06:58 +0800 Joakim Zhang wrote: > > > > Both get and set WoL will check device_can_wakeup(), if MAC supports > > > > PMT, it will set device wakeup capability. After commit 1d8e5b0f3f2c ("net: > > > > stmmac: Support WOL with phy"), device wakeup capability will be > > > > overwrite in stmmac_init_phy() according to phy's Wol feature. If phy > > > > doesn't support WoL, then MAC will lose wakeup capability. > > > > > > Let's take a step back. Can we get a minimal fix for losing the config in > > > stmmac_init_phy(), and then extend the support for WoL for devices which do > > > support wake up themselves? > > > > Sure, please review the V1, I think this is a minimal fix, then we > > can extend this as a new feature. I lost the v1 patch in my email inbox, but I found it by searching it in web and reviewed v1. If going this way(a minimal fix then make improvements in net-next) feel free to add below reviewed-by tag for the v1 patch: Reviewed-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > > Something like that, yes (you can pull the get_wol call into the same > if block). > > Andrew, would that be acceptable to you? As limited as the either/or > approach is it should not break any existing users, and the fix needs > to go to longterm 5.10. We could make the improvements in net-next? >
On Fri, May 07, 2021 at 03:46:24PM -0700, Jakub Kicinski wrote: > On Fri, 7 May 2021 10:59:12 +0000 Joakim Zhang wrote: > > > On Thu, 6 May 2021 13:06:58 +0800 Joakim Zhang wrote: > > > > Both get and set WoL will check device_can_wakeup(), if MAC supports > > > > PMT, it will set device wakeup capability. After commit 1d8e5b0f3f2c ("net: > > > > stmmac: Support WOL with phy"), device wakeup capability will be > > > > overwrite in stmmac_init_phy() according to phy's Wol feature. If phy > > > > doesn't support WoL, then MAC will lose wakeup capability. > > > > > > Let's take a step back. Can we get a minimal fix for losing the config in > > > stmmac_init_phy(), and then extend the support for WoL for devices which do > > > support wake up themselves? > > > > Sure, please review the V1, I think this is a minimal fix, then we > > can extend this as a new feature. > > https://www.spinics.net/lists/netdev/msg733531.html > > Something like that, yes (you can pull the get_wol call into the same > if block). > > Andrew, would that be acceptable to you? A minimal fix for stable is good. Andrew
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c index c5642985ef95..6d09908dec1f 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c @@ -629,35 +629,49 @@ static void stmmac_get_strings(struct net_device *dev, u32 stringset, u8 *data) /* Currently only support WOL through Magic packet. */ static void stmmac_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol) { + struct ethtool_wolinfo wol_phy = { .cmd = ETHTOOL_GWOL }; struct stmmac_priv *priv = netdev_priv(dev); - if (!priv->plat->pmt) - return phylink_ethtool_get_wol(priv->phylink, wol); - mutex_lock(&priv->lock); - if (device_can_wakeup(priv->device)) { + if (priv->plat->pmt) { wol->supported = WAKE_MAGIC | WAKE_UCAST; if (priv->hw_cap_support && !priv->dma_cap.pmt_magic_frame) wol->supported &= ~WAKE_MAGIC; - wol->wolopts = priv->wolopts; } + + phylink_ethtool_get_wol(priv->phylink, &wol_phy); + + /* Combine WoL capabilities both PHY and MAC */ + wol->supported |= wol_phy.supported; + wol->wolopts = priv->wolopts; + mutex_unlock(&priv->lock); } static int stmmac_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) { + u32 support = WAKE_MAGIC | WAKE_UCAST | WAKE_MAGICSECURE | WAKE_BCAST; + struct ethtool_wolinfo wol_phy = { .cmd = ETHTOOL_GWOL }; struct stmmac_priv *priv = netdev_priv(dev); - u32 support = WAKE_MAGIC | WAKE_UCAST; + int ret; - if (!device_can_wakeup(priv->device)) + if (wol->wolopts & ~support) return -EOPNOTSUPP; - if (!priv->plat->pmt) { - int ret = phylink_ethtool_set_wol(priv->phylink, wol); - - if (!ret) - device_set_wakeup_enable(priv->device, !!wol->wolopts); - return ret; + /* First check if can WoL from PHY */ + phylink_ethtool_get_wol(priv->phylink, &wol_phy); + if (wol->wolopts & wol_phy.supported) { + wol->wolopts &= wol_phy.supported; + ret = phylink_ethtool_set_wol(priv->phylink, wol); + + if (!ret) { + pr_info("stmmac: phy wakeup enable\n"); + device_set_wakeup_capable(priv->device, 1); + device_set_wakeup_enable(priv->device, 1); + goto wolopts_update; + } else { + return ret; + } } /* By default almost all GMAC devices support the WoL via @@ -666,18 +680,21 @@ static int stmmac_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) if ((priv->hw_cap_support) && (!priv->dma_cap.pmt_magic_frame)) wol->wolopts &= ~WAKE_MAGIC; - if (wol->wolopts & ~support) - return -EINVAL; - - if (wol->wolopts) { - pr_info("stmmac: wakeup enable\n"); + if (priv->plat->pmt && wol->wolopts) { + pr_info("stmmac: mac wakeup enable\n"); + device_set_wakeup_capable(priv->device, 1); device_set_wakeup_enable(priv->device, 1); enable_irq_wake(priv->wol_irq); - } else { + goto wolopts_update; + } + + if (!wol->wolopts) { + device_set_wakeup_capable(priv->device, 0); device_set_wakeup_enable(priv->device, 0); disable_irq_wake(priv->wol_irq); } +wolopts_update: mutex_lock(&priv->lock); priv->wolopts = wol->wolopts; mutex_unlock(&priv->lock); diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index c6f24abf6432..d62d8c28463d 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -1076,7 +1076,6 @@ static void stmmac_check_pcs_mode(struct stmmac_priv *priv) */ static int stmmac_init_phy(struct net_device *dev) { - struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL }; struct stmmac_priv *priv = netdev_priv(dev); struct device_node *node; int ret; @@ -1102,9 +1101,6 @@ static int stmmac_init_phy(struct net_device *dev) ret = phylink_connect_phy(priv->phylink, phydev); } - phylink_ethtool_get_wol(priv->phylink, &wol); - device_set_wakeup_capable(priv->device, !!wol.supported); - return ret; } @@ -4787,10 +4783,8 @@ static int stmmac_hw_init(struct stmmac_priv *priv) if (priv->plat->tx_coe) dev_info(priv->device, "TX Checksum insertion supported\n"); - if (priv->plat->pmt) { + if (priv->plat->pmt) dev_info(priv->device, "Wake-Up On Lan supported\n"); - device_set_wakeup_capable(priv->device, 1); - } if (priv->dma_cap.tsoen) dev_info(priv->device, "TSO supported\n");
Both get and set WoL will check device_can_wakeup(), if MAC supports PMT, it will set device wakeup capability. After commit 1d8e5b0f3f2c ("net: stmmac: Support WOL with phy"), device wakeup capability will be overwrite in stmmac_init_phy() according to phy's Wol feature. If phy doesn't support WoL, then MAC will lose wakeup capability. This patch combines WoL capabilities both MAC and PHY from stmmac_get_wol(), set wakeup capability and give WoL priority to the PHY in stmmac_set_wol() when enable WoL. What PHYs do implement is WAKE_MAGIC, WAKE_UCAST, WAKE_MAGICSECURE and WAKE_BCAST. Fixes: commit 1d8e5b0f3f2c ("net: stmmac: Support WOL with phy") Suggested-by: Andrew Lunn <andrew@lunn.ch> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> --- ChangeLogs: V1->V2: * combine WoL capabilities both MAC and PHY. V2->V3: * give WoL priority to the PHY. V3->V4: * improve patch subject, unwork->not working * Reverse christmas tree for variable definition * return -EOPNOTSUPP not -EINVAL when pass wolopts * enable wol sources the PHY actually supports, and let the MAC implement the rest. --- .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 55 ++++++++++++------- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 8 +-- 2 files changed, 37 insertions(+), 26 deletions(-)