* [net 01/27] dt-bindings: can: add can-controller.yaml
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 02/27] dt-bindings: can: flexcan: convert fsl,*flexcan bindings to yaml Marc Kleine-Budde
` (26 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Oleksij Rempel, Rob Herring,
Marc Kleine-Budde
From: Oleksij Rempel <o.rempel@pengutronix.de>
For now we have only node name as common rule for all CAN controllers
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Rob Herring <robh@kernel.org>
Link: https://lore.kernel.org/r/20201022075218.11880-2-o.rempel@pengutronix.de
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
.../bindings/net/can/can-controller.yaml | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/can/can-controller.yaml
diff --git a/Documentation/devicetree/bindings/net/can/can-controller.yaml b/Documentation/devicetree/bindings/net/can/can-controller.yaml
new file mode 100644
index 000000000000..9cf2ae097156
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/can-controller.yaml
@@ -0,0 +1,18 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/can/can-controller.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: CAN Controller Generic Binding
+
+maintainers:
+ - Marc Kleine-Budde <mkl@pengutronix.de>
+
+properties:
+ $nodename:
+ pattern: "^can(@.*)?$"
+
+additionalProperties: true
+
+...
base-commit: 9621618130bf7e83635367c13b9a6ee53935bb37
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 02/27] dt-bindings: can: flexcan: convert fsl,*flexcan bindings to yaml
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
2020-11-03 22:06 ` [net 01/27] dt-bindings: can: add can-controller.yaml Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 03/27] can: proc: can_remove_proc(): silence remove_proc_entry warning Marc Kleine-Budde
` (25 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Oleksij Rempel, Rob Herring,
Marc Kleine-Budde
From: Oleksij Rempel <o.rempel@pengutronix.de>
In order to automate the verification of DT nodes convert
fsl-flexcan.txt to fsl,flexcan.yaml
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Rob Herring <robh@kernel.org>
Link: https://lore.kernel.org/r/20201022075218.11880-3-o.rempel@pengutronix.de
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
.../bindings/net/can/fsl,flexcan.yaml | 135 ++++++++++++++++++
.../bindings/net/can/fsl-flexcan.txt | 57 --------
2 files changed, 135 insertions(+), 57 deletions(-)
create mode 100644 Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml
delete mode 100644 Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
diff --git a/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml b/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml
new file mode 100644
index 000000000000..43df15ba8fa4
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml
@@ -0,0 +1,135 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/can/fsl,flexcan.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title:
+ Flexcan CAN controller on Freescale's ARM and PowerPC system-on-a-chip (SOC).
+
+maintainers:
+ - Marc Kleine-Budde <mkl@pengutronix.de>
+
+allOf:
+ - $ref: can-controller.yaml#
+
+properties:
+ compatible:
+ oneOf:
+ - enum:
+ - fsl,imx8qm-flexcan
+ - fsl,imx8mp-flexcan
+ - fsl,imx6q-flexcan
+ - fsl,imx53-flexcan
+ - fsl,imx35-flexcan
+ - fsl,imx28-flexcan
+ - fsl,imx25-flexcan
+ - fsl,p1010-flexcan
+ - fsl,vf610-flexcan
+ - fsl,ls1021ar2-flexcan
+ - fsl,lx2160ar1-flexcan
+ - items:
+ - enum:
+ - fsl,imx7d-flexcan
+ - fsl,imx6ul-flexcan
+ - fsl,imx6sx-flexcan
+ - const: fsl,imx6q-flexcan
+ - items:
+ - enum:
+ - fsl,ls1028ar1-flexcan
+ - const: fsl,lx2160ar1-flexcan
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ maxItems: 2
+
+ clock-names:
+ items:
+ - const: ipg
+ - const: per
+
+ clock-frequency:
+ description: |
+ The oscillator frequency driving the flexcan device, filled in by the
+ boot loader. This property should only be used the used operating system
+ doesn't support the clocks and clock-names property.
+
+ xceiver-supply:
+ description: Regulator that powers the CAN transceiver.
+
+ big-endian:
+ $ref: /schemas/types.yaml#/definitions/flag
+ description: |
+ This means the registers of FlexCAN controller are big endian. This is
+ optional property.i.e. if this property is not present in device tree
+ node then controller is assumed to be little endian. If this property is
+ present then controller is assumed to be big endian.
+
+ fsl,stop-mode:
+ description: |
+ Register bits of stop mode control.
+
+ The format should be as follows:
+ <gpr req_gpr req_bit>
+ gpr is the phandle to general purpose register node.
+ req_gpr is the gpr register offset of CAN stop request.
+ req_bit is the bit offset of CAN stop request.
+ $ref: /schemas/types.yaml#/definitions/phandle-array
+ items:
+ - description: The 'gpr' is the phandle to general purpose register node.
+ - description: The 'req_gpr' is the gpr register offset of CAN stop request.
+ maximum: 0xff
+ - description: The 'req_bit' is the bit offset of CAN stop request.
+ maximum: 0x1f
+
+ fsl,clk-source:
+ description: |
+ Select the clock source to the CAN Protocol Engine (PE). It's SoC
+ implementation dependent. Refer to RM for detailed definition. If this
+ property is not set in device tree node then driver selects clock source 1
+ by default.
+ 0: clock source 0 (oscillator clock)
+ 1: clock source 1 (peripheral clock)
+ $ref: /schemas/types.yaml#/definitions/uint32
+ default: 1
+ minimum: 0
+ maximum: 1
+
+ wakeup-source:
+ $ref: /schemas/types.yaml#/definitions/flag
+ description:
+ Enable CAN remote wakeup.
+
+required:
+ - compatible
+ - reg
+ - interrupts
+
+additionalProperties: false
+
+examples:
+ - |
+ can@1c000 {
+ compatible = "fsl,p1010-flexcan";
+ reg = <0x1c000 0x1000>;
+ interrupts = <48 0x2>;
+ interrupt-parent = <&mpic>;
+ clock-frequency = <200000000>;
+ fsl,clk-source = <0>;
+ };
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ can@2090000 {
+ compatible = "fsl,imx6q-flexcan";
+ reg = <0x02090000 0x4000>;
+ interrupts = <0 110 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clks 1>, <&clks 2>;
+ clock-names = "ipg", "per";
+ fsl,stop-mode = <&gpr 0x34 28>;
+ };
diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
deleted file mode 100644
index e10b6eb955e1..000000000000
--- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
+++ /dev/null
@@ -1,57 +0,0 @@
-Flexcan CAN controller on Freescale's ARM and PowerPC system-on-a-chip (SOC).
-
-Required properties:
-
-- compatible : Should be "fsl,<processor>-flexcan"
-
- where <processor> is imx8qm, imx6q, imx28, imx53, imx35, imx25, p1010,
- vf610, ls1021ar2, lx2160ar1, ls1028ar1.
-
- The ls1028ar1 must be followed by lx2160ar1, e.g.
- - "fsl,ls1028ar1-flexcan", "fsl,lx2160ar1-flexcan"
-
- An implementation should also claim any of the following compatibles
- that it is fully backwards compatible with:
-
- - fsl,p1010-flexcan
-
-- reg : Offset and length of the register set for this device
-- interrupts : Interrupt tuple for this device
-
-Optional properties:
-
-- clock-frequency : The oscillator frequency driving the flexcan device
-
-- xceiver-supply: Regulator that powers the CAN transceiver
-
-- big-endian: This means the registers of FlexCAN controller are big endian.
- This is optional property.i.e. if this property is not present in
- device tree node then controller is assumed to be little endian.
- if this property is present then controller is assumed to be big
- endian.
-
-- fsl,stop-mode: register bits of stop mode control, the format is
- <&gpr req_gpr req_bit>.
- gpr is the phandle to general purpose register node.
- req_gpr is the gpr register offset of CAN stop request.
- req_bit is the bit offset of CAN stop request.
-
-- fsl,clk-source: Select the clock source to the CAN Protocol Engine (PE).
- It's SoC Implementation dependent. Refer to RM for detailed
- definition. If this property is not set in device tree node
- then driver selects clock source 1 by default.
- 0: clock source 0 (oscillator clock)
- 1: clock source 1 (peripheral clock)
-
-- wakeup-source: enable CAN remote wakeup
-
-Example:
-
- can@1c000 {
- compatible = "fsl,p1010-flexcan";
- reg = <0x1c000 0x1000>;
- interrupts = <48 0x2>;
- interrupt-parent = <&mpic>;
- clock-frequency = <200000000>; // filled in by bootloader
- fsl,clk-source = <0>; // select clock source 0 for PE
- };
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 03/27] can: proc: can_remove_proc(): silence remove_proc_entry warning
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
2020-11-03 22:06 ` [net 01/27] dt-bindings: can: add can-controller.yaml Marc Kleine-Budde
2020-11-03 22:06 ` [net 02/27] dt-bindings: can: flexcan: convert fsl,*flexcan bindings to yaml Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 04/27] can: rx-offload: don't call kfree_skb() from IRQ context Marc Kleine-Budde
` (24 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Zhang Changzhong,
Oliver Hartkopp, Marc Kleine-Budde
From: Zhang Changzhong <zhangchangzhong@huawei.com>
If can_init_proc() fail to create /proc/net/can directory, can_remove_proc()
will trigger a warning:
WARNING: CPU: 6 PID: 7133 at fs/proc/generic.c:672 remove_proc_entry+0x17b0
Kernel panic - not syncing: panic_on_warn set ...
Fix to return early from can_remove_proc() if can proc_dir does not exists.
Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com>
Link: https://lore.kernel.org/r/1594709090-3203-1-git-send-email-zhangchangzhong@huawei.com
Fixes: 8e8cda6d737d ("can: initial support for network namespaces")
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
net/can/proc.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/can/proc.c b/net/can/proc.c
index 550928b8b8a2..5ea8695f507e 100644
--- a/net/can/proc.c
+++ b/net/can/proc.c
@@ -462,6 +462,9 @@ void can_init_proc(struct net *net)
*/
void can_remove_proc(struct net *net)
{
+ if (!net->can.proc_dir)
+ return;
+
if (net->can.pde_stats)
remove_proc_entry(CAN_PROC_STATS, net->can.proc_dir);
@@ -486,6 +489,5 @@ void can_remove_proc(struct net *net)
if (net->can.pde_rcvlist_sff)
remove_proc_entry(CAN_PROC_RCVLIST_SFF, net->can.proc_dir);
- if (net->can.proc_dir)
- remove_proc_entry("can", net->proc_net);
+ remove_proc_entry("can", net->proc_net);
}
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 04/27] can: rx-offload: don't call kfree_skb() from IRQ context
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (2 preceding siblings ...)
2020-11-03 22:06 ` [net 03/27] can: proc: can_remove_proc(): silence remove_proc_entry warning Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 05/27] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard " Marc Kleine-Budde
` (23 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, linux-can, kernel, Marc Kleine-Budde
A CAN driver, using the rx-offload infrastructure, is reading CAN frames
(usually in IRQ context) from the hardware and placing it into the rx-offload
queue to be delivered to the networking stack via NAPI.
In case the rx-offload queue is full, trying to add more skbs results in the
skbs being dropped using kfree_skb(). If done from hard-IRQ context this
results in the following warning:
[ 682.552693] ------------[ cut here ]------------
[ 682.557360] WARNING: CPU: 0 PID: 3057 at net/core/skbuff.c:650 skb_release_head_state+0x74/0x84
[ 682.566075] Modules linked in: can_raw can coda_vpu flexcan dw_hdmi_ahb_audio v4l2_jpeg imx_vdoa can_dev
[ 682.575597] CPU: 0 PID: 3057 Comm: cansend Tainted: G W 5.7.0+ #18
[ 682.583098] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[ 682.589657] [<c0112628>] (unwind_backtrace) from [<c010c1c4>] (show_stack+0x10/0x14)
[ 682.597423] [<c010c1c4>] (show_stack) from [<c06c481c>] (dump_stack+0xe0/0x114)
[ 682.604759] [<c06c481c>] (dump_stack) from [<c0128f10>] (__warn+0xc0/0x10c)
[ 682.611742] [<c0128f10>] (__warn) from [<c0129314>] (warn_slowpath_fmt+0x5c/0xc0)
[ 682.619248] [<c0129314>] (warn_slowpath_fmt) from [<c0b95dec>] (skb_release_head_state+0x74/0x84)
[ 682.628143] [<c0b95dec>] (skb_release_head_state) from [<c0b95e08>] (skb_release_all+0xc/0x24)
[ 682.636774] [<c0b95e08>] (skb_release_all) from [<c0b95eac>] (kfree_skb+0x74/0x1c8)
[ 682.644479] [<c0b95eac>] (kfree_skb) from [<bf001d1c>] (can_rx_offload_queue_sorted+0xe0/0xe8 [can_dev])
[ 682.654051] [<bf001d1c>] (can_rx_offload_queue_sorted [can_dev]) from [<bf001d6c>] (can_rx_offload_get_echo_skb+0x48/0x94 [can_dev])
[ 682.666007] [<bf001d6c>] (can_rx_offload_get_echo_skb [can_dev]) from [<bf01efe4>] (flexcan_irq+0x194/0x5dc [flexcan])
[ 682.676734] [<bf01efe4>] (flexcan_irq [flexcan]) from [<c019c1ec>] (__handle_irq_event_percpu+0x4c/0x3ec)
[ 682.686322] [<c019c1ec>] (__handle_irq_event_percpu) from [<c019c5b8>] (handle_irq_event_percpu+0x2c/0x88)
[ 682.695993] [<c019c5b8>] (handle_irq_event_percpu) from [<c019c64c>] (handle_irq_event+0x38/0x5c)
[ 682.704887] [<c019c64c>] (handle_irq_event) from [<c01a1058>] (handle_fasteoi_irq+0xc8/0x180)
[ 682.713432] [<c01a1058>] (handle_fasteoi_irq) from [<c019b2c0>] (generic_handle_irq+0x30/0x44)
[ 682.722063] [<c019b2c0>] (generic_handle_irq) from [<c019b8f8>] (__handle_domain_irq+0x64/0xdc)
[ 682.730783] [<c019b8f8>] (__handle_domain_irq) from [<c06df4a4>] (gic_handle_irq+0x48/0x9c)
[ 682.739158] [<c06df4a4>] (gic_handle_irq) from [<c0100b30>] (__irq_svc+0x70/0x98)
[ 682.746656] Exception stack(0xe80e9dd8 to 0xe80e9e20)
[ 682.751725] 9dc0: 00000001 e80e8000
[ 682.759922] 9de0: e820cf80 00000000 ffffe000 00000000 eaf08fe4 00000000 600d0013 00000000
[ 682.768117] 9e00: c1732e3c c16093a8 e820d4c0 e80e9e28 c018a57c c018b870 600d0013 ffffffff
[ 682.776315] [<c0100b30>] (__irq_svc) from [<c018b870>] (lock_acquire+0x108/0x4e8)
[ 682.783821] [<c018b870>] (lock_acquire) from [<c0e938e4>] (down_write+0x48/0xa8)
[ 682.791242] [<c0e938e4>] (down_write) from [<c02818dc>] (unlink_file_vma+0x24/0x40)
[ 682.798922] [<c02818dc>] (unlink_file_vma) from [<c027a258>] (free_pgtables+0x34/0xb8)
[ 682.806858] [<c027a258>] (free_pgtables) from [<c02835a4>] (exit_mmap+0xe4/0x170)
[ 682.814361] [<c02835a4>] (exit_mmap) from [<c01248e0>] (mmput+0x5c/0x110)
[ 682.821171] [<c01248e0>] (mmput) from [<c012e910>] (do_exit+0x374/0xbe4)
[ 682.827892] [<c012e910>] (do_exit) from [<c0130888>] (do_group_exit+0x38/0xb4)
[ 682.835132] [<c0130888>] (do_group_exit) from [<c0130914>] (__wake_up_parent+0x0/0x14)
[ 682.843063] irq event stamp: 1936
[ 682.846399] hardirqs last enabled at (1935): [<c02938b0>] rmqueue+0xf4/0xc64
[ 682.853553] hardirqs last disabled at (1936): [<c0100b20>] __irq_svc+0x60/0x98
[ 682.860799] softirqs last enabled at (1878): [<bf04cdcc>] raw_release+0x108/0x1f0 [can_raw]
[ 682.869256] softirqs last disabled at (1876): [<c0b8f478>] release_sock+0x18/0x98
[ 682.876753] ---[ end trace 7bca4751ce44c444 ]---
This patch fixes the problem by replacing the kfree_skb() by
dev_kfree_skb_any(), as rx-offload might be called from threaded IRQ handlers
as well.
Fixes: ca913f1ac024 ("can: rx-offload: can_rx_offload_queue_sorted(): fix error handling, avoid skb mem leak")
Fixes: 6caf8a6d6586 ("can: rx-offload: can_rx_offload_queue_tail(): fix error handling, avoid skb mem leak")
Link: http://lore.kernel.org/r/20201019190524.1285319-3-mkl@pengutronix.de
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/rx-offload.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/rx-offload.c b/drivers/net/can/rx-offload.c
index 3b180269a92d..6e95193b215b 100644
--- a/drivers/net/can/rx-offload.c
+++ b/drivers/net/can/rx-offload.c
@@ -245,7 +245,7 @@ int can_rx_offload_queue_sorted(struct can_rx_offload *offload,
if (skb_queue_len(&offload->skb_queue) >
offload->skb_queue_len_max) {
- kfree_skb(skb);
+ dev_kfree_skb_any(skb);
return -ENOBUFS;
}
@@ -290,7 +290,7 @@ int can_rx_offload_queue_tail(struct can_rx_offload *offload,
{
if (skb_queue_len(&offload->skb_queue) >
offload->skb_queue_len_max) {
- kfree_skb(skb);
+ dev_kfree_skb_any(skb);
return -ENOBUFS;
}
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 05/27] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard IRQ context
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (3 preceding siblings ...)
2020-11-03 22:06 ` [net 04/27] can: rx-offload: don't call kfree_skb() from IRQ context Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-04 1:21 ` Jakub Kicinski
2020-11-03 22:06 ` [net 06/27] can: dev: __can_get_echo_skb(): fix real payload length return value for RTR frames Marc Kleine-Budde
` (22 subsequent siblings)
27 siblings, 1 reply; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, linux-can, kernel, Vincent Mailhol, Marc Kleine-Budde
From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
If a driver calls can_get_echo_skb() during a hardware IRQ (which is often, but
not always, the case), the 'WARN_ON(in_irq)' in
net/core/skbuff.c#skb_release_head_state() might be triggered, under network
congestion circumstances, together with the potential risk of a NULL pointer
dereference.
The root cause of this issue is the call to kfree_skb() instead of
dev_kfree_skb_irq() in net/core/dev.c#enqueue_to_backlog().
This patch prevents the skb to be freed within the call to netif_rx() by
incrementing its reference count with skb_get(). The skb is finally freed by
one of the in-irq-context safe functions: dev_consume_skb_any() or
dev_kfree_skb_any(). The "any" version is used because some drivers might call
can_get_echo_skb() in a normal context.
The reason for this issue to occur is that initially, in the core network
stack, loopback skb were not supposed to be received in hardware IRQ context.
The CAN stack is an exeption.
This bug was previously reported back in 2017 in [1] but the proposed patch
never got accepted.
While [1] directly modifies net/core/dev.c, we try to propose here a
smoother modification local to CAN network stack (the assumption
behind is that only CAN devices are affected by this issue).
[1] http://lore.kernel.org/r/57a3ffb6-3309-3ad5-5a34-e93c3fe3614d@cetitec.com
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Link: https://lore.kernel.org/r/20201002154219.4887-2-mailhol.vincent@wanadoo.fr
Fixes: 39549eef3587 ("can: CAN Network device driver and Netlink interface")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/dev.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index b70ded3760f2..73cfcd7e9517 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -538,7 +538,11 @@ unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx)
if (!skb)
return 0;
- netif_rx(skb);
+ skb_get(skb);
+ if (netif_rx(skb) == NET_RX_SUCCESS)
+ dev_consume_skb_any(skb);
+ else
+ dev_kfree_skb_any(skb);
return len;
}
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [net 05/27] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard IRQ context
2020-11-03 22:06 ` [net 05/27] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard " Marc Kleine-Budde
@ 2020-11-04 1:21 ` Jakub Kicinski
2020-11-04 4:29 ` Vincent MAILHOL
2020-11-04 8:16 ` Eric Dumazet
0 siblings, 2 replies; 41+ messages in thread
From: Jakub Kicinski @ 2020-11-04 1:21 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: netdev, davem, linux-can, kernel, Vincent Mailhol, Eric Dumazet
On Tue, 3 Nov 2020 23:06:14 +0100 Marc Kleine-Budde wrote:
> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>
> If a driver calls can_get_echo_skb() during a hardware IRQ (which is often, but
> not always, the case), the 'WARN_ON(in_irq)' in
> net/core/skbuff.c#skb_release_head_state() might be triggered, under network
> congestion circumstances, together with the potential risk of a NULL pointer
> dereference.
>
> The root cause of this issue is the call to kfree_skb() instead of
> dev_kfree_skb_irq() in net/core/dev.c#enqueue_to_backlog().
>
> This patch prevents the skb to be freed within the call to netif_rx() by
> incrementing its reference count with skb_get(). The skb is finally freed by
> one of the in-irq-context safe functions: dev_consume_skb_any() or
> dev_kfree_skb_any(). The "any" version is used because some drivers might call
> can_get_echo_skb() in a normal context.
>
> The reason for this issue to occur is that initially, in the core network
> stack, loopback skb were not supposed to be received in hardware IRQ context.
> The CAN stack is an exeption.
>
> This bug was previously reported back in 2017 in [1] but the proposed patch
> never got accepted.
>
> While [1] directly modifies net/core/dev.c, we try to propose here a
> smoother modification local to CAN network stack (the assumption
> behind is that only CAN devices are affected by this issue).
>
> [1] http://lore.kernel.org/r/57a3ffb6-3309-3ad5-5a34-e93c3fe3614d@cetitec.com
>
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> Link: https://lore.kernel.org/r/20201002154219.4887-2-mailhol.vincent@wanadoo.fr
> Fixes: 39549eef3587 ("can: CAN Network device driver and Netlink interface")
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
Hm... Why do we receive a skb with a socket attached?
At a quick glance this is some loopback, so shouldn't we skb_orphan()
in the xmit function instead?
Otherwise we should probably fix this in enqueue_to_backlog().
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index b70ded3760f2..73cfcd7e9517 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -538,7 +538,11 @@ unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx)
> if (!skb)
> return 0;
>
> - netif_rx(skb);
> + skb_get(skb);
> + if (netif_rx(skb) == NET_RX_SUCCESS)
> + dev_consume_skb_any(skb);
> + else
> + dev_kfree_skb_any(skb);
>
> return len;
> }
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [net 05/27] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard IRQ context
2020-11-04 1:21 ` Jakub Kicinski
@ 2020-11-04 4:29 ` Vincent MAILHOL
2020-11-04 8:16 ` Eric Dumazet
1 sibling, 0 replies; 41+ messages in thread
From: Vincent MAILHOL @ 2020-11-04 4:29 UTC (permalink / raw)
To: Jakub Kicinski, linux-can, Marc Kleine-Budde
Cc: netdev, davem, kernel, Eric Dumazet, Vincent Mailhol
On Wed. 4 Nov 2020 10:21, Jakub Kicinski wrote:
> On Tue, 3 Nov 2020 23:06:14 +0100 Marc Kleine-Budde wrote:
>> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>>
>> If a driver calls can_get_echo_skb() during a hardware IRQ (which is often, but
>> not always, the case), the 'WARN_ON(in_irq)' in
>> net/core/skbuff.c#skb_release_head_state() might be triggered, under network
>> congestion circumstances, together with the potential risk of a NULL pointer
>> dereference.
>>
>> The root cause of this issue is the call to kfree_skb() instead of
>> dev_kfree_skb_irq() in net/core/dev.c#enqueue_to_backlog().
>>
>> This patch prevents the skb to be freed within the call to netif_rx() by
>> incrementing its reference count with skb_get(). The skb is finally freed by
>> one of the in-irq-context safe functions: dev_consume_skb_any() or
>> dev_kfree_skb_any(). The "any" version is used because some drivers might call
>> can_get_echo_skb() in a normal context.
>>
>> The reason for this issue to occur is that initially, in the core network
>> stack, loopback skb were not supposed to be received in hardware IRQ context.
>> The CAN stack is an exception.
>>
>> This bug was previously reported back in 2017 in [1] but the proposed patch
>> never got accepted.
>>
>> While [1] directly modifies net/core/dev.c, we try to propose here a
>> smoother modification local to CAN network stack (the assumption
>> behind is that only CAN devices are affected by this issue).
>>
>> [1] http://lore.kernel.org/r/57a3ffb6-3309-3ad5-5a34-e93c3fe3614d@cetitec.com
>>
>> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>> Link: https://lore.kernel.org/r/20201002154219.4887-2-mailhol.vincent@wanadoo.fr
>> Fixes: 39549eef3587 ("can: CAN Network device driver and Netlink interface")
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>
> Hm... Why do we receive a skb with a socket attached?
>
> At a quick glance this is some loopback, so shouldn't we skb_orphan()
> in the xmit function instead?
This is a specific feature of SocketCAN. See:
https://www.kernel.org/doc/html/latest/networking/can.html#local-loopback-of-sent-frames
By default, each socket will receive the loopback packets from other
sockets but not its own sent frames. This behaviour can be controlled
by the socket option CAN_RAW_RECV_OWN_MSGS (c.f. member 'recv_own_msg'
in 'struct raw_sok':
https://elixir.bootlin.com/linux/latest/source/net/can/raw.c#L88)
This feature requires to have the socket attached to the skb.
Orphaning the skb would break it (c.f. function raw_rcv():
https://elixir.bootlin.com/linux/latest/source/net/can/raw.c#L116).
> Otherwise we should probably fix this in enqueue_to_backlog().
To my knowledge, this issue only occurs in SocketCAN, thus the idea to
try to fix it locally. But yes, replacing kfree_skb() with
dev_kfree_skb_any() in enqueue_to_backlog() would fix the issue as
well.
>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>> index b70ded3760f2..73cfcd7e9517 100644
>> --- a/drivers/net/can/dev.c
>> +++ b/drivers/net/can/dev.c
>> @@ -538,7 +538,11 @@ unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx)
>> if (!skb)
>> return 0;
>>
>> - netif_rx(skb);
>> + skb_get(skb);
>> + if (netif_rx(skb) == NET_RX_SUCCESS)
>> + dev_consume_skb_any(skb);
>> + else
>> + dev_kfree_skb_any(skb);
>>
>> return len;
>> }
>
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [net 05/27] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard IRQ context
2020-11-04 1:21 ` Jakub Kicinski
2020-11-04 4:29 ` Vincent MAILHOL
@ 2020-11-04 8:16 ` Eric Dumazet
2020-11-04 14:59 ` Oliver Hartkopp
1 sibling, 1 reply; 41+ messages in thread
From: Eric Dumazet @ 2020-11-04 8:16 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Marc Kleine-Budde, netdev, David Miller, linux-can, kernel,
Vincent Mailhol
On Wed, Nov 4, 2020 at 2:21 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 3 Nov 2020 23:06:14 +0100 Marc Kleine-Budde wrote:
> > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >
> > If a driver calls can_get_echo_skb() during a hardware IRQ (which is often, but
> > not always, the case), the 'WARN_ON(in_irq)' in
> > net/core/skbuff.c#skb_release_head_state() might be triggered, under network
> > congestion circumstances, together with the potential risk of a NULL pointer
> > dereference.
> >
> > The root cause of this issue is the call to kfree_skb() instead of
> > dev_kfree_skb_irq() in net/core/dev.c#enqueue_to_backlog().
> >
> > This patch prevents the skb to be freed within the call to netif_rx() by
> > incrementing its reference count with skb_get(). The skb is finally freed by
> > one of the in-irq-context safe functions: dev_consume_skb_any() or
> > dev_kfree_skb_any(). The "any" version is used because some drivers might call
> > can_get_echo_skb() in a normal context.
> >
> > The reason for this issue to occur is that initially, in the core network
> > stack, loopback skb were not supposed to be received in hardware IRQ context.
> > The CAN stack is an exeption.
> >
> > This bug was previously reported back in 2017 in [1] but the proposed patch
> > never got accepted.
> >
> > While [1] directly modifies net/core/dev.c, we try to propose here a
> > smoother modification local to CAN network stack (the assumption
> > behind is that only CAN devices are affected by this issue).
> >
> > [1] http://lore.kernel.org/r/57a3ffb6-3309-3ad5-5a34-e93c3fe3614d@cetitec.com
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > Link: https://lore.kernel.org/r/20201002154219.4887-2-mailhol.vincent@wanadoo.fr
> > Fixes: 39549eef3587 ("can: CAN Network device driver and Netlink interface")
> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>
> Hm... Why do we receive a skb with a socket attached?
>
> At a quick glance this is some loopback, so shouldn't we skb_orphan()
> in the xmit function instead?
Yes this would work, this seems the safest way, loopback_xmit() is a
good template for this.
>
> Otherwise we should probably fix this in enqueue_to_backlog().
This is dangerous.
If we drop packets under flood because the per-cpu backlog is full,
we might also be in _big_ trouble if the per-cpu
softnet_data.completion_queue is filling,
since we do not have a limit on this list.
What could happen is that when the memory is finally exhausted and no
more skb can be fed
to netif_rx(), a big latency spike would happen when
softnet_data.completion_queue
can finally be purged in one shot.
So skb_orphan(skb) in CAN before calling netif_rx() is better IMO.
>
> > diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> > index b70ded3760f2..73cfcd7e9517 100644
> > --- a/drivers/net/can/dev.c
> > +++ b/drivers/net/can/dev.c
> > @@ -538,7 +538,11 @@ unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx)
> > if (!skb)
> > return 0;
> >
> > - netif_rx(skb);
> > + skb_get(skb);
> > + if (netif_rx(skb) == NET_RX_SUCCESS)
> > + dev_consume_skb_any(skb);
> > + else
> > + dev_kfree_skb_any(skb);
> >
> > return len;
> > }
>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [net 05/27] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard IRQ context
2020-11-04 8:16 ` Eric Dumazet
@ 2020-11-04 14:59 ` Oliver Hartkopp
2020-11-04 16:02 ` Jakub Kicinski
0 siblings, 1 reply; 41+ messages in thread
From: Oliver Hartkopp @ 2020-11-04 14:59 UTC (permalink / raw)
To: Eric Dumazet, Jakub Kicinski
Cc: Marc Kleine-Budde, netdev, David Miller, linux-can, kernel,
Vincent Mailhol
Hello Eric,
On 04.11.20 09:16, Eric Dumazet wrote:
> On Wed, Nov 4, 2020 at 2:21 AM Jakub Kicinski <kuba@kernel.org> wrote:
>>
>> On Tue, 3 Nov 2020 23:06:14 +0100 Marc Kleine-Budde wrote:
>>> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>>>
>>> If a driver calls can_get_echo_skb() during a hardware IRQ (which is often, but
>>> not always, the case), the 'WARN_ON(in_irq)' in
>>> net/core/skbuff.c#skb_release_head_state() might be triggered, under network
>>> congestion circumstances, together with the potential risk of a NULL pointer
>>> dereference.
>>>
>>> The root cause of this issue is the call to kfree_skb() instead of
>>> dev_kfree_skb_irq() in net/core/dev.c#enqueue_to_backlog().
>>>
>>> This patch prevents the skb to be freed within the call to netif_rx() by
>>> incrementing its reference count with skb_get(). The skb is finally freed by
>>> one of the in-irq-context safe functions: dev_consume_skb_any() or
>>> dev_kfree_skb_any(). The "any" version is used because some drivers might call
>>> can_get_echo_skb() in a normal context.
>>>
>>> The reason for this issue to occur is that initially, in the core network
>>> stack, loopback skb were not supposed to be received in hardware IRQ context.
>>> The CAN stack is an exeption.
>>>
>>> This bug was previously reported back in 2017 in [1] but the proposed patch
>>> never got accepted.
>>>
>>> While [1] directly modifies net/core/dev.c, we try to propose here a
>>> smoother modification local to CAN network stack (the assumption
>>> behind is that only CAN devices are affected by this issue).
>>>
>>> [1] http://lore.kernel.org/r/57a3ffb6-3309-3ad5-5a34-e93c3fe3614d@cetitec.com
>>>
>>> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>>> Link: https://lore.kernel.org/r/20201002154219.4887-2-mailhol.vincent@wanadoo.fr
>>> Fixes: 39549eef3587 ("can: CAN Network device driver and Netlink interface")
>>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>>
>> Hm... Why do we receive a skb with a socket attached?
>>
>> At a quick glance this is some loopback, so shouldn't we skb_orphan()
>> in the xmit function instead?
>
> Yes this would work, this seems the safest way, loopback_xmit() is a
> good template for this.
>
>>
>> Otherwise we should probably fix this in enqueue_to_backlog().
>
> This is dangerous.
>
> If we drop packets under flood because the per-cpu backlog is full,
> we might also be in _big_ trouble if the per-cpu
> softnet_data.completion_queue is filling,
> since we do not have a limit on this list.
>
> What could happen is that when the memory is finally exhausted and no
> more skb can be fed
> to netif_rx(), a big latency spike would happen when
> softnet_data.completion_queue
> can finally be purged in one shot.
>
> So skb_orphan(skb) in CAN before calling netif_rx() is better IMO.
>
Unfortunately you missed the answer from Vincent, why skb_orphan() does
not work here:
https://lore.kernel.org/linux-can/CAMZ6RqJyZTcqZcq6jEzm5LLM_MMe=dYDbwvv=Y+dBR0drWuFmw@mail.gmail.com/
Best regards,
Oliver
>>
>>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>>> index b70ded3760f2..73cfcd7e9517 100644
>>> --- a/drivers/net/can/dev.c
>>> +++ b/drivers/net/can/dev.c
>>> @@ -538,7 +538,11 @@ unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx)
>>> if (!skb)
>>> return 0;
>>>
>>> - netif_rx(skb);
>>> + skb_get(skb);
>>> + if (netif_rx(skb) == NET_RX_SUCCESS)
>>> + dev_consume_skb_any(skb);
>>> + else
>>> + dev_kfree_skb_any(skb);
>>>
>>> return len;
>>> }
>>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [net 05/27] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard IRQ context
2020-11-04 14:59 ` Oliver Hartkopp
@ 2020-11-04 16:02 ` Jakub Kicinski
2020-11-04 17:46 ` Oliver Hartkopp
0 siblings, 1 reply; 41+ messages in thread
From: Jakub Kicinski @ 2020-11-04 16:02 UTC (permalink / raw)
To: Oliver Hartkopp, Vincent Mailhol
Cc: Eric Dumazet, Marc Kleine-Budde, netdev, David Miller, linux-can, kernel
On Wed, 4 Nov 2020 15:59:25 +0100 Oliver Hartkopp wrote:
> Hello Eric,
>
> On 04.11.20 09:16, Eric Dumazet wrote:
> > On Wed, Nov 4, 2020 at 2:21 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >>
> >> On Tue, 3 Nov 2020 23:06:14 +0100 Marc Kleine-Budde wrote:
> >>> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >>>
> >>> If a driver calls can_get_echo_skb() during a hardware IRQ (which is often, but
> >>> not always, the case), the 'WARN_ON(in_irq)' in
> >>> net/core/skbuff.c#skb_release_head_state() might be triggered, under network
> >>> congestion circumstances, together with the potential risk of a NULL pointer
> >>> dereference.
> >>>
> >>> The root cause of this issue is the call to kfree_skb() instead of
> >>> dev_kfree_skb_irq() in net/core/dev.c#enqueue_to_backlog().
> >>>
> >>> This patch prevents the skb to be freed within the call to netif_rx() by
> >>> incrementing its reference count with skb_get(). The skb is finally freed by
> >>> one of the in-irq-context safe functions: dev_consume_skb_any() or
> >>> dev_kfree_skb_any(). The "any" version is used because some drivers might call
> >>> can_get_echo_skb() in a normal context.
> >>>
> >>> The reason for this issue to occur is that initially, in the core network
> >>> stack, loopback skb were not supposed to be received in hardware IRQ context.
> >>> The CAN stack is an exeption.
> >>>
> >>> This bug was previously reported back in 2017 in [1] but the proposed patch
> >>> never got accepted.
> >>>
> >>> While [1] directly modifies net/core/dev.c, we try to propose here a
> >>> smoother modification local to CAN network stack (the assumption
> >>> behind is that only CAN devices are affected by this issue).
> >>>
> >>> [1] http://lore.kernel.org/r/57a3ffb6-3309-3ad5-5a34-e93c3fe3614d@cetitec.com
> >>>
> >>> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >>> Link: https://lore.kernel.org/r/20201002154219.4887-2-mailhol.vincent@wanadoo.fr
> >>> Fixes: 39549eef3587 ("can: CAN Network device driver and Netlink interface")
> >>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> >>
> >> Hm... Why do we receive a skb with a socket attached?
> >>
> >> At a quick glance this is some loopback, so shouldn't we skb_orphan()
> >> in the xmit function instead?
> >
> > Yes this would work, this seems the safest way, loopback_xmit() is a
> > good template for this.
> >
> >>
> >> Otherwise we should probably fix this in enqueue_to_backlog().
> >
> > This is dangerous.
> >
> > If we drop packets under flood because the per-cpu backlog is full,
> > we might also be in _big_ trouble if the per-cpu
> > softnet_data.completion_queue is filling,
> > since we do not have a limit on this list.
> >
> > What could happen is that when the memory is finally exhausted and no
> > more skb can be fed
> > to netif_rx(), a big latency spike would happen when
> > softnet_data.completion_queue
> > can finally be purged in one shot.
Thanks, that makes sense. So no to the enqueue_to_backlog() idea.
> > So skb_orphan(skb) in CAN before calling netif_rx() is better IMO.
> >
>
> Unfortunately you missed the answer from Vincent, why skb_orphan() does
> not work here:
>
> https://lore.kernel.org/linux-can/CAMZ6RqJyZTcqZcq6jEzm5LLM_MMe=dYDbwvv=Y+dBR0drWuFmw@mail.gmail.com/
Okay, we can take this as a quick fix but to me it seems a little
strange to be dropping what is effectively locally generated frames.
Can we use a NAPI poll model here and back pressure TX if the echo
is not keeping up?
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [net 05/27] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard IRQ context
2020-11-04 16:02 ` Jakub Kicinski
@ 2020-11-04 17:46 ` Oliver Hartkopp
2020-11-05 4:47 ` Vincent MAILHOL
0 siblings, 1 reply; 41+ messages in thread
From: Oliver Hartkopp @ 2020-11-04 17:46 UTC (permalink / raw)
To: Jakub Kicinski, Marc Kleine-Budde
Cc: Vincent Mailhol, Eric Dumazet, netdev, David Miller, linux-can, kernel
On 04.11.20 17:02, Jakub Kicinski wrote:
> On Wed, 4 Nov 2020 15:59:25 +0100 Oliver Hartkopp wrote:
>> On 04.11.20 09:16, Eric Dumazet wrote:
>>> So skb_orphan(skb) in CAN before calling netif_rx() is better IMO.
>>>
>>
>> Unfortunately you missed the answer from Vincent, why skb_orphan() does
>> not work here:
>>
>> https://lore.kernel.org/linux-can/CAMZ6RqJyZTcqZcq6jEzm5LLM_MMe=dYDbwvv=Y+dBR0drWuFmw@mail.gmail.com/
>
> Okay, we can take this as a quick fix but to me it seems a little
> strange to be dropping what is effectively locally generated frames.
Thanks! So this patch doesn't hinder Marc's PR :-)
> Can we use a NAPI poll model here and back pressure TX if the echo
> is not keeping up?
Some of the CAN network drivers already support NAPI.
@Marc: Can we also use NAPI for echo'ing the skbs?
Best regards,
Oliver
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net 05/27] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard IRQ context
2020-11-04 17:46 ` Oliver Hartkopp
@ 2020-11-05 4:47 ` Vincent MAILHOL
2020-11-05 7:44 ` Marc Kleine-Budde
0 siblings, 1 reply; 41+ messages in thread
From: Vincent MAILHOL @ 2020-11-05 4:47 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Jakub Kicinski, Marc Kleine-Budde, Eric Dumazet, netdev,
David Miller, linux-can, kernel
On Wed, 5 Nov 2020 02:46, Oliver Hartkopp wrote:
> On 04.11.20 17:02, Jakub Kicinski wrote:
>> On Wed, 4 Nov 2020 15:59:25 +0100 Oliver Hartkopp wrote:
>>> On 04.11.20 09:16, Eric Dumazet wrote:
>
>>>> So skb_orphan(skb) in CAN before calling netif_rx() is better IMO.
>>>>
>>>
>>> Unfortunately you missed the answer from Vincent, why skb_orphan() does
>>> not work here:
Hope that my answer did not go to the spam box.
>>> https://lore.kernel.org/linux-can/CAMZ6RqJyZTcqZcq6jEzm5LLM_MMe=dYDbwvv=Y+dBR0drWuFmw@mail.gmail.com/
>>
>> Okay, we can take this as a quick fix but to me it seems a little
>> strange to be dropping what is effectively locally generated frames.
For those who are not familiar with SocketCAN and to make sure that we
are all aligned here, let me give a bit of context of how the echo on CAN
skbs is usually implement in the drivers:
* In the xmit() branch, the driver would queue the skb using
can_put_echo_skb().
* Whenever the driver gets notified of a successful TX, it will loopback
the skb using can_get_echo_skb().
This is why the loopback is usually done in hardware IRQ context (but
drivers are also free to skip the second step and directly loopback the
skb in the xmit() branch).
> Thanks! So this patch doesn't hinder Marc's PR :-)
>
>> Can we use a NAPI poll model here and back pressure TX if the echo
>> is not keeping up?
>
> Some of the CAN network drivers already support NAPI.
I had a quick look at NAPI in the past and my understanding is that it
requires the ability to turn off hardware interrupts meaning that it can
be only used on some NIC, not but not, for example, on USB devices.
It would be nice to extend the NAPI with skb loopback for drivers which
already supports it but I am not sure how to include the other drivers.
> @Marc: Can we also use NAPI for echo'ing the skbs?
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net 05/27] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard IRQ context
2020-11-05 4:47 ` Vincent MAILHOL
@ 2020-11-05 7:44 ` Marc Kleine-Budde
0 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-05 7:44 UTC (permalink / raw)
To: Vincent MAILHOL, Oliver Hartkopp
Cc: Jakub Kicinski, Eric Dumazet, netdev, David Miller, linux-can, kernel
[-- Attachment #1.1: Type: text/plain, Size: 2236 bytes --]
On 11/5/20 5:47 AM, Vincent MAILHOL wrote:
>>> Okay, we can take this as a quick fix but to me it seems a little
>>> strange to be dropping what is effectively locally generated frames.
>
> For those who are not familiar with SocketCAN and to make sure that we
> are all aligned here, let me give a bit of context of how the echo on CAN
> skbs is usually implement in the drivers:
> * In the xmit() branch, the driver would queue the skb using
> can_put_echo_skb().
> * Whenever the driver gets notified of a successful TX, it will loopback
> the skb using can_get_echo_skb().
>
> This is why the loopback is usually done in hardware IRQ context (but
> drivers are also free to skip the second step and directly loopback the
> skb in the xmit() branch).
>
>> Thanks! So this patch doesn't hinder Marc's PR :-)
>>
>>> Can we use a NAPI poll model here and back pressure TX if the echo
>>> is not keeping up?
>>
>> Some of the CAN network drivers already support NAPI.
And some drivers (i.e. flexcan and mcp251xfd) already queue the ech_skb via NAPI.
> I had a quick look at NAPI in the past and my understanding is that it
> requires the ability to turn off hardware interrupts meaning that it can
> be only used on some NIC, not but not, for example, on USB devices.
>
> It would be nice to extend the NAPI with skb loopback for drivers which
> already supports it but I am not sure how to include the other drivers.
>
>> @Marc: Can we also use NAPI for echo'ing the skbs?
The flexcan driver does:
https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/net/can/flexcan.c#L1080
In the interrupt handler, the driver pushes the echo_skb into the rx-offload
queue and triggers NAPI. The rx-offload pushed the skb into the networking stack
via NAPI.
Here the code in the mcp251xfd driver:
https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c#L1237
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 |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* [net 06/27] can: dev: __can_get_echo_skb(): fix real payload length return value for RTR frames
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (4 preceding siblings ...)
2020-11-03 22:06 ` [net 05/27] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard " Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 07/27] can: can_create_echo_skb(): fix echo skb generation: always use skb_clone() Marc Kleine-Budde
` (21 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Oliver Hartkopp, Vincent Mailhol,
Marc Kleine-Budde
From: Oliver Hartkopp <socketcan@hartkopp.net>
The can_get_echo_skb() function returns the number of received bytes to
be used for netdev statistics. In the case of RTR frames we get a valid
(potential non-zero) data length value which has to be passed for further
operations. But on the wire RTR frames have no payload length. Therefore
the value to be used in the statistics has to be zero for RTR frames.
Reported-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Link: https://lore.kernel.org/r/20201020064443.80164-1-socketcan@hartkopp.net
Fixes: cf5046b309b3 ("can: dev: let can_get_echo_skb() return dlc of CAN frame")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/dev.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 73cfcd7e9517..6dee4f8f2024 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -512,9 +512,13 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr)
*/
struct sk_buff *skb = priv->echo_skb[idx];
struct canfd_frame *cf = (struct canfd_frame *)skb->data;
- u8 len = cf->len;
- *len_ptr = len;
+ /* get the real payload length for netdev statistics */
+ if (cf->can_id & CAN_RTR_FLAG)
+ *len_ptr = 0;
+ else
+ *len_ptr = cf->len;
+
priv->echo_skb[idx] = NULL;
return skb;
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 07/27] can: can_create_echo_skb(): fix echo skb generation: always use skb_clone()
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (5 preceding siblings ...)
2020-11-03 22:06 ` [net 06/27] can: dev: __can_get_echo_skb(): fix real payload length return value for RTR frames Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 08/27] can: j1939: rename jacd tool Marc Kleine-Budde
` (20 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Oleksij Rempel, Oliver Hartkopp,
Marc Kleine-Budde
From: Oleksij Rempel <o.rempel@pengutronix.de>
All user space generated SKBs are owned by a socket (unless injected into the
key via AF_PACKET). If a socket is closed, all associated skbs will be cleaned
up.
This leads to a problem when a CAN driver calls can_put_echo_skb() on a
unshared SKB. If the socket is closed prior to the TX complete handler,
can_get_echo_skb() and the subsequent delivering of the echo SKB to all
registered callbacks, a SKB with a refcount of 0 is delivered.
To avoid the problem, in can_get_echo_skb() the original SKB is now always
cloned, regardless of shared SKB or not. If the process exists it can now
safely discard its SKBs, without disturbing the delivery of the echo SKB.
The problem shows up in the j1939 stack, when it clones the incoming skb, which
detects the already 0 refcount.
We can easily reproduce this with following example:
testj1939 -B -r can0: &
cansend can0 1823ff40#0123
WARNING: CPU: 0 PID: 293 at lib/refcount.c:25 refcount_warn_saturate+0x108/0x174
refcount_t: addition on 0; use-after-free.
Modules linked in: coda_vpu imx_vdoa videobuf2_vmalloc dw_hdmi_ahb_audio vcan
CPU: 0 PID: 293 Comm: cansend Not tainted 5.5.0-rc6-00376-g9e20dcb7040d #1
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
Backtrace:
[<c010f570>] (dump_backtrace) from [<c010f90c>] (show_stack+0x20/0x24)
[<c010f8ec>] (show_stack) from [<c0c3e1a4>] (dump_stack+0x8c/0xa0)
[<c0c3e118>] (dump_stack) from [<c0127fec>] (__warn+0xe0/0x108)
[<c0127f0c>] (__warn) from [<c01283c8>] (warn_slowpath_fmt+0xa8/0xcc)
[<c0128324>] (warn_slowpath_fmt) from [<c0539c0c>] (refcount_warn_saturate+0x108/0x174)
[<c0539b04>] (refcount_warn_saturate) from [<c0ad2cac>] (j1939_can_recv+0x20c/0x210)
[<c0ad2aa0>] (j1939_can_recv) from [<c0ac9dc8>] (can_rcv_filter+0xb4/0x268)
[<c0ac9d14>] (can_rcv_filter) from [<c0aca2cc>] (can_receive+0xb0/0xe4)
[<c0aca21c>] (can_receive) from [<c0aca348>] (can_rcv+0x48/0x98)
[<c0aca300>] (can_rcv) from [<c09b1fdc>] (__netif_receive_skb_one_core+0x64/0x88)
[<c09b1f78>] (__netif_receive_skb_one_core) from [<c09b2070>] (__netif_receive_skb+0x38/0x94)
[<c09b2038>] (__netif_receive_skb) from [<c09b2130>] (netif_receive_skb_internal+0x64/0xf8)
[<c09b20cc>] (netif_receive_skb_internal) from [<c09b21f8>] (netif_receive_skb+0x34/0x19c)
[<c09b21c4>] (netif_receive_skb) from [<c0791278>] (can_rx_offload_napi_poll+0x58/0xb4)
Fixes: 0ae89beb283a ("can: add destructor for self generated skbs")
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Link: http://lore.kernel.org/r/20200124132656.22156-1-o.rempel@pengutronix.de
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
include/linux/can/skb.h | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index 900b9f4e0605..fc61cf4eff1c 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -61,21 +61,17 @@ static inline void can_skb_set_owner(struct sk_buff *skb, struct sock *sk)
*/
static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
{
- if (skb_shared(skb)) {
- struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
+ struct sk_buff *nskb;
- if (likely(nskb)) {
- can_skb_set_owner(nskb, skb->sk);
- consume_skb(skb);
- return nskb;
- } else {
- kfree_skb(skb);
- return NULL;
- }
+ nskb = skb_clone(skb, GFP_ATOMIC);
+ if (unlikely(!nskb)) {
+ kfree_skb(skb);
+ return NULL;
}
- /* we can assume to have an unshared skb with proper owner */
- return skb;
+ can_skb_set_owner(nskb, skb->sk);
+ consume_skb(skb);
+ return nskb;
}
#endif /* !_CAN_SKB_H */
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 08/27] can: j1939: rename jacd tool
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (6 preceding siblings ...)
2020-11-03 22:06 ` [net 07/27] can: can_create_echo_skb(): fix echo skb generation: always use skb_clone() Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 09/27] can: j1939: fix syntax and spelling Marc Kleine-Budde
` (19 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, linux-can, kernel, Yegor Yefremov, Marc Kleine-Budde
From: Yegor Yefremov <yegorslists@googlemail.com>
Due to naming conflicts, jacd was renamed to j1939acd in:
https://github.com/linux-can/can-utils/pull/199
Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
Link: https://lore.kernel.org/r/20201020081134.3597-1-yegorslists@googlemail.com
Link: https://github.com/linux-can/can-utils/pull/199
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Documentation/networking/j1939.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/networking/j1939.rst b/Documentation/networking/j1939.rst
index f5be243d250a..081dfc2e0452 100644
--- a/Documentation/networking/j1939.rst
+++ b/Documentation/networking/j1939.rst
@@ -376,7 +376,7 @@ only j1939.addr changed to the new SA, and must then send a valid address claim
packet. This restarts the state machine in the kernel (and any other
participant on the bus) for this NAME.
-can-utils also include the jacd tool, so it can be used as code example or as
+can-utils also include the j1939acd tool, so it can be used as code example or as
default Address Claiming daemon.
Send Examples
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 09/27] can: j1939: fix syntax and spelling
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (7 preceding siblings ...)
2020-11-03 22:06 ` [net 08/27] can: j1939: rename jacd tool Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 10/27] can: j1939: swap addr and pgn in the send example Marc Kleine-Budde
` (18 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, linux-can, kernel, Yegor Yefremov, Marc Kleine-Budde
From: Yegor Yefremov <yegorslists@googlemail.com>
This patches fixes the syntax an spelling of the j1939 documentation.
Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
Link: https://lore.kernel.org/r/20201020101043.6369-1-yegorslists@googlemail.com
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Documentation/networking/j1939.rst | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/Documentation/networking/j1939.rst b/Documentation/networking/j1939.rst
index 081dfc2e0452..be59fcece3bf 100644
--- a/Documentation/networking/j1939.rst
+++ b/Documentation/networking/j1939.rst
@@ -10,9 +10,9 @@ Overview / What Is J1939
SAE J1939 defines a higher layer protocol on CAN. It implements a more
sophisticated addressing scheme and extends the maximum packet size above 8
bytes. Several derived specifications exist, which differ from the original
-J1939 on the application level, like MilCAN A, NMEA2000 and especially
+J1939 on the application level, like MilCAN A, NMEA2000, and especially
ISO-11783 (ISOBUS). This last one specifies the so-called ETP (Extended
-Transport Protocol) which is has been included in this implementation. This
+Transport Protocol), which has been included in this implementation. This
results in a maximum packet size of ((2 ^ 24) - 1) * 7 bytes == 111 MiB.
Specifications used
@@ -32,15 +32,15 @@ sockets, we found some reasons to justify a kernel implementation for the
addressing and transport methods used by J1939.
* **Addressing:** when a process on an ECU communicates via J1939, it should
- not necessarily know its source address. Although at least one process per
+ not necessarily know its source address. Although, at least one process per
ECU should know the source address. Other processes should be able to reuse
that address. This way, address parameters for different processes
cooperating for the same ECU, are not duplicated. This way of working is
- closely related to the UNIX concept where programs do just one thing, and do
+ closely related to the UNIX concept, where programs do just one thing and do
it well.
* **Dynamic addressing:** Address Claiming in J1939 is time critical.
- Furthermore data transport should be handled properly during the address
+ Furthermore, data transport should be handled properly during the address
negotiation. Putting this functionality in the kernel eliminates it as a
requirement for _every_ user space process that communicates via J1939. This
results in a consistent J1939 bus with proper addressing.
@@ -58,7 +58,7 @@ Therefore, these parts are left to user space.
The J1939 sockets operate on CAN network devices (see SocketCAN). Any J1939
user space library operating on CAN raw sockets will still operate properly.
-Since such library does not communicate with the in-kernel implementation, care
+Since such a library does not communicate with the in-kernel implementation, care
must be taken that these two do not interfere. In practice, this means they
cannot share ECU addresses. A single ECU (or virtual ECU) address is used by
the library exclusively, or by the in-kernel system exclusively.
@@ -77,13 +77,13 @@ is composed as follows:
8 bits : PS (PDU Specific)
In J1939-21 distinction is made between PDU1 format (where PF < 240) and PDU2
-format (where PF >= 240). Furthermore, when using PDU2 format, the PS-field
+format (where PF >= 240). Furthermore, when using the PDU2 format, the PS-field
contains a so-called Group Extension, which is part of the PGN. When using PDU2
format, the Group Extension is set in the PS-field.
On the other hand, when using PDU1 format, the PS-field contains a so-called
Destination Address, which is _not_ part of the PGN. When communicating a PGN
-from user space to kernel (or visa versa) and PDU2 format is used, the PS-field
+from user space to kernel (or vice versa) and PDU2 format is used, the PS-field
of the PGN shall be set to zero. The Destination Address shall be set
elsewhere.
@@ -96,15 +96,15 @@ Addressing
Both static and dynamic addressing methods can be used.
-For static addresses, no extra checks are made by the kernel, and provided
+For static addresses, no extra checks are made by the kernel and provided
addresses are considered right. This responsibility is for the OEM or system
integrator.
For dynamic addressing, so-called Address Claiming, extra support is foreseen
-in the kernel. In J1939 any ECU is known by it's 64-bit NAME. At the moment of
+in the kernel. In J1939 any ECU is known by its 64-bit NAME. At the moment of
a successful address claim, the kernel keeps track of both NAME and source
address being claimed. This serves as a base for filter schemes. By default,
-packets with a destination that is not locally, will be rejected.
+packets with a destination that is not locally will be rejected.
Mixed mode packets (from a static to a dynamic address or vice versa) are
allowed. The BSD sockets define separate API calls for getting/setting the
@@ -153,8 +153,8 @@ described below.
In order to send data, a bind(2) must have been successful. bind(2) assigns a
local address to a socket.
-Different from CAN is that the payload data is just the data that get send,
-without it's header info. The header info is derived from the sockaddr supplied
+Different from CAN is that the payload data is just the data that get sends,
+without its header info. The header info is derived from the sockaddr supplied
to bind(2), connect(2), sendto(2) and recvfrom(2). A write(2) with size 4 will
result in a packet with 4 bytes.
@@ -191,7 +191,7 @@ can_addr.j1939.addr contains the address.
The bind(2) system call assigns the local address, i.e. the source address when
sending packages. If a PGN during bind(2) is set, it's used as a RX filter.
-I.e. only packets with a matching PGN are received. If an ADDR or NAME is set
+I.e. only packets with a matching PGN are received. If an ADDR or NAME is set
it is used as a receive filter, too. It will match the destination NAME or ADDR
of the incoming packet. The NAME filter will work only if appropriate Address
Claiming for this name was done on the CAN bus and registered/cached by the
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 10/27] can: j1939: swap addr and pgn in the send example
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (8 preceding siblings ...)
2020-11-03 22:06 ` [net 09/27] can: j1939: fix syntax and spelling Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 11/27] can: j1939: use backquotes for code samples Marc Kleine-Budde
` (17 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, linux-can, kernel, Yegor Yefremov, Marc Kleine-Budde
From: Yegor Yefremov <yegorslists@googlemail.com>
The address was wrongly assigned to the PGN field and vice versa.
Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
Link: https://lore.kernel.org/r/20201022083708.8755-1-yegorslists@googlemail.com
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Documentation/networking/j1939.rst | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/networking/j1939.rst b/Documentation/networking/j1939.rst
index be59fcece3bf..faf2eb5c5052 100644
--- a/Documentation/networking/j1939.rst
+++ b/Documentation/networking/j1939.rst
@@ -414,8 +414,8 @@ Send:
.can_family = AF_CAN,
.can_addr.j1939 = {
.name = J1939_NO_NAME;
- .pgn = 0x30,
- .addr = 0x12300,
+ .addr = 0x30,
+ .pgn = 0x12300,
},
};
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 11/27] can: j1939: use backquotes for code samples
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (9 preceding siblings ...)
2020-11-03 22:06 ` [net 10/27] can: j1939: swap addr and pgn in the send example Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 12/27] can: j1939: j1939_sk_bind(): return failure if netdev is down Marc Kleine-Budde
` (16 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, linux-can, kernel, Yegor Yefremov, Marc Kleine-Budde
From: Yegor Yefremov <yegorslists@googlemail.com>
This patch adds backquotes for code samples.
Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
Link: https://lore.kernel.org/r/20201026094442.16587-1-yegorslists@googlemail.com
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Documentation/networking/j1939.rst | 88 +++++++++++++++---------------
1 file changed, 44 insertions(+), 44 deletions(-)
diff --git a/Documentation/networking/j1939.rst b/Documentation/networking/j1939.rst
index faf2eb5c5052..0a4b73b03b99 100644
--- a/Documentation/networking/j1939.rst
+++ b/Documentation/networking/j1939.rst
@@ -131,31 +131,31 @@ API Calls
---------
On CAN, you first need to open a socket for communicating over a CAN network.
-To use J1939, #include <linux/can/j1939.h>. From there, <linux/can.h> will be
+To use J1939, ``#include <linux/can/j1939.h>``. From there, ``<linux/can.h>`` will be
included too. To open a socket, use:
.. code-block:: C
s = socket(PF_CAN, SOCK_DGRAM, CAN_J1939);
-J1939 does use SOCK_DGRAM sockets. In the J1939 specification, connections are
+J1939 does use ``SOCK_DGRAM`` sockets. In the J1939 specification, connections are
mentioned in the context of transport protocol sessions. These still deliver
-packets to the other end (using several CAN packets). SOCK_STREAM is not
+packets to the other end (using several CAN packets). ``SOCK_STREAM`` is not
supported.
-After the successful creation of the socket, you would normally use the bind(2)
-and/or connect(2) system call to bind the socket to a CAN interface. After
-binding and/or connecting the socket, you can read(2) and write(2) from/to the
-socket or use send(2), sendto(2), sendmsg(2) and the recv*() counterpart
+After the successful creation of the socket, you would normally use the ``bind(2)``
+and/or ``connect(2)`` system call to bind the socket to a CAN interface. After
+binding and/or connecting the socket, you can ``read(2)`` and ``write(2)`` from/to the
+socket or use ``send(2)``, ``sendto(2)``, ``sendmsg(2)`` and the ``recv*()`` counterpart
operations on the socket as usual. There are also J1939 specific socket options
described below.
-In order to send data, a bind(2) must have been successful. bind(2) assigns a
+In order to send data, a ``bind(2)`` must have been successful. ``bind(2)`` assigns a
local address to a socket.
Different from CAN is that the payload data is just the data that get sends,
without its header info. The header info is derived from the sockaddr supplied
-to bind(2), connect(2), sendto(2) and recvfrom(2). A write(2) with size 4 will
+to ``bind(2)``, ``connect(2)``, ``sendto(2)`` and ``recvfrom(2)``. A ``write(2)`` with size 4 will
result in a packet with 4 bytes.
The sockaddr structure has extensions for use with J1939 as specified below:
@@ -180,47 +180,47 @@ The sockaddr structure has extensions for use with J1939 as specified below:
} can_addr;
}
-can_family & can_ifindex serve the same purpose as for other SocketCAN sockets.
+``can_family`` & ``can_ifindex`` serve the same purpose as for other SocketCAN sockets.
-can_addr.j1939.pgn specifies the PGN (max 0x3ffff). Individual bits are
+``can_addr.j1939.pgn`` specifies the PGN (max 0x3ffff). Individual bits are
specified above.
-can_addr.j1939.name contains the 64-bit J1939 NAME.
+``can_addr.j1939.name`` contains the 64-bit J1939 NAME.
-can_addr.j1939.addr contains the address.
+``can_addr.j1939.addr`` contains the address.
-The bind(2) system call assigns the local address, i.e. the source address when
-sending packages. If a PGN during bind(2) is set, it's used as a RX filter.
+The ``bind(2)`` system call assigns the local address, i.e. the source address when
+sending packages. If a PGN during ``bind(2)`` is set, it's used as a RX filter.
I.e. only packets with a matching PGN are received. If an ADDR or NAME is set
it is used as a receive filter, too. It will match the destination NAME or ADDR
of the incoming packet. The NAME filter will work only if appropriate Address
Claiming for this name was done on the CAN bus and registered/cached by the
kernel.
-On the other hand connect(2) assigns the remote address, i.e. the destination
-address. The PGN from connect(2) is used as the default PGN when sending
+On the other hand ``connect(2)`` assigns the remote address, i.e. the destination
+address. The PGN from ``connect(2)`` is used as the default PGN when sending
packets. If ADDR or NAME is set it will be used as the default destination ADDR
-or NAME. Further a set ADDR or NAME during connect(2) is used as a receive
+or NAME. Further a set ADDR or NAME during ``connect(2)`` is used as a receive
filter. It will match the source NAME or ADDR of the incoming packet.
-Both write(2) and send(2) will send a packet with local address from bind(2) and
-the remote address from connect(2). Use sendto(2) to overwrite the destination
+Both ``write(2)`` and ``send(2)`` will send a packet with local address from ``bind(2)`` and the
+remote address from ``connect(2)``. Use ``sendto(2)`` to overwrite the destination
address.
-If can_addr.j1939.name is set (!= 0) the NAME is looked up by the kernel and
-the corresponding ADDR is used. If can_addr.j1939.name is not set (== 0),
-can_addr.j1939.addr is used.
+If ``can_addr.j1939.name`` is set (!= 0) the NAME is looked up by the kernel and
+the corresponding ADDR is used. If ``can_addr.j1939.name`` is not set (== 0),
+``can_addr.j1939.addr`` is used.
When creating a socket, reasonable defaults are set. Some options can be
-modified with setsockopt(2) & getsockopt(2).
+modified with ``setsockopt(2)`` & ``getsockopt(2)``.
RX path related options:
-- SO_J1939_FILTER - configure array of filters
-- SO_J1939_PROMISC - disable filters set by bind(2) and connect(2)
+- ``SO_J1939_FILTER`` - configure array of filters
+- ``SO_J1939_PROMISC`` - disable filters set by ``bind(2)`` and ``connect(2)``
By default no broadcast packets can be send or received. To enable sending or
-receiving broadcast packets use the socket option SO_BROADCAST:
+receiving broadcast packets use the socket option ``SO_BROADCAST``:
.. code-block:: C
@@ -261,26 +261,26 @@ The following diagram illustrates the RX path:
+---------------------------+
TX path related options:
-SO_J1939_SEND_PRIO - change default send priority for the socket
+``SO_J1939_SEND_PRIO`` - change default send priority for the socket
Message Flags during send() and Related System Calls
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-send(2), sendto(2) and sendmsg(2) take a 'flags' argument. Currently
+``send(2)``, ``sendto(2)`` and ``sendmsg(2)`` take a 'flags' argument. Currently
supported flags are:
-* MSG_DONTWAIT, i.e. non-blocking operation.
+* ``MSG_DONTWAIT``, i.e. non-blocking operation.
recvmsg(2)
^^^^^^^^^^
-In most cases recvmsg(2) is needed if you want to extract more information than
-recvfrom(2) can provide. For example package priority and timestamp. The
+In most cases ``recvmsg(2)`` is needed if you want to extract more information than
+``recvfrom(2)`` can provide. For example package priority and timestamp. The
Destination Address, name and packet priority (if applicable) are attached to
-the msghdr in the recvmsg(2) call. They can be extracted using cmsg(3) macros,
-with cmsg_level == SOL_J1939 && cmsg_type == SCM_J1939_DEST_ADDR,
-SCM_J1939_DEST_NAME or SCM_J1939_PRIO. The returned data is a uint8_t for
-priority and dst_addr, and uint64_t for dst_name.
+the msghdr in the ``recvmsg(2)`` call. They can be extracted using ``cmsg(3)`` macros,
+with ``cmsg_level == SOL_J1939 && cmsg_type == SCM_J1939_DEST_ADDR``,
+``SCM_J1939_DEST_NAME`` or ``SCM_J1939_PRIO``. The returned data is a ``uint8_t`` for
+``priority`` and ``dst_addr``, and ``uint64_t`` for ``dst_name``.
.. code-block:: C
@@ -305,12 +305,12 @@ Dynamic Addressing
Distinction has to be made between using the claimed address and doing an
address claim. To use an already claimed address, one has to fill in the
-j1939.name member and provide it to bind(2). If the name had claimed an address
+``j1939.name`` member and provide it to ``bind(2)``. If the name had claimed an address
earlier, all further messages being sent will use that address. And the
-j1939.addr member will be ignored.
+``j1939.addr`` member will be ignored.
An exception on this is PGN 0x0ee00. This is the "Address Claim/Cannot Claim
-Address" message and the kernel will use the j1939.addr member for that PGN if
+Address" message and the kernel will use the ``j1939.addr`` member for that PGN if
necessary.
To claim an address following code example can be used:
@@ -371,12 +371,12 @@ NAME can send packets.
If another ECU claims the address, the kernel will mark the NAME-SA expired.
No socket bound to the NAME can send packets (other than address claims). To
-claim another address, some socket bound to NAME, must bind(2) again, but with
-only j1939.addr changed to the new SA, and must then send a valid address claim
+claim another address, some socket bound to NAME, must ``bind(2)`` again, but with
+only ``j1939.addr`` changed to the new SA, and must then send a valid address claim
packet. This restarts the state machine in the kernel (and any other
participant on the bus) for this NAME.
-can-utils also include the j1939acd tool, so it can be used as code example or as
+``can-utils`` also include the ``j1939acd`` tool, so it can be used as code example or as
default Address Claiming daemon.
Send Examples
@@ -403,8 +403,8 @@ Bind:
bind(sock, (struct sockaddr *)&baddr, sizeof(baddr));
-Now, the socket 'sock' is bound to the SA 0x20. Since no connect(2) was called,
-at this point we can use only sendto(2) or sendmsg(2).
+Now, the socket 'sock' is bound to the SA 0x20. Since no ``connect(2)`` was called,
+at this point we can use only ``sendto(2)`` or ``sendmsg(2)``.
Send:
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 12/27] can: j1939: j1939_sk_bind(): return failure if netdev is down
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (10 preceding siblings ...)
2020-11-03 22:06 ` [net 11/27] can: j1939: use backquotes for code samples Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 13/27] can: isotp: Explain PDU in CAN_ISOTP help text Marc Kleine-Budde
` (15 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Zhang Changzhong, Oleksij Rempel,
Marc Kleine-Budde
From: Zhang Changzhong <zhangchangzhong@huawei.com>
When a netdev down event occurs after a successful call to
j1939_sk_bind(), j1939_netdev_notify() can handle it correctly.
But if the netdev already in down state before calling j1939_sk_bind(),
j1939_sk_release() will stay in wait_event_interruptible() blocked
forever. Because in this case, j1939_netdev_notify() won't be called and
j1939_tp_txtimer() won't call j1939_session_cancel() or other function
to clear session for ENETDOWN error, this lead to mismatch of
j1939_session_get/put() and jsk->skb_pending will never decrease to
zero.
To reproduce it use following commands:
1. ip link add dev vcan0 type vcan
2. j1939acd -r 100,80-120 1122334455667788 vcan0
3. presses ctrl-c and thread will be blocked forever
This patch adds check for ndev->flags in j1939_sk_bind() to avoid this
kind of situation and return with -ENETDOWN.
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com>
Link: https://lore.kernel.org/r/1599460308-18770-1-git-send-email-zhangchangzhong@huawei.com
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
net/can/j1939/socket.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index 1be4c898b2fa..f23966526a88 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -475,6 +475,12 @@ static int j1939_sk_bind(struct socket *sock, struct sockaddr *uaddr, int len)
goto out_release_sock;
}
+ if (!(ndev->flags & IFF_UP)) {
+ dev_put(ndev);
+ ret = -ENETDOWN;
+ goto out_release_sock;
+ }
+
priv = j1939_netdev_start(ndev);
dev_put(ndev);
if (IS_ERR(priv)) {
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 13/27] can: isotp: Explain PDU in CAN_ISOTP help text
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (11 preceding siblings ...)
2020-11-03 22:06 ` [net 12/27] can: j1939: j1939_sk_bind(): return failure if netdev is down Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 14/27] can: isotp: isotp_rcv_cf(): enable RX timeout handling in listen-only mode Marc Kleine-Budde
` (14 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Geert Uytterhoeven,
Oliver Hartkopp, Marc Kleine-Budde
From: Geert Uytterhoeven <geert+renesas@glider.be>
The help text for the CAN_ISOTP config symbol uses the acronym "PDU". However,
this acronym is not explained here, nor in Documentation/networking/can.rst.
Expand the acronym to make it easier for users to decide if they need to enable
the CAN_ISOTP option or not.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Link: https://lore.kernel.org/r/20201013141341.28487-1-geert+renesas@glider.be
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
net/can/Kconfig | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/can/Kconfig b/net/can/Kconfig
index 224e5e0283a9..7c9958df91d3 100644
--- a/net/can/Kconfig
+++ b/net/can/Kconfig
@@ -62,8 +62,9 @@ config CAN_ISOTP
communication between CAN nodes via two defined CAN Identifiers.
As CAN frames can only transport a small amount of data bytes
(max. 8 bytes for 'classic' CAN and max. 64 bytes for CAN FD) this
- segmentation is needed to transport longer PDUs as needed e.g. for
- vehicle diagnosis (UDS, ISO 14229) or IP-over-CAN traffic.
+ segmentation is needed to transport longer Protocol Data Units (PDU)
+ as needed e.g. for vehicle diagnosis (UDS, ISO 14229) or IP-over-CAN
+ traffic.
This protocol driver implements data transfers according to
ISO 15765-2:2016 for 'classic' CAN and CAN FD frame types.
If you want to perform automotive vehicle diagnostic services (UDS),
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 14/27] can: isotp: isotp_rcv_cf(): enable RX timeout handling in listen-only mode
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (12 preceding siblings ...)
2020-11-03 22:06 ` [net 13/27] can: isotp: Explain PDU in CAN_ISOTP help text Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 15/27] can: isotp: padlen(): make const array static, makes object smaller Marc Kleine-Budde
` (13 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Oliver Hartkopp, Thomas Wagner,
Marc Kleine-Budde
From: Oliver Hartkopp <socketcan@hartkopp.net>
As reported by Thomas Wagner:
https://github.com/hartkopp/can-isotp/issues/34
the timeout handling for data frames is not enabled when the isotp socket is
used in listen-only mode (sockopt CAN_ISOTP_LISTEN_MODE). This mode is enabled
by the isotpsniffer application which therefore became inconsistend with the
strict rx timeout rules when running the isotp protocol in the operational
mode.
This patch fixes this inconsistency by moving the return condition for the
listen-only mode behind the timeout handling code.
Reported-by: Thomas Wagner <thwa1@web.de>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
Link: https://github.com/hartkopp/can-isotp/issues/34
Link: https://lore.kernel.org/r/20201019120229.89326-1-socketcan@hartkopp.net
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
net/can/isotp.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 4c2062875893..a79287ef86da 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -569,10 +569,6 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,
return 0;
}
- /* no creation of flow control frames */
- if (so->opt.flags & CAN_ISOTP_LISTEN_MODE)
- return 0;
-
/* perform blocksize handling, if enabled */
if (!so->rxfc.bs || ++so->rx.bs < so->rxfc.bs) {
/* start rx timeout watchdog */
@@ -581,6 +577,10 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,
return 0;
}
+ /* no creation of flow control frames */
+ if (so->opt.flags & CAN_ISOTP_LISTEN_MODE)
+ return 0;
+
/* we reached the specified blocksize so->rxfc.bs */
isotp_send_fc(sk, ae, ISOTP_FC_CTS);
return 0;
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 15/27] can: isotp: padlen(): make const array static, makes object smaller
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (13 preceding siblings ...)
2020-11-03 22:06 ` [net 14/27] can: isotp: isotp_rcv_cf(): enable RX timeout handling in listen-only mode Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 16/27] can: ti_hecc: ti_hecc_probe(): add missed clk_disable_unprepare() in error path Marc Kleine-Budde
` (12 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, linux-can, kernel, Colin Ian King, Marc Kleine-Budde
From: Colin Ian King <colin.king@canonical.com>
Don't populate the const array plen on the stack but instead it static. Makes
the object code smaller by 926 bytes.
Before:
text data bss dec hex filename
26531 1943 64 28538 6f7a net/can/isotp.o
After:
text data bss dec hex filename
25509 2039 64 27612 6bdc net/can/isotp.o
(gcc version 10.2.0)
Signed-off-by: Colin Ian King <colin.king@canonical.com>
Link: https://lore.kernel.org/r/20201020154203.54711-1-colin.king@canonical.com
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
net/can/isotp.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/net/can/isotp.c b/net/can/isotp.c
index a79287ef86da..d78ab13bd8be 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -252,14 +252,16 @@ static void isotp_rcv_skb(struct sk_buff *skb, struct sock *sk)
static u8 padlen(u8 datalen)
{
- const u8 plen[] = {8, 8, 8, 8, 8, 8, 8, 8, 8, /* 0 - 8 */
- 12, 12, 12, 12, /* 9 - 12 */
- 16, 16, 16, 16, /* 13 - 16 */
- 20, 20, 20, 20, /* 17 - 20 */
- 24, 24, 24, 24, /* 21 - 24 */
- 32, 32, 32, 32, 32, 32, 32, 32, /* 25 - 32 */
- 48, 48, 48, 48, 48, 48, 48, 48, /* 33 - 40 */
- 48, 48, 48, 48, 48, 48, 48, 48}; /* 41 - 48 */
+ static const u8 plen[] = {
+ 8, 8, 8, 8, 8, 8, 8, 8, 8, /* 0 - 8 */
+ 12, 12, 12, 12, /* 9 - 12 */
+ 16, 16, 16, 16, /* 13 - 16 */
+ 20, 20, 20, 20, /* 17 - 20 */
+ 24, 24, 24, 24, /* 21 - 24 */
+ 32, 32, 32, 32, 32, 32, 32, 32, /* 25 - 32 */
+ 48, 48, 48, 48, 48, 48, 48, 48, /* 33 - 40 */
+ 48, 48, 48, 48, 48, 48, 48, 48 /* 41 - 48 */
+ };
if (datalen > 48)
return 64;
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 16/27] can: ti_hecc: ti_hecc_probe(): add missed clk_disable_unprepare() in error path
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (14 preceding siblings ...)
2020-11-03 22:06 ` [net 15/27] can: isotp: padlen(): make const array static, makes object smaller Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 17/27] can: xilinx_can: handle failure cases of pm_runtime_get_sync Marc Kleine-Budde
` (11 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Zhang Changzhong, Marc Kleine-Budde
From: Zhang Changzhong <zhangchangzhong@huawei.com>
The driver forgets to call clk_disable_unprepare() in error path after
a success calling for clk_prepare_enable().
Fix it by adding a clk_disable_unprepare() in error path.
Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com>
Link: https://lore.kernel.org/r/1594973079-27743-1-git-send-email-zhangchangzhong@huawei.com
Fixes: befa60113ce7 ("can: ti_hecc: add missing prepare and unprepare of the clock")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/ti_hecc.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
index 1d63006c97bc..9913f5458279 100644
--- a/drivers/net/can/ti_hecc.c
+++ b/drivers/net/can/ti_hecc.c
@@ -933,7 +933,7 @@ static int ti_hecc_probe(struct platform_device *pdev)
err = clk_prepare_enable(priv->clk);
if (err) {
dev_err(&pdev->dev, "clk_prepare_enable() failed\n");
- goto probe_exit_clk;
+ goto probe_exit_release_clk;
}
priv->offload.mailbox_read = ti_hecc_mailbox_read;
@@ -942,7 +942,7 @@ static int ti_hecc_probe(struct platform_device *pdev)
err = can_rx_offload_add_timestamp(ndev, &priv->offload);
if (err) {
dev_err(&pdev->dev, "can_rx_offload_add_timestamp() failed\n");
- goto probe_exit_clk;
+ goto probe_exit_disable_clk;
}
err = register_candev(ndev);
@@ -960,7 +960,9 @@ static int ti_hecc_probe(struct platform_device *pdev)
probe_exit_offload:
can_rx_offload_del(&priv->offload);
-probe_exit_clk:
+probe_exit_disable_clk:
+ clk_disable_unprepare(priv->clk);
+probe_exit_release_clk:
clk_put(priv->clk);
probe_exit_candev:
free_candev(ndev);
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 17/27] can: xilinx_can: handle failure cases of pm_runtime_get_sync
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (15 preceding siblings ...)
2020-11-03 22:06 ` [net 16/27] can: ti_hecc: ti_hecc_probe(): add missed clk_disable_unprepare() in error path Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 18/27] can: peak_usb: add range checking in decode operations Marc Kleine-Budde
` (10 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, linux-can, kernel, Navid Emamdoost, Marc Kleine-Budde
From: Navid Emamdoost <navid.emamdoost@gmail.com>
Calling pm_runtime_get_sync increments the counter even in case of
failure, causing incorrect ref count. Call pm_runtime_put if
pm_runtime_get_sync fails.
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
Link: https://lore.kernel.org/r/20200605033239.60664-1-navid.emamdoost@gmail.com
Fixes: 4716620d1b62 ("can: xilinx: Convert to runtime_pm")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/xilinx_can.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 6c4d00d2dbdc..48d746e18f30 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -1395,7 +1395,7 @@ static int xcan_open(struct net_device *ndev)
if (ret < 0) {
netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
__func__, ret);
- return ret;
+ goto err;
}
ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
@@ -1479,6 +1479,7 @@ static int xcan_get_berr_counter(const struct net_device *ndev,
if (ret < 0) {
netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
__func__, ret);
+ pm_runtime_put(priv->dev);
return ret;
}
@@ -1793,7 +1794,7 @@ static int xcan_probe(struct platform_device *pdev)
if (ret < 0) {
netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
__func__, ret);
- goto err_pmdisable;
+ goto err_disableclks;
}
if (priv->read_reg(priv, XCAN_SR_OFFSET) != XCAN_SR_CONFIG_MASK) {
@@ -1828,7 +1829,6 @@ static int xcan_probe(struct platform_device *pdev)
err_disableclks:
pm_runtime_put(priv->dev);
-err_pmdisable:
pm_runtime_disable(&pdev->dev);
err_free:
free_candev(ndev);
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 18/27] can: peak_usb: add range checking in decode operations
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (16 preceding siblings ...)
2020-11-03 22:06 ` [net 17/27] can: xilinx_can: handle failure cases of pm_runtime_get_sync Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 19/27] can: peak_usb: peak_usb_get_ts_time(): fix timestamp wrapping Marc Kleine-Budde
` (9 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Dan Carpenter, Stephane Grosjean,
Marc Kleine-Budde
From: Dan Carpenter <dan.carpenter@oracle.com>
These values come from skb->data so Smatch considers them untrusted. I
believe Smatch is correct but I don't have a way to test this.
The usb_if->dev[] array has 2 elements but the index is in the 0-15
range without checks. The cfd->len can be up to 255 but the maximum
valid size is CANFD_MAX_DLEN (64) so that could lead to memory
corruption.
Fixes: 0a25e1f4f185 ("can: peak_usb: add support for PEAK new CANFD USB adapters")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Link: https://lore.kernel.org/r/20200813140604.GA456946@mwanda
Acked-by: Stephane Grosjean <s.grosjean@peak-system.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 48 +++++++++++++++++-----
1 file changed, 37 insertions(+), 11 deletions(-)
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
index ab63fd9eb982..d29d20525588 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -468,12 +468,18 @@ static int pcan_usb_fd_decode_canmsg(struct pcan_usb_fd_if *usb_if,
struct pucan_msg *rx_msg)
{
struct pucan_rx_msg *rm = (struct pucan_rx_msg *)rx_msg;
- struct peak_usb_device *dev = usb_if->dev[pucan_msg_get_channel(rm)];
- struct net_device *netdev = dev->netdev;
+ struct peak_usb_device *dev;
+ struct net_device *netdev;
struct canfd_frame *cfd;
struct sk_buff *skb;
const u16 rx_msg_flags = le16_to_cpu(rm->flags);
+ if (pucan_msg_get_channel(rm) >= ARRAY_SIZE(usb_if->dev))
+ return -ENOMEM;
+
+ dev = usb_if->dev[pucan_msg_get_channel(rm)];
+ netdev = dev->netdev;
+
if (rx_msg_flags & PUCAN_MSG_EXT_DATA_LEN) {
/* CANFD frame case */
skb = alloc_canfd_skb(netdev, &cfd);
@@ -519,15 +525,21 @@ static int pcan_usb_fd_decode_status(struct pcan_usb_fd_if *usb_if,
struct pucan_msg *rx_msg)
{
struct pucan_status_msg *sm = (struct pucan_status_msg *)rx_msg;
- struct peak_usb_device *dev = usb_if->dev[pucan_stmsg_get_channel(sm)];
- struct pcan_usb_fd_device *pdev =
- container_of(dev, struct pcan_usb_fd_device, dev);
+ struct pcan_usb_fd_device *pdev;
enum can_state new_state = CAN_STATE_ERROR_ACTIVE;
enum can_state rx_state, tx_state;
- struct net_device *netdev = dev->netdev;
+ struct peak_usb_device *dev;
+ struct net_device *netdev;
struct can_frame *cf;
struct sk_buff *skb;
+ if (pucan_stmsg_get_channel(sm) >= ARRAY_SIZE(usb_if->dev))
+ return -ENOMEM;
+
+ dev = usb_if->dev[pucan_stmsg_get_channel(sm)];
+ pdev = container_of(dev, struct pcan_usb_fd_device, dev);
+ netdev = dev->netdev;
+
/* nothing should be sent while in BUS_OFF state */
if (dev->can.state == CAN_STATE_BUS_OFF)
return 0;
@@ -579,9 +591,14 @@ static int pcan_usb_fd_decode_error(struct pcan_usb_fd_if *usb_if,
struct pucan_msg *rx_msg)
{
struct pucan_error_msg *er = (struct pucan_error_msg *)rx_msg;
- struct peak_usb_device *dev = usb_if->dev[pucan_ermsg_get_channel(er)];
- struct pcan_usb_fd_device *pdev =
- container_of(dev, struct pcan_usb_fd_device, dev);
+ struct pcan_usb_fd_device *pdev;
+ struct peak_usb_device *dev;
+
+ if (pucan_ermsg_get_channel(er) >= ARRAY_SIZE(usb_if->dev))
+ return -EINVAL;
+
+ dev = usb_if->dev[pucan_ermsg_get_channel(er)];
+ pdev = container_of(dev, struct pcan_usb_fd_device, dev);
/* keep a trace of tx and rx error counters for later use */
pdev->bec.txerr = er->tx_err_cnt;
@@ -595,11 +612,17 @@ static int pcan_usb_fd_decode_overrun(struct pcan_usb_fd_if *usb_if,
struct pucan_msg *rx_msg)
{
struct pcan_ufd_ovr_msg *ov = (struct pcan_ufd_ovr_msg *)rx_msg;
- struct peak_usb_device *dev = usb_if->dev[pufd_omsg_get_channel(ov)];
- struct net_device *netdev = dev->netdev;
+ struct peak_usb_device *dev;
+ struct net_device *netdev;
struct can_frame *cf;
struct sk_buff *skb;
+ if (pufd_omsg_get_channel(ov) >= ARRAY_SIZE(usb_if->dev))
+ return -EINVAL;
+
+ dev = usb_if->dev[pufd_omsg_get_channel(ov)];
+ netdev = dev->netdev;
+
/* allocate an skb to store the error frame */
skb = alloc_can_err_skb(netdev, &cf);
if (!skb)
@@ -716,6 +739,9 @@ static int pcan_usb_fd_encode_msg(struct peak_usb_device *dev,
u16 tx_msg_size, tx_msg_flags;
u8 can_dlc;
+ if (cfd->len > CANFD_MAX_DLEN)
+ return -EINVAL;
+
tx_msg_size = ALIGN(sizeof(struct pucan_tx_msg) + cfd->len, 4);
tx_msg->size = cpu_to_le16(tx_msg_size);
tx_msg->type = cpu_to_le16(PUCAN_MSG_CAN_TX);
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 19/27] can: peak_usb: peak_usb_get_ts_time(): fix timestamp wrapping
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (17 preceding siblings ...)
2020-11-03 22:06 ` [net 18/27] can: peak_usb: add range checking in decode operations Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 20/27] can: peak_canfd: pucan_handle_can_rx(): fix echo management when loopback is on Marc Kleine-Budde
` (8 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Stephane Grosjean,
Fabian Inostroza, Marc Kleine-Budde
From: Stephane Grosjean <s.grosjean@peak-system.com>
Fabian Inostroza <fabianinostrozap@gmail.com> has discovered a potential
problem in the hardware timestamp reporting from the PCAN-USB USB CAN interface
(only), related to the fact that a timestamp of an event may precede the
timestamp used for synchronization when both records are part of the same USB
packet. However, this case was used to detect the wrapping of the time counter.
This patch details and fixes the two identified cases where this problem can
occur.
Reported-by: Fabian Inostroza <fabianinostrozap@gmail.com>
Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
Link: https://lore.kernel.org/r/20201014085631.15128-1-s.grosjean@peak-system.com
Fixes: bb4785551f64 ("can: usb: PEAK-System Technik USB adapters driver core")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/usb/peak_usb/pcan_usb_core.c | 51 ++++++++++++++++++--
1 file changed, 46 insertions(+), 5 deletions(-)
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
index d91df34e7fa8..c2764799f9ef 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -130,14 +130,55 @@ void peak_usb_get_ts_time(struct peak_time_ref *time_ref, u32 ts, ktime_t *time)
/* protect from getting time before setting now */
if (ktime_to_ns(time_ref->tv_host)) {
u64 delta_us;
+ s64 delta_ts = 0;
+
+ /* General case: dev_ts_1 < dev_ts_2 < ts, with:
+ *
+ * - dev_ts_1 = previous sync timestamp
+ * - dev_ts_2 = last sync timestamp
+ * - ts = event timestamp
+ * - ts_period = known sync period (theoretical)
+ * ~ dev_ts2 - dev_ts1
+ * *but*:
+ *
+ * - time counters wrap (see adapter->ts_used_bits)
+ * - sometimes, dev_ts_1 < ts < dev_ts2
+ *
+ * "normal" case (sync time counters increase):
+ * must take into account case when ts wraps (tsw)
+ *
+ * < ts_period > < >
+ * | | |
+ * ---+--------+----+-------0-+--+-->
+ * ts_dev_1 | ts_dev_2 |
+ * ts tsw
+ */
+ if (time_ref->ts_dev_1 < time_ref->ts_dev_2) {
+ /* case when event time (tsw) wraps */
+ if (ts < time_ref->ts_dev_1)
+ delta_ts = 1 << time_ref->adapter->ts_used_bits;
+
+ /* Otherwise, sync time counter (ts_dev_2) has wrapped:
+ * handle case when event time (tsn) hasn't.
+ *
+ * < ts_period > < >
+ * | | |
+ * ---+--------+--0-+---------+--+-->
+ * ts_dev_1 | ts_dev_2 |
+ * tsn ts
+ */
+ } else if (time_ref->ts_dev_1 < ts) {
+ delta_ts = -(1 << time_ref->adapter->ts_used_bits);
+ }
- delta_us = ts - time_ref->ts_dev_2;
- if (ts < time_ref->ts_dev_2)
- delta_us &= (1 << time_ref->adapter->ts_used_bits) - 1;
+ /* add delay between last sync and event timestamps */
+ delta_ts += (signed int)(ts - time_ref->ts_dev_2);
- delta_us += time_ref->ts_total;
+ /* add time from beginning to last sync */
+ delta_ts += time_ref->ts_total;
- delta_us *= time_ref->adapter->us_per_ts_scale;
+ /* convert ticks number into microseconds */
+ delta_us = delta_ts * time_ref->adapter->us_per_ts_scale;
delta_us >>= time_ref->adapter->us_per_ts_shift;
*time = ktime_add_us(time_ref->tv_host_0, delta_us);
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 20/27] can: peak_canfd: pucan_handle_can_rx(): fix echo management when loopback is on
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (18 preceding siblings ...)
2020-11-03 22:06 ` [net 19/27] can: peak_usb: peak_usb_get_ts_time(): fix timestamp wrapping Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 21/27] can: mcp251xfd: mcp251xfd_regmap_crc_read(): increase severity of CRC read error messages Marc Kleine-Budde
` (7 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Stephane Grosjean, Marc Kleine-Budde
From: Stephane Grosjean <s.grosjean@peak-system.com>
Echo management is driven by PUCAN_MSG_LOOPED_BACK bit, while loopback
frames are identified with PUCAN_MSG_SELF_RECEIVE bit. Those bits are set
for each outgoing frame written to the IP core so that a copy of each one
will be placed into the rx path. Thus,
- when PUCAN_MSG_LOOPED_BACK is set then the rx frame is an echo of a
previously sent frame,
- when PUCAN_MSG_LOOPED_BACK+PUCAN_MSG_SELF_RECEIVE are set, then the rx
frame is an echo AND a loopback frame. Therefore, this frame must be
put into the socket rx path too.
This patch fixes how CAN frames are handled when these are sent while the
can interface is configured in "loopback on" mode.
Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
Link: https://lore.kernel.org/r/20201013153947.28012-1-s.grosjean@peak-system.com
Fixes: 8ac8321e4a79 ("can: peak: add support for PEAK PCAN-PCIe FD CAN-FD boards")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/peak_canfd/peak_canfd.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/peak_canfd/peak_canfd.c b/drivers/net/can/peak_canfd/peak_canfd.c
index 10aa3e457c33..40c33b8a5fda 100644
--- a/drivers/net/can/peak_canfd/peak_canfd.c
+++ b/drivers/net/can/peak_canfd/peak_canfd.c
@@ -262,8 +262,7 @@ static int pucan_handle_can_rx(struct peak_canfd_priv *priv,
cf_len = get_can_dlc(pucan_msg_get_dlc(msg));
/* if this frame is an echo, */
- if ((rx_msg_flags & PUCAN_MSG_LOOPED_BACK) &&
- !(rx_msg_flags & PUCAN_MSG_SELF_RECEIVE)) {
+ if (rx_msg_flags & PUCAN_MSG_LOOPED_BACK) {
unsigned long flags;
spin_lock_irqsave(&priv->echo_lock, flags);
@@ -277,7 +276,13 @@ static int pucan_handle_can_rx(struct peak_canfd_priv *priv,
netif_wake_queue(priv->ndev);
spin_unlock_irqrestore(&priv->echo_lock, flags);
- return 0;
+
+ /* if this frame is only an echo, stop here. Otherwise,
+ * continue to push this application self-received frame into
+ * its own rx queue.
+ */
+ if (!(rx_msg_flags & PUCAN_MSG_SELF_RECEIVE))
+ return 0;
}
/* otherwise, it should be pushed into rx fifo */
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 21/27] can: mcp251xfd: mcp251xfd_regmap_crc_read(): increase severity of CRC read error messages
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (19 preceding siblings ...)
2020-11-03 22:06 ` [net 20/27] can: peak_canfd: pucan_handle_can_rx(): fix echo management when loopback is on Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-05 16:24 ` Manivannan Sadhasivam
2020-11-03 22:06 ` [net 22/27] can: mcp251xfd: mcp251xfd_regmap_nocrc_read(): fix semicolon.cocci warnings Marc Kleine-Budde
` (6 subsequent siblings)
27 siblings, 1 reply; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Marc Kleine-Budde,
Manivannan Sadhasivam, Thomas Kopp
During debugging it turned out that some people have setups where the SPI
communication is more prone to CRC errors.
Increase the severity of both the transfer retry and transfer failure message
to give users feedback without the need to recompile the driver with debug
enabled.
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: Thomas Kopp <thomas.kopp@microchip.com>
Link: http://lore.kernel.org/r/20201019190524.1285319-15-mkl@pengutronix.de
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
index ba25902dd78c..c9ffc5ea2b25 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
@@ -330,17 +330,17 @@ mcp251xfd_regmap_crc_read(void *context,
goto out;
}
- netdev_dbg(priv->ndev,
- "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x) retrying.\n",
- reg, val_len, (int)val_len, buf_rx->data,
- get_unaligned_be16(buf_rx->data + val_len));
- }
-
- if (err) {
netdev_info(priv->ndev,
- "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x).\n",
+ "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x) retrying.\n",
reg, val_len, (int)val_len, buf_rx->data,
get_unaligned_be16(buf_rx->data + val_len));
+ }
+
+ if (err) {
+ netdev_err(priv->ndev,
+ "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x).\n",
+ reg, val_len, (int)val_len, buf_rx->data,
+ get_unaligned_be16(buf_rx->data + val_len));
return err;
}
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [net 21/27] can: mcp251xfd: mcp251xfd_regmap_crc_read(): increase severity of CRC read error messages
2020-11-03 22:06 ` [net 21/27] can: mcp251xfd: mcp251xfd_regmap_crc_read(): increase severity of CRC read error messages Marc Kleine-Budde
@ 2020-11-05 16:24 ` Manivannan Sadhasivam
2020-11-05 16:39 ` Marc Kleine-Budde
0 siblings, 1 reply; 41+ messages in thread
From: Manivannan Sadhasivam @ 2020-11-05 16:24 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: netdev, davem, kuba, linux-can, kernel, Thomas Kopp
Hi Marc,
On Tue, Nov 03, 2020 at 11:06:30PM +0100, Marc Kleine-Budde wrote:
> During debugging it turned out that some people have setups where the SPI
> communication is more prone to CRC errors.
>
> Increase the severity of both the transfer retry and transfer failure message
> to give users feedback without the need to recompile the driver with debug
> enabled.
>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Cc: Thomas Kopp <thomas.kopp@microchip.com>
> Link: http://lore.kernel.org/r/20201019190524.1285319-15-mkl@pengutronix.de
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
> drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> index ba25902dd78c..c9ffc5ea2b25 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> @@ -330,17 +330,17 @@ mcp251xfd_regmap_crc_read(void *context,
> goto out;
> }
>
> - netdev_dbg(priv->ndev,
> - "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x) retrying.\n",
> - reg, val_len, (int)val_len, buf_rx->data,
> - get_unaligned_be16(buf_rx->data + val_len));
> - }
> -
> - if (err) {
> netdev_info(priv->ndev,
> - "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x).\n",
> + "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x) retrying.\n",
> reg, val_len, (int)val_len, buf_rx->data,
> get_unaligned_be16(buf_rx->data + val_len));
I'm not finding this inner debug log useful. Does the user really care
about the iterations it took to read a register? Just throwing the error
after max try seems better to me.
Thanks,
Mani
> + }
> +
> + if (err) {
> + netdev_err(priv->ndev,
> + "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x).\n",
> + reg, val_len, (int)val_len, buf_rx->data,
> + get_unaligned_be16(buf_rx->data + val_len));
>
> return err;
> }
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [net 21/27] can: mcp251xfd: mcp251xfd_regmap_crc_read(): increase severity of CRC read error messages
2020-11-05 16:24 ` Manivannan Sadhasivam
@ 2020-11-05 16:39 ` Marc Kleine-Budde
2020-11-05 18:14 ` Manivannan Sadhasivam
0 siblings, 1 reply; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-05 16:39 UTC (permalink / raw)
To: Manivannan Sadhasivam; +Cc: netdev, davem, kuba, linux-can, kernel, Thomas Kopp
[-- Attachment #1.1: Type: text/plain, Size: 2829 bytes --]
On 11/5/20 5:24 PM, Manivannan Sadhasivam wrote:
> Hi Marc,
>
> On Tue, Nov 03, 2020 at 11:06:30PM +0100, Marc Kleine-Budde wrote:
>> During debugging it turned out that some people have setups where the SPI
>> communication is more prone to CRC errors.
>>
>> Increase the severity of both the transfer retry and transfer failure message
>> to give users feedback without the need to recompile the driver with debug
>> enabled.
>>
>> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>> Cc: Thomas Kopp <thomas.kopp@microchip.com>
>> Link: http://lore.kernel.org/r/20201019190524.1285319-15-mkl@pengutronix.de
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> ---
>> drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 16 ++++++++--------
>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
>> index ba25902dd78c..c9ffc5ea2b25 100644
>> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
>> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
>> @@ -330,17 +330,17 @@ mcp251xfd_regmap_crc_read(void *context,
>> goto out;
>> }
>>
>> - netdev_dbg(priv->ndev,
>> - "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x) retrying.\n",
>> - reg, val_len, (int)val_len, buf_rx->data,
>> - get_unaligned_be16(buf_rx->data + val_len));
>> - }
>> -
>> - if (err) {
>> netdev_info(priv->ndev,
>> - "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x).\n",
>> + "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x) retrying.\n",
>> reg, val_len, (int)val_len, buf_rx->data,
>> get_unaligned_be16(buf_rx->data + val_len));
>
> I'm not finding this inner debug log useful. Does the user really care
> about the iterations it took to read a register? Just throwing the error
> after max try seems better to me.
Bitflips on the SPI should not happen, at least not regularly. Even with my
breadboard setup and max SPI frequency, I don't see any SPI CRC errors, unless
there is bad cabling. Then a retry most of the times helps and the user doesn't
notice. This drops performance...Yes logging drop performance, too, but at least
someone will notice.
My rationale was to give the user feedback and not cover a potential problem.
The driver is new and probably not widely used and I have no idea how often it
runs into read-CRC problems in production use.
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 |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [net 21/27] can: mcp251xfd: mcp251xfd_regmap_crc_read(): increase severity of CRC read error messages
2020-11-05 16:39 ` Marc Kleine-Budde
@ 2020-11-05 18:14 ` Manivannan Sadhasivam
2020-11-06 8:35 ` Marc Kleine-Budde
0 siblings, 1 reply; 41+ messages in thread
From: Manivannan Sadhasivam @ 2020-11-05 18:14 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: netdev, davem, kuba, linux-can, kernel, Thomas Kopp
On Thu, Nov 05, 2020 at 05:39:31PM +0100, Marc Kleine-Budde wrote:
> On 11/5/20 5:24 PM, Manivannan Sadhasivam wrote:
> > Hi Marc,
> >
> > On Tue, Nov 03, 2020 at 11:06:30PM +0100, Marc Kleine-Budde wrote:
> >> During debugging it turned out that some people have setups where the SPI
> >> communication is more prone to CRC errors.
> >>
> >> Increase the severity of both the transfer retry and transfer failure message
> >> to give users feedback without the need to recompile the driver with debug
> >> enabled.
> >>
> >> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >> Cc: Thomas Kopp <thomas.kopp@microchip.com>
> >> Link: http://lore.kernel.org/r/20201019190524.1285319-15-mkl@pengutronix.de
> >> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> >> ---
> >> drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 16 ++++++++--------
> >> 1 file changed, 8 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> >> index ba25902dd78c..c9ffc5ea2b25 100644
> >> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> >> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> >> @@ -330,17 +330,17 @@ mcp251xfd_regmap_crc_read(void *context,
> >> goto out;
> >> }
> >>
> >> - netdev_dbg(priv->ndev,
> >> - "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x) retrying.\n",
> >> - reg, val_len, (int)val_len, buf_rx->data,
> >> - get_unaligned_be16(buf_rx->data + val_len));
> >> - }
> >> -
> >> - if (err) {
> >> netdev_info(priv->ndev,
> >> - "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x).\n",
> >> + "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x) retrying.\n",
> >> reg, val_len, (int)val_len, buf_rx->data,
> >> get_unaligned_be16(buf_rx->data + val_len));
> >
> > I'm not finding this inner debug log useful. Does the user really care
> > about the iterations it took to read a register? Just throwing the error
> > after max try seems better to me.
>
> Bitflips on the SPI should not happen, at least not regularly. Even with my
> breadboard setup and max SPI frequency, I don't see any SPI CRC errors, unless
> there is bad cabling. Then a retry most of the times helps and the user doesn't
> notice. This drops performance...Yes logging drop performance, too, but at least
> someone will notice.
>
> My rationale was to give the user feedback and not cover a potential problem.
> The driver is new and probably not widely used and I have no idea how often it
> runs into read-CRC problems in production use.
>
Okay. Let's see if anyone complain ;)
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Thanks,
Mani
> 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 |
>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [net 21/27] can: mcp251xfd: mcp251xfd_regmap_crc_read(): increase severity of CRC read error messages
2020-11-05 18:14 ` Manivannan Sadhasivam
@ 2020-11-06 8:35 ` Marc Kleine-Budde
0 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-06 8:35 UTC (permalink / raw)
To: Manivannan Sadhasivam; +Cc: kernel, netdev, linux-can, Thomas Kopp, kuba, davem
[-- Attachment #1.1: Type: text/plain, Size: 3281 bytes --]
On 11/5/20 7:14 PM, Manivannan Sadhasivam wrote:
> On Thu, Nov 05, 2020 at 05:39:31PM +0100, Marc Kleine-Budde wrote:
>> On 11/5/20 5:24 PM, Manivannan Sadhasivam wrote:
>>> Hi Marc,
>>>
>>> On Tue, Nov 03, 2020 at 11:06:30PM +0100, Marc Kleine-Budde wrote:
>>>> During debugging it turned out that some people have setups where the SPI
>>>> communication is more prone to CRC errors.
>>>>
>>>> Increase the severity of both the transfer retry and transfer failure message
>>>> to give users feedback without the need to recompile the driver with debug
>>>> enabled.
>>>>
>>>> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>>> Cc: Thomas Kopp <thomas.kopp@microchip.com>
>>>> Link: http://lore.kernel.org/r/20201019190524.1285319-15-mkl@pengutronix.de
>>>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>>>> ---
>>>> drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 16 ++++++++--------
>>>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
>>>> index ba25902dd78c..c9ffc5ea2b25 100644
>>>> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
>>>> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
>>>> @@ -330,17 +330,17 @@ mcp251xfd_regmap_crc_read(void *context,
>>>> goto out;
>>>> }
>>>>
>>>> - netdev_dbg(priv->ndev,
>>>> - "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x) retrying.\n",
>>>> - reg, val_len, (int)val_len, buf_rx->data,
>>>> - get_unaligned_be16(buf_rx->data + val_len));
>>>> - }
>>>> -
>>>> - if (err) {
>>>> netdev_info(priv->ndev,
>>>> - "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x).\n",
>>>> + "CRC read error at address 0x%04x (length=%zd, data=%*ph, CRC=0x%04x) retrying.\n",
>>>> reg, val_len, (int)val_len, buf_rx->data,
>>>> get_unaligned_be16(buf_rx->data + val_len));
>>>
>>> I'm not finding this inner debug log useful. Does the user really care
>>> about the iterations it took to read a register? Just throwing the error
>>> after max try seems better to me.
>>
>> Bitflips on the SPI should not happen, at least not regularly. Even with my
>> breadboard setup and max SPI frequency, I don't see any SPI CRC errors, unless
>> there is bad cabling. Then a retry most of the times helps and the user doesn't
>> notice. This drops performance...Yes logging drop performance, too, but at least
>> someone will notice.
>>
>> My rationale was to give the user feedback and not cover a potential problem.
>> The driver is new and probably not widely used and I have no idea how often it
>> runs into read-CRC problems in production use.
>>
>
> Okay. Let's see if anyone complain ;)
>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
The patch is part of my pull request and has already been merged by Jakub to
net/master.
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 |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* [net 22/27] can: mcp251xfd: mcp251xfd_regmap_nocrc_read(): fix semicolon.cocci warnings
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (20 preceding siblings ...)
2020-11-03 22:06 ` [net 21/27] can: mcp251xfd: mcp251xfd_regmap_crc_read(): increase severity of CRC read error messages Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 23/27] can: mcp251xfd: remove unneeded break Marc Kleine-Budde
` (5 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, kernel test robot, Marc Kleine-Budde
From: kernel test robot <lkp@intel.com>
drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c:176:2-3: Unneeded semicolon
Remove unneeded semicolon.
Generated by: scripts/coccinelle/misc/semicolon.cocci
Fixes: 875347fe5756 ("can: mcp25xxfd: add regmap infrastructure")
Signed-off-by: kernel test robot <lkp@intel.com>
Link: https://lore.kernel.org/r/20201019120805.GA63693@ae4257e0ab22
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
index c9ffc5ea2b25..314f868b3465 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
@@ -173,7 +173,7 @@ mcp251xfd_regmap_nocrc_read(void *context,
memcpy(&buf_tx->cmd, reg, sizeof(buf_tx->cmd));
if (MCP251XFD_SANITIZE_SPI)
memset(buf_tx->data, 0x0, val_len);
- };
+ }
err = spi_sync(spi, &msg);
if (err)
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 23/27] can: mcp251xfd: remove unneeded break
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (21 preceding siblings ...)
2020-11-03 22:06 ` [net 22/27] can: mcp251xfd: mcp251xfd_regmap_nocrc_read(): fix semicolon.cocci warnings Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 24/27] can: flexcan: remove FLEXCAN_QUIRK_DISABLE_MECR quirk for LS1021A Marc Kleine-Budde
` (4 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, linux-can, kernel, Tom Rix, Marc Kleine-Budde
From: Tom Rix <trix@redhat.com>
A break is not needed if it is preceded by a return.
Signed-off-by: Tom Rix <trix@redhat.com>
Link: https://lore.kernel.org/r/20201019172412.31143-1-trix@redhat.com
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
.../net/can/spi/mcp251xfd/mcp251xfd-core.c | 22 +++++++++----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
index c3f49543ff26..9c215f7c5f81 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
@@ -75,11 +75,11 @@ static const char *__mcp251xfd_get_model_str(enum mcp251xfd_model model)
{
switch (model) {
case MCP251XFD_MODEL_MCP2517FD:
- return "MCP2517FD"; break;
+ return "MCP2517FD";
case MCP251XFD_MODEL_MCP2518FD:
- return "MCP2518FD"; break;
+ return "MCP2518FD";
case MCP251XFD_MODEL_MCP251XFD:
- return "MCP251xFD"; break;
+ return "MCP251xFD";
}
return "<unknown>";
@@ -95,21 +95,21 @@ static const char *mcp251xfd_get_mode_str(const u8 mode)
{
switch (mode) {
case MCP251XFD_REG_CON_MODE_MIXED:
- return "Mixed (CAN FD/CAN 2.0)"; break;
+ return "Mixed (CAN FD/CAN 2.0)";
case MCP251XFD_REG_CON_MODE_SLEEP:
- return "Sleep"; break;
+ return "Sleep";
case MCP251XFD_REG_CON_MODE_INT_LOOPBACK:
- return "Internal Loopback"; break;
+ return "Internal Loopback";
case MCP251XFD_REG_CON_MODE_LISTENONLY:
- return "Listen Only"; break;
+ return "Listen Only";
case MCP251XFD_REG_CON_MODE_CONFIG:
- return "Configuration"; break;
+ return "Configuration";
case MCP251XFD_REG_CON_MODE_EXT_LOOPBACK:
- return "External Loopback"; break;
+ return "External Loopback";
case MCP251XFD_REG_CON_MODE_CAN2_0:
- return "CAN 2.0"; break;
+ return "CAN 2.0";
case MCP251XFD_REG_CON_MODE_RESTRICTED:
- return "Restricted Operation"; break;
+ return "Restricted Operation";
}
return "<unknown>";
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 24/27] can: flexcan: remove FLEXCAN_QUIRK_DISABLE_MECR quirk for LS1021A
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (22 preceding siblings ...)
2020-11-03 22:06 ` [net 23/27] can: mcp251xfd: remove unneeded break Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 25/27] can: flexcan: add ECC initialization for LX2160A Marc Kleine-Budde
` (3 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Joakim Zhang, Pankaj Bansal,
Marc Kleine-Budde
From: Joakim Zhang <qiangqing.zhang@nxp.com>
After double check with Layerscape CAN owner (Pankaj Bansal), confirm that
LS1021A doesn't support ECC feature, so remove FLEXCAN_QUIRK_DISABLE_MECR
quirk.
Fixes: 99b7668c04b27 ("can: flexcan: adding platform specific details for LS1021A")
Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
Link: https://lore.kernel.org/r/20201020155402.30318-4-qiangqing.zhang@nxp.com
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/flexcan.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 4d594e977497..586e1417a697 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -405,8 +405,7 @@ static const struct flexcan_devtype_data fsl_vf610_devtype_data = {
static const struct flexcan_devtype_data fsl_ls1021a_r2_devtype_data = {
.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
- FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
- FLEXCAN_QUIRK_USE_OFF_TIMESTAMP,
+ FLEXCAN_QUIRK_BROKEN_PERR_STATE | FLEXCAN_QUIRK_USE_OFF_TIMESTAMP,
};
static const struct flexcan_devtype_data fsl_lx2160a_r1_devtype_data = {
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 25/27] can: flexcan: add ECC initialization for LX2160A
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (23 preceding siblings ...)
2020-11-03 22:06 ` [net 24/27] can: flexcan: remove FLEXCAN_QUIRK_DISABLE_MECR quirk for LS1021A Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 26/27] can: flexcan: add ECC initialization for VF610 Marc Kleine-Budde
` (2 subsequent siblings)
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Joakim Zhang, Pankaj Bansal,
Marc Kleine-Budde
From: Joakim Zhang <qiangqing.zhang@nxp.com>
After double check with Layerscape CAN owner (Pankaj Bansal), confirm
that LX2160A indeed supports ECC feature, so correct the feature table.
For SoCs with ECC supported, even use FLEXCAN_QUIRK_DISABLE_MECR quirk to
disable non-correctable errors interrupt and freeze mode, had better use
FLEXCAN_QUIRK_SUPPORT_ECC quirk to initialize all memory.
Fixes: 2c19bb43e5572 ("can: flexcan: add lx2160ar1 support")
Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
Link: https://lore.kernel.org/r/20201020155402.30318-5-qiangqing.zhang@nxp.com
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/flexcan.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 586e1417a697..c2330eab3595 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -217,7 +217,7 @@
* MX8MP FlexCAN3 03.00.17.01 yes yes no yes yes yes
* VF610 FlexCAN3 ? no yes no yes yes? no
* LS1021A FlexCAN2 03.00.04.00 no yes no no yes no
- * LX2160A FlexCAN3 03.00.23.00 no yes no no yes yes
+ * LX2160A FlexCAN3 03.00.23.00 no yes no yes yes yes
*
* Some SOCs do not have the RX_WARN & TX_WARN interrupt line connected.
*/
@@ -411,7 +411,8 @@ static const struct flexcan_devtype_data fsl_ls1021a_r2_devtype_data = {
static const struct flexcan_devtype_data fsl_lx2160a_r1_devtype_data = {
.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
- FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | FLEXCAN_QUIRK_SUPPORT_FD,
+ FLEXCAN_QUIRK_USE_OFF_TIMESTAMP | FLEXCAN_QUIRK_SUPPORT_FD |
+ FLEXCAN_QUIRK_SUPPORT_ECC,
};
static const struct can_bittiming_const flexcan_bittiming_const = {
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 26/27] can: flexcan: add ECC initialization for VF610
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (24 preceding siblings ...)
2020-11-03 22:06 ` [net 25/27] can: flexcan: add ECC initialization for LX2160A Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 27/27] can: flexcan: flexcan_remove(): disable wakeup completely Marc Kleine-Budde
2020-11-04 18:43 ` pull-request: can 2020-11-03 Jakub Kicinski
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, linux-can, kernel, Joakim Zhang, Marc Kleine-Budde
From: Joakim Zhang <qiangqing.zhang@nxp.com>
For SoCs with ECC supported, even use FLEXCAN_QUIRK_DISABLE_MECR quirk to
disable non-correctable errors interrupt and freeze mode, had better use
FLEXCAN_QUIRK_SUPPORT_ECC quirk to initialize all memory.
Fixes: cdce844865bea ("can: flexcan: add vf610 support for FlexCAN")
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
Link: https://lore.kernel.org/r/20201020155402.30318-6-qiangqing.zhang@nxp.com
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/flexcan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index c2330eab3595..06f94b6f0ebe 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -400,7 +400,7 @@ static struct flexcan_devtype_data fsl_imx8mp_devtype_data = {
static const struct flexcan_devtype_data fsl_vf610_devtype_data = {
.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_USE_OFF_TIMESTAMP |
- FLEXCAN_QUIRK_BROKEN_PERR_STATE,
+ FLEXCAN_QUIRK_BROKEN_PERR_STATE | FLEXCAN_QUIRK_SUPPORT_ECC,
};
static const struct flexcan_devtype_data fsl_ls1021a_r2_devtype_data = {
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [net 27/27] can: flexcan: flexcan_remove(): disable wakeup completely
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (25 preceding siblings ...)
2020-11-03 22:06 ` [net 26/27] can: flexcan: add ECC initialization for VF610 Marc Kleine-Budde
@ 2020-11-03 22:06 ` Marc Kleine-Budde
2020-11-04 18:43 ` pull-request: can 2020-11-03 Jakub Kicinski
27 siblings, 0 replies; 41+ messages in thread
From: Marc Kleine-Budde @ 2020-11-03 22:06 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, linux-can, kernel, Joakim Zhang, Marc Kleine-Budde
From: Joakim Zhang <qiangqing.zhang@nxp.com>
With below sequence, we can see wakeup default is enabled after re-load module,
if it was enabled before, so we need disable wakeup in flexcan_remove().
| # cat /sys/bus/platform/drivers/flexcan/5a8e0000.can/power/wakeup
| disabled
| # echo enabled > /sys/bus/platform/drivers/flexcan/5a8e0000.can/power/wakeup
| # cat /sys/bus/platform/drivers/flexcan/5a8e0000.can/power/wakeup
| enabled
| # rmmod flexcan
| # modprobe flexcan
| # cat /sys/bus/platform/drivers/flexcan/5a8e0000.can/power/wakeup
| enabled
Fixes: de3578c198c6 ("can: flexcan: add self wakeup support")
Fixes: 915f9666421c ("can: flexcan: add support for DT property 'wakeup-source'")
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
Link: https://lore.kernel.org/r/20201020184527.8190-1-qiangqing.zhang@nxp.com
[mkl: streamlined commit message]
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/flexcan.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 06f94b6f0ebe..881799bd9c5e 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -2062,6 +2062,8 @@ static int flexcan_remove(struct platform_device *pdev)
{
struct net_device *dev = platform_get_drvdata(pdev);
+ device_set_wakeup_enable(&pdev->dev, false);
+ device_set_wakeup_capable(&pdev->dev, false);
unregister_flexcandev(dev);
pm_runtime_disable(&pdev->dev);
free_candev(dev);
--
2.28.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: pull-request: can 2020-11-03
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
` (26 preceding siblings ...)
2020-11-03 22:06 ` [net 27/27] can: flexcan: flexcan_remove(): disable wakeup completely Marc Kleine-Budde
@ 2020-11-04 18:43 ` Jakub Kicinski
27 siblings, 0 replies; 41+ messages in thread
From: Jakub Kicinski @ 2020-11-04 18:43 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: netdev, davem, linux-can, kernel
On Tue, 3 Nov 2020 23:06:09 +0100 Marc Kleine-Budde wrote:
> Hello Jakub, hello David,
>
> here's a pull request for net/master consisting of 27 patches for net/master.
Pulled, thanks!
^ permalink raw reply [flat|nested] 41+ messages in thread