All of lore.kernel.org
 help / color / mirror / Atom feed
From: Oliver Hartkopp <socketcan@hartkopp.net>
To: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Cc: "Marc Kleine-Budde" <mkl@pengutronix.de>,
	linux-can@vger.kernel.org, kernel@pengutronix.de,
	"Stéphane Grosjean" <s.grosjean@peak-system.com>
Subject: Re: [net-rfc 04/16] can: dev: can_get_len(): add a helper function to get the correct length of Classical frames
Date: Sat, 24 Oct 2020 13:31:06 +0200	[thread overview]
Message-ID: <358352d1-2428-0909-9bfe-a62e68f47e28@hartkopp.net> (raw)
In-Reply-To: <CAMZ6Rq+tx93DuRYsLOpcxjRDOJxUTh-czntEYSv7tDMOwoaLQw@mail.gmail.com>



On 24.10.20 07:25, Vincent MAILHOL wrote:
> On 24.10.20 01:47, Oliver Hartkopp wrote:

>>>> Btw. do you really see any legacy SocketCAN applications (*together*
>>>> with your testing application on the same Linux host) where you don't
>>>> have the source code to check whether they properly initialize the
>>>> reserved/padding bytes?
>>
>> Do you?

You still didn't answer this hypothetical question. There must be a 
reason behind .. :-D

>> You can not take responsibility for broken implementations on other ECUs.
> 
> We clearly share a difference stance on this subject, I think it is
> time to close the debate. I agree to go with your solution.

Ok, together with this answer I also can close the debate :-)

> I saw your patch. Changing the name from raw_can to len8_dlc clears
> the concerns I had for invalid frames. Thank you.

Great! In fact it cost some time in the night where I woke up from sleep 
and think about "the overall goal what we want to archive" ;-)

The right naming lets developers make right assumptions about the 
functionality. Therefore it always remains tricky - and discussions like 
these. I needed two hours to write an rephrase the patch again and again ...

Also the ctrlmode has now a clear target of Classic CAN (CC) in its 
name: CC_LEN8_DLC

> Thank you also for depreciating the can_dlc field and introducing the
> len field in struct can_frame. It is a smart change.

Thanks. I always had the idea to fix this up after the introduction of 
CAN FD which uses a 'len' element too. I discovered myself at that time, 
that can_dlc was not the best matching choice - but Kernel APIs are 
written in stone for a good reason.

So I've checked what people do, when they need to rename structure 
elements in the Kernel API in the uapi section and found this:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=31ba514b2fd0495796

> I will now test the patch.

So far it only is an idea for naming 'things' - but I'm happy you like it!

IMO we could continue with enabling a CAN driver to support 
CAN_CTRLMODE_CC_LEN8_DLC and see how it works in reality with CAN_RAW 
and AF_PACKET - and if we would finally need CAN_RAW_LEN8_DLC or not.

Best,
Oliver

  reply	other threads:[~2020-10-24 11:31 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-19 19:05 [RFC]: can 2020-10-19 Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 01/16] can: proc: can_remove_proc(): silence remove_proc_entry warning Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 02/16] can: rx-offload: don't call kfree_skb() from IRQ context Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 03/16] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard " Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 04/16] can: dev: can_get_len(): add a helper function to get the correct length of Classical frames Marc Kleine-Budde
2020-10-19 20:35   ` Oliver Hartkopp
2020-10-20  6:35     ` Marc Kleine-Budde
2020-10-20 11:30       ` Vincent Mailhol
2020-10-20 11:48         ` Marc Kleine-Budde
2020-10-20 12:38         ` Oliver Hartkopp
2020-10-20 15:02           ` Marc Kleine-Budde
2020-10-20 16:07           ` Vincent Mailhol
2020-10-20 17:04             ` Oliver Hartkopp
2020-10-20 18:50               ` Marc Kleine-Budde
2020-10-21  0:52               ` Vincent Mailhol
2020-10-21  6:23                 ` Vincent MAILHOL
2020-10-21  7:11                   ` Joakim Zhang
2020-10-21  7:21                     ` Marc Kleine-Budde
2020-10-21  7:48                       ` Joakim Zhang
2020-10-21  9:21                   ` Oliver Hartkopp
2020-10-21  9:48                     ` Oliver Hartkopp
2020-10-21 11:55                     ` Vincent MAILHOL
2020-10-21 17:52                       ` Oliver Hartkopp
2020-10-22  3:30                         ` Vincent MAILHOL
2020-10-22  7:15                           ` Oliver Hartkopp
2020-10-22 12:23                             ` Vincent MAILHOL
2020-10-22 13:28                               ` Oliver Hartkopp
2020-10-22 15:46                                 ` Vincent MAILHOL
2020-10-22 17:06                                   ` Oliver Hartkopp
2020-10-23 10:36                                     ` Vincent MAILHOL
2020-10-23 16:47                                       ` Oliver Hartkopp
2020-10-24  5:25                                         ` Vincent MAILHOL
2020-10-24 11:31                                           ` Oliver Hartkopp [this message]
2020-10-19 19:05 ` [net-rfc 05/16] can: dev: __can_get_echo_skb(): fix the returned length of CAN frame Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 06/16] can: can_create_echo_skb(): fix echo skb generation: always use skb_clone() Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 07/16] can: j1939: j1939_sk_bind(): return failure if netdev is down Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 08/16] can: isotp: Explain PDU in CAN_ISOTP help text Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 09/16] can: isotp: enable RX timeout handling in listen-only mode Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 10/16] can: ti_hecc: add missed clk_disable_unprepare() in error path Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 11/16] can: xilinx_can: handle failure cases of pm_runtime_get_sync Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 12/16] can: peak_usb: fix timestamp wrapping Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 13/16] can: peak_canfd: fix echo management when loopback is on Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 14/16] can: mcp251xfd: mcp251xfd_regmap_crc_read(): increase severity of CRC read error messages Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 15/16] can: mcp251xfd: mcp251xfd_regmap_nocrc_read(): fix semicolon.cocci warnings Marc Kleine-Budde
2020-10-19 19:05 ` [net-rfc 16/16] can: mcp251xfd: remove unneeded break Marc Kleine-Budde

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=358352d1-2428-0909-9bfe-a62e68f47e28@hartkopp.net \
    --to=socketcan@hartkopp.net \
    --cc=kernel@pengutronix.de \
    --cc=linux-can@vger.kernel.org \
    --cc=mailhol.vincent@wanadoo.fr \
    --cc=mkl@pengutronix.de \
    --cc=s.grosjean@peak-system.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.