Re: [Line6linux-devel] POD HD500x working

[Hans P. M. <hm...@gm...>]

What do you mean with "release of workqueue" ?? The podhd_disconnect
function?

The other two (claim interface in and sysfs out) I think it might by solved
with creating a new if in podhd_init funtion:


static int podhd_init(struct usb_line6 *line6,
              const struct usb_device_id *id)
{
    int err;
    struct usb_line6_podhd *pod = (struct usb_line6_podhd *) line6;
    struct usb_interface *intf;

    line6->disconnect = podhd_disconnect;

    if (pod->line6.properties->capabilities & LINE6_CAP_CONTROL) {
        /* claim the data interface */
        intf = usb_ifnum_to_if(line6->usbdev,
                    pod->line6.properties->ctrl_if);
        if (!intf) {
            dev_err(pod->line6.ifcdev, "interface %d not found\n",
                pod->line6.properties->ctrl_if);
            return -ENODEV;
        }

        err = usb_driver_claim_interface(&podhd_driver, intf, NULL);
        if (err != 0) {
            dev_err(pod->line6.ifcdev, "can't claim interface %d, error
%d\n",
                pod->line6.properties->ctrl_if, err);
            return err;
        }
+    }

+    if (pod->line6.properties->capabilities & LINE6_CAP_CONTROL_INIT) {
        /* create sysfs entries: */
        err = snd_card_add_dev_attr(line6->card, &podhd_dev_attr_group);
        if (err < 0)
            return err;
    }

    if (pod->line6.properties->capabilities & LINE6_CAP_PCM) {
        /* initialize PCM subsystem: */
        err = line6_init_pcm(line6,
            (id->driver_info == LINE6_PODX3 ||
            id->driver_info == LINE6_PODX3LIVE) ? &podx3_pcm_properties :
            &podhd_pcm_properties);
        if (err < 0)
            return err;
    }

-    if (!(pod->line6.properties->capabilities & LINE6_CAP_CONTROL)) {
+    if (!(pod->line6.properties->capabilities & LINE6_CAP_CONTROL_INIT)) {
        /* register USB audio system directly */
        return podhd_startup_finalize(pod);
    }

    /* init device and delay registering */
    init_timer(&pod->startup_timer);
    INIT_WORK(&pod->startup_work, podhd_startup_workqueue);
    podhd_startup(pod);
    return 0;
}


That's what you where thinking? I haven't tested it, I will do as soon as I
can.

On Mon, Jun 12, 2017 at 1:46 AM, Andrej Kruták <de...@an...> wrote:

> Hi there,
>
> On Jun 12, 2017 12:30 AM, "Hans Peter Möller" <hm...@gm...> wrote:
>
> You were right, I forgot to test if disabling the initialization
> worked. I tested and it worked, the "receive length failed (error
> -11)" error disapear.
> I actually change all of the LINE6_CAP_CONTROL to
> LINE6_CAP_CONTROL_INIT and it worked. I don't know in 4.12 but in 4.10
> there is used in
>
> static void podhd_disconnect(struct usb_line6 *line6)
> {
>     struct usb_line6_podhd *pod = (struct usb_line6_podhd *)line6;
>
> /*    if (pod->line6.properties->capabilities & LINE6_CAP_CONTROL) {*/
>     if (pod->line6.properties->capabilities & LINE6_CAP_CONTROL_INIT) {
>         struct usb_interface *intf;
>
>         del_timer_sync(&pod->startup_timer);
>         cancel_work_sync(&pod->startup_work);
>
>         intf = usb_ifnum_to_if(line6->usbdev,
>                     pod->line6.properties->ctrl_if);
>         usb_driver_release_interface(&podhd_driver, intf);
>     }
> }
>
> and in podhd_init is also this:
>
> if (pod->line6.properties->capabilities & LINE6_CAP_CONTROL_INIT) {
>         /* claim the data interface */
>         intf = usb_ifnum_to_if(line6->usbdev,
>                     pod->line6.properties->ctrl_if);
>         if (!intf) {
>             dev_err(pod->line6.ifcdev, "interface %d not found\n",
>                 pod->line6.properties->ctrl_if);
>             return -ENODEV;
>         }
>
>         err = usb_driver_claim_interface(&podhd_driver, intf, NULL);
>         if (err != 0) {
>             dev_err(pod->line6.ifcdev, "can't claim interface %d, error
> %d\n",
>                 pod->line6.properties->ctrl_if, err);
>             return err;
>         }
>
>         /* create sysfs entries: */
>         err = snd_card_add_dev_attr(line6->card, &podhd_dev_attr_group);
>         if (err < 0)
>             return err;
>     }
>
> It's better to change both of the to LINE6_CAP_CONTROL_INIT?
>
> I found out that the 500x need the LINE6_CAP_CONTROL in driver.c, in
> the probe (maybe also in suspend and resume).
>
>
>
> Hmm, looking at the code, I think we'll have to refactor it a bit
> more. If you want to do it - the usb_claim/release needs to be done
> always for CAP_CONTROL. For !CONTROL_INIT, we should skip the init and
> release of workqueue and sysfs stuff. I can create the patch too and
> send it to you for testing, if you want. Don't worry, you won't lose
> credits :-)
>
>
>
> MIDI:
> I added the LINE6_CAP_CONTROL_MIDI to my pod and also added the
> midi_init procedure in podhd.c, aplaymidi -l sees the midi ports of my
> pod, but my system freezes (apparenly when my pod sends data), so
> there's more to do here.
>
> Could I do midi via the quirck? Or one device should only has one driver?
>
>
> As I wrote, the hw interface likely doesn't send midi since +/- pod
> x3. It's some proprietary binary protocol instead, so getting it to
> work is not trivial. At very least, you'd have to write a translation
> layer, but I reckon such task would be better suited for a userspace
> app.
>
>
>
> Anyway, I will wrap-up to submit the patch with audio only. I will
> read teh link you send me on how to do it.
>
> When you send me the info of how to do it, you meant this?
> https://www.kernel.org/doc/html/v4.11/process/submitting-patches.html
> because I search there for line6 and didn't found anything.
>
>
> Yup..
>
> The generic alsa-devel mailing list is ok (see MAINTAINERS file, SOUND
> section), just check the recent commit log of the driver and put a few
> relevant people on cc. You can send me the patch first if you want,
> for a quick pre-review (I'd just check that we understood each other
> and you didn't break POD X3 :) ).
>
> Regarding POD HD X, it probably depends on how sure you are that it'll
> work. If lsusb is the same, it's probably safe to assume it and add.
> If you don't have even lsusb, I'd go the safe way for now.
>
> In any case, split the patch into at least two (add CONTROL_INIT &
> pod500x(+hdx) support).
>
>
> --
> Greetings,
>
> Andrej
>
>
> On Sun, Jun 11, 2017 at 2:42 AM, Andrej Kruták <de...@an...> wrote:
> >
> > Hey there,
> >
> > On Sun, Jun 11, 2017 at 4:07 AM, Hans Peter Möller <hm...@gm...>
> wrote:
> > > Hi Andrej,
> > >    I believe I will have to sniff to get rid of that error.
> > >
> >
> > Well, if you insist... :-) I guess since it works without the
> > initialization, and noone really needs the device serial number, you
> > could for now just disable the code. But of course sniffing is more
> > educational and systematic...
> >
> >
> > > Is there a reason why in kernel 4.10 the podhd.c has no midi init? Now
> I
> > > don't see the pod midi ports, I remember that in some of my other tries
> > > (kernel 4.4 or 4.8 or quirck) I could see the midi ports of my pod in
> > > qjacktl connections (but I never test them).
> > > How is it in Kernel 4.12? How they init midi? (I tried to find the
> tree in
> > > inet but I couldn't get it). Maybe they drop midi at all.
> > >
> >
> > Perhaps you did it with the quirk thing. But the podhd driver, AFAIK,
> > never supported MIDI. Maybe pod hdNNN devices support it, but t least
> > POD X3 doesn't have a native midi interface (compared to previous
> > models) --> you would have to emulate it via the bulk binary protocol
> > somehow. Check LINE6_CAP_CONTROL_MIDI in driver.c
> >
> >
> > > In dmesg I see this warning after connecting, could this mean
> something?
> > >
> > > [  530.888811] WARNING: CPU: 1 PID: 2889 at
> > > /build/linux-hwe-edge-vebCva/linux-hwe-edge-4.10.0/drivers/
> usb/core/hcd.c:1587
> > > usb_hcd_map_urb_for_dma+0x382/0x560
> > > [  530.888813] transfer buffer not dma capable
> > >
> >
> > This sounds like host USB controller issue, nothing directly related
> > to the podhd driver probably.
> >
> >
> > --
> > Greetings,
> >
> > Andrej
> >
> >
> > >
> > > On Sat, Jun 10, 2017 at 4:48 AM, Andrej Kruták <de...@an...> wrote:
> > >>
> > >> Hi Hans Peter,
> > >>
> > >>
> > >> On Sat, Jun 10, 2017 at 5:05 AM, Hans Peter Möller <hm...@gm...
> >
> > >> wrote:
> > >>
> > >> >    I made the tests,
> > >> > - Removing LINE6_CAP_CONTROL disable playback in the unit, I can
> still
> > >> > capture but no sound come out of the unit. The "receive length
> failed
> > >> > (error
> > >> > -11)" disappear. Since Initialization is required for playback, the
> > >> > .ctrl_if
> > >> > is needed and thus the simple patch will only work in Kernel 4.9 and
> > >> > above.
> > >>
> > >> I'm not sure it would be accepted into the maintenance branches
> > >> (<4.13) anyway, so that shouldn't matter. In the end .ctrl_if is not
> > >> needed for the <4.12 kernels anyhow (it's basically there just to lock
> > >> the USB device access for the kernel).
> > >>
> > >>
> > >> > -Removing LINE6_CAP_IN_NEEDS_OUT has no effect, so it's not needed.
> > >> >
> > >> > We still have the "receive length failed (error -11)" error, any
> other
> > >> > ideas
> > >> > of how to solve it? Or is not necessary?
> > >> >
> > >>
> > >> Probably the init protocol is different then (no way to tell unless
> > >> you sniff some stuff from windows). You can check one last thing -
> > >> whether line6_read_serial_number() returns a reasonable value. Most
> > >> likely not, but...
> > >>
> > >> I would say the best solution will be to add another flag, something
> > >> like LINE6_CAP_CONTROL_INIT - and adjust podhd_init() like:
> > >>
> > >> -    if (!(pod->line6.properties->capabilities & LINE6_CAP_CONTROL))
> {
> > >> +    if (!(pod->line6.properties->capabilities &
> LINE6_CAP_CONTROL_INIT))
> > >> {
> > >>         /* register USB audio system directly */
> > >>         return podhd_startup_finalize(pod);
> > >>     }
> > >>
> > >> The flag would be only set for POD X3 for now...
> > >>
> > >>
> > >> > Just of curiosity, is there any difference in the usb_device_id
> > >> > podhd_id_table[] if I use
> > >> > { LINE6_DEVICE(0x4159), .driver_info = LINE6_PODHD500X },
> > >> > instead of:
> > >> > { LINE6_IF_NUM(0x4159, 0), .driver_info = LINE6_PODHD500X },
> > >> >
> > >> > The current table is:
> > >> > /* table of devices that work with this driver */
> > >> > static const struct usb_device_id podhd_id_table[] = {
> > >> >     /* TODO: no need to alloc data interfaces when only audio is
> used */
> > >> >     { LINE6_DEVICE(0x5057),    .driver_info = LINE6_PODHD300 },
> > >> >     { LINE6_DEVICE(0x5058),    .driver_info = LINE6_PODHD400 },
> > >> >     { LINE6_IF_NUM(0x414D, 0), .driver_info = LINE6_PODHD500_0 },
> > >> >     { LINE6_IF_NUM(0x414D, 1), .driver_info = LINE6_PODHD500_1 },
> > >> >     { LINE6_IF_NUM(0x414A, 0), .driver_info = LINE6_PODX3 },
> > >> >     { LINE6_IF_NUM(0x414B, 0), .driver_info = LINE6_PODX3LIVE },
> > >> >     { LINE6_IF_NUM(0x4159, 0), .driver_info = LINE6_PODHD500X },
> > >> >     {}
> > >> > };
> > >> >
> > >>
> > >> This depends on how the device looks like. If there is only one USB
> > >> interface, LINE6_DEVICE should be sufficient. However, POD X3 has one
> > >> interface for audio (isochronous endpoints) and one for bulk
> > >> transfers, so it needs LINE6_IF_NUM() to match the right
> > >> device=interface (plus .ctrl_if for locking of the other one...).
> > >>
> > >> --
> > >> Best regards,
> > >>
> > >> Andrej
> > >>
> > >>
> > >> > On Fri, Jun 9, 2017 at 11:50 AM, Andrej Kruták <de...@an...>
> wrote:
> > >> >>
> > >> >> Ah yes, that one patch... I already forgot about it (and of course
> > >> >> didn't check the sources at the time of merge in 4.12 :)) Ok, but
> you
> > >> >> shouldn't need the ctrl_if thing if you only use the audio stuff.
> > >> >>
> > >> >> Regarding the maxpacket error, my prints the same - Line6
> apparently
> > >> >> violates the USB specification (AFAIR bulk endpoints on HS USB 64
> is
> > >> >> invalid maxsize (aka only valid for FullSpeed USB)...). In any
> case,
> > >> >> nothing to worry about...
> > >> >>
> > >> >>
> > >> >> --
> > >> >> Greetings,
> > >> >>
> > >> >> Andrej
> > >> >>
> > >> >>
> > >> >> On Fri, Jun 9, 2017 at 4:42 PM, Hans Peter Möller <
> hm...@gm...>
> > >> >> wrote:
> > >> >> > Hi,
> > >> >> >    I will thest droping those things and see what happened.
> > >> >> > What about the " config 1 interface 1 altsetting 0 bulk endpoint
> 0x1
> > >> >> > has
> > >> >> > invalid maxpacket 64" ??? It has appear since the first time I
> plug
> > >> >> > my
> > >> >> > pod
> > >> >> > with the stock kernel 4.4.
> > >> >> >
> > >> >> > in your kernel 4.12 the if_ctrl doesn't appear? How did they
> > >> >> > implemented
> > >> >> > the
> > >> >> > x3 then?
> > >> >> >
> > >> >> > here is the podhd.c that come in my kernel
> > >> >> >
> > >> >> >
> > >> >> > https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/
> linux/+git/xenial/tree/sound/usb/line6/podhd.c?h=hwe-edge
> > >> >> > and this is the one from torvalds
> > >> >> > https://github.com/torvalds/linux/blob/master/sound/usb/
> line6/podhd.c
> > >> >> >
> > >> >> > both have the x3 implementation with the ctr_if (and mention
> you).
> > >> >> >
> > >> >> >
> > >> >> >
> > >> >> > On Fri, Jun 9, 2017 at 1:51 AM, Andrej Kruták <de...@an...>
> wrote:
> > >> >> >>
> > >> >> >> Hey,
> > >> >> >>
> > >> >> >> On Fri, Jun 9, 2017 at 5:35 AM, Hans Peter Möller
> > >> >> >> <hm...@gm...>
> > >> >> >> wrote:
> > >> >> >> > Hi Andrej,
> > >> >> >> >    good news!!
> > >> >> >> > I can capture and listen from my pod.
> > >> >> >> >
> > >> >> >>
> > >> >> >> Perfect!
> > >> >> >>
> > >> >> >>
> > >> >> >> > I compare the lsusb from de x3 and I found that it has the
> same
> > >> >> >> > alternative
> > >> >> >> > setting issue than the hd500x, which is different from hd300
> (I
> > >> >> >> > couldn't
> > >> >> >> > get
> > >> >> >> > one from h400 nor from h500). In x3 and 500x the altsetting is
> > >> >> >> > different
> > >> >> >> > for
> > >> >> >> > audio than for data.
> > >> >> >> > Since X3 is included in kernel 4.9, I decided to go over
> kernel
> > >> >> >> > 4.9
> > >> >> >> > and
> > >> >> >> > use
> > >> >> >> > that code which also appear to be more updated than the one
> here.
> > >> >> >> >
> > >> >> >> > In podhd.c I copy the same configuration used for X3. The
> > >> >> >> > .ctr_if=1
> > >> >> >> > is
> > >> >> >> > the
> > >> >> >> > difference from the others and it make the difference. This
> are
> > >> >> >> > the
> > >> >> >> > modifications I made:
> > >> >> >> >
> > >> >> >> > enum {
> > >> >> >> >     LINE6_PODHD300,
> > >> >> >> >     LINE6_PODHD400,
> > >> >> >> >     LINE6_PODHD500_0,
> > >> >> >> >     LINE6_PODHD500_1,
> > >> >> >> >     LINE6_PODX3,
> > >> >> >> >     LINE6_PODX3LIVE,
> > >> >> >> >     LINE6_PODHD500X
> > >> >> >> > };
> > >> >> >> >
> > >> >> >> > /* table of devices that work with this driver */
> > >> >> >> > static const struct usb_device_id podhd_id_table[] = {
> > >> >> >> >     /* TODO: no need to alloc data interfaces when only audio
> is
> > >> >> >> > used
> > >> >> >> > */
> > >> >> >> >     { LINE6_DEVICE(0x5057),    .driver_info = LINE6_PODHD300
> },
> > >> >> >> >     { LINE6_DEVICE(0x5058),    .driver_info = LINE6_PODHD400
> },
> > >> >> >> >     { LINE6_IF_NUM(0x414D, 0), .driver_info =
> LINE6_PODHD500_0 },
> > >> >> >> >     { LINE6_IF_NUM(0x414D, 1), .driver_info =
> LINE6_PODHD500_1 },
> > >> >> >> >     { LINE6_IF_NUM(0x414A, 0), .driver_info = LINE6_PODX3 },
> > >> >> >> >     { LINE6_IF_NUM(0x414B, 0), .driver_info = LINE6_PODX3LIVE
> },
> > >> >> >> >     { LINE6_IF_NUM(0x4159, 0), .driver_info = LINE6_PODHD500X
> },
> > >> >> >> >     {}
> > >> >> >> > };
> > >> >> >> >
> > >> >> >> >
> > >> >> >> >
> > >> >> >> >
> > >> >> >> >     [LINE6_PODHD500X] = {
> > >> >> >> >         .id = "PODHD500X",
> > >> >> >> >         .name = "POD HD500x",
> > >> >> >> >         .capabilities    = LINE6_CAP_CONTROL
> > >> >> >> >                 | LINE6_CAP_PCM | LINE6_CAP_HWMON |
> > >> >> >> > LINE6_CAP_IN_NEEDS_OUT,
> > >> >> >> >         .altsetting = 1,
> > >> >> >> >         .ep_ctrl_r = 0x81,
> > >> >> >> >         .ep_ctrl_w = 0x01,
> > >> >> >> >         .ctrl_if = 1,
> > >> >> >> >         .ep_audio_r = 0x86,
> > >> >> >> >         .ep_audio_w = 0x02,
> > >> >> >> >     },
> > >> >> >> >
> > >> >> >> >
> > >> >> >> > I still get some errors/warning in dmesg, the maxpacket are
> the
> > >> >> >> > data
> > >> >> >> > ep
> > >> >> >> > so
> > >> >> >> > that is data. The "receive length failed (error -11)" don't
> know
> > >> >> >> > what
> > >> >> >> > it
> > >> >> >> > is,
> > >> >> >> > maybe initialization? Because it only appear this two times.
> > >> >> >> >
> > >> >> >> >  Here is dmesg output:
> > >> >> >> >
> > >> >> >> > [ 4001.944114] usb 1-3: new high-speed USB device number 11
> using
> > >> >> >> > ehci-pci
> > >> >> >> > [ 4002.063668] usb 1-3: config 1 interface 1 altsetting 0 bulk
> > >> >> >> > endpoint
> > >> >> >> > 0x1
> > >> >> >> > has invalid maxpacket 64
> > >> >> >> > [ 4002.063677] usb 1-3: config 1 interface 1 altsetting 0 bulk
> > >> >> >> > endpoint
> > >> >> >> > 0x81
> > >> >> >> > has invalid maxpacket 64
> > >> >> >> > [ 4002.064779] usb 1-3: New USB device found, idVendor=0e41,
> > >> >> >> > idProduct=4159
> > >> >> >> > [ 4002.064785] usb 1-3: New USB device strings: Mfr=1,
> Product=2,
> > >> >> >> > SerialNumber=0
> > >> >> >> > [ 4002.064790] usb 1-3: Product: POD HD500X
> > >> >> >> > [ 4002.064794] usb 1-3: Manufacturer: Line 6
> > >> >> >> > [ 4002.067896] snd_usb_podhd 1-3:1.0: Line 6 POD HD500x found
> > >> >> >> > [ 4002.069846] snd_usb_podhd 1-3:1.0: Line 6 POD HD500x now
> > >> >> >> > attached
> > >> >> >> > [ 4002.577334] snd_usb_podhd 1-3:1.0: receive length failed
> (error
> > >> >> >> > -11)
> > >> >> >> > [ 4002.579660] snd_usb_podhd 1-3:1.0: receive length failed
> (error
> > >> >> >> > -11)
> > >> >> >> >
> > >> >> >>
> > >> >> >> I think this is caused by the device having different initial
> setup
> > >> >> >> requirements. But it also (most likely) means that it doesn't
> need
> > >> >> >> any
> > >> >> >> setup. Thus drop the LIBE6_CAP_CONTROL thing, see if it still
> works.
> > >> >> >>
> > >> >> >> In addition, can you try also dropping LINE6_CAP_IN_NEEDS_OUT -
> and
> > >> >> >> check whether the stand-alone recording works?
> > >> >> >>
> > >> >> >> Lastly, I don't see any ctrl_if in the structure? Not in 4.0 ...
> > >> >> >> 4.12-rc kernels at least...
> > >> >> >>
> > >> >> >>
> > >> >> >> > I believe this should be added to the podhd.c delivered in the
> > >> >> >> > kernel
> > >> >> >> > but I
> > >> >> >> > dont know how to do that.
> > >> >> >> >
> > >> >> >>
> > >> >> >> Check linux/Documentation/process/submitting-patches.rst. In
> short -
> > >> >> >> just get inspired in the commit history of sound/usb/line6,
> create a
> > >> >> >> similar patch (git format-patch) and send it to alsa-devel
> > >> >> >> mailinglist.
> > >> >> >>
> > >> >> >> Alternatively, once you test the above, I can submit the patch
> on
> > >> >> >> your
> > >> >> >> behalf too.
> > >> >> >>
> > >> >> >>
> > >> >> >> --
> > >> >> >> Best regards,
> > >> >> >>
> > >> >> >> Andrej
> > >> >> >
> > >> >> >
> > >> >
> > >> >
> > >
> > >
>