[PATCH 0/3] can: grcan: Bug fixes

[PATCH 0/3] can: grcan: Bug fixes

[Andreas Larsson]

These patches
* makes sure that DMA memory is allocated properly
* avoids the tx errata workaround needlessly being applied
* fixes a bug where the driver can be left hanging without interrupts enabled

Andreas Larsson (2):
  can: grcan: Fix broken system id check for errata workaround needs
  can: grcan: Only use the napi poll budget for rx

Daniel Hellstrom (1):
  can: grcan: use ofdev->dev when allocating DMA memory

 drivers/net/can/grcan.c | 45 +++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

-- 
2.17.1

[PATCH 1/3] can: grcan: Use ofdev->dev when allocating DMA memory

[Andreas Larsson]

From: Daniel Hellstrom <daniel@gaisler.com>

Use the device of the device tree node should be rather than the device
of the struct net_device when allocating DMA buffers.

Since commit 53b7670e5735 ("sparc: factor the dma coherent mapping into
helper") the driver oopses when using the wrong device on sparc32.

Signed-off-by: Daniel Hellstrom <daniel@gaisler.com>
Signed-off-by: Andreas Larsson <andreas@gaisler.com>
---
 drivers/net/can/grcan.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
index d0c5a7a60daf..f7c3cf941f61 100644
--- a/drivers/net/can/grcan.c
+++ b/drivers/net/can/grcan.c
@@ -248,6 +248,7 @@ struct grcan_device_config {
 struct grcan_priv {
 	struct can_priv can;	/* must be the first member */
 	struct net_device *dev;
+	struct device *ofdev_dev;
 	struct napi_struct napi;
 
 	struct grcan_registers __iomem *regs;	/* ioremap'ed registers */
@@ -921,7 +922,7 @@ static void grcan_free_dma_buffers(struct net_device *dev)
 	struct grcan_priv *priv = netdev_priv(dev);
 	struct grcan_dma *dma = &priv->dma;
 
-	dma_free_coherent(&dev->dev, dma->base_size, dma->base_buf,
+	dma_free_coherent(priv->ofdev_dev, dma->base_size, dma->base_buf,
 			  dma->base_handle);
 	memset(dma, 0, sizeof(*dma));
 }
@@ -946,7 +947,8 @@ static int grcan_allocate_dma_buffers(struct net_device *dev,
 
 	/* Extra GRCAN_BUFFER_ALIGNMENT to allow for alignment */
 	dma->base_size = lsize + ssize + GRCAN_BUFFER_ALIGNMENT;
-	dma->base_buf = dma_alloc_coherent(&dev->dev,
+
+	dma->base_buf = dma_alloc_coherent(priv->ofdev_dev,
 					   dma->base_size,
 					   &dma->base_handle,
 					   GFP_KERNEL);
@@ -1587,6 +1589,7 @@ static int grcan_setup_netdev(struct platform_device *ofdev,
 	memcpy(&priv->config, &grcan_module_config,
 	       sizeof(struct grcan_device_config));
 	priv->dev = dev;
+	priv->ofdev_dev = &ofdev->dev;
 	priv->regs = base;
 	priv->can.bittiming_const = &grcan_bittiming_const;
 	priv->can.do_set_bittiming = grcan_set_bittiming;
-- 
2.17.1

[PATCH 2/3] can: grcan: Fix broken system id check for errata workaround needs

[Andreas Larsson]

The systemid property was checked for in the wrong place of the
devicetree and compared to the wrong value.

Signed-off-by: Andreas Larsson <andreas@gaisler.com>
---
 drivers/net/can/grcan.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
index f7c3cf941f61..2f56d4bbb65c 100644
--- a/drivers/net/can/grcan.c
+++ b/drivers/net/can/grcan.c
@@ -241,7 +241,7 @@ struct grcan_device_config {
 		.rxsize		= GRCAN_DEFAULT_BUFFER_SIZE,	\
 		}
 
-#define GRCAN_TXBUG_SAFE_GRLIB_VERSION	0x4100
+#define GRCAN_TXBUG_SAFE_GRLIB_VERSION	4100
 #define GRLIB_VERSION_MASK		0xffff
 
 /* GRCAN private data structure */
@@ -1642,6 +1642,7 @@ static int grcan_setup_netdev(struct platform_device *ofdev,
 static int grcan_probe(struct platform_device *ofdev)
 {
 	struct device_node *np = ofdev->dev.of_node;
+	struct device_node *sysid_parent;
 	u32 sysid, ambafreq;
 	int irq, err;
 	void __iomem *base;
@@ -1650,10 +1651,15 @@ static int grcan_probe(struct platform_device *ofdev)
 	/* Compare GRLIB version number with the first that does not
 	 * have the tx bug (see start_xmit)
 	 */
-	err = of_property_read_u32(np, "systemid", &sysid);
-	if (!err && ((sysid & GRLIB_VERSION_MASK)
-		     >= GRCAN_TXBUG_SAFE_GRLIB_VERSION))
-		txbug = false;
+	sysid_parent = of_find_node_by_path("/ambapp0");
+	if (sysid_parent) {
+		of_node_get(sysid_parent);
+		err = of_property_read_u32(sysid_parent, "systemid", &sysid);
+		if (!err && ((sysid & GRLIB_VERSION_MASK)
+			     >= GRCAN_TXBUG_SAFE_GRLIB_VERSION))
+			txbug = false;
+		of_node_put(sysid_parent);
+	}
 
 	err = of_property_read_u32(np, "freq", &ambafreq);
 	if (err) {
-- 
2.17.1

[PATCH 3/3] can: grcan: Only use the napi poll budget for rx

[Andreas Larsson]

The previous split budget between tx and rx made it return not using the
entire budget but at the same time not having calling called
napi_complete. This sometimes led to the poll to not be called, and at
the same time with tx and rx interrupts disabled.

Signed-off-by: Andreas Larsson <andreas@gaisler.com>
---
 drivers/net/can/grcan.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
index 2f56d4bbb65c..cb98eadc3b93 100644
--- a/drivers/net/can/grcan.c
+++ b/drivers/net/can/grcan.c
@@ -1124,7 +1124,7 @@ static int grcan_close(struct net_device *dev)
 	return 0;
 }
 
-static int grcan_transmit_catch_up(struct net_device *dev, int budget)
+static void grcan_transmit_catch_up(struct net_device *dev)
 {
 	struct grcan_priv *priv = netdev_priv(dev);
 	unsigned long flags;
@@ -1132,7 +1132,7 @@ static int grcan_transmit_catch_up(struct net_device *dev, int budget)
 
 	spin_lock_irqsave(&priv->lock, flags);
 
-	work_done = catch_up_echo_skb(dev, budget, true);
+	work_done = catch_up_echo_skb(dev, -1, true);
 	if (work_done) {
 		if (!priv->resetting && !priv->closing &&
 		    !(priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY))
@@ -1146,8 +1146,6 @@ static int grcan_transmit_catch_up(struct net_device *dev, int budget)
 	}
 
 	spin_unlock_irqrestore(&priv->lock, flags);
-
-	return work_done;
 }
 
 static int grcan_receive(struct net_device *dev, int budget)
@@ -1229,19 +1227,13 @@ static int grcan_poll(struct napi_struct *napi, int budget)
 	struct net_device *dev = priv->dev;
 	struct grcan_registers __iomem *regs = priv->regs;
 	unsigned long flags;
-	int tx_work_done, rx_work_done;
-	int rx_budget = budget / 2;
-	int tx_budget = budget - rx_budget;
+	int work_done;
 
-	/* Half of the budget for receiving messages */
-	rx_work_done = grcan_receive(dev, rx_budget);
+	work_done = grcan_receive(dev, budget);
 
-	/* Half of the budget for transmitting messages as that can trigger echo
-	 * frames being received
-	 */
-	tx_work_done = grcan_transmit_catch_up(dev, tx_budget);
+	grcan_transmit_catch_up(dev);
 
-	if (rx_work_done < rx_budget && tx_work_done < tx_budget) {
+	if (work_done < budget) {
 		napi_complete(napi);
 
 		/* Guarantee no interference with a running reset that otherwise
@@ -1258,7 +1250,7 @@ static int grcan_poll(struct napi_struct *napi, int budget)
 		spin_unlock_irqrestore(&priv->lock, flags);
 	}
 
-	return rx_work_done + tx_work_done;
+	return work_done;
 }
 
 /* Work tx bug by waiting while for the risky situation to clear. If that fails,
-- 
2.17.1

Re: [PATCH 0/3] can: grcan: Bug fixes

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 959 bytes --]

On 27.04.2022 15:43:04, Andreas Larsson wrote:
> These patches
> * makes sure that DMA memory is allocated properly
> * avoids the tx errata workaround needlessly being applied
> * fixes a bug where the driver can be left hanging without interrupts enabled
> 
> Andreas Larsson (2):
>   can: grcan: Fix broken system id check for errata workaround needs
>   can: grcan: Only use the napi poll budget for rx
> 
> Daniel Hellstrom (1):
>   can: grcan: use ofdev->dev when allocating DMA memory

Thanks for the patches. Can you please add a "Fixes:" tag to each patch.
From the description it seems they should be backported to the stable
kernels, correct?

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: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3] can: grcan: Bug fixes

[Andreas Larsson]

On 2022-04-28 08:50, Marc Kleine-Budde wrote:
> On 27.04.2022 15:43:04, Andreas Larsson wrote:
>> These patches
>> * makes sure that DMA memory is allocated properly
>> * avoids the tx errata workaround needlessly being applied
>> * fixes a bug where the driver can be left hanging without interrupts enabled
>>
>> Andreas Larsson (2):
>>    can: grcan: Fix broken system id check for errata workaround needs
>>    can: grcan: Only use the napi poll budget for rx
>>
>> Daniel Hellstrom (1):
>>    can: grcan: use ofdev->dev when allocating DMA memory
> 
> Thanks for the patches. Can you please add a "Fixes:" tag to each patch.
>  From the description it seems they should be backported to the stable
> kernels, correct?


For patch 1 I can add a Fixes: that points to 53b7670e5735, which is the
patch after which the patch is needed (even though that commit is not
bad in itself, but merely exposes a wrongly used device pointer in the
grcan driver).

For patch 2 though I am not sure. I don't think the problem has always
been there, but I am not really sure what commit to point to. The fix is
at least needed for 4.9 and onward and 4.9 is the oldest stable branch
still under maintenance. Seems not to be worth the effort to find
exactly from which commit the grcan driver's quirky use of the napi
budget actually lead to problems just to make sure that it gets applied
to all currently maintained stable branches. Suggestions?

For patch 3 I can point to the driver's original commit for the grcan
driver as the problem has been there all along.

Cheers,
Andreas

Re: [PATCH 0/3] can: grcan: Bug fixes

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2336 bytes --]

On 28.04.2022 14:22:17, Andreas Larsson wrote:
> On 2022-04-28 08:50, Marc Kleine-Budde wrote:
> > On 27.04.2022 15:43:04, Andreas Larsson wrote:
> > > These patches
> > > * makes sure that DMA memory is allocated properly
> > > * avoids the tx errata workaround needlessly being applied
> > > * fixes a bug where the driver can be left hanging without interrupts enabled
> > > 
> > > Andreas Larsson (2):
> > >    can: grcan: Fix broken system id check for errata workaround needs
> > >    can: grcan: Only use the napi poll budget for rx
> > > 
> > > Daniel Hellstrom (1):
> > >    can: grcan: use ofdev->dev when allocating DMA memory
> > 
> > Thanks for the patches. Can you please add a "Fixes:" tag to each patch.
> >  From the description it seems they should be backported to the stable
> > kernels, correct?
> 
> For patch 1 I can add a Fixes: that points to 53b7670e5735, which is the
> patch after which the patch is needed (even though that commit is not
> bad in itself, but merely exposes a wrongly used device pointer in the
> grcan driver).

The Fixes tag should point to the patch the introduced the problem, not
the patch the exposes the problem :)

| 53b7670e5735 sparc: factor the dma coherent mapping into helper

> For patch 2 though I am not sure. I don't think the problem has always
> been there, but I am not really sure what commit to point to.

Point the Fixes tag to the commit that introduced the problem, if it was
always there, use the commit where the patch was added.

> The fix is at least needed for 4.9 and onward and 4.9 is the oldest
> stable branch still under maintenance. Seems not to be worth the
> effort to find exactly from which commit the grcan driver's quirky use
> of the napi budget actually lead to problems just to make sure that it
> gets applied to all currently maintained stable branches. Suggestions?

See above.

> For patch 3 I can point to the driver's original commit for the grcan
> driver as the problem has been there all along.

Fine.

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: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]