Re: [PATCH] can: mcp25xxfd: add listen-only mode - Marc Kleine-Budde

From: Marc Kleine-Budde <mkl@pengutronix.de>
To: linux-can@vger.kernel.org
Subject: Re: [PATCH] can: mcp25xxfd: add listen-only mode
Date: Tue, 23 Jun 2020 09:40:48 +0200	[thread overview]
Message-ID: <10e4e6f0-d400-fd7f-5c99-33637cdc8dbc@pengutronix.de> (raw)
In-Reply-To: <20200623051527.GE3077@x1.vandijck-laurijssen.be>

On 6/23/20 7:15 AM, Kurt Van Dijck wrote:
> On ma, 22 jun 2020 15:38:03 +0200, Marc Kleine-Budde wrote:
>> On 6/22/20 2:20 PM, Kurt Van Dijck wrote:
>>> This commit enables listen-only mode, which works internally like CANFD mode.
>>
>> Does the controller distinguish between CAN-2.0 listen only and CAN-FD listen
>> only mode?
> 
> Nope, Listen-only is a 3rd mode, and from the manual, it's similar to
> CANFD mode.
>>
>> If listen only means CAN-FD...should we add a check to open() if CAN_CTRLMODE_FD
>> and CAN_CTRLMODE_LISTENONLY is both set (or unset).
> 
> The difference between CAN-FD and CAN-2.0 is in CAN-2.0, the chip will
> send error frames on CAN-FD frame.
> That looks obvious.
> In listen-only mode, no acks or error-frames are sent, so the difference
> is not really relevant in listen-only mode.

ACK

> On the host side however, in listen-only mode with no CAN_CTRLMODE_FD,
> you could find CAN-FD packets in your socket.
> I think we could improve and filter them out in software during the
> receive handler.
> If we think that is necessary.

ACK

> It would strictly be correct, but I tend to be a bit more tolerant on
> the input, and discard the difference.
> That's why I didn't add the code. and because it's usually easy to make
> mistakes and it easily becomes a monster.

ACK

>> Please compile with #define DEBUG and look in dmesg the size of the objects in
>> the mailbox. Maybe it's worth not allocating any TEF and TX mailbox and using
>> more RX instead.
> and have runtime defined sizes. I'm not convinced that this improves the
> driver. I'm sure it makes it more complex.

The driver already has different sized RX and TX objects depending on CAN-2.0
and CAN-FD modes. (8 vs. 64 bytes of data). And it uses 8 TX and 32 RX object
for CAN-2.0, while it's 4 TX and 16 RX for CAN-FD.

But you're right, optimization for listen only can be done later.

>> We have to convert the driver from using a bit mask to modulo to get from the
>> tail and head to the actual index inside the RAM.
> 
> I confess not having studied the chip nor your driver in that detail.
> Your plan sounds good. I would not postpone submitting the driver for
> improvements like that, given that other approaches exists in vendor
> kernels.
> It sounds like a next iteration to me.

ACK. Currently the driver only supports a power-of-2 for RX and TX mailboxes.
With converting from a mask to a modulo, this would increase the RX mailboxes
from 16 to 22 in the CAN-FD mode.

I've added your patch with minor modifications to the series, mostly changing
the order of CAN_CTRLMODE_LISTENONLY and CAN_CTRLMODE_FD.

regards,
Marc

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