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 <[email protected]>
---
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)
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.
On Tue, 30 Jan 2007 21:30:29 +0000
Simon Arlott <[email protected]> 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 :(
On Thursday 1 February 2007 00:39:14 you wrote:
> On Tue, 30 Jan 2007 21:30:29 +0000
> Simon Arlott <[email protected]> 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.
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 <[email protected]> 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