linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dan Murphy <dmurphy@ti.com>
To: Wolfgang Grandegger <wg@grandegger.com>, <mkl@pengutronix.de>,
	<davem@davemloft.net>
Cc: <linux-can@vger.kernel.org>, <netdev@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v7 1/4] can: m_can: Create a m_can platform framework
Date: Fri, 8 Mar 2019 06:44:01 -0600	[thread overview]
Message-ID: <bbc9db2f-0702-4f24-db64-a41dc0607d2d@ti.com> (raw)
In-Reply-To: <5065d6ba-f195-a695-77b1-b837cac1a199@grandegger.com>

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@ti.com>
>> ---
>>
>>
>> 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.

In addition in the peripheral context the work queue does not report up to the upper layer the status.
Again the hi3110 and mcp251x drivers are written this way.

The only issue I see here is that the dropped and invalid check needs to come after the tx_skb check.

Dan


>> +
>> +		priv->tx_skb = skb;
>> +		netif_stop_queue(priv->net);
>> +		queue_work(priv->tx_wq, &priv->tx_work);
>> +	} else {
>> +		priv->tx_skb = skb;
>> +		return m_can_tx_handler(priv);
>>  	}
>>  
>>  	return NETDEV_TX_OK;
>>  }
> 
> Wolfgang.
> 


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

  reply	other threads:[~2019-03-08 12:44 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-05 15:52 Dan Murphy
2019-03-05 15:52 ` [PATCH v7 2/4] can: m_can: Rename m_can_priv to m_can_classdev Dan Murphy
2019-03-08 10:25   ` Wolfgang Grandegger
2019-03-08 12:33     ` Dan Murphy
2019-03-05 15:52 ` [PATCH v7 3/4] dt-bindings: can: tcan4x5x: Add DT bindings for TCAN4x5X driver Dan Murphy
2019-03-05 15:52 ` [PATCH v7 4/4] can: tcan4x5x: Add tcan4x5x driver to the kernel Dan Murphy
2019-03-08 10:10 ` [PATCH v7 1/4] can: m_can: Create a m_can platform framework Wolfgang Grandegger
2019-03-08 12:44   ` Dan Murphy [this message]
2019-03-08 13:29     ` Wolfgang Grandegger
2019-03-08 14:41       ` Wolfgang Grandegger
2019-03-08 15:48         ` Dan Murphy
2019-03-08 17:08           ` Wolfgang Grandegger
2019-03-08 17:25             ` Dan Murphy
2019-03-08 17:40               ` Wolfgang Grandegger
2019-03-08 17:52                 ` Dan Murphy
2019-03-08 18:06                   ` Wolfgang Grandegger
2019-03-08 20:36                     ` Dan Murphy
2019-03-11 11:19                       ` Wolfgang Grandegger

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=bbc9db2f-0702-4f24-db64-a41dc0607d2d@ti.com \
    --to=dmurphy@ti.com \
    --cc=davem@davemloft.net \
    --cc=linux-can@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mkl@pengutronix.de \
    --cc=netdev@vger.kernel.org \
    --cc=wg@grandegger.com \
    --subject='Re: [PATCH v7 1/4] can: m_can: Create a m_can platform framework' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).