Hi all, Here's a patch that I have cleaned up for context only from Option that is a USB serial / network device all in one. I'd like to see this go into 2.6.26, so any review comments by anyone who wishes to review any portion of this would be greatly apprecited. Filip, can you send me some better information for the Kconfig text talking about what devices this driver is for, and any other information you wish to show there? thanks, greg k-h ----------- Subject: USB: add option hso driver This driver is for a number of different Option devices. TODO: - review tty layer interface - review USB interfaces - remove proc files and move to debugfs - review network interfaces - add better changelog information Cc: Andrew Bird <ajb@spheresystems.co.uk> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk> Cc: Filip Aben <f.aben@option.com> Cc: Paulius Zaleckas <paulius.zaleckas@teltonika.lt> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> --- drivers/net/usb/Kconfig | 10 drivers/net/usb/Makefile | 1 drivers/net/usb/hso.c | 3349 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 3360 insertions(+) create mode 100644 drivers/net/usb/hso.c --- a/drivers/net/usb/Kconfig +++ b/drivers/net/usb/Kconfig @@ -154,6 +154,16 @@ config USB_NET_AX8817X This driver creates an interface named "ethX", where X depends on what other networking devices you have in use. +config USB_HSO + tristate "Option USB High Speed Mobile Devices" + depends on USB + default n + help + Choose this option if you have an Option High Speed Mobile + device. + + To compile this driver as a module, choose M here: the + module will be called hso. config USB_NET_CDCETHER tristate "CDC Ethernet support (smart devices such as cable modems)" --- a/drivers/net/usb/Makefile +++ b/drivers/net/usb/Makefile @@ -6,6 +6,7 @@ obj-$(CONFIG_USB_CATC) += catc.o obj-$(CONFIG_USB_KAWETH) += kaweth.o obj-$(CONFIG_USB_PEGASUS) ...
With the latest patch I always get BUG when closing my serial terminal application. I have two different modems (GTM378 and GTM380) plugged at USB hub. Here is the dump: BUG: sleeping function called from invalid context at drivers/usb/core/urb.c:542 in_atomic():1, irqs_disabled():0 Pid: 30031, comm: gtkterm Not tainted 2.6.24.4-64.fc8 #1 Call Trace: [<ffffffff811b5e40>] usb_kill_urb+0x1a/0xf1 [<ffffffff81268512>] mutex_lock+0x1e/0x2f [<ffffffff810ab2a2>] fasync_helper+0x4a/0xcd [<ffffffff88308f57>] :hso:hso_stop_serial_device+0x85/0x99 [<ffffffff8830971f>] :hso:hso_serial_close+0xea/0x110 [<ffffffff8117bf26>] release_dev+0x212/0x618 [<ffffffff8108ec31>] free_pages_and_swap_cache+0x73/0x8f [<ffffffff8117c33d>] tty_release+0x11/0x1a [<ffffffff810a152c>] __fput+0xc2/0x18f [<ffffffff8109ed3f>] filp_close+0x5d/0x65 [<ffffffff8109ff0f>] sys_close+0x8d/0xca [<ffffffff8100c005>] tracesys+0xd5/0xda BUG: scheduling while atomic: gtkterm/30031/0x10000100 Pid: 30031, comm: gtkterm Not tainted 2.6.24.4-64.fc8 #1 Call Trace: [<ffffffff812677a2>] schedule+0x91/0x706 [<ffffffff81032f45>] __cond_resched+0x2d/0x55 [<ffffffff81267f0f>] cond_resched+0x2e/0x39 [<ffffffff811b5e45>] usb_kill_urb+0x1f/0xf1 [<ffffffff81268512>] mutex_lock+0x1e/0x2f [<ffffffff810ab2a2>] fasync_helper+0x4a/0xcd [<ffffffff88308f57>] :hso:hso_stop_serial_device+0x85/0x99 [<ffffffff8830971f>] :hso:hso_serial_close+0xea/0x110 [<ffffffff8117bf26>] release_dev+0x212/0x618 [<ffffffff8108ec31>] free_pages_and_swap_cache+0x73/0x8f [<ffffffff8117c33d>] tty_release+0x11/0x1a [<ffffffff810a152c>] __fput+0xc2/0x18f [<ffffffff8109ed3f>] filp_close+0x5d/0x65 [<ffffffff8109ff0f>] sys_close+0x8d/0xca [<ffffffff8100c005>] tracesys+0xd5/0xda --
Reworked MAC address generation from USB serial number Now it is very similar to the one done in the cdc_ether.c Filip: Does the dummy MAC address have to start with 0xFA?
Hopefully fixed last bug in MAC address generation. Patch against latest Greg unified patch. Don't use my previous patch.
Incremental patch. Converted ethtool ioctl to ethtool_ops. Paulius
Updated patch with the following change: In function hso_get_drvinfo instead of writing to tmp buffer write directly to fw_version. Paulius
You were faster. We need to coordinate. Which changes are you doing now? Regards Oliver --
Actually I am tuning my newly added hso_get_drvinfo... Instead of writing to tmp buffer to write directly to fw_version. Should I post incremental patch or replacement? I always get crash when closing serial terminal application. I will post dump in couple minutes. Paulius --
Does this fix it?
Regards
Oliver
Signed-off-by: Oliver Neukum <oneukum@suse.de>
---
--- linux-2.6.25-hso/drivers/net/usb/hso.c.alt3 2008-04-16 14:01:22.000000000 +0200
+++ linux-2.6.25-hso/drivers/net/usb/hso.c 2008-04-16 15:39:24.000000000 +0200
@@ -156,7 +156,7 @@ struct hso_shared_int {
struct usb_device *usb;
int use_count;
int ref_count;
- spinlock_t shared_int_lock;
+ struct mutex shared_int_lock;
};
struct hso_net {
@@ -2040,14 +2040,14 @@ static int hso_start_serial_device(struc
}
}
} else {
- spin_lock_bh(&serial->shared_int->shared_int_lock);
+ mutex_lock(&serial->shared_int->shared_int_lock);
if (!serial->shared_int->use_count) {
result =
hso_mux_submit_intr_urb(serial->shared_int,
- hso_dev->usb, GFP_ATOMIC);
+ hso_dev->usb, flags);
}
serial->shared_int->use_count++;
- spin_unlock_bh(&serial->shared_int->shared_int_lock);
+ mutex_unlock(&serial->shared_int->shared_int_lock);
}
return result;
@@ -2070,7 +2070,7 @@ static int hso_stop_serial_device(struct
usb_kill_urb(serial->rx_urb[i]);
if (serial->shared_int) {
- spin_lock_bh(&serial->shared_int->shared_int_lock);
+ mutex_lock(&serial->shared_int->shared_int_lock);
if (serial->shared_int->use_count &&
(--serial->shared_int->use_count == 0)) {
struct urb *urb;
@@ -2079,7 +2079,7 @@ static int hso_stop_serial_device(struct
if (urb)
usb_kill_urb(urb);
}
- spin_unlock_bh(&serial->shared_int->shared_int_lock);
+ mutex_unlock(&serial->shared_int->shared_int_lock);
}
return 0;
@@ -2352,10 +2352,10 @@ static void hso_free_serial_device(struc
hso_serial_common_free(serial);
if (serial->shared_int) {
- spin_lock_bh(&serial->shared_int->shared_int_lock);
+ mutex_lock(&serial->shared_int->shared_int_lock);
if (--serial-&...Applied, thanks. greg k-h --
Yes, it does! Thank you. Paulius --
Hi,
can you test this one as well. The method used to determine whether
a device is asleep is racy. This introduces a private test.
Regards
Oliver
---
--- linux-2.6.25hso/drivers/net/usb/hso.c.alt2 2008-04-17 14:15:32.000000000 +0200
+++ linux-2.6.25hso/drivers/net/usb/hso.c 2008-04-17 16:11:50.000000000 +0200
@@ -242,6 +242,7 @@ struct hso_device {
u8 is_active;
u8 suspend_disabled;
u8 usb_gone;
+ u8 sleeping;
struct work_struct async_get_intf;
struct work_struct async_put_intf;
@@ -252,6 +253,7 @@ struct hso_device {
struct device *dev;
struct kref ref;
struct mutex mutex;
+ spinlock_t lock;
/* TODO: Not sure about the ones below */
struct proc_dir_entry *ourproc;
@@ -2184,6 +2186,7 @@ static struct hso_device *hso_create_dev
hso_dev->interface = intf;
kref_init(&hso_dev->ref);
mutex_init(&hso_dev->mutex);
+ spin_lock_init(&hso_dev->lock);
INIT_WORK(&hso_dev->async_get_intf, async_get_intf);
INIT_WORK(&hso_dev->async_put_intf, async_put_intf);
@@ -2757,12 +2760,16 @@ static void async_put_intf(struct work_s
static int hso_get_activity(struct hso_device *hso_dev)
{
- if (hso_dev->usb->state == USB_STATE_SUSPENDED) {
+ unsigned long flags;
+
+ spin_lock_irqsave(&hso_dev->lock, flags);
+ if (hso_dev->sleeping) {
if (!hso_dev->is_active) {
hso_dev->is_active = 1;
schedule_work(&hso_dev->async_get_intf);
}
}
+ spin_unlock_irqrestore(&hso_dev->lock, flags);
if (hso_dev->usb->state != USB_STATE_CONFIGURED)
return -EAGAIN;
@@ -2774,22 +2781,32 @@ static int hso_get_activity(struct hso_d
static int hso_put_activity(struct hso_device *hso_dev)
{
- if (hso_dev->usb->state != USB_STATE_SUSPENDED) {
+ unsigned long flags;
+
+ spin_lock_irqsave(&hso_dev->lock, flags);
+ if (hso_dev->sleeping) {
if (hso_dev->is_active) {
hso_dev->is_active = 0;
schedule_work(&hso_dev->a...By setting level "auto" it didn't suspend... When I tried to set "suspend" to the level I got crash: hub 1-1.3:1.0: suspend error -16 Unable to handle kernel NULL pointer dereference at 000000000000004a RIP: [<ffffffff811ce082>] usb_kill_urb+0x28/0xf5 PGD 20c36067 PUD 2228e067 PMD 0 Oops: 0000 [1] SMP CPU 0 Modules linked in: hso(U) rfkill i915 drm autofs4 fuse sunrpc loop dm_multipath ipv6 snd_hda_intel parport_pc pcspkr parport e100 snd_seq_dummy r8169 mii i2c_i801 snd_seq_oss i2c_core snd_seq_midi_event snd_seq iTCO_wdt iTCO_vendor_support ftdi_sio snd_seq_device usbserial usblp snd_pcm_oss snd_mixer_oss snd_pcm snd_timer snd_page_alloc snd_hwdep snd soundcore button sr_mod sg cdrom dm_snapshot dm_zero dm_mirror dm_mod pata_acpi ata_generic ata_piix libata sd_mod scsi_mod ext3 jbd mbcache uhci_hcd ohci_hcd ehci_hcd Pid: 2920, comm: bash Not tainted 2.6.24.4-64.fc8debug #1 RIP: 0010:[<ffffffff811ce082>] [<ffffffff811ce082>] usb_kill_urb+0x28/0xf5 RSP: 0018:ffff81001fd8dd88 EFLAGS: 00010202 RAX: 0000000000000000 RBX: 0000000000000002 RCX: ffffffff88323780 RDX: ffff81001fd8dfd8 RSI: 000000000000021e RDI: ffffffff813868d6 RBP: ffff810032c37b78 R08: 000000011fd88848 R09: 0000000000000000 R10: ffff81001fd8dd68 R11: 0000000000000028 R12: 0000000000000002 R13: ffff810037f38000 R14: 0000000000000002 R15: 0000000000000001 FS: 00002aaaaaac7f50(0000) GS:ffffffff81407000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b CR2: 000000000000004a CR3: 00000000124f4000 CR4: 00000000000006a0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 Process bash (pid: 2920, threadinfo ffff81001fd8c000, task ffff81001fd88000) Stack: 0000000000000246 ffff81003319d248 ffff81002d486e98 0000000000000246 ffff81003319d250 ffff81003319d248 ffff81003319d250 ffff810032c37b68 ffff810032c37b78 ffffffff8831ce54 0000000000000002 ffff81003d0f99b0 Call Trace: [<fff...
Does this fix it?
Regards
Oliver
---
--- linux-2.6.25hso/drivers/net/usb/hso.c.alt3 2008-04-21 13:24:47.000000000 +0200
+++ linux-2.6.25hso/drivers/net/usb/hso.c 2008-04-21 13:25:18.000000000 +0200
@@ -2052,7 +2052,7 @@
}
if (serial->tx_urb)
- usb_kill_urb(serial->rx_urb[i]);
+ usb_kill_urb(serial->tx_urb);
if (serial->shared_int) {
mutex_lock(&serial->shared_int->shared_int_lock);
--Looks like it fixed this crash. I don't see anything on dmesg and /var/log/messages when I do echo "suspend" > power/level, but that is IMO another story. Paulius --
Did you compile with CONFIG_USB_DEBUG? Do you load usbcore from initrd? In that case you need to rebuild your initrd. Regards Oliver --
Actually I am using 2.6.24.4-64.fc8debug kernel. I failed to start my 2.6.25 compiled kernel, because of some mkinitrd incompatibilities with LVM (I hate that day when I installed Fedora on LVM partition :)) According to fedora config CONFIG_USB_DEBUG is enabled. Couldn't find usbcore on initrd image... Paulius --
Stupid question :) How to test it? How can I force it to suspend? I have never used any suspend on my PC :) Paulius --
This is runtime power management. It can save energy on any computer. You compile your kernel with CONFIG_USB_SUSPEND and to see something in syslog with CONFIG_USB_DEBUG. Plug in the device, locate its sysfs directory, cd into it and do echo "auto" >power/level After a few seconds of idleness the device and the hub it's connected to (if it's the only device at that hub) should suspend. You'll get a message about it in the kernel log. Regards Oliver --
read Documentation/usb/power-management.txt -----Original Message----- From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-owner@vger.kernel.org] On Behalf Of Paulius Zaleckas Sent: Thursday, April 17, 2008 2:47 PM To: linux-usb@vger.kernel.org Cc: netdev@vger.kernel.org Subject: Re: [RFC] Patch to option HSO driver to the kernel Stupid question :) How to test it? How can I force it to suspend? I have never used any suspend on my PC :) Paulius -- --
This patch removes a creative abuse of in_interrupt(). Locking
is always done with a mutex now. No need for atomic operations.
Signed-off-by: Oliver Neukum <oneukum@suse.de>
---
--- linux-2.6.25-rc7-work/drivers/net/usb/hso.c.alt2 2008-04-15 15:21:25.000000000 +0200
+++ linux-2.6.25-rc7-work/drivers/net/usb/hso.c 2008-04-15 15:29:17.000000000 +0200
@@ -2201,21 +2201,13 @@ static int hso_stop_serial_device(struct
return -ENODEV;
for (i = 0; i < serial->num_rx_urbs; i++) {
- if (serial->rx_urb[i]
- && (serial->rx_urb[i]->status == -EINPROGRESS)) {
- if (in_interrupt())
- usb_unlink_urb(serial->rx_urb[i]);
- else
+ if (serial->rx_urb[i])
usb_kill_urb(serial->rx_urb[i]);
- }
}
- if (serial->tx_urb && (serial->tx_urb->status == -EINPROGRESS)) {
- if (in_interrupt())
- usb_unlink_urb(serial->tx_urb);
- else
- usb_kill_urb(serial->rx_urb[i]);
- }
+ if (serial->tx_urb)
+ usb_kill_urb(serial->rx_urb[i]);
+
if (serial->shared_int) {
spin_lock_bh(&serial->shared_int->shared_int_lock);
if (serial->shared_int->use_count &&
@@ -2223,12 +2215,8 @@ static int hso_stop_serial_device(struct
struct urb *urb;
urb = serial->shared_int->shared_intr_urb;
- if ((urb) && (urb->status == -EINPROGRESS)) {
- if (in_interrupt())
- usb_unlink_urb(urb);
- else
- usb_kill_urb(urb);
- }
+ if (urb)
+ usb_kill_urb(urb);
}
spin_unlock_bh(&serial->shared_int->shared_int_lock);
}
--Very nice, thanks for the change, applied. greg k-h --
The driver implements an ioctl to disable autosuspend. This seems unnecessary to me, as you can do that via sysfs, too. Is this included to work around permission problems? Can somebody explain? Regards Oliver --
This is added to support card firmware upgraders who will need to disable suspend, even on (embedded) systems that don't have sysfs mounted. Regards, Filip- --
How do they enable autosuspend without sysfs? Regards Oliver --
Good point. No idea. I suppose it's easier to rely on something that will always work, rather then making assumptions about suspend based on whether sysfs is mounted or not. ( Wasn't there a kernel recently that had autosuspend enabled by default ? ) Regards, Filip- --
Like I always like to point out, my _phone_ has sysfs mounted, I really doubt any 2.6 embedded system would not have it mounted these days, it is very useful for things like this. So I'll go delete that "special" ioctl, that's not a good thing to do, use the common interfaces that all USB devices rely on instead, don't do something different for just one type of USB device. I'm also a bit concerned about the special "set radio" ioctl as well. Filip, what uses that ioctl? If it's really necessary, can't a sysfs file do the same thing? thanks, greg k-h --
Do you have a new unified patch? I'd work against that, lest we get merge troubles. Regards Oliver --
Good idea :) Here it is. thanks, greg k-h ---------------- Subject: USB: add option hso driver This driver is for a number of different Option devices. Originally written by Option and Andrew Bird, but cleaned up massivly for acceptance into mainline by me (Greg). TODO: - remove proc files and move to debugfs - review network interfaces - add better changelog information Many thanks to the following for their help in cleaning up the driver by providing feedback and patches to it: - Paulius Zaleckas <paulius.zaleckas@teltonika.lt> - Oliver Neukum <oliver@neukum.org> - Alan Cox <alan@lxorguk.ukuu.org.uk> Cc: Andrew Bird <ajb@spheresystems.co.uk> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk> Cc: Filip Aben <f.aben@option.com> Cc: Paulius Zaleckas <paulius.zaleckas@teltonika.lt> Cc: Oliver Neukum <oliver@neukum.org> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> --- drivers/net/usb/Kconfig | 10 drivers/net/usb/Makefile | 1 drivers/net/usb/hso.c | 3246 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 3257 insertions(+) create mode 100644 drivers/net/usb/hso.c --- a/drivers/net/usb/Kconfig +++ b/drivers/net/usb/Kconfig @@ -154,6 +154,16 @@ config USB_NET_AX8817X This driver creates an interface named "ethX", where X depends on what other networking devices you have in use. +config USB_HSO + tristate "Option USB High Speed Mobile Devices" + depends on USB + default n + help + Choose this option if you have an Option HSDPA/HSUPA card. + These cards support downlink speeds of 7.2Mbps or greater. + + To compile this driver as a module, choose M here: the + module will be called hso. config USB_NET_CDCETHER tristate "CDC Ethernet support (smart devices such as cable modems)" --- a/drivers/net/usb/Makefile +++ b/drivers/net/usb/Makefile @@ -6,6 +6,7 @@ obj-$(CONFIG_USB_CATC) += catc.o obj-$(CONFIG_USB_KAWETH) += kaweth.o obj-$(CONFI...
Another one.
As autosuspend is by default disabled, it makes no sense to have a module
parameter disabling it. Just don't enable it.
Regards
Oliver
Signed-off-by: Oliver Neukum <oneukum@suse.de>
---
--- linux-2.6.25hso/drivers/net/usb/hso.c.alt 2008-04-17 14:00:09.000000000 +0200
+++ linux-2.6.25hso/drivers/net/usb/hso.c 2008-04-17 14:01:43.000000000 +0200
@@ -400,7 +400,6 @@ static int debug;
static int procfs = 1;
static int tty_major;
static int disable_net;
-static int enable_autopm;
/* driver info */
static const char driver_name[] = "hso";
@@ -555,9 +554,6 @@ static int hso_proc_options(char *buf, c
len +=
snprintf(buf + len, count - len, "tty_major: 0x%02x\n", tty_major);
len +=
- snprintf(buf + len, count - len, "enable_autopm: 0x%02x\n",
- enable_autopm);
- len +=
snprintf(buf + len, count - len, "disable_net: 0x%02x\n",
disable_net);
return len;
@@ -2718,11 +2714,6 @@ static int hso_probe(struct usb_interfac
/* save our data pointer in this device */
usb_set_intfdata(interface, hso_dev);
- if (enable_autopm == 0) {
- usb_autopm_get_interface(hso_dev->interface);
- hso_dev->suspend_disabled = 1;
- }
-
/* done */
return 0;
@@ -3213,8 +3204,3 @@ module_param(tty_major, int, S_IRUGO | S
/* set the major tty number (eg: insmod hso.ko tty_major=245) */
MODULE_PARM_DESC(disable_net, "Disable the network interface");
module_param(disable_net, int, S_IRUGO | S_IWUSR);
-
-/* enable autosuspend (default disabled) (eg: insmod hso.ko enable_autopm=1) */
-MODULE_PARM_DESC(enable_autopm,
- "Enable the auto power managment (autosuspend)");
-module_param(enable_autopm, int, S_IRUGO | S_IWUSR);
--Applied, thanks. greg k-h --
Against that one.
Change GFP_KERNEL used in interrupt and resume() to GFP_ATOMIC,
respectively GFP_NOIO
Signed-off-by: Oliver Neukum <oneukum@suse.de>
---
--- a/drivers/net/usb/hso.c 2008-04-15 21:48:10.000000000 +0200
+++ b/drivers/net/usb/hso.c 2008-04-15 21:52:48.000000000 +0200
@@ -1663,7 +1663,7 @@ static void intr_callback(struct urb *ur
}
}
/* Resubmit interrupt urb */
- hso_mux_submit_intr_urb(shared_int, urb->dev, GFP_KERNEL);
+ hso_mux_submit_intr_urb(shared_int, urb->dev, GFP_ATOMIC);
}
/* called for writing to muxed serial port */
@@ -1694,7 +1694,7 @@ static int hso_std_serial_write_data(str
serial->tx_data, serial->tx_data_count,
hso_std_serial_write_bulk_callback, serial);
- result = usb_submit_urb(serial->tx_urb, GFP_KERNEL);
+ result = usb_submit_urb(serial->tx_urb, GFP_ATOMIC);
if (result) {
dev_warn(&serial->parent->usb->dev,
"Failed to submit urb - res %d\n", result);
@@ -2040,7 +2040,7 @@ static int hso_start_net_device(struct h
/* Put it out there so the device can send us stuff */
result = usb_submit_urb(hso_net->mux_bulk_rx_urb_pool[i],
- GFP_KERNEL);
+ GFP_NOIO);
if (result)
dev_warn(&hso_dev->usb->dev,
"%s failed mux_bulk_rx_urb[%d] %d\n", __func__,
@@ -2104,7 +2104,7 @@ static int hso_start_serial_device(struc
if (!serial->shared_int->use_count) {
result =
hso_mux_submit_intr_urb(serial->shared_int,
- hso_dev->usb, GFP_KERNEL);
+ hso_dev->usb, GFP_ATOMIC);
}
serial->shared_int->use_count++;
spin_unlock_bh(&serial->shared_int->shared_int_lock);
--Thanks, now applied. greg k-h --
On Tue, 15 Apr 2008 12:00:30 -0700 More work left: - Use netif_msg_ for the message level rather than module parameter - net_device_stats are now available in dev->stats - use ethtool_ops rather than ioctl --
Thanks, I've added them to the TODO list. greg k-h --
Currently I don't think anybody uses it for Linux. I added it because our non-Linux drivers had this functionality. It's used for example by our connection managers/tools to disable the RF-part when the laptop goes to sleep ( but keeps the USB bus powered&suspended ). This is a requirement for quite some operators. But yes, I suppose the same could be done through sysfs. Regards, Filip- --
No, the correct way to do this is to support rfkill. I'll have a look at that. Regards Oliver --
This incremental patch fixes a race where write can mess with
an URB while it isn't yet returned.
Regards
Oliver
Signed-off-by: Oliver Neukum <oneukum@suse.de>
---
--- linux-2.6.25-rc7-work/drivers/net/usb/hso.c.alt 2008-04-15 13:59:21.000000000 +0200
+++ linux-2.6.25-rc7-work/drivers/net/usb/hso.c 2008-04-15 14:44:45.000000000 +0200
@@ -220,6 +220,7 @@ struct hso_serial {
unsigned long flags;
u8 rts_state;
u8 dtr_state;
+ int tx_urb_used:1;
/* from usb_serial_port */
struct tty_struct *tty;
@@ -1521,12 +1522,13 @@ static void hso_kick_transmit(struct hso
{
u8 *temp = NULL;
unsigned long flags;
+ int res;
spin_lock_irqsave(&serial->serial_lock, flags);
if (!serial->tx_buffer_count)
goto out;
- if (serial->tx_urb->status == -EINPROGRESS)
+ if (serial->tx_urb_used)
goto out;
/* Wakeup USB interface if necessary */
@@ -1540,10 +1542,13 @@ static void hso_kick_transmit(struct hso
serial->tx_data_count = serial->tx_buffer_count;
serial->tx_buffer_count = 0;
-out:
/* If temp is set, it means we switched buffers */
- if (temp && serial->write_data)
- serial->write_data(serial);
+ if (temp && serial->write_data) {
+ res = serial->write_data(serial);
+ if (res >= 0)
+ serial->tx_urb_used = 1;
+ }
+out:
spin_unlock_irqrestore(&serial->serial_lock, flags);
}
@@ -1783,6 +1788,9 @@ static void ctrl_callback(struct urb *ur
if (!serial)
return;
+ spin_lock(&serial->serial_lock);
+ serial->tx_urb_used = 0;
+ spin_unlock(&serial->serial_lock);
if (status) {
log_usb_status(status, __func__);
return;
@@ -1914,6 +1922,9 @@ static void hso_std_serial_write_bulk_ca
return;
}
+ spin_lock(&serial->serial_lock);
+ serial->tx_urb_used = 0;
+ spin_unlock(&serial->serial_lock);
if (status) {
log_usb_status(status, __func__);
return;
--Applied to the driver, thanks. greg k-h --
Bitfields should be "unsigned", I've changed this so sparse does not complain. thanks, greg k-h --
Hi,
this patch against Greg's version with Pauliaus patch applied
- uses correct CDC includes and constants
- fixes a race between disconnect and open
- fixes a race between probe and open
- corrects incorrect uses of GFP_KERNEL
- adds some error handling in open
- fixes races in access to urb->status
There's still a race condition in the write path left and the autosuspend
handling is broken in extremely interesting ways. The next patch will fix
these and I am still doing further reviews.
Regards
Oliver
Signed-off-by: Oliver Neukum <oneukum@suse.de>
---
--- linux-2.6.25-rc7-vanilla/drivers/net/usb/hso.c 2008-04-15 13:33:25.000000000 +0200
+++ linux-2.6.25-rc7-work/drivers/net/usb/hso.c 2008-04-15 13:07:21.000000000 +0200
@@ -60,6 +60,7 @@
#include <linux/ip.h>
#include <linux/proc_fs.h>
#include <linux/uaccess.h>
+#include <linux/usb/cdc.h>
#include <net/arp.h>
#include <asm/byteorder.h>
@@ -94,25 +95,10 @@
#define HSO_NET_TX_TIMEOUT (HZ*10)
-#define SEND_ENCAPSULATED_COMMAND 0x00
-#define GET_ENCAPSULATED_RESPONSE 0x01
-
/* Serial port defines and structs. */
#define HSO_THRESHOLD_THROTTLE (7*1024)
#define HSO_THRESHOLD_UNTHROTTLE (2*1024)
-/* These definitions used in the Ethernet Packet Filtering requests */
-/* See CDC Spec Table 62 */
-#define MODE_FLAG_PROMISCUOUS (1<<0)
-#define MODE_FLAG_ALL_MULTICAST (1<<1)
-#define MODE_FLAG_DIRECTED (1<<2)
-#define MODE_FLAG_BROADCAST (1<<3)
-#define MODE_FLAG_MULTICAST (1<<4)
-
-/* CDC Spec class requests - CDC Spec Table 46 */
-#define SET_ETHERNET_MULTICAST_FILTER 0x40
-#define SET_ETHERNET_PACKET_FILTER 0x43
-
#define HSO_SERIAL_FLAG_THROTTLED 0
#define HSO_SERIAL_FLAG_TX_SENT 1
#define HSO_SERIAL_FLAG_RX_SENT 2
@@ -262,6 +248,7 @@ struct hso_device {
struct device *dev;
struct kref ref;
+ struct mutex mutex;
/* TODO: Not sure about the ones below */
struct proc_dir_entry *o...Thanks a lot for these changes, I've applied them to the driver. greg k-h --
In attached patch I have rewrote MAC address generation to simple call to random_ether_addr instead of some mysterious pointer parsing :) Paulius
Could you please generate a kernel patch, not a patch against a patch? I have difficulties dealing with that. Regards Oliver --
OK. Here it goes. Paulius
Thanks, I've merged these changes in. greg k-h --
Just some obvious things to reduce the source code hopping for the review and to reduce the source code size. Regards, Please move the MODULE_OWNER(), MODULE_LICENSE(), etc. macros from the This is a real forward declaration hell. Please try to reorder the please move the module_param() stuff and MODULE_PARM_DESC() from the end This debug macro looks like it is from the very beginning of the Should all these functions be removed (and therefor not set in --
I've already stripped down a lot of them, will work on the remaining Yes. thanks for the review. greg k-h --
Not for the compiler :)
But when you are able to put things together that helps to understand
the code much better (at least for me). I'm not a wizard so when it
helps me to find code belonging together it probably helps other people
Hm - i found one by grep after 'hex2' in drivers/net/cxgb3/t3_hw.c :
static unsigned int hex2int(unsigned char c)
{
return isdigit(c) ? c - '0' : toupper(c) - 'A' + 10;
}
Same as stated above. If you would put the MODULE_PARM_DESC() right here
the formerly defined variables are documented in one go without any /*
Why not? If it is not really needed it should be omitted.
In general i'm a friend of debug-code, but when i touches the mainline
Sorry. Didn't catch that.
Regards,
Oliver
--I will be the first to admit that I'm picking a nit here, but... Would it be possible to pick one of "High Speed Option" and "Option... High Speed" and then be consistent with the acronym of "hso" or "ohs"? Or maybe call it "option_hs" or something even more clear? Mixing them just makes it more difficult for people to use grep and google. Matt --=20 Matthew Dharm Home: mdharm-usb@one-eyed-alien.= net=20 Maintainer, Linux USB Mass Storage Driver DP: And judging from the scores, Stef has the sma... =20 T: LET'S NOT GO THERE! -- Dust Puppy and Tanya User Friendly, 12/11/1997
Hi Matthew, I'd prefer the keep it as just 'hso' and change the textual description if necessary. I think module names are just arbitrary, and should not necessarily reflect the original usage too well, otherwise you end up with a situation like with the 'option' module. That module is now used for many devices from differing manufacturers and it doesn't really make much sense to load the option module to service a device from Novatel, Sierra Wireless or Huawei. Just my two pence, Andrew --
Ok, that's fine with me, what should we use for the text in the driver and the Kconfig entry? thanks, greg k-h --
" This driver is used for Option HSDPA/HSUPA cards that support downlink speeds of 7.2Mbps or greater. " How does that sound ? Regards, Filip- --
Sounds good, I've updated the Kconfig entry with it. thanks, greg k-h --
| Karl Meyer | PROBLEM: 2.6.23-rc "NETDEV WATCHDOG: eth0: transmit timed out" |
| David Miller | Slow DOWN, please!!! |
| Mark Fasheh | [PATCH 0/39] Ocfs2 updates for 2.6.28 |
| Bart Van Assche | Integration of SCST in the mainstream Linux kernel |
git: | |
| Shawn O. Pearce | Re: pack operation is thrashing my server |
| Pierre Habouzit | git send-email improvements |
| Matthieu Moy | git push to a non-bare repository |
| Shawn O. Pearce | libgit2 - a true git library |
| Elad Efrat | Integrating securelevel and kauth(9) |
| Hubert Feyrer | Re: Compressed vnd handling tested successfully |
| Lord Isildur | Re: Fork bomb protection patch |
| Matt Thomas | Re: FFS journal |
| Will Maier | cron doesn't run commands in /etc/crontab? |
| Richard Stallman | Real men don't attack straw men |
| Harald Dunkel | Re: Packet Filter: how to keep device names on hardware failure? |
| Jordi Espasa Clofent | Resolving dependencies with pkg_add |
| Question on swap as ramdisk partition | 1 hour ago | Linux kernel |
| Netfilter kernel module | 11 hours ago | Linux kernel |
| serial driver xmit problem | 14 hours ago | Linux kernel |
| Why Windows is better than Linux | 14 hours ago | Linux general |
| How can I see my kernel messages in vt12? | 21 hours ago | Linux kernel |
| Grub | 1 day ago | Linux general |
| vmalloc_fault handling in x86_64 | 1 day ago | Linux kernel |
| epoll_wait()ing on epoll FD | 1 day ago | Linux kernel |
| Framebuffer in x86_64 causes problems to multiseat | 1 day ago | Linux kernel |
| Difference between 2.4 and 2.6 regarding thread creation | 2 days ago | Linux general |
