| Message ID | 59bd00bf-7263-43d9-a438-c2930bfdb91c@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8306ee08c0ff1c78672bcc645544f26f4d652cca |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] tg3: copy only needed fields from userspace-provided EEE data | expand |
On Sun, Feb 18, 2024 at 03:49:55PM +0100, Heiner Kallweit wrote: > The current code overwrites fields in tp->eee with unchecked data from > edata, e.g. the bitmap with supported modes. ethtool properly returns > the received data from get_eee() call, but we have no guarantee that > other users of the ioctl set_eee() interface behave properly too. > Therefore copy only fields which are actually needed. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> This one needed some time for me to understand. I missed that when programming the PHY to advertise, it is hard coded what it actually advertises. So there is no need to copy the advertise linkmode from edata. I suspect this driver is broken in that it does not wait for the result of the auto-neg to enable/disable EEE in the hardware. But it could be hiding somewhere in the code and i also missed that. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On 18.02.2024 18:04, Andrew Lunn wrote: > On Sun, Feb 18, 2024 at 03:49:55PM +0100, Heiner Kallweit wrote: >> The current code overwrites fields in tp->eee with unchecked data from >> edata, e.g. the bitmap with supported modes. ethtool properly returns >> the received data from get_eee() call, but we have no guarantee that >> other users of the ioctl set_eee() interface behave properly too. >> Therefore copy only fields which are actually needed. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > This one needed some time for me to understand. I missed that when > programming the PHY to advertise, it is hard coded what it actually > advertises. So there is no need to copy the advertise linkmode from > edata. > Especially as we have the following a few lines earlier: if (!linkmode_equal(edata->advertised, tp->eee.advertised)) { netdev_warn(tp->dev, "Direct manipulation of EEE advertisement is not supported\n"); return -EINVAL; } > I suspect this driver is broken in that it does not wait for the > result of the auto-neg to enable/disable EEE in the hardware. But it > could be hiding somewhere in the code and i also missed that. > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > Andrew
Hello: This patch was applied to netdev/net-next.git (main) by David S. Miller <davem@davemloft.net>: On Sun, 18 Feb 2024 15:49:55 +0100 you wrote: > The current code overwrites fields in tp->eee with unchecked data from > edata, e.g. the bitmap with supported modes. ethtool properly returns > the received data from get_eee() call, but we have no guarantee that > other users of the ioctl set_eee() interface behave properly too. > Therefore copy only fields which are actually needed. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > [...] Here is the summary with links: - [net-next] tg3: copy only needed fields from userspace-provided EEE data https://git.kernel.org/netdev/net-next/c/8306ee08c0ff You are awesome, thank you!
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c index 51685cd29..7a07c5216 100644 --- a/drivers/net/ethernet/broadcom/tg3.c +++ b/drivers/net/ethernet/broadcom/tg3.c @@ -14200,7 +14200,9 @@ static int tg3_set_eee(struct net_device *dev, struct ethtool_keee *edata) return -EINVAL; } - tp->eee = *edata; + tp->eee.eee_enabled = edata->eee_enabled; + tp->eee.tx_lpi_enabled = edata->tx_lpi_enabled; + tp->eee.tx_lpi_timer = edata->tx_lpi_timer; tp->phy_flags |= TG3_PHYFLG_USER_CONFIGURED; tg3_warn_mgmt_link_flap(tp);
The current code overwrites fields in tp->eee with unchecked data from edata, e.g. the bitmap with supported modes. ethtool properly returns the received data from get_eee() call, but we have no guarantee that other users of the ioctl set_eee() interface behave properly too. Therefore copy only fields which are actually needed. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/ethernet/broadcom/tg3.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)