UP/DOWN and carrier are async events and it makes sense one can adjust carrier in sysfs before bringing the interface up.
Signed-off-by: Joakim Tjernlund joakim.tjernlund@infinera.com Cc: stable@vger.kernel.org --- net/core/net-sysfs.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c index a4ae65263384..3418ef7ef2d8 100644 --- a/net/core/net-sysfs.c +++ b/net/core/net-sysfs.c @@ -167,8 +167,6 @@ static DEVICE_ATTR_RO(broadcast);
static int change_carrier(struct net_device *dev, unsigned long new_carrier) { - if (!netif_running(dev)) - return -EINVAL; return dev_change_carrier(dev, (bool)new_carrier); }
@@ -191,10 +189,7 @@ static ssize_t carrier_show(struct device *dev, { struct net_device *netdev = to_net_dev(dev);
- if (netif_running(netdev)) - return sprintf(buf, fmt_dec, !!netif_carrier_ok(netdev)); - - return -EINVAL; + return sprintf(buf, fmt_dec, !!netif_carrier_ok(netdev)); } static DEVICE_ATTR_RW(carrier);
On Thu, 2 Jun 2022 02:35:23 +0200 Joakim Tjernlund wrote:
UP/DOWN and carrier are async events and it makes sense one can adjust carrier in sysfs before bringing the interface up.
Can you explain your use case?
Signed-off-by: Joakim Tjernlund joakim.tjernlund@infinera.com Cc: stable@vger.kernel.org
Seems a little too risky of a change to push into stable.
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c index a4ae65263384..3418ef7ef2d8 100644 --- a/net/core/net-sysfs.c +++ b/net/core/net-sysfs.c @@ -167,8 +167,6 @@ static DEVICE_ATTR_RO(broadcast); static int change_carrier(struct net_device *dev, unsigned long new_carrier) {
- if (!netif_running(dev))
return -EINVAL;}
On Wed, 2022-06-01 at 18:01 -0700, Jakub Kicinski wrote:
On Thu, 2 Jun 2022 02:35:23 +0200 Joakim Tjernlund wrote:
UP/DOWN and carrier are async events and it makes sense one can adjust carrier in sysfs before bringing the interface up.
Can you explain your use case?
Sure, our HW has config/state changes that makes it impossible for net driver to touch and registers or TX pkgs(can result in System Error exception in worst case.
So the user space app handlings this needs to make sure that even if say dchp brings an I/F up, there can be no HW access by the driver. To do that, carrier needs to be controlled before I/F is brought up.
Carrier reflects actual link status and this kan change at any time. I honestly don't understand why you would prevent sysfs access to carrier just because I/F is down?
Signed-off-by: Joakim Tjernlund joakim.tjernlund@infinera.com Cc: stable@vger.kernel.org
Seems a little too risky of a change to push into stable.
The change is minimal and only allows access to carrier when I/F is also down. I think this is a kernel bug and should go to stable too.
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c index a4ae65263384..3418ef7ef2d8 100644 --- a/net/core/net-sysfs.c +++ b/net/core/net-sysfs.c @@ -167,8 +167,6 @@ static DEVICE_ATTR_RO(broadcast); static int change_carrier(struct net_device *dev, unsigned long new_carrier) {
- if (!netif_running(dev))
return -EINVAL;}
On Thu, 2 Jun 2022 09:22:34 +0000 Joakim Tjernlund Joakim.Tjernlund@infinera.com wrote:
On Wed, 2022-06-01 at 18:01 -0700, Jakub Kicinski wrote:
On Thu, 2 Jun 2022 02:35:23 +0200 Joakim Tjernlund wrote:
UP/DOWN and carrier are async events and it makes sense one can adjust carrier in sysfs before bringing the interface up.
Can you explain your use case?
Sure, our HW has config/state changes that makes it impossible for net driver to touch and registers or TX pkgs(can result in System Error exception in worst case.
So the user space app handlings this needs to make sure that even if say dchp brings an I/F up, there can be no HW access by the driver. To do that, carrier needs to be controlled before I/F is brought up.
Carrier reflects actual link status and this kan change at any time. I honestly don't understand why you would prevent sysfs access to carrier just because I/F is down?
Signed-off-by: Joakim Tjernlund joakim.tjernlund@infinera.com Cc: stable@vger.kernel.org
Seems a little too risky of a change to push into stable.
The change is minimal and only allows access to carrier when I/F is also down. I think this is a kernel bug and should go to stable too.
For many devices when device is down, the phy is turned off so the state of carrier is either always down or undefined.
That is why the code had the check originally.
On Thu, 2022-06-02 at 08:56 -0700, Stephen Hemminger wrote:
On Thu, 2 Jun 2022 09:22:34 +0000 Joakim Tjernlund Joakim.Tjernlund@infinera.com wrote:
On Wed, 2022-06-01 at 18:01 -0700, Jakub Kicinski wrote:
On Thu, 2 Jun 2022 02:35:23 +0200 Joakim Tjernlund wrote:
UP/DOWN and carrier are async events and it makes sense one can adjust carrier in sysfs before bringing the interface up.
Can you explain your use case?
Sure, our HW has config/state changes that makes it impossible for net driver to touch and registers or TX pkgs(can result in System Error exception in worst case.
So the user space app handlings this needs to make sure that even if say dchp brings an I/F up, there can be no HW access by the driver. To do that, carrier needs to be controlled before I/F is brought up.
Carrier reflects actual link status and this kan change at any time. I honestly don't understand why you would prevent sysfs access to carrier just because I/F is down?
Signed-off-by: Joakim Tjernlund joakim.tjernlund@infinera.com Cc: stable@vger.kernel.org
Seems a little too risky of a change to push into stable.
The change is minimal and only allows access to carrier when I/F is also down. I think this is a kernel bug and should go to stable too.
For many devices when device is down, the phy is turned off so the state of carrier is either always down or undefined.
That is why the code had the check originally.
Maybe so but it seems to me that this limitation was put in place without much thought. Here an app takes on the role to manage carrier for that device and the app should be free to manage the carrier as it see best.
Are there some use cases that must deny sysfs carrier access to its own carrier?
Jocke
On Thu, 2 Jun 2022 16:26:18 +0000 Joakim Tjernlund wrote:
On Thu, 2022-06-02 at 08:56 -0700, Stephen Hemminger wrote:
Sure, our HW has config/state changes that makes it impossible for net driver to touch and registers or TX pkgs(can result in System Error exception in worst case.
What is "our HW", what kernel driver does it use and why can't the kernel driver take care of making sure the device is not accessed when it'd crash the system?
Maybe so but it seems to me that this limitation was put in place without much thought.
Don't make unnecessary disparaging statements about someone else's work. Whoever that person was.
On Thu, 2022-06-02 at 09:57 -0700, Jakub Kicinski wrote:
On Thu, 2 Jun 2022 16:26:18 +0000 Joakim Tjernlund wrote:
On Thu, 2022-06-02 at 08:56 -0700, Stephen Hemminger wrote:
Sure, our HW has config/state changes that makes it impossible for net driver to touch and registers or TX pkgs(can result in System Error exception in worst case.
What is "our HW", what kernel driver does it use and why can't the kernel driver take care of making sure the device is not accessed when it'd crash the system?
It is a custom asic with some homegrown controller. The full config path is too complex for kernel too know and depends on user input. The cashing/TX TMO part was not part of the design plans and I have been down this route with the HW designers without success.
Maybe so but it seems to me that this limitation was put in place without much thought.
Don't make unnecessary disparaging statements about someone else's work. Whoever that person was.
That was not meant the way you read it, sorry for being unclear. The commit from 2012 simply says: net: allow to change carrier via sysfs
Make carrier writable
On Thu, 2 Jun 2022 17:15:13 +0000 Joakim Tjernlund wrote:
What is "our HW", what kernel driver does it use and why can't the kernel driver take care of making sure the device is not accessed when it'd crash the system?
It is a custom asic with some homegrown controller. The full config path is too complex for kernel too know and depends on user input.
We have a long standing tradition of not caring about user space drivers in netdev land. I see no reason to merge this patch upstream.
Maybe so but it seems to me that this limitation was put in place without much thought.
Don't make unnecessary disparaging statements about someone else's work. Whoever that person was.
That was not meant the way you read it, sorry for being unclear. The commit from 2012 simply says: net: allow to change carrier via sysfs Make carrier writable
Yeah, IIUC the interface was created for software devices.
On Thu, 2022-06-02 at 10:52 -0700, Jakub Kicinski wrote:
On Thu, 2 Jun 2022 17:15:13 +0000 Joakim Tjernlund wrote:
What is "our HW", what kernel driver does it use and why can't the kernel driver take care of making sure the device is not accessed when it'd crash the system?
It is a custom asic with some homegrown controller. The full config path is too complex for kernel too know and depends on user input.
We have a long standing tradition of not caring about user space drivers in netdev land. I see no reason to merge this patch upstream.
This is not a user space driver. View it as a eth controller with a dum PHY which cannot convey link status. The kernel driver then needs help with managing carrier.
Maybe so but it seems to me that this limitation was put in place without much thought.
Don't make unnecessary disparaging statements about someone else's work. Whoever that person was.
That was not meant the way you read it, sorry for being unclear. The commit from 2012 simply says: net: allow to change carrier via sysfs Make carrier writable
Yeah, IIUC the interface was created for software devices.
software devices? like dummy device? I guess that is the first user of this feature but not restricted to that.
Jocke
On Thu, Jun 02, 2022 at 06:01:08PM +0000, Joakim Tjernlund wrote:
On Thu, 2022-06-02 at 10:52 -0700, Jakub Kicinski wrote:
On Thu, 2 Jun 2022 17:15:13 +0000 Joakim Tjernlund wrote:
What is "our HW", what kernel driver does it use and why can't the kernel driver take care of making sure the device is not accessed when it'd crash the system?
It is a custom asic with some homegrown controller. The full config path is too complex for kernel too know and depends on user input.
We have a long standing tradition of not caring about user space drivers in netdev land. I see no reason to merge this patch upstream.
This is not a user space driver. View it as a eth controller with a dum PHY which cannot convey link status. The kernel driver then needs help with managing carrier.
Please post the MAC driver then. We don't really like changes to the kernel without a user. You MAC driver would be such a user.
Could you also tell us more about the PHY. What capabilities does it have? I assume it is not C22 compatible. Does it at least have some sort of indicator of link? What might make sense is to use the fixed-link code. You can provide a callback which gives the actual link status up/down. And the fixed-link driver looks like a real PHY, so the MAC driver does not need to do anything special.
Andrew
On Sun, 2022-06-05 at 20:50 +0200, Andrew Lunn wrote:
On Thu, Jun 02, 2022 at 06:01:08PM +0000, Joakim Tjernlund wrote:
On Thu, 2022-06-02 at 10:52 -0700, Jakub Kicinski wrote:
On Thu, 2 Jun 2022 17:15:13 +0000 Joakim Tjernlund wrote:
What is "our HW", what kernel driver does it use and why can't the kernel driver take care of making sure the device is not accessed when it'd crash the system?
It is a custom asic with some homegrown controller. The full config path is too complex for kernel too know and depends on user input.
We have a long standing tradition of not caring about user space drivers in netdev land. I see no reason to merge this patch upstream.
This is not a user space driver. View it as a eth controller with a dum PHY which cannot convey link status. The kernel driver then needs help with managing carrier.
Please post the MAC driver then. We don't really like changes to the kernel without a user. You MAC driver would be such a user.
That driver is far from kernel proper/upstream worthy ..
Could you also tell us more about the PHY. What capabilities does it have? I assume it is not C22 compatible. Does it at least have some sort of indicator of link? What might make sense is to use the
There is no PHY really, from kernels POV it is just a DMA engine and all the setup needed to setup the full path is in US.
fixed-link code. You can provide a callback which gives the actual link status up/down. And the fixed-link driver looks like a real PHY, so the MAC driver does not need to do anything special.
The fixed PHY has the same problem as it uses the same sysfs I/F. I am just asking that the sysfs carrier I/F should be writeable and readable when I/F is DOWN. Now one cannot even read carrier status when I/F is down.
Andrew
On Thu, 2 Jun 2022 10:52:15 -0700 Jakub Kicinski kuba@kernel.org wrote:
On Thu, 2 Jun 2022 17:15:13 +0000 Joakim Tjernlund wrote:
What is "our HW", what kernel driver does it use and why can't the kernel driver take care of making sure the device is not accessed when it'd crash the system?
It is a custom asic with some homegrown controller. The full config path is too complex for kernel too know and depends on user input.
We have a long standing tradition of not caring about user space drivers in netdev land. I see no reason to merge this patch upstream.
Maybe so but it seems to me that this limitation was put in place without much thought.
Don't make unnecessary disparaging statements about someone else's work. Whoever that person was.
That was not meant the way you read it, sorry for being unclear. The commit from 2012 simply says: net: allow to change carrier via sysfs Make carrier writable
Yeah, IIUC the interface was created for software devices.
If you want to discussion of original patch see: https://patchwork.ozlabs.org/project/netdev/patch/1314715608-978-2-git-send-...
PS: if you have lots of userspace handling, you should be using netlink not sysfs for management
On Thu, 2 Jun 2022 17:15:13 +0000 Joakim Tjernlund Joakim.Tjernlund@infinera.com wrote:
On Thu, 2022-06-02 at 09:57 -0700, Jakub Kicinski wrote:
On Thu, 2 Jun 2022 16:26:18 +0000 Joakim Tjernlund wrote:
On Thu, 2022-06-02 at 08:56 -0700, Stephen Hemminger wrote:
Sure, our HW has config/state changes that makes it impossible for net driver to touch and registers or TX pkgs(can result in System Error exception in worst case.
What is "our HW", what kernel driver does it use and why can't the kernel driver take care of making sure the device is not accessed when it'd crash the system?
It is a custom asic with some homegrown controller. The full config path is too complex for kernel too know and depends on user input. The cashing/TX TMO part was not part of the design plans and I have been down this route with the HW designers without success.
Changing upstream code to support out of tree code? The risk of breaking current users for something that no one else uses is a bad idea.
Maybe so but it seems to me that this limitation was put in place without much thought.
Don't make unnecessary disparaging statements about someone else's work. Whoever that person was.
That was not meant the way you read it, sorry for being unclear. The commit from 2012 simply says: net: allow to change carrier via sysfs Make carrier writable
Setting carrier from userspace was added to support VPN's etc; in general it was not meant as hardware workaround.
Often using operstate is better with complex hardware did you look at that?
On Thu, 2022-06-02 at 10:56 -0700, Stephen Hemminger wrote:
On Thu, 2 Jun 2022 17:15:13 +0000 Joakim Tjernlund Joakim.Tjernlund@infinera.com wrote:
On Thu, 2022-06-02 at 09:57 -0700, Jakub Kicinski wrote:
On Thu, 2 Jun 2022 16:26:18 +0000 Joakim Tjernlund wrote:
On Thu, 2022-06-02 at 08:56 -0700, Stephen Hemminger wrote:
Sure, our HW has config/state changes that makes it impossible for net driver to touch and registers or TX pkgs(can result in System Error exception in worst case.
What is "our HW", what kernel driver does it use and why can't the kernel driver take care of making sure the device is not accessed when it'd crash the system?
It is a custom asic with some homegrown controller. The full config path is too complex for kernel too know and depends on user input. The cashing/TX TMO part was not part of the design plans and I have been down this route with the HW designers without success.
Changing upstream code to support out of tree code? The risk of breaking current users for something that no one else uses is a bad idea.
There are in tree users too, see fixed_phy_change_carrier(), I am not asking for adding a new feature, just making an existing one more usable.
Maybe so but it seems to me that this limitation was put in place without much thought.
Don't make unnecessary disparaging statements about someone else's work. Whoever that person was.
That was not meant the way you read it, sorry for being unclear. The commit from 2012 simply says: net: allow to change carrier via sysfs Make carrier writable
Setting carrier from userspace was added to support VPN's etc; in general it was not meant as hardware workaround.
Often using operstate is better with complex hardware did you look at that?
You mean? ls -l /sys/class/net/sgmii/operstate -r--r--r-- 1 root root 4096 Mar 9 12:46 /sys/class/net/sgmii/operstate
I did, but operstate here is not writeable. Did you perhaps mean some other operstate ?
Jocke
On Thu, 2022-06-02 at 20:09 +0200, Joakim Tjernlund wrote:
On Thu, 2022-06-02 at 10:56 -0700, Stephen Hemminger wrote:
On Thu, 2 Jun 2022 17:15:13 +0000 Joakim Tjernlund Joakim.Tjernlund@infinera.com wrote:
On Thu, 2022-06-02 at 09:57 -0700, Jakub Kicinski wrote:
On Thu, 2 Jun 2022 16:26:18 +0000 Joakim Tjernlund wrote:
On Thu, 2022-06-02 at 08:56 -0700, Stephen Hemminger wrote:
> Sure, our HW has config/state changes that makes it impossible for net driver > to touch and registers or TX pkgs(can result in System Error exception in worst case.
What is "our HW", what kernel driver does it use and why can't the kernel driver take care of making sure the device is not accessed when it'd crash the system?
It is a custom asic with some homegrown controller. The full config path is too complex for kernel too know and depends on user input. The cashing/TX TMO part was not part of the design plans and I have been down this route with the HW designers without success.
Changing upstream code to support out of tree code? The risk of breaking current users for something that no one else uses is a bad idea.
There are in tree users too, see fixed_phy_change_carrier(), I am not asking for adding a new feature, just making an existing one more usable.
Maybe so but it seems to me that this limitation was put in place without much thought.
Don't make unnecessary disparaging statements about someone else's work. Whoever that person was.
That was not meant the way you read it, sorry for being unclear. The commit from 2012 simply says: net: allow to change carrier via sysfs Make carrier writable
Setting carrier from userspace was added to support VPN's etc; in general it was not meant as hardware workaround.
Often using operstate is better with complex hardware did you look at that?
You mean? ls -l /sys/class/net/sgmii/operstate -r--r--r-- 1 root root 4096 Mar 9 12:46 /sys/class/net/sgmii/operstate
I did, but operstate here is not writeable. Did you perhaps mean some other operstate ?
Looked a little deeper and found: ip link set eth0 carrier off/on which works regardless of eth0 UP/DOWN state for me.
Exactly what I am asking net-sysfs to allow as well. if net-sysfs carrier and ip link set eth0 carrier are the same function it makes sense they have the same privs, right?
Jocke
linux-stable-mirror@lists.linaro.org
-
Andrew Lunn -
Jakub Kicinski -
Joakim Tjernlund -
Joakim Tjernlund
-
Stephen Hemminger