Hi Piotr,
Let's pick this thread up again, shall we.
I'm including our previous correspondence (including your patch) for
reference as I'm adding linux-usb as CC.
On Mon, Oct 25, 2010 at 01:06:43PM +0200, Piotr Isajew wrote:
> On Mon, Oct 25, 2010 at 10:29:13AM +0200, Johan Hovold wrote:
> > On Thu, Oct 14, 2010 at 07:34:52PM +0200, Piotr Isajew wrote:
> > > Recently I had a need to use devices from multiple vendors (each of
> > > which works with usbserial generic converter) at the same time on
> > > single machine. I would like to suggest a small modification to your
> > > generic converter code, to support that case. Please take a look at
> > > the attached patch.
> >
> > Thanks for your patch. It's a good thought, but I'd prefer to not add
> > this kind of functionality to the generic driver. The reason is that the
> > generic driver (with command line vid/pid) should only be used as last
> > resort and/or for testing purposes. If you have devices that work with
> > the generic driver then we should write specific drivers for them (which
> > in turn can use the generic implementations). This way, everyone can
> > benefit from the added support for new devices without having to specify
> > vids/pids when loading the generic driver module.
>
> In general I agree with you, but in my opinion this patch isn't
> against that policy, but only adds more flexibility in "last resort"
> cases.
>
> > What kind of devices are you using it with? What are their
> > VIDs/PIDs?
> >
> > I'd be happy to add support for these devices (unless you prefer doing
> > it yourself and sending us the patches).
>
> I'm currently using a system with ~20 usb modems. Those are Option
> Wireless (vid=0x0af0) devices - Icon 031 (pid=0xc031) and Icon 505
> (pid=0xd055) modems.
>
> Both are supported by hso driver which is present in mainstream kernel
> and works well if you have i.e. 1 modem in a system and use it for
> GPRS connections.
>
> I use my modems mostly for SMS traffic and hso driver used that way
> crashes under heavy load. When generic serial driver is used -
> everything works perfectly stable. Adding to this that Option
> officialy states that they don't provide any support for their devices
> under Linux I think we have a "last resort" case here.
I'm happy to hear that the generic driver is working as it should, but I
really think that what needs to be done is to fix the dedicated driver
for these devices (and that in itself is another argument against adding
the kind of functionality you're suggesting).
According to MAINTAINERS, hso is supposed to be maintained by Option
indeed (Jan Dumon <j.dumon@xxxxxxxxxx>). Is this perhaps no longer the
case?
[By the way, I stumbled over the disable_net module option for hso which
is supposed to be set if using the serial interface in the way you are.
I assume you're using this one?]
If the hso driver crashes this really needs to be fixed. Would you be
able to reproduce this and supply logs when running with debugging
enabled?
Finally, we also have the option usb-serial driver which integrates
better with the tty layer than hso. Perhaps this one could be used in
use-cases such as your own where the network interface isn't used at all?
What's your opinion on all of this Greg?
Thanks,
Johan
> > > --- drivers/usb/serial/generic.c.orig 2010-08-27 01:47:12.000000000 +0200
> > > +++ drivers/usb/serial/generic.c 2010-10-14 18:43:38.731000018 +0200
> > > @@ -31,16 +31,19 @@
> > > static int generic_probe(struct usb_interface *interface,
> > > const struct usb_device_id *id);
> > >
> > > -static __u16 vendor = 0x05f9;
> > > -static __u16 product = 0xffff;
> > > +#define USBSERIAL_GENERIC_MAX_IDS 8
> > > +static __u16 vendor[USBSERIAL_GENERIC_MAX_IDS+1] = {0x05f9};
> > > +static __u16 product[USBSERIAL_GENERIC_MAX_IDS+1] = {0xffff};
> > > +static int vcnt = 1;
> > > +static int pcnt = 1;
> > >
> > > -module_param(vendor, ushort, 0);
> > > -MODULE_PARM_DESC(vendor, "User specified USB idVendor");
> > > +module_param_array(vendor, ushort, &vcnt, 0);
> > > +MODULE_PARM_DESC(vendor, "User specified USB idVendors");
> > >
> > > -module_param(product, ushort, 0);
> > > -MODULE_PARM_DESC(product, "User specified USB idProduct");
> > > +module_param_array(product, ushort, &pcnt, 0);
> > > +MODULE_PARM_DESC(product, "User specified USB idProducts");
> > >
> > > -static struct usb_device_id generic_device_ids[2]; /* Initially all zeroes. */
> > > +static struct usb_device_id generic_device_ids[USBSERIAL_GENERIC_MAX_IDS+2]; /* Initially all zeroes. */
> > >
> > > /* we want to look at all devices, as the vendor/product id can change
> > > * depending on the command line argument */
> > > @@ -88,14 +91,18 @@
> > > int usb_serial_generic_register(int _debug)
> > > {
> > > int retval = 0;
> > > + int c=0,m=0;
> > >
> > > debug = _debug;
> > > #ifdef CONFIG_USB_SERIAL_GENERIC
> > > - generic_device_ids[0].idVendor = vendor;
> > > - generic_device_ids[0].idProduct = product;
> > > - generic_device_ids[0].match_flags =
> > > - USB_DEVICE_ID_MATCH_VENDOR | USB_DEVICE_ID_MATCH_PRODUCT;
> > > -
> > > + m = (vcnt < pcnt)?vcnt:pcnt;
> > > + if(m > USBSERIAL_GENERIC_MAX_IDS) m = USBSERIAL_GENERIC_MAX_IDS;
> > > + for(c = 0; c < m;c++) {
> > > + generic_device_ids[c].idVendor = vendor[c];
> > > + generic_device_ids[c].idProduct = product[c];
> > > + generic_device_ids[c].match_flags =
> > > + USB_DEVICE_ID_MATCH_VENDOR | USB_DEVICE_ID_MATCH_PRODUCT;
> > > + }
> > > /* register our generic driver with ourselves */
> > > retval = usb_serial_register(&usb_serial_generic_device);
> > > if (retval)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
