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

From: Marc Kleine-Budde <mkl@pengutronix.de>
To: "Ahmed S. Darwish" <darwish.07@gmail.com>
Cc: Olivier Sobrie <olivier@sobrie.be>,
	Oliver Hartkopp <socketcan@hartkopp.net>,
	Wolfgang Grandegger <wg@grandegger.com>,
	Andri Yngvason <andri.yngvason@marel.com>,
	Linux-CAN <linux-can@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	netdev@vger.kernel.org
Subject: Re: [PATCH v4 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions
Date: Sat, 14 Mar 2015 15:58:39 +0100	[thread overview]
Message-ID: <55044C9F.2070105@pengutronix.de> (raw)
In-Reply-To: <20150314143820.GA22071@linux>

[-- Attachment #1: Type: text/plain, Size: 2196 bytes --]

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?

> 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

> (*) http://www.kvaser.com/products/kvaser-usb-hs-ii-hsls/
> 
> (#) There are a huge list of Kvaser products having the same controller
>     but with different performance metrics, so this is quite a
>     possiblity.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]