From: Shannon Nelson <hidden> Date: 2024-12-10 17:48:49
From: Brett Creeley <brett.creeley@amd.com>
If register_netdev() fails, then the driver leaks the netdev notifier.
Fix this by calling ionic_lif_unregister() on register_netdev()
failure. This will also call ionic_lif_unregister_phc() if it has
already been registered.
While at it, remove the empty and unused nb_work and associated
ionic_lif_notify_work() function.
Fixes: 30b87ab4c0b3 ("ionic: remove lif list concept")
Signed-off-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Shannon Nelson <redacted>
---
drivers/net/ethernet/pensando/ionic/ionic.h | 1 -
drivers/net/ethernet/pensando/ionic/ionic_lif.c | 11 ++---------
2 files changed, 2 insertions(+), 10 deletions(-)
@@ -3804,10 +3804,6 @@ int ionic_lif_init(struct ionic_lif *lif)returnerr;}-staticvoidionic_lif_notify_work(structwork_struct*ws)-{-}-staticvoidionic_lif_set_netdev_info(structionic_lif*lif){structionic_admin_ctxctx={
@@ -3858,8 +3854,6 @@ int ionic_lif_register(struct ionic_lif *lif)ionic_lif_register_phc(lif);-INIT_WORK(&lif->ionic->nb_work,ionic_lif_notify_work);-lif->ionic->nb.notifier_call=ionic_lif_notify;err=register_netdevice_notifier(&lif->ionic->nb);
@@ -3869,8 +3863,8 @@ int ionic_lif_register(struct ionic_lif *lif)/* only register LIF0 for now */err=register_netdev(lif->netdev);if(err){-dev_err(lif->ionic->dev,"Cannot register net device, aborting\n");-ionic_lif_unregister_phc(lif);+dev_err(lif->ionic->dev,"Cannot register net device: %d, aborting\n",err);+ionic_lif_unregister(lif);returnerr;}
From: Shannon Nelson <hidden> Date: 2024-12-10 17:48:50
There are some FW error handling paths that can cause us to
try to destroy the workqueue more than once, so let's be sure
we're checking for that.
The case where this popped up was in an AER event where the
handlers got called in such a way that ionic_reset_prepare()
and thus ionic_dev_teardown() got called twice in a row.
The second time through the workqueue was already destroyed,
and destroy_workqueue() choked on the bad wq pointer.
We didn't hit this in AER handler testing before because at
that time we weren't using a private workqueue. Later we
replaced the use of the system workqueue with our own private
workqueue but hadn't rerun the AER handler testing since then.
Fixes: 9e25450da700 ("ionic: add private workqueue per-device")
Signed-off-by: Shannon Nelson <redacted>
---
drivers/net/ethernet/pensando/ionic/ionic_dev.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
From: Shannon Nelson <hidden> Date: 2024-12-10 17:48:50
Some calls into ionic_get_module_eeprom() don't use a single
full buffer size, but instead multiple calls with an offset.
Teach our driver to use the offset correctly so we can
respond appropriately to the caller.
Fixes: 4d03e00a2140 ("ionic: Add initial ethtool support")
Signed-off-by: Shannon Nelson <redacted>
---
drivers/net/ethernet/pensando/ionic/ionic_ethtool.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -961,8 +961,8 @@ static int ionic_get_module_eeprom(struct net_device *netdev,len=min_t(u32,sizeof(xcvr->sprom),ee->len);do{-memcpy(data,xcvr->sprom,len);-memcpy(tbuf,xcvr->sprom,len);+memcpy(data,&xcvr->sprom[ee->offset],len);+memcpy(tbuf,&xcvr->sprom[ee->offset],len);/* Let's make sure we got a consistent copy */if(!memcmp(data,tbuf,len))
From: Jacob Keller <jacob.e.keller@intel.com> Date: 2024-12-10 21:00:10
On 12/10/2024 9:48 AM, Shannon Nelson wrote:
From: Brett Creeley <brett.creeley@amd.com>
If register_netdev() fails, then the driver leaks the netdev notifier.
Fix this by calling ionic_lif_unregister() on register_netdev()
failure. This will also call ionic_lif_unregister_phc() if it has
already been registered.
While at it, remove the empty and unused nb_work and associated
ionic_lif_notify_work() function.
Fixes: 30b87ab4c0b3 ("ionic: remove lif list concept")
Signed-off-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Shannon Nelson <redacted>
---
I'm not certain about the inclusion of cleanup to drop unused code in
the same commit as an obvious fix. However, the changes as a whole seem
ok to me:
With or without splitting:
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
@@ -3804,10 +3804,6 @@ int ionic_lif_init(struct ionic_lif *lif)returnerr;}-staticvoidionic_lif_notify_work(structwork_struct*ws)-{-}-staticvoidionic_lif_set_netdev_info(structionic_lif*lif){structionic_admin_ctxctx={
@@ -3858,8 +3854,6 @@ int ionic_lif_register(struct ionic_lif *lif)ionic_lif_register_phc(lif);-INIT_WORK(&lif->ionic->nb_work,ionic_lif_notify_work);-lif->ionic->nb.notifier_call=ionic_lif_notify;err=register_netdevice_notifier(&lif->ionic->nb);
@@ -3869,8 +3863,8 @@ int ionic_lif_register(struct ionic_lif *lif)/* only register LIF0 for now */err=register_netdev(lif->netdev);if(err){-dev_err(lif->ionic->dev,"Cannot register net device, aborting\n");-ionic_lif_unregister_phc(lif);+dev_err(lif->ionic->dev,"Cannot register net device: %d, aborting\n",err);+ionic_lif_unregister(lif);returnerr;}
From: Jacob Keller <jacob.e.keller@intel.com> Date: 2024-12-10 21:02:20
On 12/10/2024 9:48 AM, Shannon Nelson wrote:
quoted hunk
There are some FW error handling paths that can cause us to
try to destroy the workqueue more than once, so let's be sure
we're checking for that.
The case where this popped up was in an AER event where the
handlers got called in such a way that ionic_reset_prepare()
and thus ionic_dev_teardown() got called twice in a row.
The second time through the workqueue was already destroyed,
and destroy_workqueue() choked on the bad wq pointer.
We didn't hit this in AER handler testing before because at
that time we weren't using a private workqueue. Later we
replaced the use of the system workqueue with our own private
workqueue but hadn't rerun the AER handler testing since then.
Fixes: 9e25450da700 ("ionic: add private workqueue per-device")
Signed-off-by: Shannon Nelson <redacted>
---
drivers/net/ethernet/pensando/ionic/ionic_dev.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
This seems like you still could race if two threads call
ionic_dev_teardown twice. Is that not possible due to some other
synchronization mechanism?
Thanks,
Jake
From: Jacob Keller <jacob.e.keller@intel.com> Date: 2024-12-10 21:03:56
On 12/10/2024 9:48 AM, Shannon Nelson wrote:
quoted hunk
Some calls into ionic_get_module_eeprom() don't use a single
full buffer size, but instead multiple calls with an offset.
Teach our driver to use the offset correctly so we can
respond appropriately to the caller.
Fixes: 4d03e00a2140 ("ionic: Add initial ethtool support")
Signed-off-by: Shannon Nelson <redacted>
---
drivers/net/ethernet/pensando/ionic/ionic_ethtool.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -961,8 +961,8 @@ static int ionic_get_module_eeprom(struct net_device *netdev,len=min_t(u32,sizeof(xcvr->sprom),ee->len);do{-memcpy(data,xcvr->sprom,len);-memcpy(tbuf,xcvr->sprom,len);+memcpy(data,&xcvr->sprom[ee->offset],len);+memcpy(tbuf,&xcvr->sprom[ee->offset],len);
Makes sense. The eeprom API doesn't require reading the entire EEPROM,
and can use offsets. Previously, you copied always from the beginning
which results in failure to copy the correct data out.
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
/* Let's make sure we got a consistent copy */
if (!memcmp(data, tbuf, len))
There are some FW error handling paths that can cause us to
try to destroy the workqueue more than once, so let's be sure
we're checking for that.
The case where this popped up was in an AER event where the
handlers got called in such a way that ionic_reset_prepare()
and thus ionic_dev_teardown() got called twice in a row.
The second time through the workqueue was already destroyed,
and destroy_workqueue() choked on the bad wq pointer.
We didn't hit this in AER handler testing before because at
that time we weren't using a private workqueue. Later we
replaced the use of the system workqueue with our own private
workqueue but hadn't rerun the AER handler testing since then.
Fixes: 9e25450da700 ("ionic: add private workqueue per-device")
Signed-off-by: Shannon Nelson <redacted>
---
drivers/net/ethernet/pensando/ionic/ionic_dev.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
This seems like you still could race if two threads call
ionic_dev_teardown twice. Is that not possible due to some other
synchronization mechanism?
Good question. Thanks for looking at this and the other patches.
This is not a race thing so much as an already-been-here thing. This
function is only called by the probe, remove, and reset_prepare threads,
all driven as PCI calls. I'm reasonably sure that they won't be called
my simultaneous threads, so we just need to be sure that we don't break
if reset_prepare and remove get called one after the other because some
PCI bus element got removed by surprise.
sln
From: Jacob Keller <jacob.e.keller@intel.com> Date: 2024-12-11 19:23:10
On 12/10/2024 1:44 PM, Nelson, Shannon wrote:
On 12/10/2024 1:02 PM, Jacob Keller wrote:
quoted
On 12/10/2024 9:48 AM, Shannon Nelson wrote:
quoted
There are some FW error handling paths that can cause us to
try to destroy the workqueue more than once, so let's be sure
we're checking for that.
The case where this popped up was in an AER event where the
handlers got called in such a way that ionic_reset_prepare()
and thus ionic_dev_teardown() got called twice in a row.
The second time through the workqueue was already destroyed,
and destroy_workqueue() choked on the bad wq pointer.
We didn't hit this in AER handler testing before because at
that time we weren't using a private workqueue. Later we
replaced the use of the system workqueue with our own private
workqueue but hadn't rerun the AER handler testing since then.
Fixes: 9e25450da700 ("ionic: add private workqueue per-device")
Signed-off-by: Shannon Nelson <redacted>
---
drivers/net/ethernet/pensando/ionic/ionic_dev.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
This seems like you still could race if two threads call
ionic_dev_teardown twice. Is that not possible due to some other
synchronization mechanism?
Good question. Thanks for looking at this and the other patches.
This is not a race thing so much as an already-been-here thing. This
function is only called by the probe, remove, and reset_prepare threads,
all driven as PCI calls. I'm reasonably sure that they won't be called
my simultaneous threads, so we just need to be sure that we don't break
if reset_prepare and remove get called one after the other because some
PCI bus element got removed by surprise.
sln
Ok. This is all serialized by the device/PCI layer then?
Makes sense.
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>