[1/3] staging: octeon: don't panic

Commit Message

Ladislav Michl April 13, 2023, 2:13 p.m. UTC

From: Ladislav Michl <[e-mail]>

It is unfortunate to halt kernel just because no network
interfaces was registered.

Signed-off-by: Ladislav Michl <[e-mail]>
---
 drivers/staging/octeon/ethernet-rx.c | 33 +++++++++++++++++++++++-----
 drivers/staging/octeon/ethernet-rx.h |  2 +-
 drivers/staging/octeon/ethernet-tx.c | 16 ++++++++------
 drivers/staging/octeon/ethernet-tx.h |  2 +-
 drivers/staging/octeon/ethernet.c    | 31 ++++++++++++++++++++++----
 5 files changed, 66 insertions(+), 18 deletions(-)

Comments

Andrew Lunn April 13, 2023, 3:57 p.m. UTC | #1

> -void cvm_oct_rx_initialize(void)
> +int cvm_oct_rx_initialize(void)
>  {
>  	int i;
>  	struct net_device *dev_for_napi = NULL;
> @@ -460,8 +460,11 @@ void cvm_oct_rx_initialize(void)
>  		}
>  	}
>  
> -	if (!dev_for_napi)
> -		panic("No net_devices were allocated.");
> +	if (!dev_for_napi) {
> +		pr_err("No net_devices were allocated.");

It is good practice to use dev_per(dev, ... You then know which device
has problem finding its net_devices.

checkpatch is probably warning you about this.

Once you have a registered netdev, you should then use netdev_err(),
netdev_dbg() etc.

However, cvm_oct_probe() in 6.3-rc6 seems to be FUBAR. As soon as you
call register_netdev(dev), the kernel can start using it, even before
that call returns. So the register_netdev(dev) should be the last
thing _probe does, once everything is set up. You can call
netdev_err() before it is registered, but the name is less
informative, something like "(unregistered)".

      Andrew

Ladislav Michl April 13, 2023, 4:14 p.m. UTC | #2

Hello Andrew,

On Thu, Apr 13, 2023 at 05:57:00PM +0200, Andrew Lunn wrote:
> > -void cvm_oct_rx_initialize(void)
> > +int cvm_oct_rx_initialize(void)
> >  {
> >  	int i;
> >  	struct net_device *dev_for_napi = NULL;
> > @@ -460,8 +460,11 @@ void cvm_oct_rx_initialize(void)
> >  		}
> >  	}
> >  
> > -	if (!dev_for_napi)
> > -		panic("No net_devices were allocated.");
> > +	if (!dev_for_napi) {
> > +		pr_err("No net_devices were allocated.");
> 
> It is good practice to use dev_per(dev, ... You then know which device
> has problem finding its net_devices.

Well, it would then need few more preparation commits to use proper
logging.

> checkpatch is probably warning you about this.
> 
> Once you have a registered netdev, you should then use netdev_err(),
> netdev_dbg() etc.

Problem with this code is that it registers netdevices in for loop,
so the only device available here is parent device to all that
netdevices (which weren't registered).

Perhaps use per netdev probe function first?

> However, cvm_oct_probe() in 6.3-rc6 seems to be FUBAR. As soon as you
> call register_netdev(dev), the kernel can start using it, even before
> that call returns. So the register_netdev(dev) should be the last
> thing _probe does, once everything is set up. You can call
> netdev_err() before it is registered, but the name is less
> informative, something like "(unregistered)".

On the side note, this (panic) cannot happen in current code as
it is using DT as a guidance only, but interfaces are hardcoded.
Later on, when DT is used to provide links, it can fail and then
kernel would panic.

>       Andrew

Thank you,
	ladis

Andrew Lunn April 13, 2023, 4:28 p.m. UTC | #3

> Problem with this code is that it registers netdevices in for loop,
> so the only device available here is parent device to all that
> netdevices (which weren't registered).

You always have pdev->dev, which you can use until you have a
registered netdev. That is a common pattern.

	   Andrew

Patch

diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c
index 965330eec80a..24ae4b3bd58b 100644
--- a/drivers/staging/octeon/ethernet-rx.c
+++ b/drivers/staging/octeon/ethernet-rx.c
@@ -448,7 +448,7 @@  void cvm_oct_poll_controller(struct net_device *dev)
 }
 #endif
 
-void cvm_oct_rx_initialize(void)
+int cvm_oct_rx_initialize(void)
 {
 	int i;
 	struct net_device *dev_for_napi = NULL;
@@ -460,8 +460,11 @@  void cvm_oct_rx_initialize(void)
 		}
 	}
 
-	if (!dev_for_napi)
-		panic("No net_devices were allocated.");
+	if (!dev_for_napi) {
+		pr_err("No net_devices were allocated.");
+		return -ENODEV;
+	}
+
 
 	for (i = 0; i < ARRAY_SIZE(oct_rx_group); i++) {
 		int ret;
@@ -479,10 +482,28 @@  void cvm_oct_rx_initialize(void)
 		/* Register an IRQ handler to receive POW interrupts */
 		ret = request_irq(oct_rx_group[i].irq, cvm_oct_do_interrupt, 0,
 				  "Ethernet", &oct_rx_group[i].napi);
-		if (ret)
-			panic("Could not acquire Ethernet IRQ %d\n",
+		if (ret) {
+			int j;
+
+			pr_err("Could not acquire Ethernet IRQ %d\n",
 			      oct_rx_group[i].irq);
 
+			for (j = 0; j < i; j++) {
+				if (!(pow_receive_groups & BIT(j)))
+					continue;
+
+				cvmx_write_csr(OCTEON_IS_MODEL(OCTEON_CN68XX) ?
+						CVMX_SSO_WQ_INT_THRX(j) :
+						CVMX_POW_WQ_INT_THRX(j),
+						0);
+				free_irq(oct_rx_group[j].irq, cvm_oct_device);
+				netif_napi_del(&oct_rx_group[j].napi);
+			}
+
+			return ret;
+		}
+
+
 		disable_irq_nosync(oct_rx_group[i].irq);
 
 		/* Enable POW interrupt when our port has at least one packet */
@@ -518,6 +539,8 @@  void cvm_oct_rx_initialize(void)
 		napi_schedule(&oct_rx_group[i].napi);
 	}
 	atomic_inc(&oct_rx_ready);
+
+	return 0;
 }
 
 void cvm_oct_rx_shutdown(void)
diff --git a/drivers/staging/octeon/ethernet-rx.h b/drivers/staging/octeon/ethernet-rx.h
index ff6482fa20d6..9a98f662d813 100644
--- a/drivers/staging/octeon/ethernet-rx.h
+++ b/drivers/staging/octeon/ethernet-rx.h
@@ -6,7 +6,7 @@ 
  */
 
 void cvm_oct_poll_controller(struct net_device *dev);
-void cvm_oct_rx_initialize(void);
+int cvm_oct_rx_initialize(void);
 void cvm_oct_rx_shutdown(void);
 
 static inline void cvm_oct_rx_refill_pool(int fill_threshold)
diff --git a/drivers/staging/octeon/ethernet-tx.c b/drivers/staging/octeon/ethernet-tx.c
index a36e36701c74..c02b023ac8c0 100644
--- a/drivers/staging/octeon/ethernet-tx.c
+++ b/drivers/staging/octeon/ethernet-tx.c
@@ -694,19 +694,21 @@  static irqreturn_t cvm_oct_tx_cleanup_watchdog(int cpl, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-void cvm_oct_tx_initialize(void)
+int cvm_oct_tx_initialize(void)
 {
-	int i;
+	int ret;
 
 	/* Disable the interrupt.  */
 	cvmx_write_csr(CVMX_CIU_TIMX(1), 0);
 	/* Register an IRQ handler to receive CIU_TIMX(1) interrupts */
-	i = request_irq(OCTEON_IRQ_TIMER1,
-			cvm_oct_tx_cleanup_watchdog, 0,
-			"Ethernet", cvm_oct_device);
+	ret = request_irq(OCTEON_IRQ_TIMER1,
+			  cvm_oct_tx_cleanup_watchdog, 0,
+			  "Ethernet", cvm_oct_device);
+
+	if (ret)
+		pr_err("Could not acquire Ethernet IRQ %d\n", OCTEON_IRQ_TIMER1);
 
-	if (i)
-		panic("Could not acquire Ethernet IRQ %d\n", OCTEON_IRQ_TIMER1);
+	return ret;
 }
 
 void cvm_oct_tx_shutdown(void)
diff --git a/drivers/staging/octeon/ethernet-tx.h b/drivers/staging/octeon/ethernet-tx.h
index 6c524668f65a..b4e328a4b499 100644
--- a/drivers/staging/octeon/ethernet-tx.h
+++ b/drivers/staging/octeon/ethernet-tx.h
@@ -9,6 +9,6 @@  netdev_tx_t cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev);
 netdev_tx_t cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev);
 int cvm_oct_transmit_qos(struct net_device *dev, void *work_queue_entry,
 			 int do_free, int qos);
-void cvm_oct_tx_initialize(void);
+int cvm_oct_tx_initialize(void);
 void cvm_oct_tx_shutdown(void);
 void cvm_oct_tx_shutdown_dev(struct net_device *dev);
diff --git a/drivers/staging/octeon/ethernet.c b/drivers/staging/octeon/ethernet.c
index f662739137b5..949ef51bf896 100644
--- a/drivers/staging/octeon/ethernet.c
+++ b/drivers/staging/octeon/ethernet.c
@@ -672,6 +672,8 @@  static void cvm_set_rgmii_delay(struct octeon_ethernet *priv, int iface,
 
 static int cvm_oct_probe(struct platform_device *pdev)
 {
+	int ret;
+	int port;
 	int num_interfaces;
 	int interface;
 	int fau = FAU_NUM_PACKET_BUFFERS_TO_FREE;
@@ -705,7 +707,6 @@  static int cvm_oct_probe(struct platform_device *pdev)
 	num_interfaces = cvmx_helper_get_number_of_interfaces();
 	for (interface = 0; interface < num_interfaces; interface++) {
 		int num_ports = cvmx_helper_ports_on_interface(interface);
-		int port;
 
 		for (port = cvmx_helper_get_ipd_port(interface, 0);
 		     port < cvmx_helper_get_ipd_port(interface, num_ports);
@@ -801,7 +802,6 @@  static int cvm_oct_probe(struct platform_device *pdev)
 		cvmx_helper_interface_mode_t imode =
 		    cvmx_helper_interface_get_mode(interface);
 		int num_ports = cvmx_helper_ports_on_interface(interface);
-		int port;
 		int port_index;
 
 		for (port_index = 0,
@@ -911,8 +911,15 @@  static int cvm_oct_probe(struct platform_device *pdev)
 		}
 	}
 
-	cvm_oct_tx_initialize();
-	cvm_oct_rx_initialize();
+	ret = cvm_oct_tx_initialize();
+	if (ret)
+		goto err;
+
+	ret = cvm_oct_rx_initialize();
+	if (ret) {
+		cvm_oct_tx_shutdown();
+		goto err;
+	}
 
 	/*
 	 * 150 uS: about 10 1500-byte packets at 1GE.
@@ -922,6 +929,22 @@  static int cvm_oct_probe(struct platform_device *pdev)
 	schedule_delayed_work(&cvm_oct_rx_refill_work, HZ);
 
 	return 0;
+
+err:
+	for (port = 0; port < TOTAL_NUMBER_OF_PORTS; port++) {
+		if (cvm_oct_device[port]) {
+			struct net_device *dev = cvm_oct_device[port];
+			struct octeon_ethernet *priv = netdev_priv(dev);
+
+			cancel_delayed_work_sync(&priv->port_periodic_work);
+
+			unregister_netdev(dev);
+			free_netdev(dev);
+			cvm_oct_device[port] = NULL;
+		}
+	}
+
+	return ret;
 }
 
 static int cvm_oct_remove(struct platform_device *pdev)