[PATCH 0/2] can: xilinx_can: Bug fixes on can driver

[PATCH 0/2] can: xilinx_can: Bug fixes on can driver

[Srinivas Neeli]

This patch series does the following:
-skip printing error message on deferred probe
-Fix usage of skb memory

Srinivas Neeli (1):
  can: xilinx_can: Fix usage of skb memory

Venkatesh Yadav Abbarapu (1):
  can: xilinx_can: skip error message on deferred probe

 drivers/net/can/xilinx_can.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH 1/2] can: xilinx_can: skip error message on deferred probe

[Srinivas Neeli]

From: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@xilinx.com>

When can clock is provided from the clock wizard, clock wizard driver
may not be available when can driver probes resulting to the error
message "bus clock not found error".

As this error message is not very useful to the end user, skip printing
in the case of deferred probe.

Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@xilinx.com>
Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
 drivers/net/can/xilinx_can.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 4a96e2dd7d77..c5f05b994435 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -1772,7 +1772,8 @@ static int xcan_probe(struct platform_device *pdev)
 
 	priv->bus_clk = devm_clk_get(&pdev->dev, devtype->bus_clk_name);
 	if (IS_ERR(priv->bus_clk)) {
-		dev_err(&pdev->dev, "bus clock not found\n");
+		if (PTR_ERR(priv->bus_clk) != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "bus clock not found\n");
 		ret = PTR_ERR(priv->bus_clk);
 		goto err_free;
 	}
-- 
2.7.4

Re: [PATCH 1/2] can: xilinx_can: skip error message on deferred probe

[Marc Kleine-Budde]

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

On 11/20/19 1:11 PM, Srinivas Neeli wrote:
> From: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@xilinx.com>
> 
> When can clock is provided from the clock wizard, clock wizard driver
       ^^^

The code looks like the "bus" clock is probed here, not the "can" clock.

> may not be available when can driver probes resulting to the error
> message "bus clock not found error".
> 
> As this error message is not very useful to the end user, skip printing
> in the case of deferred probe.
> 
> Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@xilinx.com>
> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>  drivers/net/can/xilinx_can.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 4a96e2dd7d77..c5f05b994435 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -1772,7 +1772,8 @@ static int xcan_probe(struct platform_device *pdev)
>  
>  	priv->bus_clk = devm_clk_get(&pdev->dev, devtype->bus_clk_name);
              ^^^^^^^
>  	if (IS_ERR(priv->bus_clk)) {
> -		dev_err(&pdev->dev, "bus clock not found\n");
> +		if (PTR_ERR(priv->bus_clk) != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "bus clock not found\n");
>  		ret = PTR_ERR(priv->bus_clk);
>  		goto err_free;
>  	}
> 

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 1/2] can: xilinx_can: skip error message on deferred probe

[Appana Durga Kedareswara Rao]

Hi,

> -----Original Message-----
> From: Srinivas Neeli <srinivas.neeli@xilinx.com>
> Sent: Wednesday, November 20, 2019 5:41 PM
> To: wg@grandegger.com; mkl@pengutronix.de; davem@davemloft.net;
> Michal Simek <michals@xilinx.com>; Appana Durga Kedareswara Rao
> <appanad@xilinx.com>
> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git
> <git@xilinx.com>; Naga Sureshkumar Relli <nagasure@xilinx.com>;
> Venkatesh Yadav Abbarapu <VABBARAP@xilinx.com>; Srinivas Neeli
> <sneeli@xilinx.com>
> Subject: [PATCH 1/2] can: xilinx_can: skip error message on deferred probe
> 
> From: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@xilinx.com>
> 
> When can clock is provided from the clock wizard, clock wizard driver may
> not be available when can driver probes resulting to the error message "bus
> clock not found error".
> 
> As this error message is not very useful to the end user, skip printing in the
> case of deferred probe.
> 
> Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@xilinx.com>
> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>

@Srinivas Neeli: Please send v2 with improved commit message as Marc suggested, feel free to add 
Reviewed-by: Appana Durga Kedareswara Rao <appana.durga.rao@xilinx.com> in v2. 

Regards,
Kedar.
> ---
>  drivers/net/can/xilinx_can.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c index
> 4a96e2dd7d77..c5f05b994435 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -1772,7 +1772,8 @@ static int xcan_probe(struct platform_device
> *pdev)
> 
>  	priv->bus_clk = devm_clk_get(&pdev->dev, devtype->bus_clk_name);
>  	if (IS_ERR(priv->bus_clk)) {
> -		dev_err(&pdev->dev, "bus clock not found\n");
> +		if (PTR_ERR(priv->bus_clk) != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "bus clock not found\n");
>  		ret = PTR_ERR(priv->bus_clk);
>  		goto err_free;
>  	}
> --
> 2.7.4

[PATCH 2/2] can: xilinx_can: Fix usage of skb memory

[Srinivas Neeli]

As per linux can framework, driver not allowed to touch the skb memory
after can_put_echo_skb() call.
This patch fixes the same.
https://www.spinics.net/lists/linux-can/msg02199.html

Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
 drivers/net/can/xilinx_can.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index c5f05b994435..536b0f8272f6 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -191,6 +191,8 @@ struct xcan_devtype_data {
  * @bus_clk:			Pointer to struct clk
  * @can_clk:			Pointer to struct clk
  * @devtype:			Device type specific constants
+ * @cfd:			Variable to struct canfd_frame
+ * @is_canfd:			For checking canfd or not
  */
 struct xcan_priv {
 	struct can_priv can;
@@ -208,6 +210,8 @@ struct xcan_priv {
 	struct clk *bus_clk;
 	struct clk *can_clk;
 	struct xcan_devtype_data devtype;
+	struct canfd_frame cfd;
+	bool is_canfd;
 };
 
 /* CAN Bittiming constants as per Xilinx CAN specs */
@@ -543,14 +547,13 @@ static int xcan_do_set_mode(struct net_device *ndev, enum can_mode mode)
 /**
  * xcan_write_frame - Write a frame to HW
  * @priv:		Driver private data structure
- * @skb:		sk_buff pointer that contains data to be Txed
+ * @cf:			canfd_frame pointer that contains data to be Txed
  * @frame_offset:	Register offset to write the frame to
  */
-static void xcan_write_frame(struct xcan_priv *priv, struct sk_buff *skb,
+static void xcan_write_frame(struct xcan_priv *priv, struct canfd_frame *cf,
 			     int frame_offset)
 {
 	u32 id, dlc, data[2] = {0, 0};
-	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
 	u32 ramoff, dwindex = 0, i;
 
 	/* Watch carefully on the bit sequence */
@@ -581,7 +584,7 @@ static void xcan_write_frame(struct xcan_priv *priv, struct sk_buff *skb,
 	}
 
 	dlc = can_len2dlc(cf->len) << XCAN_DLCR_DLC_SHIFT;
-	if (can_is_canfd_skb(skb)) {
+	if (priv->is_canfd) {
 		if (cf->flags & CANFD_BRS)
 			dlc |= XCAN_DLCR_BRS_MASK;
 		dlc |= XCAN_DLCR_EDL_MASK;
@@ -633,6 +636,9 @@ static int xcan_start_xmit_fifo(struct sk_buff *skb, struct net_device *ndev)
 	struct xcan_priv *priv = netdev_priv(ndev);
 	unsigned long flags;
 
+	priv->cfd = *((struct canfd_frame *)skb->data);
+	priv->is_canfd = can_is_canfd_skb(skb);
+
 	/* Check if the TX buffer is full */
 	if (unlikely(priv->read_reg(priv, XCAN_SR_OFFSET) &
 			XCAN_SR_TXFLL_MASK))
@@ -644,7 +650,7 @@ static int xcan_start_xmit_fifo(struct sk_buff *skb, struct net_device *ndev)
 
 	priv->tx_head++;
 
-	xcan_write_frame(priv, skb, XCAN_TXFIFO_OFFSET);
+	xcan_write_frame(priv, &priv->cfd, XCAN_TXFIFO_OFFSET);
 
 	/* Clear TX-FIFO-empty interrupt for xcan_tx_interrupt() */
 	if (priv->tx_max > 1)
@@ -671,6 +677,9 @@ static int xcan_start_xmit_mailbox(struct sk_buff *skb, struct net_device *ndev)
 	struct xcan_priv *priv = netdev_priv(ndev);
 	unsigned long flags;
 
+	priv->cfd = *((struct canfd_frame *)skb->data);
+	priv->is_canfd = can_is_canfd_skb(skb);
+
 	if (unlikely(priv->read_reg(priv, XCAN_TRR_OFFSET) &
 		     BIT(XCAN_TX_MAILBOX_IDX)))
 		return -ENOSPC;
@@ -681,7 +690,7 @@ static int xcan_start_xmit_mailbox(struct sk_buff *skb, struct net_device *ndev)
 
 	priv->tx_head++;
 
-	xcan_write_frame(priv, skb,
+	xcan_write_frame(priv, &priv->cfd,
 			 XCAN_TXMSG_FRAME_OFFSET(XCAN_TX_MAILBOX_IDX));
 
 	/* Mark buffer as ready for transmit */
-- 
2.7.4

Re: [PATCH 2/2] can: xilinx_can: Fix usage of skb memory

[Marc Kleine-Budde]

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

On 11/20/19 1:11 PM, Srinivas Neeli wrote:
> As per linux can framework, driver not allowed to touch the skb memory
> after can_put_echo_skb() call.
> This patch fixes the same.
> https://www.spinics.net/lists/linux-can/msg02199.html

NACK.

Don't make a copy of the CAN frame, move the can_put_echo_skb() after
you don't need the skb anymore.

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 0/2] can: xilinx_can: Bug fixes on can driver

[Marc Kleine-Budde]

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

On 11/20/19 1:11 PM, Srinivas Neeli wrote:
> This patch series does the following:
> -skip printing error message on deferred probe
> -Fix usage of skb memory

BTW: I'm looking for an official Maintainer for the xlinx_can driver.

The Maintainer will get an entry in the MAINTAINERS file, should test
new patches and give Reviewed-bys.

Is there a volunteer?

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 0/2] can: xilinx_can: Bug fixes on can driver

[Appana Durga Kedareswara Rao]

Hi Marc,

> 
> On 11/20/19 1:11 PM, Srinivas Neeli wrote:
> > This patch series does the following:
> > -skip printing error message on deferred probe -Fix usage of skb
> > memory
> 
> BTW: I'm looking for an official Maintainer for the xlinx_can driver.
> 
> The Maintainer will get an entry in the MAINTAINERS file, should test new
> patches and give Reviewed-bys.
> 
> Is there a volunteer?

Sure will volunteer the maintainership, will send the maintainer fragment patch... 

Regards,
Kedar.
> 
> 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 |