[PATCH 3/3] cxacru: Store all device status information and report it when atm_proc_read is called.

[PATCH 3/3] cxacru: Store all device status information and report it when atm_proc_read is called.

[Simon Arlott]

There is much more (useful) line status information than is output through usbatm, this patch stores the status array and overrides atm_proc_read to output all of it.

This change may affect users polling the /proc/net/atm/cxacru file for line up/down status, however better status information is already provided through INFO level printks.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>

---
 drivers/usb/atm/cxacru.c |  144 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 144 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index 1fa0844..1049e34 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -159,6 +159,7 @@ struct cxacru_data {
 
 	int line_status;
 	struct delayed_work poll_work;
+	u32 cxinf_status[CXINF_MAX];
 
 	/* contol handles */
 	struct mutex cm_serialize;
@@ -170,6 +171,146 @@ struct cxacru_data {
 	struct completion snd_done;
 };
 
+static int cxacru_proc_read(struct usbatm_data *usbatm_instance,
+	struct atm_dev *atm_dev, loff_t * pos, char *page)
+{
+	struct cxacru_data *instance = usbatm_instance->driver_data;
+	u32 *cxinf = instance->cxinf_status;
+	int left = *pos;
+
+	if (!left--)
+		return sprintf(page, "# %s\n", usbatm_instance->description);
+
+	if (!left--) {
+		if (cxinf[CXINF_LINE_STATUS] == 5) {
+			return sprintf(page, "# UP %u/%u\n",
+				cxinf[CXINF_DOWNSTREAM_RATE],
+				cxinf[CXINF_UPSTREAM_RATE]);
+		} else {
+			return sprintf(page, "# DOWN\n");
+		}
+	}
+
+	if (!left--)
+		return sprintf(page, "MAC=%02x:%02x:%02x:%02x:%02x:%02x\n",
+					atm_dev->esi[0], atm_dev->esi[1],
+					atm_dev->esi[2], atm_dev->esi[3],
+					atm_dev->esi[4], atm_dev->esi[5]);
+
+	if (!left--)
+		switch (cxinf[CXINF_LINK_STATUS]) {
+		case 1: return sprintf(page, "LINK_STATUS=\"not connected\"\n");
+		case 2: return sprintf(page, "LINK_STATUS=\"connected\"\n");
+		case 3: return sprintf(page, "LINK_STATUS=\"lost\"\n");
+		default:
+			return sprintf(page, "LINK_STATUS=\"unknown (%u)\"\n",
+						cxinf[CXINF_LINK_STATUS]);
+		}
+
+	if (!left--)
+		switch (cxinf[CXINF_LINE_STATUS]) {
+		case 0: return sprintf(page, "LINE_STATUS=\"down\"\n");
+		case 1: return sprintf(page, "LINE_STATUS=\"attempting to activate\"\n");
+		case 2: return sprintf(page, "LINE_STATUS=\"training\"\n");
+		case 3: return sprintf(page, "LINE_STATUS=\"channel analysis\"\n");
+		case 4: return sprintf(page, "LINE_STATUS=\"exchange\"\n");
+		case 5: return sprintf(page, "LINE_STATUS=\"up\"\n");
+		case 6: return sprintf(page, "LINE_STATUS=\"waiting\"\n");
+		case 7: return sprintf(page, "LINE_STATUS=\"initialising\"\n");
+		default:
+			return sprintf(page, "LINE_STATUS=\"unknown (%u)\"\n",
+						cxinf[CXINF_LINE_STATUS]);
+		}
+
+	if (!left--)
+		return sprintf(page, "\n");
+
+	if (cxinf[CXINF_LINE_STATUS] == 5) /* up */
+		if (!left--)
+			return sprintf(page, "DOWNSTREAM_RATE=%u\n",
+					cxinf[CXINF_DOWNSTREAM_RATE]);
+	if (!left--)
+		return sprintf(page,
+			"DOWNSTREAM_SNR_MARGIN=%d.%02u\n"
+			"DOWNSTREAM_ATTENUATION=%d.%02u\n"
+			"DOWNSTREAM_BITS_PER_FRAME=%u\n"
+			"DOWNSTREAM_CRC_ERRORS=%u\n"
+			"DOWNSTREAM_FEC_ERRORS=%u\n"
+			"DOWNSTREAM_HEC_ERRORS=%u\n"
+			"\n",
+			cxinf[CXINF_DOWNSTREAM_SNR_MARGIN] / 100,
+			cxinf[CXINF_DOWNSTREAM_SNR_MARGIN] % 100,
+			cxinf[CXINF_DOWNSTREAM_ATTENUATION] / 100,
+			cxinf[CXINF_DOWNSTREAM_ATTENUATION] % 100,
+			cxinf[CXINF_DOWNSTREAM_BITS_PER_FRAME],
+			cxinf[CXINF_DOWNSTREAM_CRC_ERRORS],
+			cxinf[CXINF_DOWNSTREAM_FEC_ERRORS],
+			cxinf[CXINF_DOWNSTREAM_HEC_ERRORS]);
+
+	if (cxinf[CXINF_LINE_STATUS] == 5) /* up */
+		if (!left--)
+			return sprintf(page, "UPSTREAM_RATE=%u\n",
+					cxinf[CXINF_UPSTREAM_RATE]);
+	if (!left--)
+		return sprintf(page,
+			"UPSTREAM_SNR_MARGIN=%d.%02u\n"
+			"UPSTREAM_ATTENUATION=%d.%02u\n"
+			"UPSTREAM_BITS_PER_FRAME=%u\n"
+			"UPSTREAM_CRC_ERRORS=%u\n"
+			"UPSTREAM_FEC_ERRORS=%u\n"
+			"UPSTREAM_HEC_ERRORS=%u\n"
+			"TRANSMITTER_POWER=%d\n"
+			"\n",
+			cxinf[CXINF_UPSTREAM_SNR_MARGIN] / 100,
+			cxinf[CXINF_UPSTREAM_SNR_MARGIN] % 100,
+			cxinf[CXINF_UPSTREAM_ATTENUATION] / 100,
+			cxinf[CXINF_UPSTREAM_ATTENUATION] % 100,
+			cxinf[CXINF_UPSTREAM_BITS_PER_FRAME],
+			cxinf[CXINF_UPSTREAM_CRC_ERRORS],
+			cxinf[CXINF_UPSTREAM_FEC_ERRORS],
+			cxinf[CXINF_UPSTREAM_HEC_ERRORS],
+			cxinf[CXINF_TRANSMITTER_POWER] - 256);
+
+	if (!left--)
+		return sprintf(page,
+			"AAL5_TX=%u\n"
+			"AAL5_TX_ERR=%u\n"
+			"AAL5_RX=%u\n"
+			"AAL5_RX_ERR=%u\n"
+			"AAL5_RX_DROP=%u\n"
+			"\n",
+			atomic_read(&atm_dev->stats.aal5.tx),
+			atomic_read(&atm_dev->stats.aal5.tx_err),
+			atomic_read(&atm_dev->stats.aal5.rx),
+			atomic_read(&atm_dev->stats.aal5.rx_err),
+			atomic_read(&atm_dev->stats.aal5.rx_drop));
+
+	if (!left--)
+		return sprintf(page,
+			"ADSL_HEADEND=%u\n"
+			"ADSL_HEADEND_ENVIRONMENT=%u\n"
+			"STARTUP_ATTEMPTS=%u\n"
+			"LINE_STARTABLE=%u\n"
+			"CONTROLLER_VERSION=%u\n",
+			cxinf[CXINF_ADSL_HEADEND],
+			cxinf[CXINF_ADSL_HEADEND_ENVIRONMENT],
+			cxinf[CXINF_STARTUP_ATTEMPTS],
+			cxinf[CXINF_LINE_STARTABLE],
+			cxinf[CXINF_CONTROLLER_VERSION]);
+
+	if (!left--)
+		switch (cxinf[CXINF_MODULATION]) {
+		case 1: return sprintf(page, "MODULATION=\"ANSI T1.413\"\n");
+		case 2: return sprintf(page, "MODULATION=\"ITU-T G.992.1 (G.DMT)\"\n");
+		case 3: return sprintf(page, "MODULATION=\"ITU-T G.992.2 (G.LITE)\"\n");
+		default:
+			return sprintf(page, "MODULATION=\"unknown (%u)\"\n",
+						cxinf[CXINF_MODULATION]);
+		}
+
+	return 0;
+}
+
 /* the following three functions are stolen from drivers/usb/core/message.c */
 static void cxacru_blocking_completion(struct urb *urb)
 {
@@ -395,6 +536,8 @@ static void cxacru_poll_status(struct wo
 		goto reschedule;
 	}
 
+	memcpy(instance->cxinf_status, buf, sizeof(instance->cxinf_status));
+
 	if (instance->line_status == buf[CXINF_LINE_STATUS])
 		goto reschedule;
 
@@ -842,6 +985,7 @@ static struct usbatm_driver cxacru_drive
 	.heavy_init	= cxacru_heavy_init,
 	.unbind		= cxacru_unbind,
 	.atm_start	= cxacru_atm_start,
+	.proc_read	= cxacru_proc_read,
 	.bulk_in	= CXACRU_EP_DATA,
 	.bulk_out	= CXACRU_EP_DATA,
 	.rx_padding	= 3,
-- 
1.4.3.1


-- 
Simon Arlott (subscribed to lkml, don't CC)

Re: [PATCH 3/3] cxacru: Store all device status information and report it when atm_proc_read is called.

[Duncan Sands]

Hi Simon,

> +static int cxacru_proc_read(struct usbatm_data *usbatm_instance,
> +	struct atm_dev *atm_dev, loff_t * pos, char *page)
> +{
> +	struct cxacru_data *instance = usbatm_instance->driver_data;
> +	u32 *cxinf = instance->cxinf_status;
> +	int left = *pos;

if there was no successfull call to cxacru_poll_status before this,
then you will output junk.  Please initialize cxinf_status to something
sensible in cxacru_bind.

> +       if (!left--)
> +               return sprintf(page, "MAC=%02x:%02x:%02x:%02x:%02x:%02x\n",
> +                                       atm_dev->esi[0], atm_dev->esi[1],
> +                                       atm_dev->esi[2], atm_dev->esi[3],
> +                                       atm_dev->esi[4], atm_dev->esi[5]);

Rather than duplicating code in usbatm, I think it would be better to
split the various output bits in usbatm_atm_proc_read into their own
functions, and export them, so they can be called from both
usbatm_atm_proc_read and cxacru_proc_read.

Best wishes,

Duncan.

Re: [PATCH 3/3] cxacru: Store all device status information and report it when atm_proc_read is called.

[Andrew Morton]

On Tue, 30 Jan 2007 21:30:29 +0000
Simon Arlott <simon@arlott.org> wrote:

> +static int cxacru_proc_read(struct usbatm_data *usbatm_instance,
> +	struct atm_dev *atm_dev, loff_t * pos, char *page)
> +{
> +	struct cxacru_data *instance = usbatm_instance->driver_data;
> +	u32 *cxinf = instance->cxinf_status;
> +	int left = *pos;
> +
> +	if (!left--)
> +		return sprintf(page, "# %s\n", usbatm_instance->description);
> +
> +	if (!left--) {
> +		if (cxinf[CXINF_LINE_STATUS] == 5) {
> +			return sprintf(page, "# UP %u/%u\n",
> +				cxinf[CXINF_DOWNSTREAM_RATE],
> +				cxinf[CXINF_UPSTREAM_RATE]);
> +		} else {
> +			return sprintf(page, "# DOWN\n");
> +		}
> +	}

hm, how well-tested was this proc interface?  The pread() and lseek()
behaviour might be strange.

I guess as long as it doesn't oops, hang or anything like that then it'll
be OK.  Anyone who does anything apart from a single big-fat-read from a 
procfile has a good chance of getting into trouble :(

Re: [PATCH 3/3] cxacru: Store all device status information and report it when atm_proc_read is called.

[Duncan Sands]

On Thursday 1 February 2007 00:39:14 you wrote:
> On Tue, 30 Jan 2007 21:30:29 +0000
> Simon Arlott <simon@arlott.org> wrote:
> 
> > +static int cxacru_proc_read(struct usbatm_data *usbatm_instance,
> > +	struct atm_dev *atm_dev, loff_t * pos, char *page)
> > +{
> > +	struct cxacru_data *instance = usbatm_instance->driver_data;
> > +	u32 *cxinf = instance->cxinf_status;
> > +	int left = *pos;
> > +
> > +	if (!left--)
> > +		return sprintf(page, "# %s\n", usbatm_instance->description);
> > +
> > +	if (!left--) {
> > +		if (cxinf[CXINF_LINE_STATUS] == 5) {
> > +			return sprintf(page, "# UP %u/%u\n",
> > +				cxinf[CXINF_DOWNSTREAM_RATE],
> > +				cxinf[CXINF_UPSTREAM_RATE]);
> > +		} else {
> > +			return sprintf(page, "# DOWN\n");
> > +		}
> > +	}
> 
> hm, how well-tested was this proc interface?  The pread() and lseek()
> behaviour might be strange.
> 
> I guess as long as it doesn't oops, hang or anything like that then it'll
> be OK.  Anyone who does anything apart from a single big-fat-read from a 
> procfile has a good chance of getting into trouble :(

All the ATM drivers seem to do it like this.  That doesn't mean they are
right of course!  But I never saw anyone complain on the ATM mailing list.
On the other hand, why does Simon want this?  If he has written a user space
tool that extracts bits from the proc file (eg: to tell users what's going
on) then he could run into trouble, depending on how he implements it.

Ciao,

Duncan.

Re: [PATCH 3/3] cxacru: Store all device status information and report it when atm_proc_read is called.

[Simon Arlott]

[-- Attachment #1: Type: text/plain, Size: 2920 bytes --]

On 01/02/07 09:15, Duncan Sands wrote:
> On Thursday 1 February 2007 00:39:14 you wrote:
>> On Tue, 30 Jan 2007 21:30:29 +0000
>> Simon Arlott <simon@arlott.org> wrote:
>>
>>> +static int cxacru_proc_read(struct usbatm_data *usbatm_instance,
>>> +	struct atm_dev *atm_dev, loff_t * pos, char *page)
>>> +{
>>> +	struct cxacru_data *instance = usbatm_instance->driver_data;
>>> +	u32 *cxinf = instance->cxinf_status;
>>> +	int left = *pos;
>>> +
>>> +	if (!left--)
>>> +		return sprintf(page, "# %s\n", usbatm_instance->description);
>>> +
>>> +	if (!left--) {
>>> +		if (cxinf[CXINF_LINE_STATUS] == 5) {
>>> +			return sprintf(page, "# UP %u/%u\n",
>>> +				cxinf[CXINF_DOWNSTREAM_RATE],
>>> +				cxinf[CXINF_UPSTREAM_RATE]);
>>> +		} else {
>>> +			return sprintf(page, "# DOWN\n");
>>> +		}
>>> +	}
>> hm, how well-tested was this proc interface?  The pread() and lseek()
>> behaviour might be strange.
>>
>> I guess as long as it doesn't oops, hang or anything like that then it'll
>> be OK.  Anyone who does anything apart from a single big-fat-read from a 
>> procfile has a good chance of getting into trouble :(

Well, several reads of at least 172B... but that scenario works without any problems.

> All the ATM drivers seem to do it like this.  That doesn't mean they are
> right of course!  But I never saw anyone complain on the ATM mailing list.

I just copied it from usbatm; it looks like this could be done using seq_file instead.

> On the other hand, why does Simon want this?  If he has written a user space
> tool that extracts bits from the proc file (eg: to tell users what's going
> on) then he could run into trouble, depending on how he implements it.

No special user space tool, I just want other people to be able to get the same useful line information from cxacru that most ADSL driver have. (I do have a small "cxacru-watch" program which execs "cmd.up" etc. on status changes, but that is just sent printks via syslog-ng).


It looks like the weird proc interface prevents my intention to allow ". /proc/net/atm/cxacru\:0" to work and set $LINE_STATUS etc., at least with bash:
open("/proc/net/atm/cxacru:0", O_RDONLY|O_LARGEFILE) = 3
fstat64(3, {st_mode=S_IFREG|0444, st_size=0, ...}) = 0
read(3, "", 0)                          = 0
close(3)                                = 0

Does anyone have any thoughts on the output format? The standard usbatm output isn't going to be compatible with the above if a common MAC/AAL5 header were used.


When I have the time and can acquire another conexant usb modem for testing I'll fix the usbatm /proc interfaces. It would be also good if the module could reliably be reloaded - most (but not all) of the time it fails to initialise the device again.

There's still an issue with khubd going into a loop if the firmware doesn't exist or if the device is unplugged.

-- 
Simon Arlott


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 829 bytes --]