[PATCH 1/8] cxacru: return an empty value for modulation if there is no connection

[PATCH 1/8] cxacru: return an empty value for modulation if there is no connection

[Simon Arlott]

When there is no connection, return an empty string
instead of "0" for the connection modulation.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
 Documentation/networking/cxacru.txt |    1 +
 drivers/usb/atm/cxacru.c            |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/networking/cxacru.txt b/Documentation/networking/cxacru.txt
index b074681..3532cee 100644
--- a/Documentation/networking/cxacru.txt
+++ b/Documentation/networking/cxacru.txt
@@ -61,6 +61,7 @@ several sysfs attribute files for retrieving device statistics:
 * mac_address
 
 * modulation
+	"" (when not connected)
 	"ANSI T1.413"
 	"ITU-T G.992.1 (G.DMT)"
 	"ITU-T G.992.2 (G.LITE)"
diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index 56802d2..4a26a6c 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -267,12 +267,12 @@ static ssize_t cxacru_sysfs_showattr_LINE(u32 value, char *buf)
 static ssize_t cxacru_sysfs_showattr_MODU(u32 value, char *buf)
 {
 	static char *str[] = {
-			NULL,
+			"",
 			"ANSI T1.413",
 			"ITU-T G.992.1 (G.DMT)",
 			"ITU-T G.992.2 (G.LITE)"
 	};
-	if (unlikely(value >= ARRAY_SIZE(str) || str[value] == NULL))
+	if (unlikely(value >= ARRAY_SIZE(str)))
 		return snprintf(buf, PAGE_SIZE, "%u\n", value);
 	return snprintf(buf, PAGE_SIZE, "%s\n", str[value]);
 }
-- 
1.6.3.3

-- 
Simon Arlott

[PATCH 2/8] cxacru: check data length is not negative

[Simon Arlott]

When attempting to read data that is not actually
an array of values, the length may be negative
which causes an Oops due to a likely access off
the end of the data array.

This bug should not occur under normal use unless
the device returns an invalid response.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
 drivers/usb/atm/cxacru.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index 4a26a6c..8da4a06 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -596,7 +596,7 @@ static int cxacru_cm_get_array(struct cxacru_data *instance, enum cxacru_cm_requ
 	len = ret / 4;
 	for (offb = 0; offb < len; ) {
 		int l = le32_to_cpu(buf[offb++]);
-		if (l > stride || l > (len - offb) / 2) {
+		if (l < 0 || l > stride || l > (len - offb) / 2) {
 			if (printk_ratelimit())
 				usb_err(instance->usbatm, "invalid data length from cm %#x: %d\n",
 					cm, l);
-- 
1.6.3.3

-- 
Simon Arlott

[PATCH 3/8] cxacru: check device isn't being removed during sysfs calls

[Simon Arlott]

It is possible for usb_get_intfdata() to return NULL if
sysfs is accessed while the module is being unloaded or
the device is being removed.

Move the access code to inline functions in usbatm.h,
and return -ENODEV if any of the pointers is NULL.

It should not be possible for the instance data or atm
device to be invalid until after unbind() completes and
the sysfs attributes have been removed.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
 drivers/usb/atm/cxacru.c |   45 ++++++++++++++++++++++++++-------------------
 drivers/usb/atm/usbatm.c |    1 +
 drivers/usb/atm/usbatm.h |   30 ++++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index 8da4a06..f2e7414 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -200,9 +200,12 @@ static DEVICE_ATTR(_name, S_IWUSR | S_IRUGO, \
 static ssize_t cxacru_sysfs_show_##_name(struct device *dev, \
 	struct device_attribute *attr, char *buf) \
 { \
-	struct usb_interface *intf = to_usb_interface(dev); \
-	struct usbatm_data *usbatm_instance = usb_get_intfdata(intf); \
-	struct cxacru_data *instance = usbatm_instance->driver_data; \
+	struct cxacru_data *instance = to_usbatm_driver_data(\
+		to_usb_interface(dev)); \
+\
+	if (instance == NULL) \
+		return -ENODEV; \
+\
 	return cxacru_sysfs_showattr_##_type(instance->card_info[_value], buf); \
 } \
 CXACRU__ATTR_INIT(_name)
@@ -288,9 +291,11 @@ static ssize_t cxacru_sysfs_showattr_MODU(u32 value, char *buf)
 static ssize_t cxacru_sysfs_show_mac_address(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
-	struct usb_interface *intf = to_usb_interface(dev);
-	struct usbatm_data *usbatm_instance = usb_get_intfdata(intf);
-	struct atm_dev *atm_dev = usbatm_instance->atm_dev;
+	struct atm_dev *atm_dev = to_usbatm_atm_dev(
+			to_usb_interface(dev));
+
+	if (atm_dev == NULL)
+		return -ENODEV;
 
 	return snprintf(buf, PAGE_SIZE, "%pM\n", atm_dev->esi);
 }
@@ -298,12 +303,15 @@ static ssize_t cxacru_sysfs_show_mac_address(struct device *dev,
 static ssize_t cxacru_sysfs_show_adsl_state(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
-	struct usb_interface *intf = to_usb_interface(dev);
-	struct usbatm_data *usbatm_instance = usb_get_intfdata(intf);
-	struct cxacru_data *instance = usbatm_instance->driver_data;
-	u32 value = instance->card_info[CXINF_LINE_STARTABLE];
-
 	static char *str[] = { "running", "stopped" };
+	struct cxacru_data *instance = to_usbatm_driver_data(
+			to_usb_interface(dev));
+	u32 value;
+
+	if (instance == NULL)
+		return -ENODEV;
+
+	value = instance->card_info[CXINF_LINE_STARTABLE];
 	if (unlikely(value >= ARRAY_SIZE(str)))
 		return snprintf(buf, PAGE_SIZE, "%u\n", value);
 	return snprintf(buf, PAGE_SIZE, "%s\n", str[value]);
@@ -312,9 +320,8 @@ static ssize_t cxacru_sysfs_show_adsl_state(struct device *dev,
 static ssize_t cxacru_sysfs_store_adsl_state(struct device *dev,
 	struct device_attribute *attr, const char *buf, size_t count)
 {
-	struct usb_interface *intf = to_usb_interface(dev);
-	struct usbatm_data *usbatm_instance = usb_get_intfdata(intf);
-	struct cxacru_data *instance = usbatm_instance->driver_data;
+	struct cxacru_data *instance = to_usbatm_driver_data(
+			to_usb_interface(dev));
 	int ret;
 	int poll = -1;
 	char str_cmd[8];
@@ -328,13 +335,16 @@ static ssize_t cxacru_sysfs_store_adsl_state(struct device *dev,
 		return -EINVAL;
 	ret = 0;
 
+	if (instance == NULL)
+		return -ENODEV;
+
 	if (mutex_lock_interruptible(&instance->adsl_state_serialize))
 		return -ERESTARTSYS;
 
 	if (!strcmp(str_cmd, "stop") || !strcmp(str_cmd, "restart")) {
 		ret = cxacru_cm(instance, CM_REQUEST_CHIP_ADSL_LINE_STOP, NULL, 0, NULL, 0);
 		if (ret < 0) {
-			atm_err(usbatm_instance, "change adsl state:"
+			atm_err(instance->usbatm, "change adsl state:"
 				" CHIP_ADSL_LINE_STOP returned %d\n", ret);
 
 			ret = -EIO;
@@ -354,7 +364,7 @@ static ssize_t cxacru_sysfs_store_adsl_state(struct device *dev,
 	if (!strcmp(str_cmd, "start") || !strcmp(str_cmd, "restart")) {
 		ret = cxacru_cm(instance, CM_REQUEST_CHIP_ADSL_LINE_START, NULL, 0, NULL, 0);
 		if (ret < 0) {
-			atm_err(usbatm_instance, "change adsl state:"
+			atm_err(instance->usbatm, "change adsl state:"
 				" CHIP_ADSL_LINE_START returned %d\n", ret);
 
 			ret = -EIO;
@@ -649,9 +659,6 @@ static int cxacru_atm_start(struct usbatm_data *usbatm_instance,
 {
 	struct cxacru_data *instance = usbatm_instance->driver_data;
 	struct usb_interface *intf = usbatm_instance->usb_intf;
-	/*
-	struct atm_dev *atm_dev = usbatm_instance->atm_dev;
-	*/
 	int ret;
 	int start_polling = 1;
 
diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
index fbea856..4038043 100644
--- a/drivers/usb/atm/usbatm.c
+++ b/drivers/usb/atm/usbatm.c
@@ -1333,6 +1333,7 @@ void usbatm_usb_disconnect(struct usb_interface *intf)
 	if (instance->atm_dev) {
 		sysfs_remove_link(&instance->atm_dev->class_dev.kobj, "device");
 		atm_dev_deregister(instance->atm_dev);
+		instance->atm_dev = NULL;
 	}
 
 	usbatm_put_instance(instance);	/* taken in usbatm_usb_probe */
diff --git a/drivers/usb/atm/usbatm.h b/drivers/usb/atm/usbatm.h
index f6f4508..629b39c 100644
--- a/drivers/usb/atm/usbatm.h
+++ b/drivers/usb/atm/usbatm.h
@@ -204,4 +204,34 @@ struct usbatm_data {
 	struct urb *urbs[0];
 };
 
+static inline void *to_usbatm_driver_data(struct usb_interface *intf)
+{
+	struct usbatm_data *usbatm_instance;
+
+	if (intf == NULL)
+		return NULL;
+
+	usbatm_instance = usb_get_intfdata(intf);
+
+	if (usbatm_instance == NULL) /* set NULL before unbind() */
+		return NULL;
+
+	return usbatm_instance->driver_data; /* set NULL after unbind() */
+}
+
+static inline void *to_usbatm_atm_dev(struct usb_interface *intf)
+{
+	struct usbatm_data *usbatm_instance;
+
+	if (intf == NULL)
+		return NULL;
+
+	usbatm_instance = usb_get_intfdata(intf);
+
+	if (usbatm_instance == NULL) /* set NULL before unbind() */
+		return NULL;
+
+	return usbatm_instance->atm_dev; /* set NULL after unbind() */
+}
+
 #endif	/* _USBATM_H_ */
-- 
1.6.3.3

-- 
Simon Arlott

Re: [PATCH 3/8] cxacru: check device isn't being removed during sysfs calls

[Duncan Sands]

Hi Simon,

> +static inline void *to_usbatm_driver_data(struct usb_interface *intf)
> +{
> +	struct usbatm_data *usbatm_instance;
> +
> +	if (intf == NULL)
> +		return NULL;
> +
> +	usbatm_instance = usb_get_intfdata(intf);
> +
> +	if (usbatm_instance == NULL) /* set NULL before unbind() */
> +		return NULL;
> +
> +	return usbatm_instance->driver_data; /* set NULL after unbind() */
> +}
> +
> +static inline void *to_usbatm_atm_dev(struct usb_interface *intf)
> +{
> +	struct usbatm_data *usbatm_instance;
> +
> +	if (intf == NULL)
> +		return NULL;
> +
> +	usbatm_instance = usb_get_intfdata(intf);
> +
> +	if (usbatm_instance == NULL) /* set NULL before unbind() */
> +		return NULL;
> +
> +	return usbatm_instance->atm_dev; /* set NULL after unbind() */
> +}
> +

why not collapse these two into one that just returns usbatm_instance,
and have users extract the ->driver_data or ->atm_dev fields?

Ciao,

Duncan.

Re: [PATCH 3/8] cxacru: check device isn't being removed during sysfs calls

[Simon Arlott]

On 21/11/09 15:24, Duncan Sands wrote:
>> +static inline void *to_usbatm_driver_data(struct usb_interface *intf)
>> +static inline void *to_usbatm_atm_dev(struct usb_interface *intf)
> 
> why not collapse these two into one that just returns usbatm_instance,
> and have users extract the ->driver_data or ->atm_dev fields?

I didn't feel that the driver(s) should be accessing the fields from
usbatm_instance directly to get at atm_dev. Although this new atm_dev
function only has one caller, and cxacru already access atm_dev directly
in cxacru_poll_status() so I'll remove the second function and access
cxacru_data's usbatm pointer, which won't be NULL.

-- 
Simon Arlott

[PATCH 3/8 (v2)] cxacru: check device isn't being removed during sysfs calls

[Simon Arlott]

It is possible for usb_get_intfdata() to return NULL if
sysfs is accessed while the module is being unloaded or
the device is being removed.

Move the access code to an inline function in usbatm.h,
and return -ENODEV if any of the pointers are NULL.

It should not be possible for the instance data or atm
device to be invalid until after unbind() completes and
the sysfs attributes have been removed.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
 drivers/usb/atm/cxacru.c |   48 ++++++++++++++++++++++++++-------------------
 drivers/usb/atm/usbatm.c |    1 +
 drivers/usb/atm/usbatm.h |   15 ++++++++++++++
 3 files changed, 44 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index 8da4a06..4bead3d 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -200,9 +200,12 @@ static DEVICE_ATTR(_name, S_IWUSR | S_IRUGO, \
 static ssize_t cxacru_sysfs_show_##_name(struct device *dev, \
 	struct device_attribute *attr, char *buf) \
 { \
-	struct usb_interface *intf = to_usb_interface(dev); \
-	struct usbatm_data *usbatm_instance = usb_get_intfdata(intf); \
-	struct cxacru_data *instance = usbatm_instance->driver_data; \
+	struct cxacru_data *instance = to_usbatm_driver_data(\
+		to_usb_interface(dev)); \
+\
+	if (instance == NULL) \
+		return -ENODEV; \
+\
 	return cxacru_sysfs_showattr_##_type(instance->card_info[_value], buf); \
 } \
 CXACRU__ATTR_INIT(_name)
@@ -288,22 +291,28 @@ static ssize_t cxacru_sysfs_showattr_MODU(u32 value, char *buf)
 static ssize_t cxacru_sysfs_show_mac_address(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
-	struct usb_interface *intf = to_usb_interface(dev);
-	struct usbatm_data *usbatm_instance = usb_get_intfdata(intf);
-	struct atm_dev *atm_dev = usbatm_instance->atm_dev;
+	struct cxacru_data *instance = to_usbatm_driver_data(
+			to_usb_interface(dev));
 
-	return snprintf(buf, PAGE_SIZE, "%pM\n", atm_dev->esi);
+	if (instance == NULL || instance->usbatm->atm_dev == NULL)
+		return -ENODEV;
+
+	return snprintf(buf, PAGE_SIZE, "%pM\n",
+		instance->usbatm->atm_dev->esi);
 }
 
 static ssize_t cxacru_sysfs_show_adsl_state(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
-	struct usb_interface *intf = to_usb_interface(dev);
-	struct usbatm_data *usbatm_instance = usb_get_intfdata(intf);
-	struct cxacru_data *instance = usbatm_instance->driver_data;
-	u32 value = instance->card_info[CXINF_LINE_STARTABLE];
-
 	static char *str[] = { "running", "stopped" };
+	struct cxacru_data *instance = to_usbatm_driver_data(
+			to_usb_interface(dev));
+	u32 value;
+
+	if (instance == NULL)
+		return -ENODEV;
+
+	value = instance->card_info[CXINF_LINE_STARTABLE];
 	if (unlikely(value >= ARRAY_SIZE(str)))
 		return snprintf(buf, PAGE_SIZE, "%u\n", value);
 	return snprintf(buf, PAGE_SIZE, "%s\n", str[value]);
@@ -312,9 +321,8 @@ static ssize_t cxacru_sysfs_show_adsl_state(struct device *dev,
 static ssize_t cxacru_sysfs_store_adsl_state(struct device *dev,
 	struct device_attribute *attr, const char *buf, size_t count)
 {
-	struct usb_interface *intf = to_usb_interface(dev);
-	struct usbatm_data *usbatm_instance = usb_get_intfdata(intf);
-	struct cxacru_data *instance = usbatm_instance->driver_data;
+	struct cxacru_data *instance = to_usbatm_driver_data(
+			to_usb_interface(dev));
 	int ret;
 	int poll = -1;
 	char str_cmd[8];
@@ -328,13 +336,16 @@ static ssize_t cxacru_sysfs_store_adsl_state(struct device *dev,
 		return -EINVAL;
 	ret = 0;
 
+	if (instance == NULL)
+		return -ENODEV;
+
 	if (mutex_lock_interruptible(&instance->adsl_state_serialize))
 		return -ERESTARTSYS;
 
 	if (!strcmp(str_cmd, "stop") || !strcmp(str_cmd, "restart")) {
 		ret = cxacru_cm(instance, CM_REQUEST_CHIP_ADSL_LINE_STOP, NULL, 0, NULL, 0);
 		if (ret < 0) {
-			atm_err(usbatm_instance, "change adsl state:"
+			atm_err(instance->usbatm, "change adsl state:"
 				" CHIP_ADSL_LINE_STOP returned %d\n", ret);
 
 			ret = -EIO;
@@ -354,7 +365,7 @@ static ssize_t cxacru_sysfs_store_adsl_state(struct device *dev,
 	if (!strcmp(str_cmd, "start") || !strcmp(str_cmd, "restart")) {
 		ret = cxacru_cm(instance, CM_REQUEST_CHIP_ADSL_LINE_START, NULL, 0, NULL, 0);
 		if (ret < 0) {
-			atm_err(usbatm_instance, "change adsl state:"
+			atm_err(instance->usbatm, "change adsl state:"
 				" CHIP_ADSL_LINE_START returned %d\n", ret);
 
 			ret = -EIO;
@@ -649,9 +660,6 @@ static int cxacru_atm_start(struct usbatm_data *usbatm_instance,
 {
 	struct cxacru_data *instance = usbatm_instance->driver_data;
 	struct usb_interface *intf = usbatm_instance->usb_intf;
-	/*
-	struct atm_dev *atm_dev = usbatm_instance->atm_dev;
-	*/
 	int ret;
 	int start_polling = 1;
 
diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
index fbea856..4038043 100644
--- a/drivers/usb/atm/usbatm.c
+++ b/drivers/usb/atm/usbatm.c
@@ -1333,6 +1333,7 @@ void usbatm_usb_disconnect(struct usb_interface *intf)
 	if (instance->atm_dev) {
 		sysfs_remove_link(&instance->atm_dev->class_dev.kobj, "device");
 		atm_dev_deregister(instance->atm_dev);
+		instance->atm_dev = NULL;
 	}
 
 	usbatm_put_instance(instance);	/* taken in usbatm_usb_probe */
diff --git a/drivers/usb/atm/usbatm.h b/drivers/usb/atm/usbatm.h
index f6f4508..0863f85 100644
--- a/drivers/usb/atm/usbatm.h
+++ b/drivers/usb/atm/usbatm.h
@@ -204,4 +204,19 @@ struct usbatm_data {
 	struct urb *urbs[0];
 };
 
+static inline void *to_usbatm_driver_data(struct usb_interface *intf)
+{
+	struct usbatm_data *usbatm_instance;
+
+	if (intf == NULL)
+		return NULL;
+
+	usbatm_instance = usb_get_intfdata(intf);
+
+	if (usbatm_instance == NULL) /* set NULL before unbind() */
+		return NULL;
+
+	return usbatm_instance->driver_data; /* set NULL after unbind() */
+}
+
 #endif	/* _USBATM_H_ */
-- 
1.6.3.3

-- 
Simon Arlott

[PATCH 4/8] cxacru: document how to interact with the flash memory

[Simon Arlott]

These commands were found by accident... fortunately
it still works even if the flash memory is erased,
despite having no USB device IDs.

Some example sysfs code for raw command access:
  http://simon.arlott.org/pub/cxacru/raw.c

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
 drivers/usb/atm/cxacru.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index f2e7414..41568b1 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -105,6 +105,26 @@ enum cxacru_cm_request {
 	CM_REQUEST_MAX,
 };
 
+/* commands for interaction with the flash memory
+ * 
+ * read:  response is the contents of the first 60 bytes of flash memory
+ * write: request contains the 60 bytes of data to write to flash memory
+ *        response is the contents of the first 60 bytes of flash memory
+ *
+ * layout: PP PP VV VV  MM MM MM MM  MM MM ?? ??  SS SS SS SS  SS SS SS SS
+ *         SS SS SS SS  SS SS SS SS  00 00 00 00  00 00 00 00  00 00 00 00
+ *         00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00  00 00 00 00
+ *
+ *   P: le16  USB Product ID
+ *   V: le16  USB Vendor ID
+ *   M: be48  MAC Address
+ *   S: le16  ASCII Serial Number
+ */
+enum cxacru_cm_flash {
+	CM_FLASH_READ = 0xa1,
+	CM_FLASH_WRITE = 0xa2
+};
+
 /* reply codes to the commands above */
 enum cxacru_cm_status {
 	CM_STATUS_UNDEFINED,
-- 
1.6.3.3

-- 
Simon Arlott

[PATCH 5/8] cxacru: firmware writes on OHCI are slow, log progress

[Simon Arlott]

Firmware writing takes 256ms per 4KB with OHCI, which
is very slow compared to 7ms per 4KB with UHCI.

Until I have access to a hardware USB analyser it may
not be possible to determine why this happens.

Instead of appearing to do nothing, log progress when
writing firmware and then log the ATM device information
when finished. Remove an unnecessary 4 second delay.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
 drivers/usb/atm/cxacru.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index 41568b1..c6ec10e 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -724,6 +724,9 @@ static int cxacru_atm_start(struct usbatm_data *usbatm_instance,
 	mutex_unlock(&instance->poll_state_serialize);
 	mutex_unlock(&instance->adsl_state_serialize);
 
+	printk(KERN_INFO "%s%d: %s %pM\n", atm_dev->type, atm_dev->number,
+			usbatm_instance->description, atm_dev->esi);
+
 	if (start_polling)
 		cxacru_poll_status(&instance->poll_work.work);
 	return 0;
@@ -938,6 +941,7 @@ static void cxacru_upload_firmware(struct cxacru_data *instance,
 	}
 
 	/* Firmware */
+	usb_info(usbatm, "loading firmware\n");
 	ret = cxacru_fw(usb_dev, FW_WRITE_MEM, 0x2, 0x0, FW_ADDR, fw->data, fw->size);
 	if (ret) {
 		usb_err(usbatm, "Firmware upload failed: %d\n", ret);
@@ -946,6 +950,7 @@ static void cxacru_upload_firmware(struct cxacru_data *instance,
 
 	/* Boot ROM patch */
 	if (instance->modem_type->boot_rom_patch) {
+		usb_info(usbatm, "loading boot ROM patch\n");
 		ret = cxacru_fw(usb_dev, FW_WRITE_MEM, 0x2, 0x0, BR_ADDR, bp->data, bp->size);
 		if (ret) {
 			usb_err(usbatm, "Boot ROM patching failed: %d\n", ret);
@@ -960,6 +965,7 @@ static void cxacru_upload_firmware(struct cxacru_data *instance,
 		return;
 	}
 
+	usb_info(usbatm, "starting device\n");
 	if (instance->modem_type->boot_rom_patch) {
 		val = cpu_to_le32(BR_ADDR);
 		ret = cxacru_fw(usb_dev, FW_WRITE_MEM, 0x2, 0x0, BR_STACK_ADDR, (u8 *) &val, 4);
@@ -1003,8 +1009,6 @@ static void cxacru_upload_firmware(struct cxacru_data *instance,
 				return;
 			}
 		}
-
-	msleep_interruptible(4000);
 }
 
 static int cxacru_find_firmware(struct cxacru_data *instance,
-- 
1.6.3.3

-- 
Simon Arlott

[PATCH 6/8] cxacru: add write-only sysfs attribute for modem configuration

[Simon Arlott]

The modem can be configured using CM_REQUEST_CARD_DATA_SET,
although CM_REQUEST_CARD_DATA_GET does not return any data.

Tested by setting the modulation (0x0a) option.

There is a list of parameters in the following archive,
but the meaning of many of them is not well documented:
  http://sourceforge.net/project/shownotes.php?release_id=301825

This source also indicates that the highest parameter set
is 0x4a but this varies by model so an arbitrary limit of
0x7f has been used (the index is a 32-bit integer).

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
 Documentation/networking/cxacru.txt |    9 ++++
 drivers/usb/atm/cxacru.c            |   76 ++++++++++++++++++++++++++++++++++-
 2 files changed, 84 insertions(+), 1 deletions(-)

diff --git a/Documentation/networking/cxacru.txt b/Documentation/networking/cxacru.txt
index 3532cee..f4fbf4c 100644
--- a/Documentation/networking/cxacru.txt
+++ b/Documentation/networking/cxacru.txt
@@ -15,6 +15,15 @@ several sysfs attribute files for retrieving device statistics:
 * adsl_headend_environment
 	Information about the remote headend.
 
+* adsl_config
+	Configuration writing interface.
+	Write parameters in hexadecimal format <index>=<value>,
+	separated by whitespace, e.g.:
+		"1=0 a=5"
+	Up to 7 parameters at a time will be sent and the modem will restart
+	the ADSL connection when any value is set. These are logged for future
+	reference.
+
 * downstream_attenuation (dB)
 * downstream_bits_per_frame
 * downstream_rate (kbps)
diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index c6ec10e..4168574 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -52,6 +52,7 @@ static const char cxacru_driver_name[] = "cxacru";
 #define CXACRU_EP_DATA		0x02	/* Bulk in/out */
 
 #define CMD_PACKET_SIZE		64	/* Should be maxpacket(ep)? */
+#define CMD_MAX_CONFIG		((CMD_PACKET_SIZE / 4 - 1) / 2)
 
 /* Addresses */
 #define PLLFCLK_ADDR	0x00350068
@@ -216,6 +217,10 @@ static DEVICE_ATTR(_name, S_IRUGO, cxacru_sysfs_show_##_name, NULL)
 static DEVICE_ATTR(_name, S_IWUSR | S_IRUGO, \
 	cxacru_sysfs_show_##_name, cxacru_sysfs_store_##_name)
 
+#define CXACRU_SET_INIT(_name) \
+static DEVICE_ATTR(_name, S_IWUSR, \
+	NULL, cxacru_sysfs_store_##_name)
+
 #define CXACRU_ATTR_INIT(_value, _type, _name) \
 static ssize_t cxacru_sysfs_show_##_name(struct device *dev, \
 	struct device_attribute *attr, char *buf) \
@@ -232,10 +237,12 @@ CXACRU__ATTR_INIT(_name)
 
 #define CXACRU_ATTR_CREATE(_v, _t, _name) CXACRU_DEVICE_CREATE_FILE(_name)
 #define CXACRU_CMD_CREATE(_name)          CXACRU_DEVICE_CREATE_FILE(_name)
+#define CXACRU_SET_CREATE(_name)          CXACRU_DEVICE_CREATE_FILE(_name)
 #define CXACRU__ATTR_CREATE(_name)        CXACRU_DEVICE_CREATE_FILE(_name)
 
 #define CXACRU_ATTR_REMOVE(_v, _t, _name) CXACRU_DEVICE_REMOVE_FILE(_name)
 #define CXACRU_CMD_REMOVE(_name)          CXACRU_DEVICE_REMOVE_FILE(_name)
+#define CXACRU_SET_REMOVE(_name)          CXACRU_DEVICE_REMOVE_FILE(_name)
 #define CXACRU__ATTR_REMOVE(_name)        CXACRU_DEVICE_REMOVE_FILE(_name)
 
 static ssize_t cxacru_sysfs_showattr_u32(u32 value, char *buf)
@@ -437,6 +444,72 @@ static ssize_t cxacru_sysfs_store_adsl_state(struct device *dev,
 	return ret;
 }
 
+/* CM_REQUEST_CARD_DATA_GET times out, so no show attribute */
+
+static ssize_t cxacru_sysfs_store_adsl_config(struct device *dev,
+	struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct cxacru_data *instance = to_usbatm_driver_data(
+			to_usb_interface(dev));
+	int len = strlen(buf);
+	int ret, pos, num;
+	__le32 data[CMD_PACKET_SIZE / 4];
+
+	if (!capable(CAP_NET_ADMIN))
+		return -EACCES;
+
+	if (instance == NULL)
+		return -ENODEV;
+
+	pos = 0;
+	num = 0;
+	while (pos < len) {
+		int tmp;
+		u32 index;
+		u32 value;
+
+		ret = sscanf(buf + pos, "%x=%x%n", &index, &value, &tmp);
+		if (ret < 2)
+			return -EINVAL;
+		if (index < 0 || index > 0x7f)
+			return -EINVAL;
+		pos += tmp;
+
+		/* skip trailing newline */
+		if (buf[pos] == '\n' && pos == len-1)
+			pos++;
+
+		data[num * 2 + 1] = cpu_to_le32(index);
+		data[num * 2 + 2] = cpu_to_le32(value);
+		num++;
+
+		/* send config values when data buffer is full
+		 * or no more data
+		 */
+		if (pos >= len || num >= CMD_MAX_CONFIG) {
+			char log[CMD_MAX_CONFIG * 12 + 1]; /* %02x=%08x */
+
+			data[0] = cpu_to_le32(num);
+			ret = cxacru_cm(instance, CM_REQUEST_CARD_DATA_SET,
+				(u8 *) data, 4 + num * 8, NULL, 0);
+			if (ret < 0) {
+				atm_err(instance->usbatm,
+					"set card data returned %d\n", ret);
+				return -EIO;
+			}
+
+			for (tmp = 0; tmp < num; tmp++)
+				snprintf(log + tmp*12, 13, " %02x=%08x",
+					le32_to_cpu(data[tmp * 2 + 1]),
+					le32_to_cpu(data[tmp * 2 + 2]));
+			atm_info(instance->usbatm, "config%s\n", log);
+			num = 0;
+		}
+	}
+
+	return len;
+}
+
 /*
  * All device attributes are included in CXACRU_ALL_FILES
  * so that the same list can be used multiple times:
@@ -472,7 +545,8 @@ CXACRU_ATTR_##_action(CXINF_MODULATION,                MODU, modulation); \
 CXACRU_ATTR_##_action(CXINF_ADSL_HEADEND,              u32,  adsl_headend); \
 CXACRU_ATTR_##_action(CXINF_ADSL_HEADEND_ENVIRONMENT,  u32,  adsl_headend_environment); \
 CXACRU_ATTR_##_action(CXINF_CONTROLLER_VERSION,        u32,  adsl_controller_version); \
-CXACRU_CMD_##_action(                                        adsl_state);
+CXACRU_CMD_##_action(                                        adsl_state); \
+CXACRU_SET_##_action(                                        adsl_config);
 
 CXACRU_ALL_FILES(INIT);
 
-- 
1.6.3.3

-- 
Simon Arlott

[PATCH 7/8] cxacru: remove cxacru-cf.bin loader

[Simon Arlott]

This has never worked properly because wsize passed to
cxacru_cm() is incorrectly set to the number of values
instead of the data bytes. The maximum number of values
that can be set at once is 7 which means the device will
not get enough data to work with and none of the
configuration values will be used.

At least one existing cxacru-cf.bin file contains invalid
data which will prevent the modem from syncing properly.

Fixing it is likely to break existing systems, and the
new sysfs interface for setting configuration parameters
can provide the same functionality. A script is provided
to convert from the original format.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
 Documentation/networking/00-INDEX     |    2 +
 Documentation/networking/cxacru-cf.py |   48 +++++++++++++++++++++++++++++++++
 Documentation/networking/cxacru.txt   |    6 ++++
 drivers/usb/atm/cxacru.c              |   31 ++-------------------
 4 files changed, 59 insertions(+), 28 deletions(-)
 create mode 100644 Documentation/networking/cxacru-cf.py

diff --git a/Documentation/networking/00-INDEX b/Documentation/networking/00-INDEX
index 50189bf..fe5c099 100644
--- a/Documentation/networking/00-INDEX
+++ b/Documentation/networking/00-INDEX
@@ -32,6 +32,8 @@ cs89x0.txt
 	- the Crystal LAN (CS8900/20-based) Ethernet ISA adapter driver
 cxacru.txt
 	- Conexant AccessRunner USB ADSL Modem
+cxacru-cf.py
+	- Conexant AccessRunner USB ADSL Modem configuration file parser
 de4x5.txt
 	- the Digital EtherWORKS DE4?? and DE5?? PCI Ethernet driver
 decnet.txt
diff --git a/Documentation/networking/cxacru-cf.py b/Documentation/networking/cxacru-cf.py
new file mode 100644
index 0000000..b41d298
--- /dev/null
+++ b/Documentation/networking/cxacru-cf.py
@@ -0,0 +1,48 @@
+#!/usr/bin/env python
+# Copyright 2009 Simon Arlott
+#
+# This program is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by the Free
+# Software Foundation; either version 2 of the License, or (at your option)
+# any later version.
+#
+# This program is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+# more details.
+#
+# You should have received a copy of the GNU General Public License along with
+# this program; if not, write to the Free Software Foundation, Inc., 59
+# Temple Place - Suite 330, Boston, MA  02111-1307, USA.
+#
+# Usage: cxacru-cf.py < cxacru-cf.bin
+# Output: values string suitable for the sysfs adsl_config attribute
+#
+# Warning: cxacru-cf.bin with MD5 hash cdbac2689969d5ed5d4850f117702110
+# contains mis-aligned values which will stop the modem from being able
+# to make a connection. If the first and last two bytes are removed then
+# the values become valid, but the modulation will be forced to ANSI
+# T1.413 only which may not be appropriate.
+#
+# The original binary format is a packed list of le32 values.
+
+import sys
+import struct
+
+i = 0
+while True:
+	buf = sys.stdin.read(4)
+
+	if len(buf) == 0:
+		break
+	elif len(buf) != 4:
+		sys.stdout.write("\n")
+		sys.stderr.write("Error: read {0} not 4 bytes\n".format(len(buf)))
+		sys.exit(1)
+
+	if i > 0:
+		sys.stdout.write(" ")
+	sys.stdout.write("{0:x}={1}".format(i, struct.unpack("<I", buf)[0]))
+	i += 1
+
+sys.stdout.write("\n")
diff --git a/Documentation/networking/cxacru.txt b/Documentation/networking/cxacru.txt
index f4fbf4c..2cce044 100644
--- a/Documentation/networking/cxacru.txt
+++ b/Documentation/networking/cxacru.txt
@@ -4,6 +4,12 @@ While it is capable of managing/maintaining the ADSL connection without the
 module loaded, the device will sometimes stop responding after unloading the
 driver and it is necessary to unplug/remove power to the device to fix this.
 
+Note: support for cxacru-cf.bin has been removed. It was not loaded correctly
+so it had no effect on the device configuration. Fixing it could have stopped
+existing devices working when an invalid configuration is supplied.
+
+There is a script cxacru-cf.py to convert an existing file to the sysfs form.
+
 Detected devices will appear as ATM devices named "cxacru". In /sys/class/atm/
 these are directories named cxacruN where N is the device number. A symlink
 named device points to the USB interface device's directory which contains
diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index 4168574..7bd7d39 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -977,11 +977,9 @@ cleanup:
 
 static void cxacru_upload_firmware(struct cxacru_data *instance,
 				   const struct firmware *fw,
-				   const struct firmware *bp,
-				   const struct firmware *cf)
+				   const struct firmware *bp)
 {
 	int ret;
-	int off;
 	struct usbatm_data *usbatm = instance->usbatm;
 	struct usb_device *usb_dev = usbatm->usb_dev;
 	__le16 signature[] = { usb_dev->descriptor.idVendor,
@@ -1065,24 +1063,6 @@ static void cxacru_upload_firmware(struct cxacru_data *instance,
 		usb_err(usbatm, "modem failed to initialize: %d\n", ret);
 		return;
 	}
-
-	/* Load config data (le32), doing one packet at a time */
-	if (cf)
-		for (off = 0; off < cf->size / 4; ) {
-			__le32 buf[CMD_PACKET_SIZE / 4 - 1];
-			int i, len = min_t(int, cf->size / 4 - off, CMD_PACKET_SIZE / 4 / 2 - 1);
-			buf[0] = cpu_to_le32(len);
-			for (i = 0; i < len; i++, off++) {
-				buf[i * 2 + 1] = cpu_to_le32(off);
-				memcpy(buf + i * 2 + 2, cf->data + off * 4, 4);
-			}
-			ret = cxacru_cm(instance, CM_REQUEST_CARD_DATA_SET,
-					(u8 *) buf, len, NULL, 0);
-			if (ret < 0) {
-				usb_err(usbatm, "load config data failed: %d\n", ret);
-				return;
-			}
-		}
 }
 
 static int cxacru_find_firmware(struct cxacru_data *instance,
@@ -1108,7 +1088,7 @@ static int cxacru_find_firmware(struct cxacru_data *instance,
 static int cxacru_heavy_init(struct usbatm_data *usbatm_instance,
 			     struct usb_interface *usb_intf)
 {
-	const struct firmware *fw, *bp, *cf;
+	const struct firmware *fw, *bp;
 	struct cxacru_data *instance = usbatm_instance->driver_data;
 
 	int ret = cxacru_find_firmware(instance, "fw", &fw);
@@ -1126,13 +1106,8 @@ static int cxacru_heavy_init(struct usbatm_data *usbatm_instance,
 		}
 	}
 
-	if (cxacru_find_firmware(instance, "cf", &cf))		/* optional */
-		cf = NULL;
-
-	cxacru_upload_firmware(instance, fw, bp, cf);
+	cxacru_upload_firmware(instance, fw, bp);
 
-	if (cf)
-		release_firmware(cf);
 	if (instance->modem_type->boot_rom_patch)
 		release_firmware(bp);
 	release_firmware(fw);
-- 
1.6.3.3

-- 
Simon Arlott

[PATCH 8/8] cxacru: increment driver version

[Simon Arlott]

Changes:
  Return an empty string for modulation
    when there is no connection
  Fix sysfs unload race conditions
  Log firmware load process, remove delay
  Add new configuration interface
  Remove cxacru-cf.bin

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
 drivers/usb/atm/cxacru.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index 7bd7d39..4d80885 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -5,6 +5,7 @@
  *  Copyright (C) 2004 David Woodhouse, Duncan Sands, Roman Kagan
  *  Copyright (C) 2005 Duncan Sands, Roman Kagan (rkagan % mail ! ru)
  *  Copyright (C) 2007 Simon Arlott
+ *  Copyright (C) 2009 Simon Arlott
  *
  *  This program is free software; you can redistribute it and/or modify it
  *  under the terms of the GNU General Public License as published by the Free
@@ -43,7 +44,7 @@
 #include "usbatm.h"
 
 #define DRIVER_AUTHOR	"Roman Kagan, David Woodhouse, Duncan Sands, Simon Arlott"
-#define DRIVER_VERSION	"0.3"
+#define DRIVER_VERSION	"0.4"
 #define DRIVER_DESC	"Conexant AccessRunner ADSL USB modem driver"
 
 static const char cxacru_driver_name[] = "cxacru";
-- 
1.6.3.3

-- 
Simon Arlott