[Intel-wired-lan] [PATCH iwl-net 1/2] ice: Fix entering Safe Mode

All of lore.kernel.org
 help / color / mirror / Atom feed

* [Intel-wired-lan] [PATCH iwl-net 1/2] ice: Fix entering Safe Mode
@ 2024-09-20 11:55 ` Marcin Szycik
  0 siblings, 0 replies; 10+ messages in thread
From: Marcin Szycik @ 2024-09-20 11:55 UTC (permalink / raw)
  To: intel-wired-lan
  Cc: netdev, Marcin Szycik, Przemek Kitszel, mateusz.polchlopek

If DDP package is missing or corrupted, the driver should enter Safe Mode.
Instead, an error is returned and probe fails.

Don't check return value of ice_init_ddp_config() to fix this.

Repro:
* Remove or rename DDP package (/lib/firmware/intel/ice/ddp/ice.pkg)
* Load ice

Fixes: cc5776fe1832 ("ice: Enable switching default Tx scheduler topology")
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 0f5c9d347806..7b6725d652e1 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4748,9 +4748,7 @@ int ice_init_dev(struct ice_pf *pf)
 
 	ice_init_feature_support(pf);
 
-	err = ice_init_ddp_config(hw, pf);
-	if (err)
-		return err;
+	ice_init_ddp_config(hw, pf);
 
 	/* if ice_init_ddp_config fails, ICE_FLAG_ADV_FEATURES bit won't be
 	 * set in pf->state, which will cause ice_is_safe_mode to return
-- 
2.45.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH iwl-net 1/2] ice: Fix entering Safe Mode
@ 2024-09-20 11:55 ` Marcin Szycik
  0 siblings, 0 replies; 10+ messages in thread
From: Marcin Szycik @ 2024-09-20 11:55 UTC (permalink / raw)
  To: intel-wired-lan
  Cc: netdev, mateusz.polchlopek, Marcin Szycik, Przemek Kitszel

If DDP package is missing or corrupted, the driver should enter Safe Mode.
Instead, an error is returned and probe fails.

Don't check return value of ice_init_ddp_config() to fix this.

Repro:
* Remove or rename DDP package (/lib/firmware/intel/ice/ddp/ice.pkg)
* Load ice

Fixes: cc5776fe1832 ("ice: Enable switching default Tx scheduler topology")
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 0f5c9d347806..7b6725d652e1 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4748,9 +4748,7 @@ int ice_init_dev(struct ice_pf *pf)
 
 	ice_init_feature_support(pf);
 
-	err = ice_init_ddp_config(hw, pf);
-	if (err)
-		return err;
+	ice_init_ddp_config(hw, pf);
 
 	/* if ice_init_ddp_config fails, ICE_FLAG_ADV_FEATURES bit won't be
 	 * set in pf->state, which will cause ice_is_safe_mode to return
-- 
2.45.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [Intel-wired-lan] [PATCH iwl-net 2/2] ice: Fix netif_is_ice() in Safe Mode
  2024-09-20 11:55 ` Marcin Szycik
@ 2024-09-20 11:55   ` Marcin Szycik
  -1 siblings, 0 replies; 10+ messages in thread
From: Marcin Szycik @ 2024-09-20 11:55 UTC (permalink / raw)
  To: intel-wired-lan
  Cc: netdev, Marcin Szycik, Przemek Kitszel, mateusz.polchlopek

netif_is_ice() works by checking the pointer to netdev ops. However, it
only checks for the default ice_netdev_ops, not ice_netdev_safe_mode_ops,
so in Safe Mode it always returns false, which is unintuitive. While it
doesn't look like netif_is_ice() is currently being called anywhere in Safe
Mode, this could change and potentially lead to unexpected behaviour.

Fixes: df006dd4b1dc ("ice: Add initial support framework for LAG")
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 7b6725d652e1..b58ef3e69f9c 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -87,7 +87,8 @@ ice_indr_setup_tc_cb(struct net_device *netdev, struct Qdisc *sch,
 
 bool netif_is_ice(const struct net_device *dev)
 {
-	return dev && (dev->netdev_ops == &ice_netdev_ops);
+	return dev && (dev->netdev_ops == &ice_netdev_ops ||
+		       dev->netdev_ops == &ice_netdev_safe_mode_ops);
 }
 
 /**
-- 
2.45.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH iwl-net 2/2] ice: Fix netif_is_ice() in Safe Mode
@ 2024-09-20 11:55   ` Marcin Szycik
  0 siblings, 0 replies; 10+ messages in thread
From: Marcin Szycik @ 2024-09-20 11:55 UTC (permalink / raw)
  To: intel-wired-lan
  Cc: netdev, mateusz.polchlopek, Marcin Szycik, Przemek Kitszel

netif_is_ice() works by checking the pointer to netdev ops. However, it
only checks for the default ice_netdev_ops, not ice_netdev_safe_mode_ops,
so in Safe Mode it always returns false, which is unintuitive. While it
doesn't look like netif_is_ice() is currently being called anywhere in Safe
Mode, this could change and potentially lead to unexpected behaviour.

Fixes: df006dd4b1dc ("ice: Add initial support framework for LAG")
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 7b6725d652e1..b58ef3e69f9c 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -87,7 +87,8 @@ ice_indr_setup_tc_cb(struct net_device *netdev, struct Qdisc *sch,
 
 bool netif_is_ice(const struct net_device *dev)
 {
-	return dev && (dev->netdev_ops == &ice_netdev_ops);
+	return dev && (dev->netdev_ops == &ice_netdev_ops ||
+		       dev->netdev_ops == &ice_netdev_safe_mode_ops);
 }
 
 /**
-- 
2.45.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [Intel-wired-lan] [PATCH iwl-net 1/2] ice: Fix entering Safe Mode
  2024-09-20 11:55 ` Marcin Szycik
@ 2024-09-20 12:03   ` Maciej Fijalkowski
  -1 siblings, 0 replies; 10+ messages in thread
From: Maciej Fijalkowski @ 2024-09-20 12:03 UTC (permalink / raw)
  To: Marcin Szycik
  Cc: netdev, intel-wired-lan, Przemek Kitszel, mateusz.polchlopek

On Fri, Sep 20, 2024 at 01:55:09PM +0200, Marcin Szycik wrote:
> If DDP package is missing or corrupted, the driver should enter Safe Mode.
> Instead, an error is returned and probe fails.
> 
> Don't check return value of ice_init_ddp_config() to fix this.

no one else checks the retval after your fix so adjust it to return void.

> 
> Repro:
> * Remove or rename DDP package (/lib/firmware/intel/ice/ddp/ice.pkg)
> * Load ice
> 
> Fixes: cc5776fe1832 ("ice: Enable switching default Tx scheduler topology")
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_main.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index 0f5c9d347806..7b6725d652e1 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -4748,9 +4748,7 @@ int ice_init_dev(struct ice_pf *pf)
>  
>  	ice_init_feature_support(pf);
>  
> -	err = ice_init_ddp_config(hw, pf);
> -	if (err)
> -		return err;
> +	ice_init_ddp_config(hw, pf);
>  
>  	/* if ice_init_ddp_config fails, ICE_FLAG_ADV_FEATURES bit won't be
>  	 * set in pf->state, which will cause ice_is_safe_mode to return
> -- 
> 2.45.0
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH iwl-net 1/2] ice: Fix entering Safe Mode
@ 2024-09-20 12:03   ` Maciej Fijalkowski
  0 siblings, 0 replies; 10+ messages in thread
From: Maciej Fijalkowski @ 2024-09-20 12:03 UTC (permalink / raw)
  To: Marcin Szycik
  Cc: intel-wired-lan, netdev, mateusz.polchlopek, Przemek Kitszel

On Fri, Sep 20, 2024 at 01:55:09PM +0200, Marcin Szycik wrote:
> If DDP package is missing or corrupted, the driver should enter Safe Mode.
> Instead, an error is returned and probe fails.
> 
> Don't check return value of ice_init_ddp_config() to fix this.

no one else checks the retval after your fix so adjust it to return void.

> 
> Repro:
> * Remove or rename DDP package (/lib/firmware/intel/ice/ddp/ice.pkg)
> * Load ice
> 
> Fixes: cc5776fe1832 ("ice: Enable switching default Tx scheduler topology")
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_main.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index 0f5c9d347806..7b6725d652e1 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -4748,9 +4748,7 @@ int ice_init_dev(struct ice_pf *pf)
>  
>  	ice_init_feature_support(pf);
>  
> -	err = ice_init_ddp_config(hw, pf);
> -	if (err)
> -		return err;
> +	ice_init_ddp_config(hw, pf);
>  
>  	/* if ice_init_ddp_config fails, ICE_FLAG_ADV_FEATURES bit won't be
>  	 * set in pf->state, which will cause ice_is_safe_mode to return
> -- 
> 2.45.0
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Intel-wired-lan] [PATCH iwl-net 1/2] ice: Fix entering Safe Mode
  2024-09-20 12:03   ` Maciej Fijalkowski
@ 2024-09-20 13:24     ` Marcin Szycik
  -1 siblings, 0 replies; 10+ messages in thread
From: Marcin Szycik @ 2024-09-20 13:24 UTC (permalink / raw)
  To: Maciej Fijalkowski
  Cc: netdev, mateusz.polchlopek, intel-wired-lan, Przemek Kitszel



On 20.09.2024 14:03, Maciej Fijalkowski wrote:
> On Fri, Sep 20, 2024 at 01:55:09PM +0200, Marcin Szycik wrote:
>> If DDP package is missing or corrupted, the driver should enter Safe Mode.
>> Instead, an error is returned and probe fails.
>>
>> Don't check return value of ice_init_ddp_config() to fix this.
> 
> no one else checks the retval after your fix so adjust it to return void.

Thanks, will fix in v2.

>>
>> Repro:
>> * Remove or rename DDP package (/lib/firmware/intel/ice/ddp/ice.pkg)
>> * Load ice
>>
>> Fixes: cc5776fe1832 ("ice: Enable switching default Tx scheduler topology")
>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
>> ---
>>  drivers/net/ethernet/intel/ice/ice_main.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>> index 0f5c9d347806..7b6725d652e1 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> @@ -4748,9 +4748,7 @@ int ice_init_dev(struct ice_pf *pf)
>>  
>>  	ice_init_feature_support(pf);
>>  
>> -	err = ice_init_ddp_config(hw, pf);
>> -	if (err)
>> -		return err;
>> +	ice_init_ddp_config(hw, pf);
>>  
>>  	/* if ice_init_ddp_config fails, ICE_FLAG_ADV_FEATURES bit won't be
>>  	 * set in pf->state, which will cause ice_is_safe_mode to return
>> -- 
>> 2.45.0
>>
>>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Intel-wired-lan] [PATCH iwl-net 1/2] ice: Fix entering Safe Mode
@ 2024-09-20 13:24     ` Marcin Szycik
  0 siblings, 0 replies; 10+ messages in thread
From: Marcin Szycik @ 2024-09-20 13:24 UTC (permalink / raw)
  To: Maciej Fijalkowski
  Cc: netdev, intel-wired-lan, Przemek Kitszel, mateusz.polchlopek



On 20.09.2024 14:03, Maciej Fijalkowski wrote:
> On Fri, Sep 20, 2024 at 01:55:09PM +0200, Marcin Szycik wrote:
>> If DDP package is missing or corrupted, the driver should enter Safe Mode.
>> Instead, an error is returned and probe fails.
>>
>> Don't check return value of ice_init_ddp_config() to fix this.
> 
> no one else checks the retval after your fix so adjust it to return void.

Thanks, will fix in v2.

>>
>> Repro:
>> * Remove or rename DDP package (/lib/firmware/intel/ice/ddp/ice.pkg)
>> * Load ice
>>
>> Fixes: cc5776fe1832 ("ice: Enable switching default Tx scheduler topology")
>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
>> ---
>>  drivers/net/ethernet/intel/ice/ice_main.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>> index 0f5c9d347806..7b6725d652e1 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> @@ -4748,9 +4748,7 @@ int ice_init_dev(struct ice_pf *pf)
>>  
>>  	ice_init_feature_support(pf);
>>  
>> -	err = ice_init_ddp_config(hw, pf);
>> -	if (err)
>> -		return err;
>> +	ice_init_ddp_config(hw, pf);
>>  
>>  	/* if ice_init_ddp_config fails, ICE_FLAG_ADV_FEATURES bit won't be
>>  	 * set in pf->state, which will cause ice_is_safe_mode to return
>> -- 
>> 2.45.0
>>
>>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Intel-wired-lan] [PATCH iwl-net 1/2] ice: Fix entering Safe Mode
  2024-09-20 12:03   ` Maciej Fijalkowski
@ 2024-09-20 17:07     ` Brett Creeley
  -1 siblings, 0 replies; 10+ messages in thread
From: Brett Creeley @ 2024-09-20 17:07 UTC (permalink / raw)
  To: Maciej Fijalkowski, Marcin Szycik
  Cc: netdev, intel-wired-lan, Przemek Kitszel, mateusz.polchlopek



On 9/20/2024 5:03 AM, Maciej Fijalkowski wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Fri, Sep 20, 2024 at 01:55:09PM +0200, Marcin Szycik wrote:
>> If DDP package is missing or corrupted, the driver should enter Safe Mode.
>> Instead, an error is returned and probe fails.
>>
>> Don't check return value of ice_init_ddp_config() to fix this.
> 
> no one else checks the retval after your fix so adjust it to return void.
> 
>>
>> Repro:
>> * Remove or rename DDP package (/lib/firmware/intel/ice/ddp/ice.pkg)
>> * Load ice
>>
>> Fixes: cc5776fe1832 ("ice: Enable switching default Tx scheduler topology")
>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/ice_main.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>> index 0f5c9d347806..7b6725d652e1 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> @@ -4748,9 +4748,7 @@ int ice_init_dev(struct ice_pf *pf)
>>
>>        ice_init_feature_support(pf);
>>
>> -     err = ice_init_ddp_config(hw, pf);
>> -     if (err)
>> -             return err;
>> +     ice_init_ddp_config(hw, pf);

As an alternative solution you could potentially do the following, which 
would make the flow more readable:

err = ice_init_ddp_config(hw, pf);
if (err || ice_is_safe_mode(pf))
	ice_set_safe_mode_caps(hw);

Also, should there be some sort of messaging if the device goes into 
safe mode? I wonder if a dev_dbg() would be better than nothing.

>>
>>        /* if ice_init_ddp_config fails, ICE_FLAG_ADV_FEATURES bit won't be
>>         * set in pf->state, which will cause ice_is_safe_mode to return >> --
>> 2.45.0
>>
>>
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH iwl-net 1/2] ice: Fix entering Safe Mode
@ 2024-09-20 17:07     ` Brett Creeley
  0 siblings, 0 replies; 10+ messages in thread
From: Brett Creeley @ 2024-09-20 17:07 UTC (permalink / raw)
  To: Maciej Fijalkowski, Marcin Szycik
  Cc: intel-wired-lan, netdev, mateusz.polchlopek, Przemek Kitszel



On 9/20/2024 5:03 AM, Maciej Fijalkowski wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Fri, Sep 20, 2024 at 01:55:09PM +0200, Marcin Szycik wrote:
>> If DDP package is missing or corrupted, the driver should enter Safe Mode.
>> Instead, an error is returned and probe fails.
>>
>> Don't check return value of ice_init_ddp_config() to fix this.
> 
> no one else checks the retval after your fix so adjust it to return void.
> 
>>
>> Repro:
>> * Remove or rename DDP package (/lib/firmware/intel/ice/ddp/ice.pkg)
>> * Load ice
>>
>> Fixes: cc5776fe1832 ("ice: Enable switching default Tx scheduler topology")
>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/ice_main.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>> index 0f5c9d347806..7b6725d652e1 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> @@ -4748,9 +4748,7 @@ int ice_init_dev(struct ice_pf *pf)
>>
>>        ice_init_feature_support(pf);
>>
>> -     err = ice_init_ddp_config(hw, pf);
>> -     if (err)
>> -             return err;
>> +     ice_init_ddp_config(hw, pf);

As an alternative solution you could potentially do the following, which 
would make the flow more readable:

err = ice_init_ddp_config(hw, pf);
if (err || ice_is_safe_mode(pf))
	ice_set_safe_mode_caps(hw);

Also, should there be some sort of messaging if the device goes into 
safe mode? I wonder if a dev_dbg() would be better than nothing.

>>
>>        /* if ice_init_ddp_config fails, ICE_FLAG_ADV_FEATURES bit won't be
>>         * set in pf->state, which will cause ice_is_safe_mode to return >> --
>> 2.45.0
>>
>>
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-09-20 17:08 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-20 11:55 [Intel-wired-lan] [PATCH iwl-net 1/2] ice: Fix entering Safe Mode Marcin Szycik
2024-09-20 11:55 ` Marcin Szycik
2024-09-20 11:55 ` [Intel-wired-lan] [PATCH iwl-net 2/2] ice: Fix netif_is_ice() in " Marcin Szycik
2024-09-20 11:55   ` Marcin Szycik
2024-09-20 12:03 ` [Intel-wired-lan] [PATCH iwl-net 1/2] ice: Fix entering " Maciej Fijalkowski
2024-09-20 12:03   ` Maciej Fijalkowski
2024-09-20 13:24   ` [Intel-wired-lan] " Marcin Szycik
2024-09-20 13:24     ` Marcin Szycik
2024-09-20 17:07   ` Brett Creeley
2024-09-20 17:07     ` Brett Creeley

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.