Re: [PATCH v4 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions

Ahmed S. Darwish
2015-03-14T15:19:45Z

On Sat, Mar 14, 2015 at 03:58:39PM +0100, Marc Kleine-Budde wrote:
> On 03/14/2015 03:38 PM, Ahmed S. Darwish wrote:
> >> Applied to can. This will go into David's net tree and finally into
> >> net-next. Then I'll apply patches 2+3. Nag me, if I forget about them ;)
> >>
> >
> > Thanks! :-)
> >
> > So if I've understood correctly, this patch will go to -rc5 and
> > the rest will go into net-next?
> >
> > If so, IMHO patch #2 should also go to -rc5. I know it's usually
> > frowned up on to add further patches at this late -rc stage, but
> > here's my logic:
> >
> > The original driver code just _arbitrarily_ assumed a MAX_TX_URB
> > value of 16 parallel transmissions. This value was chosen, it seems,
> > because the driver was heavily based on esd_usb2.c and the esd code
> > just did so :-(
> >
> > Meanwhile, in the Kvaser hardware at hand, if I've increased the
> > driver's max parallel transmissions little above the recommended
> > limit reported by firmware, the firmware breaks up badly, reports a
> > massive list of internal errors, and the candump traces becomes
> > sort of an internal mess hardly related to the actual frames sent
> > and received.
>
> In this particular hardware, what's the limit as reported by the firmware?
>

48 max oustanding tx, which is quite big in itself it seems.

Other drivers in the tree range between 10 (Peak) and 20 tx (8dev).

> > In my case, I was lucky that the brand we own here (*) had a higher
> > max outstanding transmissions value than 16. But if there is hardware
>
> Okay - higher.
>
> > out there with a max oustanding tx support < 16 (#), such hardware
> > will break badly under a heavy transmission load :-(
>
> I see.
>
> If you add this motivation to the patch description and let the subject
> reflect that this is a "fix" or safety measure rather than a possible
> performance improvement, no-one will say anything against this patch :D
>

True; I admit the "fix" part should've been clearer :-)

Will send a better worded commit message then.

Thanks a lot,
Darwish