| Message ID | 20210120093713.4000363-3-danieller@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Support setting lanes via ethtool | expand |
On Wed, Jan 20, 2021 at 3:21 AM Danielle Ratson <danieller@nvidia.com> wrote: > + link_ksettings->link_mode = -1; > + err = dev->ethtool_ops->get_link_ksettings(dev, link_ksettings); > + if (err) > + return err; > + > + if (link_ksettings->link_mode != -1) { > + link_info = &link_mode_params[link_ksettings->link_mode]; > + link_ksettings->base.speed = link_info->speed; > + link_ksettings->lanes = link_info->lanes; > + link_ksettings->base.duplex = link_info->duplex; > + } Why isn't this also handled using a capability bit as is done for lanes? Is link_mode read-only? Should it / will it always be? If not, can drivers also derive the other fields if asked to set link_mode? That would be easy enough. Why don't we simply allow user space to set link mode directly too (in addition to being able to constrain lanes for autoneg or forced speeds)? Regards, Edwin Peer
> -----Original Message----- > From: Edwin Peer <edwin.peer@broadcom.com> > Sent: Thursday, January 21, 2021 1:39 AM > To: Danielle Ratson <danieller@nvidia.com> > Cc: netdev <netdev@vger.kernel.org>; David S . Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Jiri Pirko > <jiri@nvidia.com>; Andrew Lunn <andrew@lunn.ch>; f.fainelli@gmail.com; Michal Kubecek <mkubecek@suse.cz>; mlxsw > <mlxsw@nvidia.com>; Ido Schimmel <idosch@nvidia.com> > Subject: Re: [PATCH net-next v3 2/7] ethtool: Get link mode in use instead of speed and duplex parameters > > On Wed, Jan 20, 2021 at 3:21 AM Danielle Ratson <danieller@nvidia.com> wrote: > > > + link_ksettings->link_mode = -1; > > + err = dev->ethtool_ops->get_link_ksettings(dev, link_ksettings); > > + if (err) > > + return err; > > + > > + if (link_ksettings->link_mode != -1) { > > + link_info = &link_mode_params[link_ksettings->link_mode]; > > + link_ksettings->base.speed = link_info->speed; > > + link_ksettings->lanes = link_info->lanes; > > + link_ksettings->base.duplex = link_info->duplex; > > + } > > Why isn't this also handled using a capability bit as is done for > lanes? Is link_mode read-only? Should it / will it always be? If not, > can drivers also derive the other fields if asked to set link_mode? The link_mode param is only for deriving all the speed, lanes and duplex params in ethtool instead of deriving in driver and then passing each individual, as Michal asked. > That would be easy enough. Why don't we simply allow user space to set > link mode directly too (in addition to being able to constrain lanes > for autoneg or forced speeds)? It is already possible to do using 'advertise' parameter. > > Regards, > Edwin Peer
On Sun, Jan 24, 2021 at 12:36 AM Danielle Ratson <danieller@nvidia.com> wrote: > > Why isn't this also handled using a capability bit as is done for > > lanes? Is link_mode read-only? Should it / will it always be? If not, > > can drivers also derive the other fields if asked to set link_mode? > > The link_mode param is only for deriving all the speed, lanes and duplex params in ethtool instead of deriving in driver and then > passing each individual, as Michal asked. I understand the benefit of deriving the dependent fields in core code rather than in each driver, I just don't think this is necessarily mutually exclusive with being able to force a particular link mode at the driver API, making link_mode R/W (and even extend this interface to user space). For a driver that works internally in terms of the link_mode it's returning, this would be more natural. > > That would be easy enough. Why don't we simply allow user space to set > > link mode directly too (in addition to being able to constrain lanes > > for autoneg or forced speeds)? > > It is already possible to do using 'advertise' parameter. That's not the same thing. If it were, you wouldn't need the lanes parameter in the first place. Regards, Edwin Peer
> -----Original Message----- > From: Edwin Peer <edwin.peer@broadcom.com> > Sent: Monday, January 25, 2021 8:04 PM > To: Danielle Ratson <danieller@nvidia.com> > Cc: netdev <netdev@vger.kernel.org>; David S . Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Jiri Pirko > <jiri@nvidia.com>; Andrew Lunn <andrew@lunn.ch>; f.fainelli@gmail.com; Michal Kubecek <mkubecek@suse.cz>; mlxsw > <mlxsw@nvidia.com>; Ido Schimmel <idosch@nvidia.com> > Subject: Re: [PATCH net-next v3 2/7] ethtool: Get link mode in use instead of speed and duplex parameters > > On Sun, Jan 24, 2021 at 12:36 AM Danielle Ratson <danieller@nvidia.com> wrote: > > > > Why isn't this also handled using a capability bit as is done for > > > lanes? Is link_mode read-only? Should it / will it always be? If not, > > > can drivers also derive the other fields if asked to set link_mode? > > > > The link_mode param is only for deriving all the speed, lanes and duplex params in ethtool instead of deriving in driver and then > > passing each individual, as Michal asked. > > I understand the benefit of deriving the dependent fields in core code > rather than in each driver, I just don't think this is necessarily > mutually exclusive with being able to force a particular link mode at > the driver API, making link_mode R/W (and even extend this interface > to user space). For a driver that works internally in terms of the > link_mode it's returning, this would be more natural. I am not sure I fully understood you, but it seems like some expansion that can be done in the future if needed, and doesn't need to hold that patchset back. Thanks, Danielle > > > > That would be easy enough. Why don't we simply allow user space to set > > > link mode directly too (in addition to being able to constrain lanes > > > for autoneg or forced speeds)? > > > > It is already possible to do using 'advertise' parameter. > > That's not the same thing. If it were, you wouldn't need the lanes > parameter in the first place. > > Regards, > Edwin Peer
On Tue, Jan 26, 2021 at 9:09 AM Danielle Ratson <danieller@nvidia.com> wrote: > > I understand the benefit of deriving the dependent fields in core code > > rather than in each driver, I just don't think this is necessarily > > mutually exclusive with being able to force a particular link mode at > > the driver API, making link_mode R/W (and even extend this interface > > to user space). For a driver that works internally in terms of the > > link_mode it's returning, this would be more natural. > > I am not sure I fully understood you, but it seems like some expansion that can be > done in the future if needed, and doesn't need to hold that patchset back. For one thing, it's cleaner if the driver API is symmetric. The proposed solution sets attributes in terms of speeds and lanes, etc., but it gets them in terms of a compound link_info. But, this asymmetry aside, if link_mode may eventually become R/W at the driver API, as you suggest, then it is more appropriate to guard it with a capability bit, as has been done for lanes, rather than use the -1 special value to indicate that the driver did not set it. Regards, Edwin Peer
> -----Original Message----- > From: Edwin Peer <edwin.peer@broadcom.com> > Sent: Tuesday, January 26, 2021 7:14 PM > To: Danielle Ratson <danieller@nvidia.com> > Cc: netdev <netdev@vger.kernel.org>; David S . Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Jiri Pirko > <jiri@nvidia.com>; Andrew Lunn <andrew@lunn.ch>; f.fainelli@gmail.com; Michal Kubecek <mkubecek@suse.cz>; mlxsw > <mlxsw@nvidia.com>; Ido Schimmel <idosch@nvidia.com> > Subject: Re: [PATCH net-next v3 2/7] ethtool: Get link mode in use instead of speed and duplex parameters > > On Tue, Jan 26, 2021 at 9:09 AM Danielle Ratson <danieller@nvidia.com> wrote: > > > > I understand the benefit of deriving the dependent fields in core code > > > rather than in each driver, I just don't think this is necessarily > > > mutually exclusive with being able to force a particular link mode at > > > the driver API, making link_mode R/W (and even extend this interface > > > to user space). For a driver that works internally in terms of the > > > link_mode it's returning, this would be more natural. > > > > I am not sure I fully understood you, but it seems like some expansion that can be > > done in the future if needed, and doesn't need to hold that patchset back. > > For one thing, it's cleaner if the driver API is symmetric. The > proposed solution sets attributes in terms of speeds and lanes, etc., > but it gets them in terms of a compound link_info. But, this asymmetry > aside, if link_mode may eventually become R/W at the driver API, as > you suggest, then it is more appropriate to guard it with a capability > bit, as has been done for lanes, rather than use the -1 special value > to indicate that the driver did not set it. > > Regards, > Edwin Peer This patchset adds lanes parameter, not link_mode. The link_mode addition was added as a read-only parameter for the reasons we mentioned, and I am not sure that implementing the symmetric side is relevant for this patchset. Michal, do you think we will use the Write side of the link_mode parameter? And if so, do you think it is relevant for this specific patchset? Thanks, Danielle
On Wed, Jan 27, 2021 at 01:22:02PM +0000, Danielle Ratson wrote: > > -----Original Message----- > > From: Edwin Peer <edwin.peer@broadcom.com> > > Sent: Tuesday, January 26, 2021 7:14 PM > > To: Danielle Ratson <danieller@nvidia.com> > > Cc: netdev <netdev@vger.kernel.org>; David S . Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Jiri Pirko > > <jiri@nvidia.com>; Andrew Lunn <andrew@lunn.ch>; f.fainelli@gmail.com; Michal Kubecek <mkubecek@suse.cz>; mlxsw > > <mlxsw@nvidia.com>; Ido Schimmel <idosch@nvidia.com> > > Subject: Re: [PATCH net-next v3 2/7] ethtool: Get link mode in use instead of speed and duplex parameters > > > > For one thing, it's cleaner if the driver API is symmetric. The > > proposed solution sets attributes in terms of speeds and lanes, > > etc., but it gets them in terms of a compound link_info. But, this > > asymmetry aside, if link_mode may eventually become R/W at the > > driver API, as you suggest, then it is more appropriate to guard it > > with a capability bit, as has been done for lanes, rather than use > > the -1 special value to indicate that the driver did not set it. > > > > Regards, > > Edwin Peer > > This patchset adds lanes parameter, not link_mode. The link_mode > addition was added as a read-only parameter for the reasons we > mentioned, and I am not sure that implementing the symmetric side is > relevant for this patchset. > > Michal, do you think we will use the Write side of the link_mode > parameter? It makes sense, IMHO. Unless we also add "media" (or whatever name would be most appropriate) parameter, we cannot in general fully determine the link mode by speed, duplex and lanes. And using "advertise" to select a specific mode with disabled autonegotiation would be rather confusing, I believe. (At the moment, ethtool does not even support syntax like "advertise <mode>" but it will be easy to support "advertise <mode>... [--]" and I think we should extend the syntax to support it, regardless of what we choose.) So if we want to allow user to pick a specific link node by name or bit mask (or rather index?), I would prefer using a separate parameter. > And if so, do you think it is relevant for this specific > patchset? I don't see an obvious problem with leaving that part for later so I would say it's up to you. Michal
> -----Original Message----- > From: Michal Kubecek <mkubecek@suse.cz> > Sent: Thursday, January 28, 2021 10:27 PM > To: Danielle Ratson <danieller@nvidia.com> > Cc: Edwin Peer <edwin.peer@broadcom.com>; netdev <netdev@vger.kernel.org>; David S . Miller <davem@davemloft.net>; Jakub > Kicinski <kuba@kernel.org>; Jiri Pirko <jiri@nvidia.com>; Andrew Lunn <andrew@lunn.ch>; f.fainelli@gmail.com; mlxsw > <mlxsw@nvidia.com>; Ido Schimmel <idosch@nvidia.com> > Subject: Re: [PATCH net-next v3 2/7] ethtool: Get link mode in use instead of speed and duplex parameters > > On Wed, Jan 27, 2021 at 01:22:02PM +0000, Danielle Ratson wrote: > > > -----Original Message----- > > > From: Edwin Peer <edwin.peer@broadcom.com> > > > Sent: Tuesday, January 26, 2021 7:14 PM > > > To: Danielle Ratson <danieller@nvidia.com> > > > Cc: netdev <netdev@vger.kernel.org>; David S . Miller > > > <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Jiri Pirko > > > <jiri@nvidia.com>; Andrew Lunn <andrew@lunn.ch>; > > > f.fainelli@gmail.com; Michal Kubecek <mkubecek@suse.cz>; mlxsw > > > <mlxsw@nvidia.com>; Ido Schimmel <idosch@nvidia.com> > > > Subject: Re: [PATCH net-next v3 2/7] ethtool: Get link mode in use > > > instead of speed and duplex parameters > > > > > > For one thing, it's cleaner if the driver API is symmetric. The > > > proposed solution sets attributes in terms of speeds and lanes, > > > etc., but it gets them in terms of a compound link_info. But, this > > > asymmetry aside, if link_mode may eventually become R/W at the > > > driver API, as you suggest, then it is more appropriate to guard it > > > with a capability bit, as has been done for lanes, rather than use > > > the -1 special value to indicate that the driver did not set it. > > > > > > Regards, > > > Edwin Peer > > > > This patchset adds lanes parameter, not link_mode. The link_mode > > addition was added as a read-only parameter for the reasons we > > mentioned, and I am not sure that implementing the symmetric side is > > relevant for this patchset. > > > > Michal, do you think we will use the Write side of the link_mode > > parameter? > > It makes sense, IMHO. Unless we also add "media" (or whatever name would be most appropriate) parameter, we cannot in general > fully determine the link mode by speed, duplex and lanes. And using "advertise" to select a specific mode with disabled > autonegotiation would be rather confusing, I believe. (At the moment, ethtool does not even support syntax like "advertise <mode>" > but it will be easy to support "advertise <mode>... [--]" and I think we should extend the syntax to support it, regardless of what we > choose.) So if we want to allow user to pick a specific link node by name or bit mask (or rather index?), I would prefer using a separate > parameter. > > > And if so, do you think it is relevant for this specific > > patchset? > > I don't see an obvious problem with leaving that part for later so I would say it's up to you. > > Michal Thanks Michal. Edwin, adding the a new parameter requires a new patchset in my opinion. Implementing the symmetrical side of the link_mode get, however can be a part of this set. But, the problem with that would be that, as Michal said, speed lanes and duplex can't provide us a single link_mode because of the media. And since link_mode is one bit parameter, passing it to the driver while randomly choosing the media, may cause either information loss, or even fail to sync on a link mode if the chosen media is specifically not supported in the lower levels. So, in my opinion it is related to adding the new parameter we discussed, and should be done in a separate set. Thanks, Danielle
On Sun, Jan 31, 2021 at 7:33 AM Danielle Ratson <danieller@nvidia.com> wrote: > Edwin, adding the a new parameter requires a new patchset in my opinion. > Implementing the symmetrical side of the link_mode get, however can be a > part of this set. But, the problem with that would be that, as Michal said, > speed lanes and duplex can't provide us a single link_mode because of the > media. And since link_mode is one bit parameter, passing it to the driver > while randomly choosing the media, may cause either information loss, or > even fail to sync on a link mode if the chosen media is specifically not > supported in the lower levels. So, in my opinion it is related to adding the > new parameter we discussed, and should be done in a separate set. Media is a little special in the sense that it physically depends on what's plugged in. In that sense, media is truly read only. Setting it won't change what's plugged in, so not having a separate knob for it is probably okay, as it can be inferred from the active link mode on the query side. I don't see why the driver can't error if asked to set a link mode having an incompatible media? The link modes corresponding to media that's not plugged in wouldn't be listed in the supported set, so there's no reason the user should expect to be able to set those. There's no ambiguity or information loss if you refuse to set modes that don't have the matching media attached. Regards, Edwin Peer
> -----Original Message----- > From: Edwin Peer <edwin.peer@broadcom.com> > Sent: Sunday, January 31, 2021 7:39 PM > To: Danielle Ratson <danieller@nvidia.com> > Cc: Michal Kubecek <mkubecek@suse.cz>; netdev <netdev@vger.kernel.org>; David S . Miller <davem@davemloft.net>; Jakub > Kicinski <kuba@kernel.org>; Jiri Pirko <jiri@nvidia.com>; Andrew Lunn <andrew@lunn.ch>; f.fainelli@gmail.com; mlxsw > <mlxsw@nvidia.com>; Ido Schimmel <idosch@nvidia.com> > Subject: Re: [PATCH net-next v3 2/7] ethtool: Get link mode in use instead of speed and duplex parameters > > On Sun, Jan 31, 2021 at 7:33 AM Danielle Ratson <danieller@nvidia.com> wrote: > > > Edwin, adding the a new parameter requires a new patchset in my opinion. > > Implementing the symmetrical side of the link_mode get, however can be a > > part of this set. But, the problem with that would be that, as Michal said, > > speed lanes and duplex can't provide us a single link_mode because of the > > media. And since link_mode is one bit parameter, passing it to the driver > > while randomly choosing the media, may cause either information loss, or > > even fail to sync on a link mode if the chosen media is specifically not > > supported in the lower levels. So, in my opinion it is related to adding the > > new parameter we discussed, and should be done in a separate set. > > Media is a little special in the sense that it physically depends on > what's plugged in. In that sense, media is truly read only. Setting it > won't change what's plugged in, so not having a separate knob for it > is probably okay, as it can be inferred from the active link mode on > the query side. > > I don't see why the driver can't error if asked to set a link mode > having an incompatible media? The link modes corresponding to media > that's not plugged in wouldn't be listed in the supported set, so > there's no reason the user should expect to be able to set those. > There's no ambiguity or information loss if you refuse to set modes > that don't have the matching media attached. > > Regards, > Edwin Peer Ok, ill send another version with the symmetrical side. Ethtool will try to compose a supported link_mode from the parameters from user space and will choose randomly between the supported ones. Sounds ok? Thanks, Danielle
On Mon, Feb 1, 2021 at 5:49 AM Danielle Ratson <danieller@nvidia.com> wrote: > Ok, ill send another version with the symmetrical side. Ethtool will try to compose > a supported link_mode from the parameters from user space and will choose > randomly between the supported ones. Sounds ok? I think it should be deterministic. It should be possible to select the appropriate mode either based on the current media type or the current link mode (which implies a media type). Alternatively, if the user space request only specifies a subset, such as speed, fall back to the existing behaviour and don't supply the request to the driver in the form of a compound link mode in those cases (perhaps indicating this by not setting the capability bit). The former approach has the potential to tidy up drivers if we decide that drivers providing the capability can ignore the other fields and rely solely on link mode, the latter is no worse than what we have today. Regards, Edwin Peer
On Mon, 1 Feb 2021 10:14:35 -0800 Edwin Peer wrote: > On Mon, Feb 1, 2021 at 5:49 AM Danielle Ratson <danieller@nvidia.com> wrote: > > Ok, ill send another version with the symmetrical side. Ethtool > > will try to compose a supported link_mode from the parameters from > > user space and will choose randomly between the supported ones. > > Sounds ok? > > I think it should be deterministic. It should be possible to select > the appropriate mode either based on the current media type or the > current link mode (which implies a media type). Alternatively, if the > user space request only specifies a subset, such as speed, fall back > to the existing behaviour and don't supply the request to the driver > in the form of a compound link mode in those cases (perhaps indicating > this by not setting the capability bit). The former approach has the > potential to tidy up drivers if we decide that drivers providing the > capability can ignore the other fields and rely solely on link mode, > the latter is no worse than what we have today. The media part is beginning to sound concerning. Every time we under-specify an interface we end up with #vendors different interpretations. And since HW is programmed by FW in most high speed devices we can't even review the right thing is done. At least it's clear what setting a number of lanes means.
On Mon, Feb 1, 2021 at 12:29 PM Jakub Kicinski <kuba@kernel.org> wrote: > > I think it should be deterministic. It should be possible to select > > the appropriate mode either based on the current media type or the > > current link mode (which implies a media type). Alternatively, if the > > user space request only specifies a subset, such as speed, fall back > > to the existing behaviour and don't supply the request to the driver > > in the form of a compound link mode in those cases (perhaps indicating > > this by not setting the capability bit). The former approach has the > > potential to tidy up drivers if we decide that drivers providing the > > capability can ignore the other fields and rely solely on link mode, > > the latter is no worse than what we have today. > > The media part is beginning to sound concerning. Every time we > under-specify an interface we end up with #vendors different > interpretations. And since HW is programmed by FW in most high > speed devices we can't even review the right thing is done. Each link mode implies a very specific media type, the kernel can reject illegal combinations based on the supported bitmask before calling upon the driver to select it. Regards, Edwin Peer
On Mon, 1 Feb 2021 13:05:10 -0800 Edwin Peer wrote: > On Mon, Feb 1, 2021 at 12:29 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > I think it should be deterministic. It should be possible to select > > > the appropriate mode either based on the current media type or the > > > current link mode (which implies a media type). Alternatively, if the > > > user space request only specifies a subset, such as speed, fall back > > > to the existing behaviour and don't supply the request to the driver > > > in the form of a compound link mode in those cases (perhaps indicating > > > this by not setting the capability bit). The former approach has the > > > potential to tidy up drivers if we decide that drivers providing the > > > capability can ignore the other fields and rely solely on link mode, > > > the latter is no worse than what we have today. > > > > The media part is beginning to sound concerning. Every time we > > under-specify an interface we end up with #vendors different > > interpretations. And since HW is programmed by FW in most high > > speed devices we can't even review the right thing is done. > > Each link mode implies a very specific media type, the kernel can > reject illegal combinations based on the supported bitmask before > calling upon the driver to select it. Are you talking about validation against a driver-supplied list of HW-supported modes, or SFP-supported modes for a currently plugged in module? If I'm reading prior responses right it is the former. The concern is around "what happens if user selected nnG-SR4 but user plugged in nnG-CR4". The MAC/PHY/serdes settings will be identical.
On Mon, Feb 1, 2021 at 1:41 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > The media part is beginning to sound concerning. Every time we > > > under-specify an interface we end up with #vendors different > > > interpretations. And since HW is programmed by FW in most high > > > speed devices we can't even review the right thing is done. > > > > Each link mode implies a very specific media type, the kernel can > > reject illegal combinations based on the supported bitmask before > > calling upon the driver to select it. > > Are you talking about validation against a driver-supplied list of > HW-supported modes, or SFP-supported modes for a currently plugged > in module? Should they not be the same thing? > The concern is around "what happens if user selected nnG-SR4 but user > plugged in nnG-CR4". The MAC/PHY/serdes settings will be identical. Yes, there would be multiple link modes that map to the same speed and lane combination, but that doesn't mean you need to accept them if the media doesn't match what's plugged in also. In the above scenario, the supported mask should not list SR because CR is physically plugged in. That way, the user knows what options are legal and the kernel knows what it can reject. Regards, Edwin Peer
On Mon, 1 Feb 2021 13:59:45 -0800 Edwin Peer wrote: > On Mon, Feb 1, 2021 at 1:41 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > Each link mode implies a very specific media type, the kernel can > > > reject illegal combinations based on the supported bitmask before > > > calling upon the driver to select it. > > > > Are you talking about validation against a driver-supplied list of > > HW-supported modes, or SFP-supported modes for a currently plugged > > in module? > > Should they not be the same thing? Okay. Does "supported modes" in ethtool for bnxt get ANDed with the supported modes of the plugged in module? What happens when no module is plugged in? List all? I've surveyed this behavior a few years back and three vendors I tested all had different interpretation on what to list in supported modes :/ > > The concern is around "what happens if user selected nnG-SR4 but user > > plugged in nnG-CR4". The MAC/PHY/serdes settings will be identical. > > Yes, there would be multiple link modes that map to the same speed and > lane combination, but that doesn't mean you need to accept them if the > media doesn't match what's plugged in also. In the above scenario, the > supported mask should not list SR because CR is physically plugged in. > That way, the user knows what options are legal and the kernel knows > what it can reject. If the modes depend on what's plugged in - what happens if cable gets removed? We (you, Danielle, I) can agree what we think is best, but history teaches us that doesn't matter in long run. We had a similar conversation when it comes to FEC. There simply is no way for upstream developers to review the behavior is correct.
On Mon, Feb 1, 2021 at 2:20 PM Jakub Kicinski <kuba@kernel.org> wrote: > > Should they not be the same thing? > > Okay. Does "supported modes" in ethtool for bnxt get ANDed with the > supported modes of the plugged in module? > > What happens when no module is plugged in? List all? > > I've surveyed this behavior a few years back and three vendors I tested > all had different interpretation on what to list in supported modes :/ Fair enough, I can't argue there. ;) > > Yes, there would be multiple link modes that map to the same speed and > > lane combination, but that doesn't mean you need to accept them if the > > media doesn't match what's plugged in also. In the above scenario, the > > supported mask should not list SR because CR is physically plugged in. > > That way, the user knows what options are legal and the kernel knows > > what it can reject. > > If the modes depend on what's plugged in - what happens if cable gets > removed? We (you, Danielle, I) can agree what we think is best, but > history teaches us that doesn't matter in long run. We had a similar > conversation when it comes to FEC. There simply is no way for upstream > developers to review the behavior is correct. Given that supported is only defined in the context of autoneg today, once could still specify. But again, you raise a fair concern. The asymmetry in interface is still ugly though, you get to decide which ugly is worse. :P Regards, Edwin Peer
On Mon, 1 Feb 2021 16:14:05 -0800 Edwin Peer wrote: > > > Yes, there would be multiple link modes that map to the same speed and > > > lane combination, but that doesn't mean you need to accept them if the > > > media doesn't match what's plugged in also. In the above scenario, the > > > supported mask should not list SR because CR is physically plugged in. > > > That way, the user knows what options are legal and the kernel knows > > > what it can reject. > > > > If the modes depend on what's plugged in - what happens if cable gets > > removed? We (you, Danielle, I) can agree what we think is best, but > > history teaches us that doesn't matter in long run. We had a similar > > conversation when it comes to FEC. There simply is no way for upstream > > developers to review the behavior is correct. > > Given that supported is only defined in the context of autoneg today, > once could still specify. But again, you raise a fair concern. > > The asymmetry in interface is still ugly though, you get to decide > which ugly is worse. :P Let's move with this series as is. We can see how prevalent the use of link_mode is on get side first.
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 1ab13c5dfb2f..ec4cd3921c67 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -129,6 +129,7 @@ struct ethtool_link_ksettings { __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising); } link_modes; u32 lanes; + enum ethtool_link_mode_bit_indices link_mode; }; /** diff --git a/net/ethtool/common.c b/net/ethtool/common.c index 24036e3055a1..399eee4a2051 100644 --- a/net/ethtool/common.c +++ b/net/ethtool/common.c @@ -197,6 +197,120 @@ const char link_mode_names[][ETH_GSTRING_LEN] = { }; static_assert(ARRAY_SIZE(link_mode_names) == __ETHTOOL_LINK_MODE_MASK_NBITS); +#define __DEFINE_LINK_MODE_PARAMS(_speed, _type, _lanes, _duplex) \ + [ETHTOOL_LINK_MODE(_speed, _type, _duplex)] = { \ + .speed = SPEED_ ## _speed, \ + .lanes = _lanes, \ + .duplex = __DUPLEX_ ## _duplex \ + } +#define __DUPLEX_Half DUPLEX_HALF +#define __DUPLEX_Full DUPLEX_FULL +#define __DEFINE_SPECIAL_MODE_PARAMS(_mode) \ + [ETHTOOL_LINK_MODE_ ## _mode ## _BIT] = { \ + .speed = SPEED_UNKNOWN, \ + .lanes = ETHTOOL_LANES_UNKNOWN, \ + .duplex = DUPLEX_UNKNOWN, \ + } + +const struct link_mode_info link_mode_params[] = { + __DEFINE_LINK_MODE_PARAMS(10, T, 1, Half), + __DEFINE_LINK_MODE_PARAMS(10, T, 1, Full), + __DEFINE_LINK_MODE_PARAMS(100, T, 1, Half), + __DEFINE_LINK_MODE_PARAMS(100, T, 1, Full), + __DEFINE_LINK_MODE_PARAMS(1000, T, 1, Half), + __DEFINE_LINK_MODE_PARAMS(1000, T, 1, Full), + __DEFINE_SPECIAL_MODE_PARAMS(Autoneg), + __DEFINE_SPECIAL_MODE_PARAMS(TP), + __DEFINE_SPECIAL_MODE_PARAMS(AUI), + __DEFINE_SPECIAL_MODE_PARAMS(MII), + __DEFINE_SPECIAL_MODE_PARAMS(FIBRE), + __DEFINE_SPECIAL_MODE_PARAMS(BNC), + __DEFINE_LINK_MODE_PARAMS(10000, T, 1, Full), + __DEFINE_SPECIAL_MODE_PARAMS(Pause), + __DEFINE_SPECIAL_MODE_PARAMS(Asym_Pause), + __DEFINE_LINK_MODE_PARAMS(2500, X, 1, Full), + __DEFINE_SPECIAL_MODE_PARAMS(Backplane), + __DEFINE_LINK_MODE_PARAMS(1000, KX, 1, Full), + __DEFINE_LINK_MODE_PARAMS(10000, KX4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(10000, KR, 1, Full), + [ETHTOOL_LINK_MODE_10000baseR_FEC_BIT] = { + .speed = SPEED_10000, + .duplex = DUPLEX_FULL, + }, + __DEFINE_LINK_MODE_PARAMS(20000, MLD2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(20000, KR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(40000, KR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(40000, CR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(40000, SR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(40000, LR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(56000, KR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(56000, CR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(56000, SR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(56000, LR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(25000, CR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(25000, KR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(25000, SR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(50000, CR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(50000, KR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(100000, KR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(100000, SR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(100000, CR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(100000, LR4_ER4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(50000, SR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(1000, X, 1, Full), + __DEFINE_LINK_MODE_PARAMS(10000, CR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(10000, SR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(10000, LR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(10000, LRM, 1, Full), + __DEFINE_LINK_MODE_PARAMS(10000, ER, 1, Full), + __DEFINE_LINK_MODE_PARAMS(2500, T, 1, Full), + __DEFINE_LINK_MODE_PARAMS(5000, T, 1, Full), + __DEFINE_SPECIAL_MODE_PARAMS(FEC_NONE), + __DEFINE_SPECIAL_MODE_PARAMS(FEC_RS), + __DEFINE_SPECIAL_MODE_PARAMS(FEC_BASER), + __DEFINE_LINK_MODE_PARAMS(50000, KR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(50000, SR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(50000, CR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(50000, LR_ER_FR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(50000, DR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(100000, KR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(100000, SR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(100000, CR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(100000, LR2_ER2_FR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(100000, DR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(200000, KR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(200000, SR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(200000, LR4_ER4_FR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(200000, DR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(200000, CR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(100, T1, 1, Full), + __DEFINE_LINK_MODE_PARAMS(1000, T1, 1, Full), + __DEFINE_LINK_MODE_PARAMS(400000, KR8, 8, Full), + __DEFINE_LINK_MODE_PARAMS(400000, SR8, 8, Full), + __DEFINE_LINK_MODE_PARAMS(400000, LR8_ER8_FR8, 8, Full), + __DEFINE_LINK_MODE_PARAMS(400000, DR8, 8, Full), + __DEFINE_LINK_MODE_PARAMS(400000, CR8, 8, Full), + __DEFINE_SPECIAL_MODE_PARAMS(FEC_LLRS), + __DEFINE_LINK_MODE_PARAMS(100000, KR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(100000, SR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(100000, LR_ER_FR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(100000, DR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(100000, CR, 1, Full), + __DEFINE_LINK_MODE_PARAMS(200000, KR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(200000, SR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(200000, LR2_ER2_FR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(200000, DR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(200000, CR2, 2, Full), + __DEFINE_LINK_MODE_PARAMS(400000, KR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(400000, SR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(400000, LR4_ER4_FR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(400000, DR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(400000, CR4, 4, Full), + __DEFINE_LINK_MODE_PARAMS(100, FX, 1, Half), + __DEFINE_LINK_MODE_PARAMS(100, FX, 1, Full), +}; +static_assert(ARRAY_SIZE(link_mode_params) == __ETHTOOL_LINK_MODE_MASK_NBITS); + const char netif_msg_class_names[][ETH_GSTRING_LEN] = { [NETIF_MSG_DRV_BIT] = "drv", [NETIF_MSG_PROBE_BIT] = "probe", diff --git a/net/ethtool/common.h b/net/ethtool/common.h index 3d9251c95a8b..625c8472700c 100644 --- a/net/ethtool/common.h +++ b/net/ethtool/common.h @@ -14,6 +14,12 @@ #define __SOF_TIMESTAMPING_CNT (const_ilog2(SOF_TIMESTAMPING_LAST) + 1) +struct link_mode_info { + int speed; + u32 lanes; + u8 duplex; +}; + extern const char netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN]; extern const char @@ -23,6 +29,7 @@ tunable_strings[__ETHTOOL_TUNABLE_COUNT][ETH_GSTRING_LEN]; extern const char phy_tunable_strings[__ETHTOOL_PHY_TUNABLE_COUNT][ETH_GSTRING_LEN]; extern const char link_mode_names[][ETH_GSTRING_LEN]; +extern const struct link_mode_info link_mode_params[]; extern const char netif_msg_class_names[][ETH_GSTRING_LEN]; extern const char wol_mode_names[][ETH_GSTRING_LEN]; extern const char sof_timestamping_names[][ETH_GSTRING_LEN]; diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c index 771688e1b0da..24783b71c584 100644 --- a/net/ethtool/ioctl.c +++ b/net/ethtool/ioctl.c @@ -426,13 +426,29 @@ struct ethtool_link_usettings { int __ethtool_get_link_ksettings(struct net_device *dev, struct ethtool_link_ksettings *link_ksettings) { + const struct link_mode_info *link_info; + int err; + ASSERT_RTNL(); if (!dev->ethtool_ops->get_link_ksettings) return -EOPNOTSUPP; memset(link_ksettings, 0, sizeof(*link_ksettings)); - return dev->ethtool_ops->get_link_ksettings(dev, link_ksettings); + + link_ksettings->link_mode = -1; + err = dev->ethtool_ops->get_link_ksettings(dev, link_ksettings); + if (err) + return err; + + if (link_ksettings->link_mode != -1) { + link_info = &link_mode_params[link_ksettings->link_mode]; + link_ksettings->base.speed = link_info->speed; + link_ksettings->lanes = link_info->lanes; + link_ksettings->base.duplex = link_info->duplex; + } + + return 0; } EXPORT_SYMBOL(__ethtool_get_link_ksettings); diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c index fb7d73250864..caf9111c5270 100644 --- a/net/ethtool/linkmodes.c +++ b/net/ethtool/linkmodes.c @@ -4,6 +4,8 @@ #include "common.h" #include "bitset.h" +/* LINKMODES_GET */ + struct linkmodes_req_info { struct ethnl_req_info base; }; @@ -150,125 +152,6 @@ const struct ethnl_request_ops ethnl_linkmodes_request_ops = { /* LINKMODES_SET */ -struct link_mode_info { - int speed; - u32 lanes; - u8 duplex; -}; - -#define __DEFINE_LINK_MODE_PARAMS(_speed, _type, _lanes, _duplex) \ - [ETHTOOL_LINK_MODE(_speed, _type, _duplex)] = { \ - .speed = SPEED_ ## _speed, \ - .lanes = _lanes, \ - .duplex = __DUPLEX_ ## _duplex \ - } -#define __DUPLEX_Half DUPLEX_HALF -#define __DUPLEX_Full DUPLEX_FULL -#define __DEFINE_SPECIAL_MODE_PARAMS(_mode) \ - [ETHTOOL_LINK_MODE_ ## _mode ## _BIT] = { \ - .speed = SPEED_UNKNOWN, \ - .lanes = ETHTOOL_LANES_UNKNOWN, \ - .duplex = DUPLEX_UNKNOWN, \ - } - -static const struct link_mode_info link_mode_params[] = { - __DEFINE_LINK_MODE_PARAMS(10, T, 1, Half), - __DEFINE_LINK_MODE_PARAMS(10, T, 1, Full), - __DEFINE_LINK_MODE_PARAMS(100, T, 1, Half), - __DEFINE_LINK_MODE_PARAMS(100, T, 1, Full), - __DEFINE_LINK_MODE_PARAMS(1000, T, 1, Half), - __DEFINE_LINK_MODE_PARAMS(1000, T, 1, Full), - __DEFINE_SPECIAL_MODE_PARAMS(Autoneg), - __DEFINE_SPECIAL_MODE_PARAMS(TP), - __DEFINE_SPECIAL_MODE_PARAMS(AUI), - __DEFINE_SPECIAL_MODE_PARAMS(MII), - __DEFINE_SPECIAL_MODE_PARAMS(FIBRE), - __DEFINE_SPECIAL_MODE_PARAMS(BNC), - __DEFINE_LINK_MODE_PARAMS(10000, T, 1, Full), - __DEFINE_SPECIAL_MODE_PARAMS(Pause), - __DEFINE_SPECIAL_MODE_PARAMS(Asym_Pause), - __DEFINE_LINK_MODE_PARAMS(2500, X, 1, Full), - __DEFINE_SPECIAL_MODE_PARAMS(Backplane), - __DEFINE_LINK_MODE_PARAMS(1000, KX, 1, Full), - __DEFINE_LINK_MODE_PARAMS(10000, KX4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(10000, KR, 1, Full), - [ETHTOOL_LINK_MODE_10000baseR_FEC_BIT] = { - .speed = SPEED_10000, - .duplex = DUPLEX_FULL, - }, - __DEFINE_LINK_MODE_PARAMS(20000, MLD2, 2, Full), - __DEFINE_LINK_MODE_PARAMS(20000, KR2, 2, Full), - __DEFINE_LINK_MODE_PARAMS(40000, KR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(40000, CR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(40000, SR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(40000, LR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(56000, KR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(56000, CR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(56000, SR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(56000, LR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(25000, CR, 1, Full), - __DEFINE_LINK_MODE_PARAMS(25000, KR, 1, Full), - __DEFINE_LINK_MODE_PARAMS(25000, SR, 1, Full), - __DEFINE_LINK_MODE_PARAMS(50000, CR2, 2, Full), - __DEFINE_LINK_MODE_PARAMS(50000, KR2, 2, Full), - __DEFINE_LINK_MODE_PARAMS(100000, KR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(100000, SR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(100000, CR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(100000, LR4_ER4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(50000, SR2, 2, Full), - __DEFINE_LINK_MODE_PARAMS(1000, X, 1, Full), - __DEFINE_LINK_MODE_PARAMS(10000, CR, 1, Full), - __DEFINE_LINK_MODE_PARAMS(10000, SR, 1, Full), - __DEFINE_LINK_MODE_PARAMS(10000, LR, 1, Full), - __DEFINE_LINK_MODE_PARAMS(10000, LRM, 1, Full), - __DEFINE_LINK_MODE_PARAMS(10000, ER, 1, Full), - __DEFINE_LINK_MODE_PARAMS(2500, T, 1, Full), - __DEFINE_LINK_MODE_PARAMS(5000, T, 1, Full), - __DEFINE_SPECIAL_MODE_PARAMS(FEC_NONE), - __DEFINE_SPECIAL_MODE_PARAMS(FEC_RS), - __DEFINE_SPECIAL_MODE_PARAMS(FEC_BASER), - __DEFINE_LINK_MODE_PARAMS(50000, KR, 1, Full), - __DEFINE_LINK_MODE_PARAMS(50000, SR, 1, Full), - __DEFINE_LINK_MODE_PARAMS(50000, CR, 1, Full), - __DEFINE_LINK_MODE_PARAMS(50000, LR_ER_FR, 1, Full), - __DEFINE_LINK_MODE_PARAMS(50000, DR, 1, Full), - __DEFINE_LINK_MODE_PARAMS(100000, KR2, 2, Full), - __DEFINE_LINK_MODE_PARAMS(100000, SR2, 2, Full), - __DEFINE_LINK_MODE_PARAMS(100000, CR2, 2, Full), - __DEFINE_LINK_MODE_PARAMS(100000, LR2_ER2_FR2, 2, Full), - __DEFINE_LINK_MODE_PARAMS(100000, DR2, 2, Full), - __DEFINE_LINK_MODE_PARAMS(200000, KR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(200000, SR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(200000, LR4_ER4_FR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(200000, DR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(200000, CR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(100, T1, 1, Full), - __DEFINE_LINK_MODE_PARAMS(1000, T1, 1, Full), - __DEFINE_LINK_MODE_PARAMS(400000, KR8, 8, Full), - __DEFINE_LINK_MODE_PARAMS(400000, SR8, 8, Full), - __DEFINE_LINK_MODE_PARAMS(400000, LR8_ER8_FR8, 8, Full), - __DEFINE_LINK_MODE_PARAMS(400000, DR8, 8, Full), - __DEFINE_LINK_MODE_PARAMS(400000, CR8, 8, Full), - __DEFINE_SPECIAL_MODE_PARAMS(FEC_LLRS), - __DEFINE_LINK_MODE_PARAMS(100000, KR, 1, Full), - __DEFINE_LINK_MODE_PARAMS(100000, SR, 1, Full), - __DEFINE_LINK_MODE_PARAMS(100000, LR_ER_FR, 1, Full), - __DEFINE_LINK_MODE_PARAMS(100000, DR, 1, Full), - __DEFINE_LINK_MODE_PARAMS(100000, CR, 1, Full), - __DEFINE_LINK_MODE_PARAMS(200000, KR2, 2, Full), - __DEFINE_LINK_MODE_PARAMS(200000, SR2, 2, Full), - __DEFINE_LINK_MODE_PARAMS(200000, LR2_ER2_FR2, 2, Full), - __DEFINE_LINK_MODE_PARAMS(200000, DR2, 2, Full), - __DEFINE_LINK_MODE_PARAMS(200000, CR2, 2, Full), - __DEFINE_LINK_MODE_PARAMS(400000, KR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(400000, SR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(400000, LR4_ER4_FR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(400000, DR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(400000, CR4, 4, Full), - __DEFINE_LINK_MODE_PARAMS(100, FX, 1, Half), - __DEFINE_LINK_MODE_PARAMS(100, FX, 1, Full), -}; - const struct nla_policy ethnl_linkmodes_set_policy[] = { [ETHTOOL_A_LINKMODES_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy), @@ -294,9 +177,6 @@ static bool ethnl_auto_linkmodes(struct ethtool_link_ksettings *ksettings, DECLARE_BITMAP(old_adv, __ETHTOOL_LINK_MODE_MASK_NBITS); unsigned int i; - BUILD_BUG_ON(ARRAY_SIZE(link_mode_params) != - __ETHTOOL_LINK_MODE_MASK_NBITS); - bitmap_copy(old_adv, advertising, __ETHTOOL_LINK_MODE_MASK_NBITS); for (i = 0; i < __ETHTOOL_LINK_MODE_MASK_NBITS; i++) {
Currently, when user space queries the link's parameters, as speed and duplex, each parameter is passed from the driver to ethtool. Instead, get the link mode bit in use, and derive each of the parameters from it in ethtool. Signed-off-by: Danielle Ratson <danieller@nvidia.com> --- Notes: v3: * Remove 'ETHTOOL_A_LINKMODES_LINK_MODE' from Documentation since it is not used. * Remove LINK_MODE_UNKNOWN from uapi. * Remove an unnecessary loop. * Move link_mode_info and link_mode_params to common file. * Move the speed, duplex and lanes derivation to the wrapper __ethtool_get_link_ksettings(). include/linux/ethtool.h | 1 + net/ethtool/common.c | 114 ++++++++++++++++++++++++++++++++++++ net/ethtool/common.h | 7 +++ net/ethtool/ioctl.c | 18 +++++- net/ethtool/linkmodes.c | 124 +--------------------------------------- 5 files changed, 141 insertions(+), 123 deletions(-)