diff mbox

atm : fix /sys/devices/virtual/atm/X/carrier(ATM_PHY_SIG_UNKNOWN)

Message ID 20100214175136.GA15891@frolo.macqel
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Philippe De Muyter Feb. 14, 2010, 5:51 p.m. UTC 
Hi Chas,

On Sat, Feb 13, 2010 at 06:24:34PM -0500, Chas Williams (CONTRACTOR) wrote:
> In message <20100213215633.GA4345@frolo.macqel>,Philippe De Muyter writes:
> >value '1' while actually carrier was not yet established and real carrier
> >state was not yet known to linux.  I propose to fix that by using '?' as the
> >/sys/devices/virtual/atm/cxacru0/carrier value when carrier state is not yet
> >known to linux.  Any other value except '1' would also be OK for me.
> 
> this is sort of intentional because some drivers dont actually implement
> atm_dev->signal.  ATM_PHY_SIG_UNKNOWN and ATM_PHY_SIG_LOST should
> likely be carrier = 0.  if the driver doesnt isnt going to handle
> changing the state of atm_dev->signal it should just set the value to
> ATM_PHY_SIG_FOUND.
> 
> i dont like the idea of carrier being '?' -- carrier is either true or false.
> you have it or you dont.

OK for me.  I would agree with

	carrier = 0 when signal == ATM_PHY_SIG_UNKNOWN,

but currently we have

	carrier = 1 when signal == ATM_PHY_SIG_UNKNOWN

cxacru itself does the right thing : as soon as carrier state is known,
signal is set to ATM_PHY_SIG_LOST or ATM_PHY_SIG_FOUND, but
atm_sysfs.c::show_carrier is wrong.

So here is a revised patch :

--

The carrier field of /sys/devices/virtual/atm/cxacru0 shows 1 when carrier
is actually down (but unknown to linux).  Make it show 0 instead in that case.

Signed-off-by: Philippe De Muyter <phdm@macqel.be>

--
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

Comments

chas williams - CONTRACTOR Feb. 14, 2010, 9:34 p.m. UTC | #1 
In message <20100214175136.GA15891@frolo.macqel>,Philippe De Muyter writes:
>OK for me.  I would agree with
>
>	carrier = 0 when signal == ATM_PHY_SIG_UNKNOWN,
>
>but currently we have
>
>	carrier = 1 when signal == ATM_PHY_SIG_UNKNOWN

not quite what i said (or perhaps i didnt clearly convey my intent).
ATM_PHY_SIG_UNKNOWN is set during the driver create/initialize.
unfortunately, some drivers never bothers to set signal so it winds up
always being unknown.  however, for those drivers, we cant assume the
carrier is 0, or nothing will happen if someone is watching for the
carrier to be 1.

so, all drivers, if they arent going to support setting signal should
just assign it to 1.  i would sort of prefer that ATM_PHY_SIG_UNKNOWN
simply didnt exist.  as i said before there is carrier or there is
not.  if the driver doesnt support checking/knowing the carrier status
it should simply say 'yes i have carrier'.

i will see if i cant workup a little patch for this today.
--
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
David Miller Feb. 17, 2010, 6:16 a.m. UTC | #2 
From: "Chas Williams (CONTRACTOR)" <chas@cmf.nrl.navy.mil>
Date: Sun, 14 Feb 2010 16:34:40 -0500

> as i said before there is carrier or there is not.  if the driver
> doesnt support checking/knowing the carrier status it should simply
> say 'yes i have carrier'.

I agree %100 and this is how we handle similar situations
for other networking device types.
--
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
Philippe De Muyter Feb. 17, 2010, 11:35 a.m. UTC | #3 
Hi Chas, David,

On Tue, Feb 16, 2010 at 10:16:51PM -0800, David Miller wrote:
> From: "Chas Williams (CONTRACTOR)" <chas@cmf.nrl.navy.mil>
> Date: Sun, 14 Feb 2010 16:34:40 -0500
> 
> > as i said before there is carrier or there is not.  if the driver
> > doesnt support checking/knowing the carrier status it should simply
> > say 'yes i have carrier'.
> 
> I agree %100 and this is how we handle similar situations
> for other networking device types.

I also agree 100% with that : the low-level driver must do that, not the
generic class driver.  Here cxacru is punished because other atm drivers
forget to set signal = ATM_PHY_SIG_FOUND.

Philippe
--
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
diff mbox

Patch

--- a/net/atm/atm_sysfs.c	2010-02-14 18:17:05.604508129 +0100
+++ b/net/atm/atm_sysfs.c	2010-02-14 18:15:08.316917041 +0100
@@ -63,8 +63,7 @@  static ssize_t show_carrier(struct devic
 	char *pos = buf;
 	struct atm_dev *adev = to_atm_dev(cdev);
 
-	pos += sprintf(pos, "%d\n",
-		       adev->signal == ATM_PHY_SIG_LOST ? 0 : 1);
+	pos += sprintf(pos, "%d\n", adev->signal == ATM_PHY_SIG_FOUND);
 
 	return pos - buf;
 }