This reverts commit 7da11ba5c5066dadc2e96835a6233d56d7b7764a.
After introduction of this change we encountered following new error
message on various i.MX plattforms (flexcan)
flexcan 53fc8000.can can0: __can_get_echo_skb: BUG! Trying to echo non
existing skb: can_priv::echo_skb[0]
The introduction of the message was a mistake because
priv->echo_skb[idx] = NULL is a perfectly valid in following case:
If CAN_RAW_LOOPBACK is disabled (setsockopt) in applications, the
pkt_type of the tx skb's given to can_put_echo_skb is set to
PACKET_LOOPBACK. In this case can_put_echo_skb will not set
priv->echo_skb[idx]. It is therefore kept NULL.
(As additional argument for revert: The order of check and usage of idx
was changed. idx is used to access an array element before checking it's
boundaries)
Signed-off-by: Manfred Schlaegl <[email protected]>
---
drivers/net/can/dev.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 3b3f88ffab53..c05e4d50d43d 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -480,8 +480,6 @@ EXPORT_SYMBOL_GPL(can_put_echo_skb);
struct sk_buff *__can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr)
{
struct can_priv *priv = netdev_priv(dev);
- struct sk_buff *skb = priv->echo_skb[idx];
- struct canfd_frame *cf;
if (idx >= priv->echo_skb_max) {
netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n",
@@ -489,20 +487,21 @@ struct sk_buff *__can_get_echo_skb(struct net_device *dev, unsigned int idx, u8
return NULL;
}
- if (!skb) {
- netdev_err(dev, "%s: BUG! Trying to echo non existing skb: can_priv::echo_skb[%u]\n",
- __func__, idx);
- return NULL;
- }
+ if (priv->echo_skb[idx]) {
+ /* Using "struct canfd_frame::len" for the frame
+ * length is supported on both CAN and CANFD frames.
+ */
+ struct sk_buff *skb = priv->echo_skb[idx];
+ struct canfd_frame *cf = (struct canfd_frame *)skb->data;
+ u8 len = cf->len;
- /* Using "struct canfd_frame::len" for the frame
- * length is supported on both CAN and CANFD frames.
- */
- cf = (struct canfd_frame *)skb->data;
- *len_ptr = cf->len;
- priv->echo_skb[idx] = NULL;
+ *len_ptr = len;
+ priv->echo_skb[idx] = NULL;
- return skb;
+ return skb;
+ }
+
+ return NULL;
}
/*
--
2.11.0
On 12/19/18 7:39 PM, Manfred Schlaegl wrote:
> This reverts commit 7da11ba5c5066dadc2e96835a6233d56d7b7764a.
>
> After introduction of this change we encountered following new error
> message on various i.MX plattforms (flexcan)
> flexcan 53fc8000.can can0: __can_get_echo_skb: BUG! Trying to echo non
> existing skb: can_priv::echo_skb[0]
Doh! I should have tested more extensive. Sorry.
> The introduction of the message was a mistake because
> priv->echo_skb[idx] = NULL is a perfectly valid in following case:
> If CAN_RAW_LOOPBACK is disabled (setsockopt) in applications, the
> pkt_type of the tx skb's given to can_put_echo_skb is set to
> PACKET_LOOPBACK. In this case can_put_echo_skb will not set
> priv->echo_skb[idx]. It is therefore kept NULL.
>
> (As additional argument for revert: The order of check and usage of idx
> was changed. idx is used to access an array element before checking it's
> boundaries)
>
> Signed-off-by: Manfred Schlaegl <[email protected]>
Applied to linux-can.
Tnx,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
Manfred Schlaegl | Leitung Entwicklung Linz
GINZINGER ELECTRONIC SYSTEMS GMBH
Tel.: +43 7723 5422 153
Mobil: +43 676 841 208 253
Mail: [email protected]
Web: http://www.ginzinger.com
On 04.01.19 16:23, Marc Kleine-Budde wrote:
> On 12/19/18 7:39 PM, Manfred Schlaegl wrote:
>> This reverts commit 7da11ba5c5066dadc2e96835a6233d56d7b7764a.
>>
>> After introduction of this change we encountered following new error
>> message on various i.MX plattforms (flexcan)
>> flexcan 53fc8000.can can0: __can_get_echo_skb: BUG! Trying to echo non
>> existing skb: can_priv::echo_skb[0]
>
> Doh! I should have tested more extensive. Sorry.
>
>> The introduction of the message was a mistake because
>> priv->echo_skb[idx] = NULL is a perfectly valid in following case:
>> If CAN_RAW_LOOPBACK is disabled (setsockopt) in applications, the
>> pkt_type of the tx skb's given to can_put_echo_skb is set to
>> PACKET_LOOPBACK. In this case can_put_echo_skb will not set
>> priv->echo_skb[idx]. It is therefore kept NULL.
>>
>> (As additional argument for revert: The order of check and usage of idx
>> was changed. idx is used to access an array element before checking it's
>> boundaries)
>>
>> Signed-off-by: Manfred Schlaegl <[email protected]>
>
> Applied to linux-can.
Great, thanks!
>
> Tnx,
> Marc
>
________________________________________
Ginzinger electronic systems GmbH
Gewerbegebiet Pirath 16
4952 Weng im Innkreis
http://www.ginzinger.com
Firmenbuchnummer: FN 364958d
Firmenbuchgericht: Ried im Innkreis
UID-Nr.: ATU66521089
Diese Nachricht ist vertraulich und darf nicht an andere Personen weitergegeben oder von diesen verwendet werden. Verständigen Sie uns, wenn Sie irrtümlich eine Mitteilung empfangen haben.
This message is confidential. It may not be disclosed to, or used by, anyone other than the addressee. If you receive this message by mistake, please advise the sender.