Message ID | 1432157730-26865-2-git-send-email-aparames@broadcom.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
From: Arun Parameswaran <aparames@broadcom.com> Date: Wed, 20 May 2015 14:35:30 -0700 > When trying to configure the settings for PHY1, using commands > like 'ethtool -s eth0 phyad 1 speed 100', the 'ethtool' seems to > modify other settings apart from the speed of the PHY1, in the > above case. > > The ethtool seems to query the settings for PHY0, and use this > as the base to apply the new settings to the PHY1. This is > causing the other settings of the PHY 1 to be wrongly > configured. > > The issue is caused by the '_ethtool_get_settings()' API, which > gets called because of the 'ETHTOOL_GSET' command, is clearing > the 'cmd' pointer (of type 'struct ethtool_cmd') by calling > memset. This clears all the parameters (if any) passed for the > 'ETHTOOL_GSET' cmd. So the driver's callback is always invoked > with 'cmd->phy_address' as '0'. > > The '_ethtool_get_settings()' is called from other files in the > 'net/core'. So the fix is applied to the 'ethtool_get_settings()' > which is only called in the context of the 'ethtool'. > > Signed-off-by: Arun Parameswaran <aparames@broadcom.com> > Reviewed-by: Ray Jui <rjui@broadcom.com> > Reviewed-by: Scott Branden <sbranden@broadcom.com> Applied and queued up for -stable, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2015-05-22 at 16:15 -0400, David Miller wrote: > From: Arun Parameswaran <aparames@broadcom.com> > Date: Wed, 20 May 2015 14:35:30 -0700 > > > When trying to configure the settings for PHY1, using commands > > like 'ethtool -s eth0 phyad 1 speed 100', the 'ethtool' seems to > > modify other settings apart from the speed of the PHY1, in the > > above case. > > > > The ethtool seems to query the settings for PHY0, and use this > > as the base to apply the new settings to the PHY1. This is > > causing the other settings of the PHY 1 to be wrongly > > configured. > > > > The issue is caused by the '_ethtool_get_settings()' API, which > > gets called because of the 'ETHTOOL_GSET' command, is clearing > > the 'cmd' pointer (of type 'struct ethtool_cmd') by calling > > memset. This clears all the parameters (if any) passed for the > > 'ETHTOOL_GSET' cmd. So the driver's callback is always invoked > > with 'cmd->phy_address' as '0'. > > > > The '_ethtool_get_settings()' is called from other files in the > > 'net/core'. So the fix is applied to the 'ethtool_get_settings()' > > which is only called in the context of the 'ethtool'. > > > > Signed-off-by: Arun Parameswaran <aparames@broadcom.com> > > Reviewed-by: Ray Jui <rjui@broadcom.com> > > Reviewed-by: Scott Branden <sbranden@broadcom.com> > > Applied and queued up for -stable, thanks. Please revert this. This is an incompatible API change, not a bug fix. The established semantics are that 'phyad' is filled in by the driver; it is not a parameter to the ETHTOOL_GSET command. Ben.
From: Ben Hutchings <ben@decadent.org.uk> Date: Sun, 31 May 2015 20:54:06 +0100 > On Fri, 2015-05-22 at 16:15 -0400, David Miller wrote: >> From: Arun Parameswaran <aparames@broadcom.com> >> Date: Wed, 20 May 2015 14:35:30 -0700 >> >> > When trying to configure the settings for PHY1, using commands >> > like 'ethtool -s eth0 phyad 1 speed 100', the 'ethtool' seems to >> > modify other settings apart from the speed of the PHY1, in the >> > above case. >> > >> > The ethtool seems to query the settings for PHY0, and use this >> > as the base to apply the new settings to the PHY1. This is >> > causing the other settings of the PHY 1 to be wrongly >> > configured. >> > >> > The issue is caused by the '_ethtool_get_settings()' API, which >> > gets called because of the 'ETHTOOL_GSET' command, is clearing >> > the 'cmd' pointer (of type 'struct ethtool_cmd') by calling >> > memset. This clears all the parameters (if any) passed for the >> > 'ETHTOOL_GSET' cmd. So the driver's callback is always invoked >> > with 'cmd->phy_address' as '0'. >> > >> > The '_ethtool_get_settings()' is called from other files in the >> > 'net/core'. So the fix is applied to the 'ethtool_get_settings()' >> > which is only called in the context of the 'ethtool'. >> > >> > Signed-off-by: Arun Parameswaran <aparames@broadcom.com> >> > Reviewed-by: Ray Jui <rjui@broadcom.com> >> > Reviewed-by: Scott Branden <sbranden@broadcom.com> >> >> Applied and queued up for -stable, thanks. > > Please revert this. This is an incompatible API change, not a bug fix. > The established semantics are that 'phyad' is filled in by the driver; > it is not a parameter to the ETHTOOL_GSET command. But then how in the world can the user specify specific PHY ADs for a device that will respond to more than one? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2015-05-31 at 17:19 -0700, David Miller wrote: > From: Ben Hutchings <ben@decadent.org.uk> > Date: Sun, 31 May 2015 20:54:06 +0100 > > > On Fri, 2015-05-22 at 16:15 -0400, David Miller wrote: > >> From: Arun Parameswaran <aparames@broadcom.com> > >> Date: Wed, 20 May 2015 14:35:30 -0700 > >> > >> > When trying to configure the settings for PHY1, using commands > >> > like 'ethtool -s eth0 phyad 1 speed 100', the 'ethtool' seems to > >> > modify other settings apart from the speed of the PHY1, in the > >> > above case. > >> > > >> > The ethtool seems to query the settings for PHY0, and use this > >> > as the base to apply the new settings to the PHY1. This is > >> > causing the other settings of the PHY 1 to be wrongly > >> > configured. > >> > > >> > The issue is caused by the '_ethtool_get_settings()' API, which > >> > gets called because of the 'ETHTOOL_GSET' command, is clearing > >> > the 'cmd' pointer (of type 'struct ethtool_cmd') by calling > >> > memset. This clears all the parameters (if any) passed for the > >> > 'ETHTOOL_GSET' cmd. So the driver's callback is always invoked > >> > with 'cmd->phy_address' as '0'. > >> > > >> > The '_ethtool_get_settings()' is called from other files in the > >> > 'net/core'. So the fix is applied to the 'ethtool_get_settings()' > >> > which is only called in the context of the 'ethtool'. > >> > > >> > Signed-off-by: Arun Parameswaran <aparames@broadcom.com> > >> > Reviewed-by: Ray Jui <rjui@broadcom.com> > >> > Reviewed-by: Scott Branden <sbranden@broadcom.com> > >> > >> Applied and queued up for -stable, thanks. > > > > Please revert this. This is an incompatible API change, not a bug fix. > > The established semantics are that 'phyad' is filled in by the driver; > > it is not a parameter to the ETHTOOL_GSET command. > > But then how in the world can the user specify specific PHY ADs for > a device that will respond to more than one? ETHTOOL_SSET sets the current PHY address and ETHTOOL_GSET gets it. If multiple PHYs need to be configured for a single link then the driver should configure them all at the same time rather than making it the administrator's problem. What we can't support with the current API are: - multiple physical links behind a single net device (different configuration possible for each link) - multiple PHYs are needed for a single link, and the driver can't automatically decide which to use (multiple addresses to set) Ben.
On 15-06-01 11:05 AM, Ben Hutchings wrote: > On Sun, 2015-05-31 at 17:19 -0700, David Miller wrote: >> From: Ben Hutchings <ben@decadent.org.uk> >> Date: Sun, 31 May 2015 20:54:06 +0100 >> >>> On Fri, 2015-05-22 at 16:15 -0400, David Miller wrote: >>>> From: Arun Parameswaran <aparames@broadcom.com> >>>> Date: Wed, 20 May 2015 14:35:30 -0700 >>>> >>>>> When trying to configure the settings for PHY1, using commands >>>>> like 'ethtool -s eth0 phyad 1 speed 100', the 'ethtool' seems to >>>>> modify other settings apart from the speed of the PHY1, in the >>>>> above case. >>>>> >>>>> The ethtool seems to query the settings for PHY0, and use this >>>>> as the base to apply the new settings to the PHY1. This is >>>>> causing the other settings of the PHY 1 to be wrongly >>>>> configured. >>>>> >>>>> The issue is caused by the '_ethtool_get_settings()' API, which >>>>> gets called because of the 'ETHTOOL_GSET' command, is clearing >>>>> the 'cmd' pointer (of type 'struct ethtool_cmd') by calling >>>>> memset. This clears all the parameters (if any) passed for the >>>>> 'ETHTOOL_GSET' cmd. So the driver's callback is always invoked >>>>> with 'cmd->phy_address' as '0'. >>>>> >>>>> The '_ethtool_get_settings()' is called from other files in the >>>>> 'net/core'. So the fix is applied to the 'ethtool_get_settings()' >>>>> which is only called in the context of the 'ethtool'. >>>>> >>>>> Signed-off-by: Arun Parameswaran <aparames@broadcom.com> >>>>> Reviewed-by: Ray Jui <rjui@broadcom.com> >>>>> Reviewed-by: Scott Branden <sbranden@broadcom.com> >>>> >>>> Applied and queued up for -stable, thanks. >>> >>> Please revert this. This is an incompatible API change, not a bug fix. >>> The established semantics are that 'phyad' is filled in by the driver; >>> it is not a parameter to the ETHTOOL_GSET command. >> >> But then how in the world can the user specify specific PHY ADs for >> a device that will respond to more than one? > > ETHTOOL_SSET sets the current PHY address and ETHTOOL_GSET gets it. > > If multiple PHYs need to be configured for a single link then the driver > should configure them all at the same time rather than making it the > administrator's problem. > > What we can't support with the current API are: > - multiple physical links behind a single net device (different > configuration possible for each link) > - multiple PHYs are needed for a single link, and the driver can't > automatically decide which to use (multiple addresses to set) > > Ben. > The patch doesn't affect the current functionality except that the programs/'ethtool' using the interface needs to ensure the command structure integrity. Kernel should transport the data to and from the driver and let the programs/ethtool handle the data/command. It is better if the Kernel doesn't manipulate the command/data being requested/passed, in this case it is the hard-coding of PHY id (by the memset). This adds flexibility to the interface. This patch along with the change suggested in the 'ethtool' app (separate patch set sent for the app), provides the flexibility to be able to query/set a specific PHY irrespective of the driver design. In our SoC, we have one MAC connected to multiple PHYs with an internal switch in between. We have only one net device as we can have only one MAC address and this better reflects the hardware state. It would be nice for the 'ethtool' to be flexible to support querying specific PHY irrespective of the net implementation, but that is being discussed in the other thread. Thanks Arun -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Arun Parameswaran <aparames@broadcom.com> Date: Mon, 1 Jun 2015 14:41:43 -0700 > It would be nice for the 'ethtool' to be flexible to support querying > specific PHY irrespective of the net implementation, but that is being > discussed in the other thread. Please stop arguing about this, it isn't valid. Your device is a switch, and therefore needs to be represented properly with the proper number of net_device objects. Even more importantly, the ethtool API is established and you cannot change these semantics without potentially breaking lots of applications and libraries out there. Your change is reverted, and I will absolutely not entertain any attempt to again change the semantics of this ethtool operation. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15-06-01 02:46 PM, David Miller wrote: > From: Arun Parameswaran <aparames@broadcom.com> > Date: Mon, 1 Jun 2015 14:41:43 -0700 > >> It would be nice for the 'ethtool' to be flexible to support querying >> specific PHY irrespective of the net implementation, but that is being >> discussed in the other thread. > > Please stop arguing about this, it isn't valid. > > Your device is a switch, and therefore needs to be represented properly > with the proper number of net_device objects. > > Even more importantly, the ethtool API is established and you cannot > change these semantics without potentially breaking lots of applications > and libraries out there. > > Your change is reverted, and I will absolutely not entertain any > attempt to again change the semantics of this ethtool operation. > > Thanks. > I apologize if the patch broke any conventions, it was not my intend. I understand the implications on other programs that use the interface. Just so that I don’t make this mistake in the future and to understand this better, does this mean that the 'phyad' parameter specified in the 'ethtool' command line is ignored ? Thanks Arun -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2015-06-01 at 16:53 -0700, Arun Parameswaran wrote: > On 15-06-01 02:46 PM, David Miller wrote: > > From: Arun Parameswaran <aparames@broadcom.com> > > Date: Mon, 1 Jun 2015 14:41:43 -0700 > > > >> It would be nice for the 'ethtool' to be flexible to support querying > >> specific PHY irrespective of the net implementation, but that is being > >> discussed in the other thread. > > > > Please stop arguing about this, it isn't valid. > > > > Your device is a switch, and therefore needs to be represented properly > > with the proper number of net_device objects. > > > > Even more importantly, the ethtool API is established and you cannot > > change these semantics without potentially breaking lots of applications > > and libraries out there. > > > > Your change is reverted, and I will absolutely not entertain any > > attempt to again change the semantics of this ethtool operation. > > > > Thanks. > > > I apologize if the patch broke any conventions, it was not my intend. I > understand the implications on other programs that use the interface. > > Just so that I don’t make this mistake in the future and to understand this > better, does this mean that the 'phyad' parameter specified in the > 'ethtool' command line is ignored ? There are some very old Ethernet cards where the driver can't automatically detect which PHY address to use, or where the MAC can be attached to one of several different PHYs. This is what the 'phyad' parameter is used for. I think the only remaining driver that supports changing the PHY address like this is natsemi. Ben.
diff --git a/net/core/ethtool.c b/net/core/ethtool.c index 1d00b89..1347e11 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -359,7 +359,15 @@ static int ethtool_get_settings(struct net_device *dev, void __user *useraddr) int err; struct ethtool_cmd cmd; - err = __ethtool_get_settings(dev, &cmd); + if (!dev->ethtool_ops->get_settings) + return -EOPNOTSUPP; + + if (copy_from_user(&cmd, useraddr, sizeof(cmd))) + return -EFAULT; + + cmd.cmd = ETHTOOL_GSET; + + err = dev->ethtool_ops->get_settings(dev, &cmd); if (err < 0) return err;