[PATCH net 0/3] ionic: minor code fixes

[PATCH net 0/3] ionic: minor code fixes

[Shannon Nelson]

These are a couple of code fixes for the ionic driver.

Brett Creeley (1):
  ionic: Fix netdev notifier unregister on failure

Shannon Nelson (2):
  ionic: no double destroy workqueue
  ionic: use ee->offset when returning sprom data

 drivers/net/ethernet/pensando/ionic/ionic.h         |  1 -
 drivers/net/ethernet/pensando/ionic/ionic_dev.c     |  5 ++++-
 drivers/net/ethernet/pensando/ionic/ionic_ethtool.c |  4 ++--
 drivers/net/ethernet/pensando/ionic/ionic_lif.c     | 11 ++---------
 4 files changed, 8 insertions(+), 13 deletions(-)

-- 
2.17.1

[PATCH net 1/3] ionic: Fix netdev notifier unregister on failure

[Shannon Nelson]

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 <shannon.nelson@amd.com>
---
 drivers/net/ethernet/pensando/ionic/ionic.h     |  1 -
 drivers/net/ethernet/pensando/ionic/ionic_lif.c | 11 ++---------
 2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h
index 1c61390677f7..faaf96af506d 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic.h
@@ -59,7 +59,6 @@ struct ionic {
 	DECLARE_BITMAP(intrs, IONIC_INTR_CTRL_REGS_MAX);
 	cpumask_var_t *affinity_masks;
 	struct delayed_work doorbell_check_dwork;
-	struct work_struct nb_work;
 	struct notifier_block nb;
 	struct rw_semaphore vf_op_lock;	/* lock for VF operations */
 	struct ionic_vf *vfs;
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index 40496587b2b3..bfa24c659d84 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -3804,10 +3804,6 @@ int ionic_lif_init(struct ionic_lif *lif)
 	return err;
 }
 
-static void ionic_lif_notify_work(struct work_struct *ws)
-{
-}
-
 static void ionic_lif_set_netdev_info(struct ionic_lif *lif)
 {
 	struct ionic_admin_ctx ctx = {
@@ -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);
 		return err;
 	}
 
@@ -3885,7 +3879,6 @@ void ionic_lif_unregister(struct ionic_lif *lif)
 {
 	if (lif->ionic->nb.notifier_call) {
 		unregister_netdevice_notifier(&lif->ionic->nb);
-		cancel_work_sync(&lif->ionic->nb_work);
 		lif->ionic->nb.notifier_call = NULL;
 	}
 
-- 
2.17.1

Re: [PATCH net 1/3] ionic: Fix netdev notifier unregister on failure

[Jacob Keller]

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 <shannon.nelson@amd.com>
> ---

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>


>  drivers/net/ethernet/pensando/ionic/ionic.h     |  1 -
>  drivers/net/ethernet/pensando/ionic/ionic_lif.c | 11 ++---------
>  2 files changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h
> index 1c61390677f7..faaf96af506d 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic.h
> +++ b/drivers/net/ethernet/pensando/ionic/ionic.h
> @@ -59,7 +59,6 @@ struct ionic {
>  	DECLARE_BITMAP(intrs, IONIC_INTR_CTRL_REGS_MAX);
>  	cpumask_var_t *affinity_masks;
>  	struct delayed_work doorbell_check_dwork;
> -	struct work_struct nb_work;
>  	struct notifier_block nb;
>  	struct rw_semaphore vf_op_lock;	/* lock for VF operations */
>  	struct ionic_vf *vfs;
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> index 40496587b2b3..bfa24c659d84 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> @@ -3804,10 +3804,6 @@ int ionic_lif_init(struct ionic_lif *lif)
>  	return err;
>  }
>  
> -static void ionic_lif_notify_work(struct work_struct *ws)
> -{
> -}
> -
>  static void ionic_lif_set_netdev_info(struct ionic_lif *lif)
>  {
>  	struct ionic_admin_ctx ctx = {
> @@ -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);
>  		return err;
>  	}
>  
> @@ -3885,7 +3879,6 @@ void ionic_lif_unregister(struct ionic_lif *lif)
>  {
>  	if (lif->ionic->nb.notifier_call) {
>  		unregister_netdevice_notifier(&lif->ionic->nb);
> -		cancel_work_sync(&lif->ionic->nb_work);
>  		lif->ionic->nb.notifier_call = NULL;
>  	}
>  

Re: [PATCH net 1/3] ionic: Fix netdev notifier unregister on failure

[Jakub Kicinski]

On Tue, 10 Dec 2024 12:59:31 -0800 Jacob Keller wrote:
> I'm not certain about the inclusion of cleanup to drop unused code in
> the same commit as an obvious fix. 

+1, please separate the nb_work removal to a net-next commit
-- 
pw-bot: cr

Re: [PATCH net 1/3] ionic: Fix netdev notifier unregister on failure

[Nelson, Shannon]

On 12/11/2024 8:28 PM, Jakub Kicinski wrote:
> On Tue, 10 Dec 2024 12:59:31 -0800 Jacob Keller wrote:
>> I'm not certain about the inclusion of cleanup to drop unused code in
>> the same commit as an obvious fix.
> 
> +1, please separate the nb_work removal to a net-next commit
> --
> pw-bot: cr


Will do - thanks.
sln

[PATCH net 2/3] ionic: no double destroy workqueue

[Shannon Nelson]

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 <shannon.nelson@amd.com>
---
 drivers/net/ethernet/pensando/ionic/ionic_dev.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.c b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
index 9e42d599840d..57edcde9e6f8 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
@@ -277,7 +277,10 @@ void ionic_dev_teardown(struct ionic *ionic)
 	idev->phy_cmb_pages = 0;
 	idev->cmb_npages = 0;
 
-	destroy_workqueue(ionic->wq);
+	if (ionic->wq) {
+		destroy_workqueue(ionic->wq);
+		ionic->wq = NULL;
+	}
 	mutex_destroy(&idev->cmb_inuse_lock);
 }
 
-- 
2.17.1

Re: [PATCH net 2/3] ionic: no double destroy workqueue

[Jacob Keller]

On 12/10/2024 9:48 AM, Shannon Nelson wrote:
> 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 <shannon.nelson@amd.com>
> ---
>  drivers/net/ethernet/pensando/ionic/ionic_dev.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.c b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
> index 9e42d599840d..57edcde9e6f8 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
> @@ -277,7 +277,10 @@ void ionic_dev_teardown(struct ionic *ionic)
>  	idev->phy_cmb_pages = 0;
>  	idev->cmb_npages = 0;
>  
> -	destroy_workqueue(ionic->wq);
> +	if (ionic->wq) {
> +		destroy_workqueue(ionic->wq);
> +		ionic->wq = NULL;
> +	}

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

>  	mutex_destroy(&idev->cmb_inuse_lock);
>  }
>  

Re: [PATCH net 2/3] ionic: no double destroy workqueue

[Nelson, Shannon]

On 12/10/2024 1:02 PM, Jacob Keller wrote:
> On 12/10/2024 9:48 AM, Shannon Nelson wrote:
>> 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 <shannon.nelson@amd.com>
>> ---
>>   drivers/net/ethernet/pensando/ionic/ionic_dev.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.c b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
>> index 9e42d599840d..57edcde9e6f8 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
>> @@ -277,7 +277,10 @@ void ionic_dev_teardown(struct ionic *ionic)
>>        idev->phy_cmb_pages = 0;
>>        idev->cmb_npages = 0;
>>
>> -     destroy_workqueue(ionic->wq);
>> +     if (ionic->wq) {
>> +             destroy_workqueue(ionic->wq);
>> +             ionic->wq = NULL;
>> +     }
> 
> 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


> 
> Thanks,
> Jake
> 
>>        mutex_destroy(&idev->cmb_inuse_lock);
>>   }
>>
> 

Re: [PATCH net 2/3] ionic: no double destroy workqueue

[Jacob Keller]

On 12/10/2024 1:44 PM, Nelson, Shannon wrote:
> On 12/10/2024 1:02 PM, Jacob Keller wrote:
>> On 12/10/2024 9:48 AM, Shannon Nelson wrote:
>>> 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 <shannon.nelson@amd.com>
>>> ---
>>>   drivers/net/ethernet/pensando/ionic/ionic_dev.c | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.c b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
>>> index 9e42d599840d..57edcde9e6f8 100644
>>> --- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
>>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
>>> @@ -277,7 +277,10 @@ void ionic_dev_teardown(struct ionic *ionic)
>>>        idev->phy_cmb_pages = 0;
>>>        idev->cmb_npages = 0;
>>>
>>> -     destroy_workqueue(ionic->wq);
>>> +     if (ionic->wq) {
>>> +             destroy_workqueue(ionic->wq);
>>> +             ionic->wq = NULL;
>>> +     }
>>
>> 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>

> 
> 
>>
>> Thanks,
>> Jake
>>
>>>        mutex_destroy(&idev->cmb_inuse_lock);
>>>   }
>>>
>>
> 

[PATCH net 3/3] ionic: use ee->offset when returning sprom data

[Shannon Nelson]

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 <shannon.nelson@amd.com>
---
 drivers/net/ethernet/pensando/ionic/ionic_ethtool.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
index dda22fa4448c..9b7f78b6cdb1 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
@@ -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))
-- 
2.17.1

Re: [PATCH net 3/3] ionic: use ee->offset when returning sprom data

[Jacob Keller]

On 12/10/2024 9:48 AM, Shannon Nelson wrote:
> 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 <shannon.nelson@amd.com>
> ---
>  drivers/net/ethernet/pensando/ionic/ionic_ethtool.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> index dda22fa4448c..9b7f78b6cdb1 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> @@ -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))