[PATCH] net-sysfs: allow changing sysfs carrier when interface is down

[PATCH] net-sysfs: allow changing sysfs carrier when interface is down

[Joakim Tjernlund]

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);
 
-- 
2.35.1

Re: [PATCH] net-sysfs: allow changing sysfs carrier when interface is down

[Jakub Kicinski]

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;
>  	return dev_change_carrier(dev, (bool)new_carrier);
>  }

Re: [PATCH] net-sysfs: allow changing sysfs carrier when interface is down

[Joakim Tjernlund]

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;
> >  	return dev_change_carrier(dev, (bool)new_carrier);
> >  }

Re: [PATCH] net-sysfs: allow changing sysfs carrier when interface is down

[Stephen Hemminger]

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.

Re: [PATCH] net-sysfs: allow changing sysfs carrier when interface is down

[Joakim Tjernlund]

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

Re: [PATCH] net-sysfs: allow changing sysfs carrier when interface is down

[Jakub Kicinski]

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.

Re: [PATCH] net-sysfs: allow changing sysfs carrier when interface is down

[Joakim Tjernlund]

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

Re: [PATCH] net-sysfs: allow changing sysfs carrier when interface is down

[Jakub Kicinski]

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.

Re: [PATCH] net-sysfs: allow changing sysfs carrier when interface is down

[Joakim Tjernlund]

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

Re: [PATCH] net-sysfs: allow changing sysfs carrier when interface is down

[Andrew Lunn]

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

Re: [PATCH] net-sysfs: allow changing sysfs carrier when interface is down

[Joakim Tjernlund]

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

Re: [PATCH] net-sysfs: allow changing sysfs carrier when interface is down

[Stephen Hemminger]

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-email-jpirko@redhat.com/

PS: if you have lots of userspace handling, you should be using netlink not sysfs for management

Re: [PATCH] net-sysfs: allow changing sysfs carrier when interface is down

[Stephen Hemminger]

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?

Re: [PATCH] net-sysfs: allow changing sysfs carrier when interface is down

[Joakim Tjernlund]

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

Re: [PATCH] net-sysfs: allow changing sysfs carrier when interface is down

[Joakim Tjernlund]

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