Re: [PATCH] Add support for JULABO PRESTO to cdc_acm

[Fabio Luongo <f.langufo.l@gmail.com>]

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