From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Jander Subject: Re: [PATCH 02/14] can: rx-fifo: Increase MB size limit from 32 to 64 Date: Thu, 6 Nov 2014 17:20:52 +0100 Message-ID: <20141106172052.6299c1d1@archvile> References: <1415262853-22907-1-git-send-email-david@protonic.nl> <1415262853-22907-3-git-send-email-david@protonic.nl> <545B3457.9030207@pengutronix.de> <20141106170330.54b6ed7e@archvile> <545B9C3E.1010002@pengutronix.de> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: Received: from protonic.xs4all.nl ([83.163.252.89]:27694 "EHLO protonic.xs4all.nl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751156AbaKFQUQ (ORCPT ); Thu, 6 Nov 2014 11:20:16 -0500 In-Reply-To: <545B9C3E.1010002@pengutronix.de> Sender: linux-can-owner@vger.kernel.org List-ID: To: Marc Kleine-Budde Cc: linux-can@vger.kernel.org, Wolfgang Grandegger , Alexander Stein On Thu, 06 Nov 2014 17:05:18 +0100 Marc Kleine-Budde wrote: > On 11/06/2014 05:03 PM, David Jander wrote: > > On Thu, 06 Nov 2014 09:41:59 +0100 > > Marc Kleine-Budde wrote: > > > >> On 11/06/2014 09:34 AM, David Jander wrote: > >>> Signed-off-by: David Jander > >>> --- > >>> drivers/net/can/dev.c | 24 ++++++++++++------------ > >>> include/linux/can/dev.h | 12 ++++++------ > >>> 2 files changed, 18 insertions(+), 18 deletions(-) > >>> > >>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c > >>> index c1e53e9..930b9f4 100644 > >>> --- a/drivers/net/can/dev.c > >>> +++ b/drivers/net/can/dev.c > >>> @@ -289,20 +289,20 @@ static unsigned int can_rx_fifo_inc(struct > >>> can_rx_fifo *fifo, unsigned int *val) return (*val)--; > >>> } > >>> > >>> -static u32 can_rx_fifo_mask_low(struct can_rx_fifo *fifo) > >>> +static u64 can_rx_fifo_mask_low(struct can_rx_fifo *fifo) > >>> { > >>> if (fifo->inc) > >>> - return ~0U >> (32 + fifo->low_first - fifo->high_first) > >>> << fifo->low_first; > >>> + return ~0LLU >> (64 + fifo->low_first - > >>> fifo->high_first) << fifo->low_first; else > >>> - return ~0U >> (32 - fifo->low_first + fifo->high_first) > >>> << (fifo->high_first + 1); > >>> + return ~0LLU >> (64 - fifo->low_first + > >>> fifo->high_first) << (fifo->high_first + 1); > >> > >> Yesterday I stumbled over GENMASK_ULL > >> http://lxr.free-electrons.com/source/include/linux/bitops.h#L22 > >> Does it make sense to use it here? > > > > Looks like its exactly what is needed here.... nice. > > > > Btw, there is a subtle bug in the line you linked to: > > > >> #define GENMASK_ULL(h, l) (((U64_C(1) << ((h) - (l) + 1)) - 1) << > >> (l)) > > > > The _ULL in the name of the macro seems to hint to "unsigned long long", > > while the implementation seems to assume that it means U64_C. This is > > obviously wrong, although for most platforms that are currently capable of > > running Linux, those are probably equivalent... > > #define U64_C(x) x ## ULL This line: http://lxr.free-electrons.com/source/include/asm-generic/int-ll64.h#L4 ...seems to indicate, that it may not always be the case :-) AFAIK, C99 specifies that "long long" should at _least_ have 64 bit length, but says nothing about when it may be larger. OTOH, I have yet to find a machine/compiler combination that defines "long long" as more than 64bit... at least not running Linux. A correct definition should be trivial though: #define GENMASK_ULL(h, l) (((1ULL << ((h) - (l) + 1)) - 1) << (l)) Best regards, -- David Jander Protonic Holland.