[PATCH] net: phy: smsc: reintroduced unconditional soft reset

[Manfred Schlaegl]

We detected some problems using the smsc lan8720a in combination with
i.MX28 and tracked this down to commit 21009686662f ("net: phy: smsc: move
smsc_phy_config_init reset part in a soft_reset function")
With 2100968666 the generic soft reset is replaced by a specific function
which handles power down state correctly. But additionally the soft reset
itself got conditional and is therefore also only performed if the phy is
in power down state.

This patch keeps the conditional wake up from power down, but
re-introduces the unconditional soft reset using the generic soft reset
function.
It was tested on linux-4.1.25 and linux-4.7.0-rc2.

Signed-off-by: Manfred Schlaegl 
---
 drivers/net/phy/smsc.c | 17 -
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index 2e21e93..b62c4aa 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -75,22 +75,13 @@ static int smsc_phy_reset(struct phy_device *phydev)
 * in all capable mode before using it.
 */
if ((rc & MII_LAN83C185_MODE_MASK) == MII_LAN83C185_MODE_POWERDOWN) {
-   int timeout = 5;
-
-   /* set "all capable" mode and reset the phy */
+   /* set "all capable" mode */
rc |= MII_LAN83C185_MODE_ALL;
phy_write(phydev, MII_LAN83C185_SPECIAL_MODES, rc);
-   phy_write(phydev, MII_BMCR, BMCR_RESET);
-
-   /* wait end of reset (max 500 ms) */
-   do {
-   udelay(10);
-   if (timeout-- == 0)
-   return -1;
-   rc = phy_read(phydev, MII_BMCR);
-   } while (rc & BMCR_RESET);
}
-   return 0;
+
+   /* reset the phy */
+   return genphy_soft_reset(phydev);
 }
 
 static int lan911x_config_init(struct phy_device *phydev)
-- 
2.1.4

[PATCH] can: fix loss of frames due to wrong assumption in raw_rcv

[Manfred Schlaegl]

I've detected a massive loss of can frames on i.MX6 using flexcan
driver with 4.1-rc8 and tracked this down to following commit:
514ac99c64b22d83b52dfee3b8becaa69a92bc4a - "can: fix multiple delivery
of a single CAN frame for overlapping CAN filters"

514ac99c64b22d83b52dfee3b8becaa69a92bc4a introduces a frame equality
check. Since the sk_buff pointer is not sufficient to do this (buffers
are reused), the check also compares time stamps.
In short: pointer+time stamp was assumed as unique key to a specific
frame.
The problem with this is, that the time stamp is an optional property
and not set per default.
In our case (flexcan) the time stamp is always zero, so the equality
check is reduced to equality of buffer pointers, resulting in a lot of
dropped frames.

Possible solutions I thought of:
 1. Every driver has to set a time stamp
(possibly error prone and hard to enforce?)
 2. Change the equality check
 3. Fulfil the requirements of the equality check by setting a
time stamp per default.

This patch fixes the problem with solution 3. A time stamp is set at
time of allocation in alloc_can_skb.
The time stamp may be overridden later, but the function of the equality
check is ensured.

I'm not really deep in linux network subsystem, so there may exists
more elegant solutions for the problem.

Signed-off-by: Manfred Schlaegl 
---
 drivers/net/can/dev.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index b0f6924..282e2e7 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -575,6 +575,7 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, 
struct can_frame **cf)
if (unlikely(!skb))
return NULL;
 
+   __net_timestamp(skb);
skb->protocol = htons(ETH_P_CAN);
skb->pkt_type = PACKET_BROADCAST;
skb->ip_summed = CHECKSUM_UNNECESSARY;
-- 
1.7.10.4




signature.asc
Description: OpenPGP digital signature

Re: [PATCH] can: fix loss of frames due to wrong assumption in raw_rcv

[Manfred Schlaegl]

Hello Oliver,

On 2015-06-21 00:42, Oliver Hartkopp wrote:

>> 514ac99c64b22d83b52dfee3b8becaa69a92bc4a introduces a frame equality
>> check. Since the sk_buff pointer is not sufficient to do this (buffers
>> are reused), the check also compares time stamps.
>> In short: pointer+time stamp was assumed as unique key to a specific
>> frame.
>> The problem with this is, that the time stamp is an optional property
>> and not set per default.
>> In our case (flexcan) the time stamp is always zero, so the equality
>> check is reduced to equality of buffer pointers, resulting in a lot of
>> dropped frames.
> 
> The question is why your system did not generate a timestamp at the time of
> skb reception.
> 
> Usually when netif_rx(), netif_rx_ni() is invoked the timestamp is set in the
> following reception process.
> 
> flexcan.c only uses netif_receive_skb() - but all theses functions set the
> timestamp
> 
>   net_timestamp_check(netdev_tstamp_prequeue, skb);
> 
> depending on netdev_tstamp_prequeue which is configured by
> 
> /proc/sys/net/core/netdev_tstamp_prequeue
> 
> See the idea of netdev_tstamp_prequeue here:
> 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/net/core/sysctl_net_core.c?id=3b098e2d7c693796cc4dffb07caa249fc0f70771
> 
Thank you for the background information!
I've also noticed your patch [PATCH - regression 4.1-rc8] can: fix loss of CAN 
frames in raw_rcv

> Can you tell me the output of /proc/sys/net/core/netdev_tstamp_prequeue on
> your machine?

/proc/sys/net/core/netdev_tstamp_prequeue is set to 1 (unmodified, default)

I tried to dig a little deeper in timestamping:
 1. (net/core/dev.c) I found that static_key_false(&netstamp_needed) is always 
0, resulting that the timestamp is never set by net_timestamp_check in 
netif_receive_skb_internal.
 2. (net/core/dev.c) static_key_false(&netstamp_needed) is 0 because 
net_enable_timestamp is never called.
 3. (net/core/sock.c) net_enable_timestamp is never called because 
SK_FLAGS_TIMESTAMP is not set
 4. (net/core/sock.c) SK_FLAGS_TIMESTAMP is not set because neither of 
SOCK_TIMESTAMP or SOCK_TIMESTAMPING_RX_SOFTWARE is set 
 5. (net/core/sock.c) SOCK_TIMESTAMP or SOCK_TIMESTAMPING_RX_SOFTWARE is not 
set because timestamping is an optional feature (according to 
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/timestamping.txt?id=b953c0d234bc72e8489d3bf51a276c5c4ec85345)
 not enabled in my use case (even if netdev_tstamp_prequeue is set to 1)

So the original assumption for the was correct: The correctness of the skb 
equality check depends on a feature that is not enabled by default 
(respectively user configurable).
Do you agree with this?

> 
> Thanks again for your investigation!
Sure!

Best regards,
Manfred

--
To unsubscribe from this list: send the line "unsubscribe netdev" in

Re: [PATCH - regression 4.1-rc8] can: fix loss of CAN frames in raw_rcv

[Manfred Schlaegl]

Hello!

On 2015-06-21 18:50, Oliver Hartkopp wrote:
> As reported by Manfred Schlaegl here
> 
>http://marc.info/?l=linux-netdev&m=143482089824232&w=2
> 
> commit 514ac99c64b "can: fix multiple delivery of a single CAN frame for
> overlapping CAN filters" requires the skb->tstamp to be set to check for
> identical CAN skbs.
> 
> As net timestamping is influenced by several players (netstamp_needed and
> netdev_tstamp_prequeue) Manfred missed a proper timestamp which leads to
> CAN frame loss.
> 
> As skb timestamping became now mandatory for CAN related skbs this patch
> makes sure that received CAN skbs always have a proper timestamp set.
> Maybe there's a better solution in the future but this patch fixes the
> CAN frame loss so far.
> 

I'm not sure, but maybe this patch (and also my original one) opens a new 
potential issue with timestamps.

If the timestamp is set at allocation time, this cancels setting the timestamp 
at delivery (by net_timestamp_check in, for example, 
netif_receive_skb_internal.) -> So it changes the behavior of timestamping 
(http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/timestamping.txt?id=b953c0d234bc72e8489d3bf51a276c5c4ec85345)
 generally.

Hypothetical example: If timestamping is enabled by the user and there is a 
significant delay between allocation and delivery of a skb (early allocation in 
driver or something) the timestamp does not reflect the reception time anymore.

What do you thing about this?

best regards,
Manfred
--
To unsubscribe from this list: send the line "unsubscribe netdev" in

Re: [PATCH - regression 4.1-rc8] can: fix loss of CAN frames in raw_rcv

[Manfred Schlaegl]

Hello Oliver,

On 2015-06-22 12:34, Oliver Hartkopp wrote:
> Hello Manfred,
> 
> On 22.06.2015 12:10, Manfred Schlaegl wrote:
>> On 2015-06-21 18:50, Oliver Hartkopp wrote:
>>> As reported by Manfred Schlaegl here
>>>
>>> http://marc.info/?l=linux-netdev&m=143482089824232&w=2
>>>
>>> commit 514ac99c64b "can: fix multiple delivery of a single CAN frame for
>>> overlapping CAN filters" requires the skb->tstamp to be set to check for
>>> identical CAN skbs.
>>>
>>> As net timestamping is influenced by several players (netstamp_needed and
>>> netdev_tstamp_prequeue) Manfred missed a proper timestamp which leads to
>>> CAN frame loss.
>>>
>>> As skb timestamping became now mandatory for CAN related skbs this patch
>>> makes sure that received CAN skbs always have a proper timestamp set.
>>> Maybe there's a better solution in the future but this patch fixes the
>>> CAN frame loss so far.
>>>
>>
>> I'm not sure, but maybe this patch (and also my original one) opens a new 
>> potential issue with timestamps.
>>
>> If the timestamp is set at allocation time, this cancels setting the 
>> timestamp at delivery (by net_timestamp_check in, for example, 
>> netif_receive_skb_internal.) -> So it changes the behavior of timestamping 
>> (http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/timestamping.txt?id=b953c0d234bc72e8489d3bf51a276c5c4ec85345)
>>  generally.
> 
> The only change is that the timestamps for CAN skbs are generated always.
> The idea behind the timestamping by user control is to omit the timestamping 
> when it's not needed. There's no user visible change in behaviour when the 
> timestamp is set in the CAN skbs all time.
> 
>> Hypothetical example: If timestamping is enabled by the user and there is a 
>> significant delay between allocation and delivery of a skb (early allocation 
>> in driver or something) the timestamp does not reflect the reception time 
>> anymore.
> 
> The change only affects CAN skbs.
> These skbs are allocated at CAN frame reception time, filled with content and 
> then sent to the network layer.
> 
> AFAICS the timestamp becomes more precise for CAN related skbs.
> I did not see any case of 'early allocation' in linux/drivers/net/can, did 
> you?

No, I also did not find this case in current driver implementations -- because 
of that I gave the hypothetical example.
I just was worried about that this may be a potential latent issue for future 
driver implementations and wanted to indicate this.

But I trust your expertise, so if you are fine with it, I'm too. ;-)

Best regards,
Manfred
--
To unsubscribe from this list: send the line "unsubscribe netdev" in

[PATCH] net: fec: hard phy reset on open

[Manfred Schlaegl]

We have seen some problems with auto negotiation on i.MX6 using LAN8720,
after interface down/up.

In our configuration, the ptp clock is used externally as reference
clock for the phy. Some phys (e.g. LAN8720) needs a stable clock while
and after hard reset.
Before this patch, the driver disabled the clock on close but did no
hard reset on open, after enabling the clocks again.

A solution that prevents disabling the clocks on close was considered,
but discarded because of bad power saving behavior.

This patch saves the reset dt properties on probe and does a reset on
every open after clocks where enabled, to make sure the clock is stable
while and after hard reset.

Tested on i.MX6 and i.MX28, both using LAN8720.

Signed-off-by: Manfred Schlaegl 
---
 drivers/net/ethernet/freescale/fec.h  |  4 ++
 drivers/net/ethernet/freescale/fec_main.c | 77 +--
 2 files changed, 47 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h 
b/drivers/net/ethernet/freescale/fec.h
index c865135..379e619 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -498,6 +498,10 @@ struct fec_enet_private {
struct clk *clk_enet_out;
struct clk *clk_ptp;
 
+   int phy_reset;
+   bool phy_reset_active_high;
+   int phy_reset_msec;
+
bool ptp_clk_on;
struct mutex ptp_clk_mutex;
unsigned int num_tx_queues;
diff --git a/drivers/net/ethernet/freescale/fec_main.c 
b/drivers/net/ethernet/freescale/fec_main.c
index 48a033e..8cc1ec5 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2802,6 +2802,22 @@ static int fec_enet_alloc_buffers(struct net_device 
*ndev)
return 0;
 }
 
+static void fec_reset_phy(struct fec_enet_private *fep)
+{
+   if (!gpio_is_valid(fep->phy_reset))
+   return;
+
+   gpio_set_value_cansleep(fep->phy_reset, !!fep->phy_reset_active_high);
+
+   if (fep->phy_reset_msec > 20)
+   msleep(fep->phy_reset_msec);
+   else
+   usleep_range(fep->phy_reset_msec * 1000,
+fep->phy_reset_msec * 1000 + 1000);
+
+   gpio_set_value_cansleep(fep->phy_reset, !fep->phy_reset_active_high);
+}
+
 static int
 fec_enet_open(struct net_device *ndev)
 {
@@ -2817,6 +2833,8 @@ fec_enet_open(struct net_device *ndev)
if (ret)
goto clk_enable;
 
+   fec_reset_phy(fep);
+
/* I should reset the ring buffers here, but I don't yet know
 * a simple way to do that.
 */
@@ -3183,52 +3201,39 @@ static int fec_enet_init(struct net_device *ndev)
return 0;
 }
 
-#ifdef CONFIG_OF
-static void fec_reset_phy(struct platform_device *pdev)
+static int
+fec_get_reset_phy(struct platform_device *pdev, int *msec, int *phy_reset,
+ bool *active_high)
 {
-   int err, phy_reset;
-   bool active_high = false;
-   int msec = 1;
+   int err;
struct device_node *np = pdev->dev.of_node;
 
-   if (!np)
-   return;
+   if (!np || !of_device_is_available(np))
+   return 0;
 
-   of_property_read_u32(np, "phy-reset-duration", &msec);
+   of_property_read_u32(np, "phy-reset-duration", msec);
/* A sane reset duration should not be longer than 1s */
-   if (msec > 1000)
-   msec = 1;
+   if (*msec > 1000)
+   *msec = 1;
 
-   phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
-   if (!gpio_is_valid(phy_reset))
-   return;
+   *phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
+   if (!gpio_is_valid(*phy_reset))
+   return 0;
 
-   active_high = of_property_read_bool(np, "phy-reset-active-high");
+   *active_high = of_property_read_bool(np, "phy-reset-active-high");
 
-   err = devm_gpio_request_one(&pdev->dev, phy_reset,
-   active_high ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
-   "phy-reset");
+   err = devm_gpio_request_one(&pdev->dev, *phy_reset,
+   *active_high ?
+   GPIOF_OUT_INIT_HIGH :
+   GPIOF_OUT_INIT_LOW,
+   "phy-reset");
if (err) {
dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err);
-   return;
+   return err;
}
 
-   if (msec > 20)
-   msleep(msec);
-   else
-   usleep_range(msec * 1000, msec * 1000 + 1000);
-
-   gpio_set_value_cansleep(phy_reset, !active_high);
-}
-#else /* CONFIG_OF */
-static void fec_reset_phy(struct platform_device *pdev)
-{
-   /*
-* In case of platform

Re: [PATCH] net: fec: hard phy reset on open

[Manfred Schlaegl]

On 2016-10-24 16:03, Andy Duan wrote:
> From: manfred.schla...@gmx.at   Sent: Monday, 
> October 24, 2016 5:26 PM
>> To: Andy Duan 
>> Cc: netdev@vger.kernel.org; linux-ker...@vger.kernel.org
>> Subject: [PATCH] net: fec: hard phy reset on open
>>
>> We have seen some problems with auto negotiation on i.MX6 using LAN8720,
>> after interface down/up.
>>
>> In our configuration, the ptp clock is used externally as reference clock for
>> the phy. Some phys (e.g. LAN8720) needs a stable clock while and after hard
>> reset.
>> Before this patch, the driver disabled the clock on close but did no hard 
>> reset
>> on open, after enabling the clocks again.
>>
>> A solution that prevents disabling the clocks on close was considered, but
>> discarded because of bad power saving behavior.
>>
>> This patch saves the reset dt properties on probe and does a reset on every
>> open after clocks where enabled, to make sure the clock is stable while and
>> after hard reset.
>>
>> Tested on i.MX6 and i.MX28, both using LAN8720.
>>
>> Signed-off-by: Manfred Schlaegl 
>> ---

> This patch did hard reset to let phy stable.

> Firstly, you should do reset before clock enable.
I have to disagree here.
The phy demands(datasheet + tests) a stable clock at the time of (hard-)reset 
and after this. Therefore the clock has to be enabled before the hard reset.
(This is exactly the reason for the patch.)

Generally: The sense of a reset is to defer the start of digital circuit until 
the environment (power, clocks, ...) has stabilized.

Furthermore: Before this patch the hard reset was done in fec_probe. And here 
also after the clocks were enabled.

Whats was your argument to do it the other way in this special case?

> Secondly, we suggest to do phy reset in phy driver, not MAC driver.
I was not sure, if you meant a soft-, or hard-reset here.

In case you are talking about soft reset:
Yes, the phy drivers perform a soft reset. Sadly a soft reset is not sufficient 
in this case - The phy recovers only on a hard reset from lost clock. 
(datasheet + tests)

In case you're talking about hard reset:
Intuitively I would also think, that the (hard-)reset should be handled by the 
phy driver, but this is not reality in given implementations.

Documentation/devicetree/bindings/net/fsl-fec.txt says

- phy-reset-gpios : Should specify the gpio for phy reset

It is explicitly talked about phy-reset here. And the (hard-)reset was handled 
by the fec driver also before this patch (once on probe).

> 
> Regards,
> Andy

Thanks for your feedback!

Best regards,
Manfred



> 
>>  drivers/net/ethernet/freescale/fec.h  |  4 ++
>>  drivers/net/ethernet/freescale/fec_main.c | 77 +---
>> ---
>>  2 files changed, 47 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/fec.h
>> b/drivers/net/ethernet/freescale/fec.h
>> index c865135..379e619 100644
>> --- a/drivers/net/ethernet/freescale/fec.h
>> +++ b/drivers/net/ethernet/freescale/fec.h
>> @@ -498,6 +498,10 @@ struct fec_enet_private {
>>  struct clk *clk_enet_out;
>>  struct clk *clk_ptp;
>>
>> +int phy_reset;
>> +bool phy_reset_active_high;
>> +int phy_reset_msec;
>> +
>>  bool ptp_clk_on;
>>  struct mutex ptp_clk_mutex;
>>  unsigned int num_tx_queues;
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index 48a033e..8cc1ec5 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -2802,6 +2802,22 @@ static int fec_enet_alloc_buffers(struct
>> net_device *ndev)
>>  return 0;
>>  }
>>
>> +static void fec_reset_phy(struct fec_enet_private *fep) {
>> +if (!gpio_is_valid(fep->phy_reset))
>> +return;
>> +
>> +gpio_set_value_cansleep(fep->phy_reset, !!fep-
>>> phy_reset_active_high);
>> +
>> +if (fep->phy_reset_msec > 20)
>> +msleep(fep->phy_reset_msec);
>> +else
>> +usleep_range(fep->phy_reset_msec * 1000,
>> + fep->phy_reset_msec * 1000 + 1000);
>> +
>> +gpio_set_value_cansleep(fep->phy_reset, !fep-
>>> phy_reset_active_high);
>> +}
>> +
>>  static int
>>  fec_enet_open(struct net_device *ndev)
>>  {
>> @@ -2817,6 +2833,8 @@ fec_enet_open(struct net_device *ndev)
>>  if (ret)
>>  goto clk_enable;
>>
>> +fec_reset_phy(fep);
>> +
>>  /* I should r

Re: [PATCH] Revert "can: dev: __can_get_echo_skb(): print error message, if trying to echo non existing skb"

[Manfred Schlaegl]



Manfred Schlaegl | Leitung Entwicklung Linz 

GINZINGER ELECTRONIC SYSTEMS GMBH

Tel.: +43 7723 5422 153
Mobil: +43 676 841 208 253
Mail: manfred.schla...@ginzinger.com
Web: 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 
> 
> Applied to linux-can.

Great, thanks!

> 
> Tnx,
> Marc
> 






Ginzinger electronic systems GmbH
Gewerbegebiet Pirath 16
4952 Weng im Innkreis
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.


smime.p7s
Description: S/MIME cryptographic signature

[PATCH] Revert "can: dev: __can_get_echo_skb(): print error message, if trying to echo non existing skb"

[Manfred Schlaegl]

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 
---
 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



smime.p7s
Description: S/MIME cryptographic signature