[PATCH net 0/2] ionic: fw upgrade bug fixes

[PATCH net 0/2] ionic: fw upgrade bug fixes

[Shannon Nelson]

These patches address a couple of issues found in additional
fw-upgrade testing.

Shannon Nelson (2):
  ionic: add LIF_READY state to close probe-open race
  ionic: refresh devinfo after fw-upgrade

 drivers/net/ethernet/pensando/ionic/ionic_lif.c |  7 +++++++
 drivers/net/ethernet/pensando/ionic/ionic_lif.h | 11 +++++++++++
 2 files changed, 18 insertions(+)

-- 
2.17.1

[PATCH net 1/2] ionic: add LIF_READY state to close probe-open race

[Shannon Nelson]

Add a bit of state to the lif to signify when the queues are
ready to be used.  This closes an ionic_probe()/ionic_open()
race condition where the driver has registered the netdev
and signaled Link Up while running under ionic_probe(),
which NetworkManager or other user processes can see and then
try to bring up the device before the initial pass through
ionic_link_status_check() has finished.  NetworkManager's
thread can get into __dev_open() and set __LINK_STATE_START
in dev->state before the ionic_probe() thread makes it to the
netif_running() check, which results in the ionic_probe() thread
trying to start the queues before the queues have completed
their initialization.

Adding a LIF_QREADY flag allows us to prevent this condition by
signaling whether the Tx/Rx queues are initialized and ready.

Fixes: c672412f6172 ("ionic: remove lifs on fw reset")
Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
 drivers/net/ethernet/pensando/ionic/ionic_lif.c |  6 ++++++
 drivers/net/ethernet/pensando/ionic/ionic_lif.h | 11 +++++++++++
 2 files changed, 17 insertions(+)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index 5acf4f46c268..2dc513f43fd4 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -1627,6 +1627,9 @@ static int ionic_start_queues(struct ionic_lif *lif)
 {
 	int err;
 
+	if (!test_bit(IONIC_LIF_F_QREADY, lif->state))
+		return 0;
+
 	if (test_and_set_bit(IONIC_LIF_F_UP, lif->state))
 		return 0;
 
@@ -1652,6 +1655,7 @@ int ionic_open(struct net_device *netdev)
 	err = ionic_txrx_init(lif);
 	if (err)
 		goto err_out;
+	set_bit(IONIC_LIF_F_QREADY, lif->state);
 
 	/* don't start the queues until we have link */
 	if (netif_carrier_ok(netdev)) {
@@ -1663,6 +1667,7 @@ int ionic_open(struct net_device *netdev)
 	return 0;
 
 err_txrx_deinit:
+	clear_bit(IONIC_LIF_F_QREADY, lif->state);
 	ionic_txrx_deinit(lif);
 err_out:
 	ionic_txrx_free(lif);
@@ -1685,6 +1690,7 @@ int ionic_stop(struct net_device *netdev)
 	if (test_bit(IONIC_LIF_F_FW_RESET, lif->state))
 		return 0;
 
+	clear_bit(IONIC_LIF_F_QREADY, lif->state);
 	ionic_stop_queues(lif);
 	ionic_txrx_deinit(lif);
 	ionic_txrx_free(lif);
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
index 5d4ffda5c05f..a7bf85c17354 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
@@ -121,9 +121,20 @@ struct ionic_lif_sw_stats {
 	u64 rx_csum_error;
 };
 
+/**
+ * enum ionic_lif_state_flags - driver LIF states
+ * @IONIC_LIF_F_INITED:		LIF configured, adminq running
+ * @IONIC_LIF_F_SW_DEBUG_STATS:	Ethtool printing of extra stats is enabled
+ * @IONIC_LIF_F_QREADY:		LIF queues are configured and ready for UP
+ * @IONIC_LIF_F_UP:		LIF is fully UP and running
+ * @IONIC_LIF_F_LINK_CHECK_REQUESTED:	Link check has been requested
+ * @IONIC_LIF_F_QUEUE_RESET:	LIF is resetting all queues
+ * @IONIC_LIF_F_FW_RESET:	FW is going through reset
+ */
 enum ionic_lif_state_flags {
 	IONIC_LIF_F_INITED,
 	IONIC_LIF_F_SW_DEBUG_STATS,
+	IONIC_LIF_F_QREADY,
 	IONIC_LIF_F_UP,
 	IONIC_LIF_F_LINK_CHECK_REQUESTED,
 	IONIC_LIF_F_QUEUE_RESET,
-- 
2.17.1

[PATCH net 2/2] ionic: refresh devinfo after fw-upgrade

[Shannon Nelson]

Make sure we can report the new FW version after a
fw-upgrade has finished by re-reading the device's
fw version information.

Fixes: c672412f6172 ("ionic: remove lifs on fw reset")
Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
 drivers/net/ethernet/pensando/ionic/ionic_lif.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index 2dc513f43fd4..2a87bfc50cc6 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -2122,6 +2122,7 @@ static void ionic_lif_handle_fw_up(struct ionic_lif *lif)
 
 	dev_info(ionic->dev, "FW Up: restarting LIFs\n");
 
+	ionic_init_devinfo(ionic);
 	err = ionic_qcqs_alloc(lif);
 	if (err)
 		goto err_out;
-- 
2.17.1

Re: [PATCH net 1/2] ionic: add LIF_READY state to close probe-open race

[David Miller]

From: Shannon Nelson <snelson@pensando.io>
Date: Wed, 29 Apr 2020 11:37:38 -0700

> Add a bit of state to the lif to signify when the queues are
> ready to be used.  This closes an ionic_probe()/ionic_open()
> race condition where the driver has registered the netdev
> and signaled Link Up while running under ionic_probe(),
> which NetworkManager or other user processes can see and then
> try to bring up the device before the initial pass through
> ionic_link_status_check() has finished.  NetworkManager's
> thread can get into __dev_open() and set __LINK_STATE_START
> in dev->state before the ionic_probe() thread makes it to the
> netif_running() check, which results in the ionic_probe() thread
> trying to start the queues before the queues have completed
> their initialization.
> 
> Adding a LIF_QREADY flag allows us to prevent this condition by
> signaling whether the Tx/Rx queues are initialized and ready.
> 
> Fixes: c672412f6172 ("ionic: remove lifs on fw reset")
> Signed-off-by: Shannon Nelson <snelson@pensando.io>

I would rather you fix this by adjusting the visibility.

You should only call register_netdevice() when all of the device
setup and software state initialization is complete.

This improper ordering is the true cause of this bug and adding
this new boolean state is simply papering over the core issue.

Thanks.

Re: [PATCH net 1/2] ionic: add LIF_READY state to close probe-open race

[Shannon Nelson]

On 4/29/20 12:38 PM, David Miller wrote:
> From: Shannon Nelson <snelson@pensando.io>
> Date: Wed, 29 Apr 2020 11:37:38 -0700
>
>> Add a bit of state to the lif to signify when the queues are
>> ready to be used.  This closes an ionic_probe()/ionic_open()
>> race condition where the driver has registered the netdev
>> and signaled Link Up while running under ionic_probe(),
>> which NetworkManager or other user processes can see and then
>> try to bring up the device before the initial pass through
>> ionic_link_status_check() has finished.  NetworkManager's
>> thread can get into __dev_open() and set __LINK_STATE_START
>> in dev->state before the ionic_probe() thread makes it to the
>> netif_running() check, which results in the ionic_probe() thread
>> trying to start the queues before the queues have completed
>> their initialization.
>>
>> Adding a LIF_QREADY flag allows us to prevent this condition by
>> signaling whether the Tx/Rx queues are initialized and ready.
>>
>> Fixes: c672412f6172 ("ionic: remove lifs on fw reset")
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> I would rather you fix this by adjusting the visibility.
>
> You should only call register_netdevice() when all of the device
> setup and software state initialization is complete.
>
> This improper ordering is the true cause of this bug and adding
> this new boolean state is simply papering over the core issue.
>
> Thanks.

It seems my impatience to see the Link Up message is a problem.  It 
looka as if I push the link check out of the probe thread and let the 
watchdog discover it later, all is much better.  I'll followup with a v2.

sln