Re: [PATCH v7 1/4] can: m_can: Create a m_can platform framework

[Dan Murphy]

On 3/8/19 11:08 AM, Wolfgang Grandegger wrote:
> Hello,
> 
> Am 08.03.19 um 16:48 schrieb Dan Murphy:
>> Wolfgang
>>
>> On 3/8/19 8:41 AM, Wolfgang Grandegger wrote:
>>> Hello Dan,
>>>
>>> thinking more about it...
>>>
>>> Am 08.03.19 um 14:29 schrieb Wolfgang Grandegger:
>>>> Hello Dan,
>>>>
>>>> Am 08.03.19 um 13:44 schrieb Dan Murphy:
>>>>> Wolfgang
>>>>>
>>>>> On 3/8/19 4:10 AM, Wolfgang Grandegger wrote:
>>>>>> Hallo Dan,
>>>>>>
>>>>>> Am 05.03.19 um 16:52 schrieb Dan Murphy:
>>>>>>> Create a m_can platform framework that peripherial
>>>>>>> devices can register to and use common code and register sets.
>>>>>>> The peripherial devices may provide read/write and configuration
>>>>>>> support of the IP.
>>>>>>>
>>>>>>> Signed-off-by: Dan Murphy <dmurphy@xxxxxx>
>>>>>>> ---
>>>>>>>
>>>>>>>
>>>>>>> v7 - Fixed remaining new checkpatch issues, removed CSR setting, fixed tx hard
>>>>>>> start function to return tx_busy, and renamed device callbacks - https://lore.kernel.org/patchwork/patch/1047220/
>>>>>>>
>>>>>>> v6 - Squashed platform patch to this patch for bissectablity, fixed coding style
>>>>>>> issues, updated Kconfig help, placed mcan reg offsets back into c file, renamed
>>>>>>> priv->skb to priv->tx_skb and cleared perp interrupts at ISR start -
>>>>>>> Patch 1 comments - https://lore.kernel.org/patchwork/patch/1042446/
>>>>>>> Patch 2 comments - https://lore.kernel.org/patchwork/patch/1042442/
>>>>>>>
>>>>>>>  drivers/net/can/m_can/Kconfig          |  13 +-
>>>>>>>  drivers/net/can/m_can/Makefile         |   1 +
>>>>>>>  drivers/net/can/m_can/m_can.c          | 700 +++++++++++++------------
>>>>>>>  drivers/net/can/m_can/m_can.h          | 110 ++++
>>>>>>>  drivers/net/can/m_can/m_can_platform.c | 202 +++++++
>>>>>>>  5 files changed, 682 insertions(+), 344 deletions(-)
>>>>>>>  create mode 100644 drivers/net/can/m_can/m_can.h
>>>>>>>  create mode 100644 drivers/net/can/m_can/m_can_platform.c
>>>>>>>
>>>>>>> diff --git a/drivers/net/can/m_can/Kconfig b/drivers/net/can/m_can/Kconfig
>>>>>>> index 04f20dd39007..f7119fd72df4 100644
>>>>>>> --- a/drivers/net/can/m_can/Kconfig
>>>>>>> +++ b/drivers/net/can/m_can/Kconfig
>>>>>>> @@ -1,5 +1,14 @@
>>>>>>>  config CAN_M_CAN
>>>>>>> +	tristate "Bosch M_CAN support"
>>>>>>> +	---help---
>>>>>>> +	  Say Y here if you want support for Bosch M_CAN controller framework.
>>>>>>> +	  This is common support for devices that embed the Bosch M_CAN IP.
>>>>>>> +
>>>>>>> +config CAN_M_CAN_PLATFORM
>>>>>>> +	tristate "Bosch M_CAN support for io-mapped devices"
>>>>>>>  	depends on HAS_IOMEM
>>>>>>> -	tristate "Bosch M_CAN devices"
>>>>>>> +	depends on CAN_M_CAN
>>>>>>>  	---help---
>>>>>>> -	  Say Y here if you want to support for Bosch M_CAN controller.
>>>>>>> +	  Say Y here if you want support for IO Mapped Bosch M_CAN controller.
>>>>>>> +	  This support is for devices that have the Bosch M_CAN controller
>>>>>>> +	  IP embedded into the device and the IP is IO Mapped to the processor.
>>>>>>> diff --git a/drivers/net/can/m_can/Makefile b/drivers/net/can/m_can/Makefile
>>>>>>> index 8bbd7f24f5be..057bbcdb3c74 100644
>>>>>>> --- a/drivers/net/can/m_can/Makefile
>>>>>>> +++ b/drivers/net/can/m_can/Makefile
>>>>>>> @@ -3,3 +3,4 @@
>>>>>>>  #
>>>>>>>  
>>>>>>>  obj-$(CONFIG_CAN_M_CAN) += m_can.o
>>>>>>> +obj-$(CONFIG_CAN_M_CAN_PLATFORM) += m_can_platform.o
>>>>>>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>>>>>>> index 9b449400376b..a60278d94126 100644
>>>>>>> --- a/drivers/net/can/m_can/m_can.c
>>>>>>> +++ b/drivers/net/can/m_can/m_can.c
>>>>>>
>>>>>> ... snip...
>>>>>>
>>>>>>> +static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
>>>>>>> +				    struct net_device *dev)
>>>>>>> +{
>>>>>>> +	struct m_can_priv *priv = netdev_priv(dev);
>>>>>>> +
>>>>>>> +	if (can_dropped_invalid_skb(dev, skb))
>>>>>>> +		return NETDEV_TX_OK;
>>>>>>> +
>>>>>>> +	if (priv->is_peripherial) {
>>>>>>> +		if (priv->tx_skb) {
>>>>>>> +			netdev_err(dev, "hard_xmit called while tx busy\n");
>>>>>>> +			return NETDEV_TX_BUSY;
>>>>>>> +		}
>>>>>>
>>>>>> The problem with that approach is, that the upper layer will try to
>>>>>> resubmit the current "skb" but not the previous "tx_skb". And the
>>>>>> previous "tx_skb" has not been freed yet. I would just drop and free the
>>>>>> skb and return NETDEV_TX_OK in m_can_tx_handler() for peripheral devices
>>>>>> (like can_dropped_invalid_skb() does).
>>>>>>
>>>>>
>>>>> OK.
>>>>>
>>>>> So would this also be a bug in the hi3110 and mcp251x drivers (line 521) as well because besides checking tx_length
>>>>> this is how these drivers are written.
>>>>
>>>> This is different. When entering the "start_xmit" routine, the previous
>>>> TX is still in progress. It will (hopefully) complete soon. Therefore
>>>> returning NETDEV_TX_BUSY is OK. The "start_xmit" routine will be
>>>> recalled soon with the same "skb". That scenario should/could also not
>>>> happen.
>>>
>>> In principle, this also applies to the m_can peripheral devices. If
>>> tx_skb is not NULL, the TX is still in progress and returning
>>> NETDEV_TX_BUSY is just fine.
>>>
>>>>
>>>> In contrast, in "m_can_tx_handler()", the skb could not be handled
>>>> because the FIFO is full. The "start_xmit" routine for peripheral
>>>> devices for that skb already returned NETDEV_TX_OK. Therefore the only
>>>> meaningful action is to drop the skb. Also this error should not happen
>>>> and if, something is going really wrong. Therefore I think, a
>>>> WARN_ONCE() would be even more appropriate. But that should be a
>>>> separate patch.
>>>
>>> But that's a different issue/error. The tx_skb cannot be processed in
>>> "m_can_tx_handler()". Either we drop it or we re-queue it (retry later).
>>>
>>
>> OK I am a bit confused on this.  Are you saying this is not an issue?
>> Or are you saying I need to check for tx_len like the other code?
> 
> If you check for tx_skb in the "start_xmit" routine like the hi3110 and
> mcp251x, it will work the same way. But only, if the "tx_handler()" has
> fully processed the message. It simple means, the TX is still in
> progress and will complete soon. But in "m_can_tx_handler()" we return
> without handling the message! It will never be sent and freed. Or will
> the "m_can_tx_handler()" retry?
> 

I am not seeing where we are not handling the message in the m_can_tx_handler()

In the peripheral code the work is queued up.  And the work thread is m_can_tx_work_queue.

This in turn calls the m_can_tx_handler and the worker is blocked until return which means the message
would have been processed.

If there is no issue and the handler returns OK then the skb is set to null.
Otherwise the only other time that the skb will not be null is if the FIFO was full.

Plus there can only be one work queue at a time so the processing is synchronous.
If the upper layer decides to send another packet before the prior one is complete then it will get
a TX busy return.

IOmapped calls are blocked on return so this is not an issue.  We cannot do it the same way with peripherals due to the
atomic context of the request.

Dan

>> Again if this code is an issue here I believe this is an issue in the hi3110 and mcp251x
> 
> I don't think so.
> 
> Sorry for confusion.
> 
> Wolfgang.
> 


-- 
------------------
Dan Murphy