[PATCH] USB speedtouch: better proc info

[PATCH] USB speedtouch: better proc info

[Duncan Sands]

Output the correct device name, show the state of the device (for debugging) and of the
ADSL line (anyone want to write a graphical utility to show this, like under windows?).  We
no longer consult the usb_device struct in udsl_atm_proc_read, so don't take a reference
to it.  Against Greg's current 2.5 USB tree.


 speedtouch.c |   68 ++++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 58 insertions(+), 10 deletions(-)


--- 1.62/drivers/usb/misc/speedtouch.c	Fri Feb 21 10:21:00 2003
+++ 1.64/drivers/usb/misc/speedtouch.c	Mon Feb 24 10:17:09 2003
@@ -156,6 +156,7 @@
 
 	/* usb device part */
 	struct usb_device *usb_dev;
+	char description [64];
 	int firmware_loaded;
 
 	/* atm device part */
@@ -698,8 +699,6 @@
 
 	dbg ("udsl_atm_dev_close: killing tasklet");
 	tasklet_kill (&instance->send_tasklet);
-	dbg ("udsl_atm_dev_close: freeing USB device");
-	usb_put_dev (instance->usb_dev);
 	dbg ("udsl_atm_dev_close: freeing instance");
 	kfree (instance);
 }
@@ -722,8 +721,10 @@
 	}
 
 	if (!left--)
-		return sprintf (page, "SpeedTouch USB %s-%s (%02x:%02x:%02x:%02x:%02x:%02x)\n",
-				instance->usb_dev->bus->bus_name, instance->usb_dev->devpath,
+		return sprintf (page, "%s\n", instance->description);
+
+	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]);
 
@@ -735,6 +736,30 @@
 				atomic_read (&atm_dev->stats.aal5.rx_err),
 				atomic_read (&atm_dev->stats.aal5.rx_drop));
 
+	if (!left--) {
+		switch (atm_dev->signal) {
+		case ATM_PHY_SIG_FOUND:
+			sprintf (page, "Line up");
+			break;
+		case ATM_PHY_SIG_LOST:
+			sprintf (page, "Line down");
+			break;
+		default:
+			sprintf (page, "Line state unknown");
+			break;
+		}
+
+		if (instance->usb_dev) {
+			if (!instance->firmware_loaded)
+				strcat (page, ", no firmware\n");
+			else
+				strcat (page, ", firmware loaded\n");
+		} else
+			strcat (page, ", disconnected\n");
+
+		return strlen (page);
+	}
+
 	return 0;
 }
 
@@ -867,7 +892,8 @@
 	struct udsl_instance_data *instance;
 	unsigned char mac_str [13];
 	unsigned char mac [6];
-	int i;
+	int i, length;
+	char *buf;
 
 	dbg ("Trying device with Vendor=0x%x, Product=0x%x, ifnum %d",
 		dev->descriptor.idVendor, dev->descriptor.idProduct, ifnum);
@@ -962,7 +988,7 @@
 
 	instance->atm_dev->ci_range.vpi_bits = ATM_CI_MAX;
 	instance->atm_dev->ci_range.vci_bits = ATM_CI_MAX;
-	instance->atm_dev->signal = ATM_PHY_SIG_LOST;
+	instance->atm_dev->signal = ATM_PHY_SIG_UNKNOWN;
 
 	/* tmp init atm device, set to 128kbit */
 	instance->atm_dev->link_rate = 128 * 1000 / 424;
@@ -976,14 +1002,34 @@
 
 	memcpy (instance->atm_dev->esi, mac, 6);
 
-	wmb ();
+	/* device description */
+	buf = instance->description;
+	length = sizeof (instance->description);
+
+	if ((i = usb_string (dev, dev->descriptor.iProduct, buf, length)) < 0)
+		goto finish;
+
+	buf += i;
+	length -= i;
+
+	i = snprintf (buf, length, " (");
+	buf += i;
+	length -= i;
+
+	if (length <= 0 || (i = usb_make_path (dev, buf, length)) < 0)
+		goto finish;
 
+	buf += i;
+	length -= i;
+
+	snprintf (buf, length, ")");
+
+finish:
+	/* ready for ATM callbacks */
 	instance->atm_dev->dev_data = instance;
 
 	usb_set_intfdata (intf, instance);
 
-	usb_get_dev (dev);
-
 	return 0;
 
 fail:
@@ -1116,8 +1162,10 @@
 	for (i = 0; i < UDSL_NUMBER_SND_BUFS; i++)
 		kfree (instance->all_buffers[i].base);
 
+	instance->usb_dev = NULL;
+
 	/* atm finalize */
-	shutdown_atm_dev (instance->atm_dev);
+	shutdown_atm_dev (instance->atm_dev); /* frees instance */
 }
 
 

Re: [PATCH] USB speedtouch: better proc info

[Oliver Neukum]

Am Montag, 24. Februar 2003 10:58 schrieb Duncan Sands:
> Output the correct device name, show the state of the device (for
> debugging) and of the ADSL line (anyone want to write a graphical utility
> to show this, like under windows?).  We no longer consult the usb_device
> struct in udsl_atm_proc_read, so don't take a reference to it.  Against
> Greg's current 2.5 USB tree.

First of all, let me say that you're doing wonders with this driver.
But this particular patch I don't like. It improves stuff that should
be removed. More specifically:

1. Does anything prevent you from using the medium detection
hooks the network layer provides?
2. What need is there to export manufacturer id and mac address
again?
3. Doesn't the rest belong into sysfs rather than procfs?

	Regards
		Oliver

Re: [PATCH] USB speedtouch: better proc info

[Duncan Sands]

On Monday 24 February 2003 11:43, Oliver Neukum wrote:
> Am Montag, 24. Februar 2003 10:58 schrieb Duncan Sands:
> > Output the correct device name, show the state of the device (for
> > debugging) and of the ADSL line (anyone want to write a graphical utility
> > to show this, like under windows?).  We no longer consult the usb_device
> > struct in udsl_atm_proc_read, so don't take a reference to it.  Against
> > Greg's current 2.5 USB tree.
>
> First of all, let me say that you're doing wonders with this driver.
> But this particular patch I don't like. It improves stuff that should
> be removed. More specifically:
>
> 1. Does anything prevent you from using the medium detection
> hooks the network layer provides?
> 2. What need is there to export manufacturer id and mac address
> again?
> 3. Doesn't the rest belong into sysfs rather than procfs?

[Note to Chas: the speedtouch is a USB/ATM modem.  The driver
lurks in drivers/usb/misc/speedtouch.c]

Hi Oliver, thanks for your comments.  While I agree with you in
principle, I disagree in practice.  The driver exports the following
information in /proc/net/atm/speedtch:

(1) name and location of the USB device
(2) MAC address (serial number)
(3) AAL5 transmission statistics
(4) Line status
(5) Modem status

(1) is needed in order to work out which modem corresponds to
which ATM device.  This should be dealt with using sysfs, however
the ATM layer has not yet been ported to sysfs.  Until it is, this
seems like the best way to export this information.

(2) and (3) are redundant - they are published by the ATM layer
in other proc files.  I thought about removing them, but decided
against it because (a) it can be convenient having everything in
one proc file, and (b) it is backwards compatible with the 2.4
out-of-kernel driver.  They could go.

You suggested (in a private mail) using netif_carrier_on/off to
export (4).  The ATM layer already has a method for reporting this,
and I use it: set the ATM_PHY_SIG_FOUND/LOST bits in
atm_dev->signal.  The problem is that the ATM layer doesn't do
anything with this info (like export it to user space).  So I think
it is fair enough to export it in the proc file while waiting for the
ATM layer to be fixed.

As for (5), this could be exported using sysfs.  Since it is a
USB matter, I guess I could do this now.  So this could also go.

All the best,

Duncan.

Re: [PATCH] USB speedtouch: better proc info

[Oliver Neukum]

> Hi Oliver, thanks for your comments.  While I agree with you in
> principle, I disagree in practice.  The driver exports the following
> information in /proc/net/atm/speedtch:
>
> (1) name and location of the USB device
> (2) MAC address (serial number)
> (3) AAL5 transmission statistics
> (4) Line status
> (5) Modem status
>
> (1) is needed in order to work out which modem corresponds to
> which ATM device.  This should be dealt with using sysfs, however
> the ATM layer has not yet been ported to sysfs.  Until it is, this
> seems like the best way to export this information.

For the time being I agree.

> (2) and (3) are redundant - they are published by the ATM layer
> in other proc files.  I thought about removing them, but decided
> against it because (a) it can be convenient having everything in
> one proc file, and (b) it is backwards compatible with the 2.4
> out-of-kernel driver.  They could go.

As long as you have a proc file at all, you may as well print them, IMHO.

> You suggested (in a private mail) using netif_carrier_on/off to
> export (4).  The ATM layer already has a method for reporting this,
> and I use it: set the ATM_PHY_SIG_FOUND/LOST bits in
> atm_dev->signal.  The problem is that the ATM layer doesn't do
> anything with this info (like export it to user space).  So I think
> it is fair enough to export it in the proc file while waiting for the
> ATM layer to be fixed.

Yes, but I think that you should notify the network layer too.
I see no reason any network driver shouldn't report this directly.
There's nothing specific to ATM in losing signal.
It's purely physical thing low level drivers should deal with.

> As for (5), this could be exported using sysfs.  Since it is a
> USB matter, I guess I could do this now.  So this could also go.

Yes.

	Regards
		Oliver

Re: [PATCH] USB speedtouch: better proc info

[Duncan Sands]

Hi Oliver,

On Tuesday 25 February 2003 21:54, Oliver Neukum wrote:
> > Hi Oliver, thanks for your comments.  While I agree with you in
> > principle, I disagree in practice.  The driver exports the following
> > information in /proc/net/atm/speedtch:
> >
> > (1) name and location of the USB device
> > (2) MAC address (serial number)
> > (3) AAL5 transmission statistics
> > (4) Line status
> > (5) Modem status
> >
> > (1) is needed in order to work out which modem corresponds to
> > which ATM device.  This should be dealt with using sysfs, however
> > the ATM layer has not yet been ported to sysfs.  Until it is, this
> > seems like the best way to export this information.
>
> For the time being I agree.
>
> > (2) and (3) are redundant - they are published by the ATM layer
> > in other proc files.  I thought about removing them, but decided
> > against it because (a) it can be convenient having everything in
> > one proc file, and (b) it is backwards compatible with the 2.4
> > out-of-kernel driver.  They could go.
>
> As long as you have a proc file at all, you may as well print them, IMHO.
>
> > You suggested (in a private mail) using netif_carrier_on/off to
> > export (4).  The ATM layer already has a method for reporting this,
> > and I use it: set the ATM_PHY_SIG_FOUND/LOST bits in
> > atm_dev->signal.  The problem is that the ATM layer doesn't do
> > anything with this info (like export it to user space).  So I think
> > it is fair enough to export it in the proc file while waiting for the
> > ATM layer to be fixed.
>
> Yes, but I think that you should notify the network layer too.
> I see no reason any network driver shouldn't report this directly.
> There's nothing specific to ATM in losing signal.
> It's purely physical thing low level drivers should deal with.

I can't notify the network layer because there is no net_device I can
get hold of.

> > As for (5), this could be exported using sysfs.  Since it is a
> > USB matter, I guess I could do this now.  So this could also go.
>
> Yes.

Shall I ask Greg to apply the patch?

All the best,

Duncan.