Re: [PATCH] can: dev: can_restart(): post buffer from the right context

Re: [PATCH] can: dev: can_restart(): post buffer from the right context

[Alejandro]

From: Alejandro Concepcion Rodriguez<alejandro@acoro.eu>

netif_rx() is meant to be called from interrupt contexts. can_restart()
may be called by can_restart_work(), which is called from a worqueue, so
it may run in process context. Use netif_rx_any_context() which invokes
the correct code path depending on context.

Co-developed-by: Loris Fauster<loris.fauster@ttcontrol.com>
Signed-off-by: Loris Fauster<loris.fauster@ttcontrol.com>
Signed-off-by: Alejandro Concepcion Rodriguez<alejandro@acoro.eu>
---
  drivers/net/can/dev.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index b70ded3760f2..83114f8e8c24 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -584,7 +584,7 @@ static void can_restart(struct net_device *dev)
  
  	cf->can_id |= CAN_ERR_RESTARTED;
  
-	netif_rx(skb);
+	netif_rx_any_context(skb);
  
  	stats->rx_packets++;
  	stats->rx_bytes += cf->can_dlc;
-- 
2.17.1

Re: [PATCH] can: dev: can_restart(): post buffer from the right context

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1536 bytes --]

On 11/5/20 10:51 PM, Alejandro wrote:
> From: Alejandro Concepcion Rodriguez<alejandro@acoro.eu>
> 
> netif_rx() is meant to be called from interrupt contexts. can_restart()
> may be called by can_restart_work(), which is called from a worqueue, so
> it may run in process context. Use netif_rx_any_context() which invokes
> the correct code path depending on context.
> 
> Co-developed-by: Loris Fauster<loris.fauster@ttcontrol.com>
> Signed-off-by: Loris Fauster<loris.fauster@ttcontrol.com>
> Signed-off-by: Alejandro Concepcion Rodriguez<alejandro@acoro.eu>

I think we either call can_restart() from a netlink callback via
can_restart_now() or via the can_restart_work(). So we should always use
netif_rx_ni(skb), right?

> ---
>   drivers/net/can/dev.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index b70ded3760f2..83114f8e8c24 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -584,7 +584,7 @@ static void can_restart(struct net_device *dev)
>   
>   	cf->can_id |= CAN_ERR_RESTARTED;
>   
> -	netif_rx(skb);
> +	netif_rx_any_context(skb);
>   
>   	stats->rx_packets++;
>   	stats->rx_bytes += cf->can_dlc;
> 

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

Re: [PATCH] can: dev: can_restart(): post buffer from the right context

[Alejandro]

On 6/11/20 11:25, Marc Kleine-Budde wrote:

> On 11/5/20 10:51 PM, Alejandro wrote:
>> From: Alejandro Concepcion Rodriguez<alejandro@acoro.eu>
>>
>> netif_rx() is meant to be called from interrupt contexts. can_restart()
>> may be called by can_restart_work(), which is called from a worqueue, so
>> it may run in process context. Use netif_rx_any_context() which invokes
>> the correct code path depending on context.
>>
>> Co-developed-by: Loris Fauster<loris.fauster@ttcontrol.com>
>> Signed-off-by: Loris Fauster<loris.fauster@ttcontrol.com>
>> Signed-off-by: Alejandro Concepcion Rodriguez<alejandro@acoro.eu>
> I think we either call can_restart() from a netlink callback via
> can_restart_now() or via the can_restart_work(). So we should always use
> netif_rx_ni(skb), right?

Right, I think that currently it is as you say. However, it seems that
can_restart_now() has public visibility (/linux/can/dev.h), and even though
it doesn't seem to be used by other CAN drivers for now, I guess it could
potentially be used in future. netif_rx_any_context() should avoid issues
if can_restart_now() is called from an ISR later.

>
>> ---
>>    drivers/net/can/dev.c | 2 +-
>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>> index b70ded3760f2..83114f8e8c24 100644
>> --- a/drivers/net/can/dev.c
>> +++ b/drivers/net/can/dev.c
>> @@ -584,7 +584,7 @@ static void can_restart(struct net_device *dev)
>>    
>>    	cf->can_id |= CAN_ERR_RESTARTED;
>>    
>> -	netif_rx(skb);
>> +	netif_rx_any_context(skb);
>>    
>>    	stats->rx_packets++;
>>    	stats->rx_bytes += cf->can_dlc;
>>
> Marc
>
BR,
Alejandro

Re: [PATCH] can: dev: can_restart(): post buffer from the right context

[Alejandro]

Hi Marc,

My understanding is that can_restart_now() may be called in interrupt context
by some CAN driver, so netif_rx_any_context() would be safer, but I could be
wrong and maybe can_restart_now() is really thought to be called in process
context always (so better to use netif_rx_ni()). What do you think? should I
update the patch?

Thanks in advance,
Alejandro

On 6/11/20 18:33, Alejandro wrote:

> On 6/11/20 11:25, Marc Kleine-Budde wrote:
>
>> On 11/5/20 10:51 PM, Alejandro wrote:
>>> From: Alejandro Concepcion Rodriguez<alejandro@acoro.eu>
>>>
>>> netif_rx() is meant to be called from interrupt contexts. can_restart()
>>> may be called by can_restart_work(), which is called from a worqueue, so
>>> it may run in process context. Use netif_rx_any_context() which invokes
>>> the correct code path depending on context.
>>>
>>> Co-developed-by: Loris Fauster<loris.fauster@ttcontrol.com>
>>> Signed-off-by: Loris Fauster<loris.fauster@ttcontrol.com>
>>> Signed-off-by: Alejandro Concepcion Rodriguez<alejandro@acoro.eu>
>> I think we either call can_restart() from a netlink callback via
>> can_restart_now() or via the can_restart_work(). So we should always use
>> netif_rx_ni(skb), right?
> Right, I think that currently it is as you say. However, it seems that
> can_restart_now() has public visibility (/linux/can/dev.h), and even though
> it doesn't seem to be used by other CAN drivers for now, I guess it could
> potentially be used in future. netif_rx_any_context() should avoid issues
> if can_restart_now() is called from an ISR later.
>
>>> ---
>>>     drivers/net/can/dev.c | 2 +-
>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>>> index b70ded3760f2..83114f8e8c24 100644
>>> --- a/drivers/net/can/dev.c
>>> +++ b/drivers/net/can/dev.c
>>> @@ -584,7 +584,7 @@ static void can_restart(struct net_device *dev)
>>>     
>>>     	cf->can_id |= CAN_ERR_RESTARTED;
>>>     
>>> -	netif_rx(skb);
>>> +	netif_rx_any_context(skb);
>>>     
>>>     	stats->rx_packets++;
>>>     	stats->rx_bytes += cf->can_dlc;
>>>
>> Marc
>>
> BR,
> Alejandro

Re: [PATCH] can: dev: can_restart(): post buffer from the right context

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1766 bytes --]

On 11/6/20 6:33 PM, Alejandro wrote:
> On 6/11/20 11:25, Marc Kleine-Budde wrote:
> 
>> On 11/5/20 10:51 PM, Alejandro wrote:
>>> From: Alejandro Concepcion Rodriguez<alejandro@acoro.eu>
>>>
>>> netif_rx() is meant to be called from interrupt contexts. can_restart()
>>> may be called by can_restart_work(), which is called from a worqueue, so
>>> it may run in process context. Use netif_rx_any_context() which invokes
>>> the correct code path depending on context.
>>>
>>> Co-developed-by: Loris Fauster<loris.fauster@ttcontrol.com>
>>> Signed-off-by: Loris Fauster<loris.fauster@ttcontrol.com>
>>> Signed-off-by: Alejandro Concepcion Rodriguez<alejandro@acoro.eu>
>> I think we either call can_restart() from a netlink callback via
>> can_restart_now() or via the can_restart_work(). So we should always use
>> netif_rx_ni(skb), right?
> 
> Right, I think that currently it is as you say. However, it seems that
> can_restart_now() has public visibility (/linux/can/dev.h),

Yes, but it doesn't export it's symbols. I've removed the function from the
header file and everything still compiles.

> and even though
> it doesn't seem to be used by other CAN drivers for now, I guess it could
> potentially be used in future. netif_rx_any_context() should avoid issues
> if can_restart_now() is called from an ISR later.

I don't see a valid usecase where can_restart_now() is called from an ISR. We
can change this if an uscase pops up later.

regards,
Marc

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: dev: can_restart(): post buffer from the right context

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 730 bytes --]

On 11/11/20 9:55 PM, Alejandro wrote:
> My understanding is that can_restart_now() may be called in interrupt context
> by some CAN driver, so netif_rx_any_context() would be safer, but I could be
> wrong and maybe can_restart_now() is really thought to be called in process
> context always (so better to use netif_rx_ni()). What do you think? should I
> update the patch?

I've updated the patch and applied it to linux-can/testing.

Tnx,
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 --]

Re: [PATCH] can: dev: can_restart(): post buffer from the right context

[Alejandro]

Thanks for quick response. I'll send a second version of the patch if that's OK :).

BR,
Alejandro

On 11/11/20 22:48, Marc Kleine-Budde wrote:

> On 11/6/20 6:33 PM, Alejandro wrote:
>> On 6/11/20 11:25, Marc Kleine-Budde wrote:
>>
>>> On 11/5/20 10:51 PM, Alejandro wrote:
>>>> From: Alejandro Concepcion Rodriguez<alejandro@acoro.eu>
>>>>
>>>> netif_rx() is meant to be called from interrupt contexts. can_restart()
>>>> may be called by can_restart_work(), which is called from a worqueue, so
>>>> it may run in process context. Use netif_rx_any_context() which invokes
>>>> the correct code path depending on context.
>>>>
>>>> Co-developed-by: Loris Fauster<loris.fauster@ttcontrol.com>
>>>> Signed-off-by: Loris Fauster<loris.fauster@ttcontrol.com>
>>>> Signed-off-by: Alejandro Concepcion Rodriguez<alejandro@acoro.eu>
>>> I think we either call can_restart() from a netlink callback via
>>> can_restart_now() or via the can_restart_work(). So we should always use
>>> netif_rx_ni(skb), right?
>> Right, I think that currently it is as you say. However, it seems that
>> can_restart_now() has public visibility (/linux/can/dev.h),
> Yes, but it doesn't export it's symbols. I've removed the function from the
> header file and everything still compiles.
>
>> and even though
>> it doesn't seem to be used by other CAN drivers for now, I guess it could
>> potentially be used in future. netif_rx_any_context() should avoid issues
>> if can_restart_now() is called from an ISR later.
> I don't see a valid usecase where can_restart_now() is called from an ISR. We
> can change this if an uscase pops up later.
>
> regards,
> Marc
>
>

Re: [PATCH] can: dev: can_restart(): post buffer from the right context

[Alejandro]

Ok, thanks!

On 11/11/20 23:04, Marc Kleine-Budde wrote:

> On 11/11/20 9:55 PM, Alejandro wrote:
>> My understanding is that can_restart_now() may be called in interrupt context
>> by some CAN driver, so netif_rx_any_context() would be safer, but I could be
>> wrong and maybe can_restart_now() is really thought to be called in process
>> context always (so better to use netif_rx_ni()). What do you think? should I
>> update the patch?
> I've updated the patch and applied it to linux-can/testing.
>
> Tnx,
> Marc
>
>

[PATCH] can: dev: can_restart(): post buffer from the right context

[Alejandro]

From: Alejandro Concepcion Rodriguez <alejandro@acoro.eu>

netif_rx() is meant to be called from interrupt contexts. can_restart()
may be called by can_restart_work(), which is called from a worqueue, so
it may run in process context. Use netif_rx_any_context() which invokes
the correct code path depending on context.

Co-developed-by: Loris Fauster <loris.fauster@ttcontrol.com>
Signed-off-by: Loris Fauster <loris.fauster@ttcontrol.com>
Signed-off-by: Alejandro Concepcion Rodriguez <alejandro@acoro.eu>
---
  drivers/net/can/dev.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index b70ded3760f2..83114f8e8c24 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -584,7 +584,7 @@ static void can_restart(struct net_device *dev)
  
  	cf->can_id |= CAN_ERR_RESTARTED;
  
-	netif_rx(skb);
+	netif_rx_any_context(skb);
  
  	stats->rx_packets++;
  	stats->rx_bytes += cf->can_dlc;
-- 
2.17.1