| Message ID | 20240927134404.110284-1-f.langufo.l@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for JULABO PRESTO to cdc_acm | expand |
On Fri, Sep 27, 2024 at 03:44:04PM +0200, Fabio Luongo wrote: > JULABO PRESTO chillers on Windows use the usbser.sys driver > for communication, so the same functionality should be achievable > on Linux using the cdc_acm driver. > > However, cdc_acm does not accomodate the quirkness of these devices, > as they fail normal probing ("Zero length descriptor references"), > but they also feature a single USB interface instead of two. > > This patch extends the effect of the `NO_UNION_NORMAL` quirk > to cover the features of JULABO PRESTO devices. > > Signed-off-by: Fabio Luongo <f.langufo.l@gmail.com> > --- > drivers/usb/class/cdc-acm.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c > index 605fea461102..d77c84c6e878 100644 > --- a/drivers/usb/class/cdc-acm.c > +++ b/drivers/usb/class/cdc-acm.c > @@ -1210,6 +1210,8 @@ static int acm_probe(struct usb_interface *intf, > if (quirks == NO_UNION_NORMAL) { > data_interface = usb_ifnum_to_if(usb_dev, 1); > control_interface = usb_ifnum_to_if(usb_dev, 0); > + if (!data_interface) > + data_interface = control_interface; That feels wrong, how can we send data out both for different things? > /* we would crash */ > if (!data_interface || !control_interface) > return -ENODEV; > @@ -1284,6 +1286,8 @@ static int acm_probe(struct usb_interface *intf, > if (data_intf_num != call_intf_num) > dev_dbg(&intf->dev, "Separate call control interface. That is not fully supported.\n"); > > +skip_normal_probe: > + > if (control_interface == data_interface) { > /* some broken devices designed for windows work this way */ > dev_warn(&intf->dev,"Control and data interfaces are not separated!\n"); > @@ -1303,8 +1307,6 @@ static int acm_probe(struct usb_interface *intf, > goto made_compressed_probe; > } > > -skip_normal_probe: Why the movement of the goto tag? thanks, greg k-h
On Wed, Oct 16, 2024 at 10:11:15AM +0200, Greg KH wrote: > On Fri, Sep 27, 2024 at 03:44:04PM +0200, Fabio Luongo wrote: > > JULABO PRESTO chillers on Windows use the usbser.sys driver > > for communication, so the same functionality should be achievable > > on Linux using the cdc_acm driver. > > > > However, cdc_acm does not accomodate the quirkness of these devices, > > as they fail normal probing ("Zero length descriptor references"), > > but they also feature a single USB interface instead of two. > > > > This patch extends the effect of the `NO_UNION_NORMAL` quirk > > to cover the features of JULABO PRESTO devices. > > > > Signed-off-by: Fabio Luongo <f.langufo.l@gmail.com> > > --- > > drivers/usb/class/cdc-acm.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c > > index 605fea461102..d77c84c6e878 100644 > > --- a/drivers/usb/class/cdc-acm.c > > +++ b/drivers/usb/class/cdc-acm.c > > @@ -1210,6 +1210,8 @@ static int acm_probe(struct usb_interface *intf, > > if (quirks == NO_UNION_NORMAL) { > > data_interface = usb_ifnum_to_if(usb_dev, 1); > > control_interface = usb_ifnum_to_if(usb_dev, 0); > > + if (!data_interface) > > + data_interface = control_interface; > > That feels wrong, how can we send data out both for different things? My understanding is that we still have the correct number of (i.e. 3) endpoints as in the case of properly implemented CDC devices, except they all belong to the same interface, instead of being split across two, so it should only be a matter of identifying which EP is for control and which EPs are for data. Indeed, I think this is what the current driver does via the call to `usb_find_common_endpoints`. > > > /* we would crash */ > > if (!data_interface || !control_interface) > > return -ENODEV; > > @@ -1284,6 +1286,8 @@ static int acm_probe(struct usb_interface *intf, > > if (data_intf_num != call_intf_num) > > dev_dbg(&intf->dev, "Separate call control interface. That is not fully supported.\n"); > > > > +skip_normal_probe: > > + > > if (control_interface == data_interface) { > > /* some broken devices designed for windows work this way */ > > dev_warn(&intf->dev,"Control and data interfaces are not separated!\n"); > > @@ -1303,8 +1307,6 @@ static int acm_probe(struct usb_interface *intf, > > goto made_compressed_probe; > > } > > > > -skip_normal_probe: > > Why the movement of the goto tag? Since `NO_UNION_NORMAL` allows for collapsed interfaces for data and control after these changes, the label was moved to the `if` that stands just above its current position, where the case `control_interface == data_interface` is handled. As a general comment, my understanding is that these changes should not affect the devices which the driver already supports: the `data_interface = control_interface` assignment is done only as a last attempt to save a probing that would fail with ENODEV; the extra `if` from the `goto` label movement should not get executed by the currently supported devices, as they should have distinct interfaces. Thanks, Fabio L > > thanks, > > greg k-h
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c index 605fea461102..d77c84c6e878 100644 --- a/drivers/usb/class/cdc-acm.c +++ b/drivers/usb/class/cdc-acm.c @@ -1210,6 +1210,8 @@ static int acm_probe(struct usb_interface *intf, if (quirks == NO_UNION_NORMAL) { data_interface = usb_ifnum_to_if(usb_dev, 1); control_interface = usb_ifnum_to_if(usb_dev, 0); + if (!data_interface) + data_interface = control_interface; /* we would crash */ if (!data_interface || !control_interface) return -ENODEV; @@ -1284,6 +1286,8 @@ static int acm_probe(struct usb_interface *intf, if (data_intf_num != call_intf_num) dev_dbg(&intf->dev, "Separate call control interface. That is not fully supported.\n"); +skip_normal_probe: + if (control_interface == data_interface) { /* some broken devices designed for windows work this way */ dev_warn(&intf->dev,"Control and data interfaces are not separated!\n"); @@ -1303,8 +1307,6 @@ static int acm_probe(struct usb_interface *intf, goto made_compressed_probe; } -skip_normal_probe: - /*workaround for switched interfaces */ if (data_interface->cur_altsetting->desc.bInterfaceClass != USB_CLASS_CDC_DATA) { if (control_interface->cur_altsetting->desc.bInterfaceClass == USB_CLASS_CDC_DATA) { @@ -1787,6 +1789,9 @@ static const struct usb_device_id acm_ids[] = { { USB_DEVICE(0x0572, 0x1349), /* Hiro (Conexant) USB MODEM H50228 */ .driver_info = NO_UNION_NORMAL, /* has no union descriptor */ }, + { USB_DEVICE(0x233c, 0x7633), /* JULABO PRESTO */ + .driver_info = NO_UNION_NORMAL, + }, { USB_DEVICE(0x20df, 0x0001), /* Simtec Electronics Entropy Key */ .driver_info = QUIRK_CONTROL_LINE_STATE, }, { USB_DEVICE(0x2184, 0x001c) }, /* GW Instek AFG-2225 */
JULABO PRESTO chillers on Windows use the usbser.sys driver for communication, so the same functionality should be achievable on Linux using the cdc_acm driver. However, cdc_acm does not accomodate the quirkness of these devices, as they fail normal probing ("Zero length descriptor references"), but they also feature a single USB interface instead of two. This patch extends the effect of the `NO_UNION_NORMAL` quirk to cover the features of JULABO PRESTO devices. Signed-off-by: Fabio Luongo <f.langufo.l@gmail.com> --- drivers/usb/class/cdc-acm.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)