Re: [PATCH] ALSA: usb-audio: add Pioneer DJ DDJ-SZ support

[Geraldo Nascimento]

Hi Takashi,

On Mon, Sep 7, 2026 at 4:57 AM Takashi Iwai <tiwai@xxxxxxx> wrote:
>
> On Sat, 05 Sep 2026 08:19:37 +0200,
> Geraldo Nascimento wrote:
> >
> > On Sat, Sep 5, 2026 at 2:35 AM Hanh Kieu <hhkieu@xxxxxxxxx> wrote:
> > >
> > > Hi Geraldo,
> > >
> > > I looked further into the is_pioneer_implicit_fb() check you pointed out.
> > >
> > > It seems the DATA case is intentional. In the original Pioneer DJM-900NXS2 work, Fabian Lesniak described the observed behavior as:
> > >
> > > “the playback streams also tries to start the capture endpoint as sync source.”
> > >
> > > https://gist.github.com/flesniak/074ab23bbc833663b782f44174eae6a4
> > >
> > > Takashi Iwai later formalized this in the Pioneer implicit-feedback handling, explicitly accepting either USB_ENDPOINT_USAGE_DATA or USB_ENDPOINT_USAGE_IMPLICIT_FB for the secondary IN endpoint before passing it to add_implicit_fb_sync_ep():
> > >
> > > https://github.com/torvalds/linux/commit/167c9dc84ec384c0940359e067301883ad2b42a8
> > >
> >
> > Every other Pioneer device has that USB_ENDPOINT_USAGE_IMPLICIT_FB
> > on the IN endpoint. Yours would be the first without it in the IN
> > endpoint. Unless
> > your hardware doesn't rely on implicit feedback for syncing then there's a good
> > chance that implicit.c short-circuiting and still picking up implicit
> > feedback for you
> > isn't correct, but what you need to do is cool down now and wait for proper
> > maintainer response which takes time.
>
> Accepting both DATA and IMPLICIT_FB usages is intentional, yes.
> But I don't follow what's the problem there -- in this case, the
> bmAttributes of the secondary EP shows it's DATA, no?  Then this check
> must pass.

If you check the original patch submission by Hanh, you'll see he did not
bother to include USB_ENDPOINT_USAGE_IMPLICIT_FB as a flag for
EP attributes, claiming it was unnecessary and the implicit feedback was
picked-up regardless.

Hahn already traced the functions return values, he is getting a true value
out of that function (and thus picking up implicit feedback for his device)
even though he does not add USB_ENDPOINT_USAGE_IMPLICIT_FB
to the IN EP Attributes on his original submission.

>
> The condition is:
>
>         if (!usb_endpoint_is_isoc_in(epd) ||
>             (epd->bmAttributes & USB_ENDPOINT_SYNCTYPE) != USB_ENDPOINT_SYNC_ASYNC ||
>             ((epd->bmAttributes & USB_ENDPOINT_USAGE_MASK) !=
>              USB_ENDPOINT_USAGE_DATA &&
>              (epd->bmAttributes & USB_ENDPOINT_USAGE_MASK) !=
>              USB_ENDPOINT_USAGE_IMPLICIT_FB))
>                 return false;
>
>         return false;

Yes, and that means when that first inequality is false, C short-circuits the
logical AND and never checks USB_ENDPOINT_USAGE_IMPLICIT_FB
like it should.

>
> ... and maybe it's better to be rewritten like:
>
>         sync = (epd->bmAttributes & USB_ENDPOINT_SYNCTYPE);
>         usage = (epd->bmAttributes & USB_ENDPOINT_USAGE_MASK);
>
>         if (usb_endpoint_is_isoc_in(epd) &&
>             sync == USB_ENDPOINT_SYNC_ASYNC &&
>             (usage == USB_ENDPOINT_USAGE_DATA ||
>              usage == USB_ENDPOINT_USAGE_IMPLICIT_FB))
>                 return true;
>
>         return false;

But then, again, you would be disregarding the meaning of the flag
USB_ENDPOINT_USAGE_IMPLICIT_FB and in practice just making
the first check.

>
> So it should return true.
> Please double-check which test actually fails.
>

The tests don't fail, the function returns true and implicit fb is engaged,
it's just the second test never runs.

Should Pioneer release hardware in the future that does not need implicit
feedback to be engaged, provided all the other particulars are equal, this
function as-is would enforce implicit feedback on a device we don't want
it applied. It's a bit far-fetched but do I see what I mean now? Do you see
the logical error now?

Thanks,
Geraldo Nascimento

>
> thanks,
>
> Takashi