* [Intel-wired-lan] [PATCH iwl-net] ice: fix PTP cleanup on driver removal in error path
@ 2025-10-08 11:58 ` Grzegorz Nitka
0 siblings, 0 replies; 6+ messages in thread
From: Grzegorz Nitka @ 2025-10-08 11:58 UTC (permalink / raw)
To: intel-wired-lan; +Cc: Aleksandr Loktionov, netdev, Przemek Kitszel
Improve PTP feature cleanup in error path by adding explicit call to
ice_ptp_cleanup_pf in the case in which PTP feature is not fully
operational at the time of driver removal (which is indicated by
ptp->state flag).
At the driver probe, if PTP feature is supported, each PF adds its own
port to the list of ports controlled by ice_adapter object.
Analogously, at the driver remove, it's expected each PF is
responsible for removing previously added port from the list.
If for some reason (like errors in reset handling, NVM update etc.), PTP
feature has not rebuilt successfully, the driver is still responsible for
proper clearing ice_adapter port list. It's done by calling
ice_ptp_cleanup_pf function.
Otherwise, the following call trace is observed when ice_adapter object
is freed (port list is not empty, as it is expected at this stage):
[ T93022] ------------[ cut here ]------------
[ T93022] WARNING: CPU: 10 PID: 93022 at
ice/ice_adapter.c:67 ice_adapter_put+0xef/0x100 [ice]
...
[ T93022] RIP: 0010:ice_adapter_put+0xef/0x100 [ice]
...
[ T93022] Call Trace:
[ T93022] <TASK>
[ T93022] ? ice_adapter_put+0xef/0x100 [ice
33d2647ad4f6d866d41eefff1806df37c68aef0c]
[ T93022] ? __warn.cold+0xb0/0x10e
[ T93022] ? ice_adapter_put+0xef/0x100 [ice
33d2647ad4f6d866d41eefff1806df37c68aef0c]
[ T93022] ? report_bug+0xd8/0x150
[ T93022] ? handle_bug+0xe9/0x110
[ T93022] ? exc_invalid_op+0x17/0x70
[ T93022] ? asm_exc_invalid_op+0x1a/0x20
[ T93022] ? ice_adapter_put+0xef/0x100 [ice
33d2647ad4f6d866d41eefff1806df37c68aef0c]
[ T93022] pci_device_remove+0x42/0xb0
[ T93022] device_release_driver_internal+0x19f/0x200
[ T93022] driver_detach+0x48/0x90
[ T93022] bus_remove_driver+0x70/0xf0
[ T93022] pci_unregister_driver+0x42/0xb0
[ T93022] ice_module_exit+0x10/0xdb0 [ice
33d2647ad4f6d866d41eefff1806df37c68aef0c]
...
[ T93022] ---[ end trace 0000000000000000 ]---
[ T93022] ice: module unloaded
Fixes: e800654e85b5 ("ice: Use ice_adapter for PTP shared data instead of auxdev")
Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
drivers/net/ethernet/intel/ice/ice_ptp.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index fb0f6365a6d6..c43a7973d70f 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -3282,8 +3282,10 @@ void ice_ptp_init(struct ice_pf *pf)
*/
void ice_ptp_release(struct ice_pf *pf)
{
- if (pf->ptp.state != ICE_PTP_READY)
+ if (pf->ptp.state != ICE_PTP_READY) {
+ ice_ptp_cleanup_pf(pf);
return;
+ }
pf->ptp.state = ICE_PTP_UNINIT;
base-commit: 8b223715f39c8a944abff2831c47d5509fdb6e57
--
2.39.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH iwl-net] ice: fix PTP cleanup on driver removal in error path
@ 2025-10-08 11:58 ` Grzegorz Nitka
0 siblings, 0 replies; 6+ messages in thread
From: Grzegorz Nitka @ 2025-10-08 11:58 UTC (permalink / raw)
To: intel-wired-lan
Cc: netdev, Grzegorz Nitka, Aleksandr Loktionov, Przemek Kitszel
Improve PTP feature cleanup in error path by adding explicit call to
ice_ptp_cleanup_pf in the case in which PTP feature is not fully
operational at the time of driver removal (which is indicated by
ptp->state flag).
At the driver probe, if PTP feature is supported, each PF adds its own
port to the list of ports controlled by ice_adapter object.
Analogously, at the driver remove, it's expected each PF is
responsible for removing previously added port from the list.
If for some reason (like errors in reset handling, NVM update etc.), PTP
feature has not rebuilt successfully, the driver is still responsible for
proper clearing ice_adapter port list. It's done by calling
ice_ptp_cleanup_pf function.
Otherwise, the following call trace is observed when ice_adapter object
is freed (port list is not empty, as it is expected at this stage):
[ T93022] ------------[ cut here ]------------
[ T93022] WARNING: CPU: 10 PID: 93022 at
ice/ice_adapter.c:67 ice_adapter_put+0xef/0x100 [ice]
...
[ T93022] RIP: 0010:ice_adapter_put+0xef/0x100 [ice]
...
[ T93022] Call Trace:
[ T93022] <TASK>
[ T93022] ? ice_adapter_put+0xef/0x100 [ice
33d2647ad4f6d866d41eefff1806df37c68aef0c]
[ T93022] ? __warn.cold+0xb0/0x10e
[ T93022] ? ice_adapter_put+0xef/0x100 [ice
33d2647ad4f6d866d41eefff1806df37c68aef0c]
[ T93022] ? report_bug+0xd8/0x150
[ T93022] ? handle_bug+0xe9/0x110
[ T93022] ? exc_invalid_op+0x17/0x70
[ T93022] ? asm_exc_invalid_op+0x1a/0x20
[ T93022] ? ice_adapter_put+0xef/0x100 [ice
33d2647ad4f6d866d41eefff1806df37c68aef0c]
[ T93022] pci_device_remove+0x42/0xb0
[ T93022] device_release_driver_internal+0x19f/0x200
[ T93022] driver_detach+0x48/0x90
[ T93022] bus_remove_driver+0x70/0xf0
[ T93022] pci_unregister_driver+0x42/0xb0
[ T93022] ice_module_exit+0x10/0xdb0 [ice
33d2647ad4f6d866d41eefff1806df37c68aef0c]
...
[ T93022] ---[ end trace 0000000000000000 ]---
[ T93022] ice: module unloaded
Fixes: e800654e85b5 ("ice: Use ice_adapter for PTP shared data instead of auxdev")
Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
drivers/net/ethernet/intel/ice/ice_ptp.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index fb0f6365a6d6..c43a7973d70f 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -3282,8 +3282,10 @@ void ice_ptp_init(struct ice_pf *pf)
*/
void ice_ptp_release(struct ice_pf *pf)
{
- if (pf->ptp.state != ICE_PTP_READY)
+ if (pf->ptp.state != ICE_PTP_READY) {
+ ice_ptp_cleanup_pf(pf);
return;
+ }
pf->ptp.state = ICE_PTP_UNINIT;
base-commit: 8b223715f39c8a944abff2831c47d5509fdb6e57
--
2.39.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] ice: fix PTP cleanup on driver removal in error path
2025-10-08 11:58 ` Grzegorz Nitka
@ 2025-10-08 13:38 ` Vadim Fedorenko
-1 siblings, 0 replies; 6+ messages in thread
From: Vadim Fedorenko @ 2025-10-08 13:38 UTC (permalink / raw)
To: Grzegorz Nitka, intel-wired-lan
Cc: netdev, Aleksandr Loktionov, Przemek Kitszel
On 08/10/2025 12:58, Grzegorz Nitka wrote:
> Improve PTP feature cleanup in error path by adding explicit call to
> ice_ptp_cleanup_pf in the case in which PTP feature is not fully
> operational at the time of driver removal (which is indicated by
> ptp->state flag).
> At the driver probe, if PTP feature is supported, each PF adds its own
> port to the list of ports controlled by ice_adapter object.
> Analogously, at the driver remove, it's expected each PF is
> responsible for removing previously added port from the list.
> If for some reason (like errors in reset handling, NVM update etc.), PTP
> feature has not rebuilt successfully, the driver is still responsible for
> proper clearing ice_adapter port list. It's done by calling
> ice_ptp_cleanup_pf function.
> Otherwise, the following call trace is observed when ice_adapter object
> is freed (port list is not empty, as it is expected at this stage):
>
> [ T93022] ------------[ cut here ]------------
> [ T93022] WARNING: CPU: 10 PID: 93022 at
> ice/ice_adapter.c:67 ice_adapter_put+0xef/0x100 [ice]
> ...
> [ T93022] RIP: 0010:ice_adapter_put+0xef/0x100 [ice]
> ...
> [ T93022] Call Trace:
> [ T93022] <TASK>
> [ T93022] ? ice_adapter_put+0xef/0x100 [ice
> 33d2647ad4f6d866d41eefff1806df37c68aef0c]
> [ T93022] ? __warn.cold+0xb0/0x10e
> [ T93022] ? ice_adapter_put+0xef/0x100 [ice
> 33d2647ad4f6d866d41eefff1806df37c68aef0c]
> [ T93022] ? report_bug+0xd8/0x150
> [ T93022] ? handle_bug+0xe9/0x110
> [ T93022] ? exc_invalid_op+0x17/0x70
> [ T93022] ? asm_exc_invalid_op+0x1a/0x20
> [ T93022] ? ice_adapter_put+0xef/0x100 [ice
> 33d2647ad4f6d866d41eefff1806df37c68aef0c]
> [ T93022] pci_device_remove+0x42/0xb0
> [ T93022] device_release_driver_internal+0x19f/0x200
> [ T93022] driver_detach+0x48/0x90
> [ T93022] bus_remove_driver+0x70/0xf0
> [ T93022] pci_unregister_driver+0x42/0xb0
> [ T93022] ice_module_exit+0x10/0xdb0 [ice
> 33d2647ad4f6d866d41eefff1806df37c68aef0c]
> ...
> [ T93022] ---[ end trace 0000000000000000 ]---
> [ T93022] ice: module unloaded
>
> Fixes: e800654e85b5 ("ice: Use ice_adapter for PTP shared data instead of auxdev")
> Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
> drivers/net/ethernet/intel/ice/ice_ptp.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index fb0f6365a6d6..c43a7973d70f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -3282,8 +3282,10 @@ void ice_ptp_init(struct ice_pf *pf)
> */
> void ice_ptp_release(struct ice_pf *pf)
> {
> - if (pf->ptp.state != ICE_PTP_READY)
> + if (pf->ptp.state != ICE_PTP_READY) {
> + ice_ptp_cleanup_pf(pf);
> return;
> + }
>
> pf->ptp.state = ICE_PTP_UNINIT;
>
ice_ptp_cleanup_pf() removes ptp->port.list_node, which is inited in
ice_ptp_setup_pf(), but ice_ptp_init() may fail before
ice_ptp_setup_pf() is called, and it will keep pf->ptp.state =
ICE_PTP_ERROR. the cleanup then will work on uninitialized data.
It looks like it's better to make proper clean up in ice_ptp_setup_pf()
on error path rather then modify ice_ptp_cleanup_pf().
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH iwl-net] ice: fix PTP cleanup on driver removal in error path
@ 2025-10-08 13:38 ` Vadim Fedorenko
0 siblings, 0 replies; 6+ messages in thread
From: Vadim Fedorenko @ 2025-10-08 13:38 UTC (permalink / raw)
To: Grzegorz Nitka, intel-wired-lan
Cc: netdev, Aleksandr Loktionov, Przemek Kitszel
On 08/10/2025 12:58, Grzegorz Nitka wrote:
> Improve PTP feature cleanup in error path by adding explicit call to
> ice_ptp_cleanup_pf in the case in which PTP feature is not fully
> operational at the time of driver removal (which is indicated by
> ptp->state flag).
> At the driver probe, if PTP feature is supported, each PF adds its own
> port to the list of ports controlled by ice_adapter object.
> Analogously, at the driver remove, it's expected each PF is
> responsible for removing previously added port from the list.
> If for some reason (like errors in reset handling, NVM update etc.), PTP
> feature has not rebuilt successfully, the driver is still responsible for
> proper clearing ice_adapter port list. It's done by calling
> ice_ptp_cleanup_pf function.
> Otherwise, the following call trace is observed when ice_adapter object
> is freed (port list is not empty, as it is expected at this stage):
>
> [ T93022] ------------[ cut here ]------------
> [ T93022] WARNING: CPU: 10 PID: 93022 at
> ice/ice_adapter.c:67 ice_adapter_put+0xef/0x100 [ice]
> ...
> [ T93022] RIP: 0010:ice_adapter_put+0xef/0x100 [ice]
> ...
> [ T93022] Call Trace:
> [ T93022] <TASK>
> [ T93022] ? ice_adapter_put+0xef/0x100 [ice
> 33d2647ad4f6d866d41eefff1806df37c68aef0c]
> [ T93022] ? __warn.cold+0xb0/0x10e
> [ T93022] ? ice_adapter_put+0xef/0x100 [ice
> 33d2647ad4f6d866d41eefff1806df37c68aef0c]
> [ T93022] ? report_bug+0xd8/0x150
> [ T93022] ? handle_bug+0xe9/0x110
> [ T93022] ? exc_invalid_op+0x17/0x70
> [ T93022] ? asm_exc_invalid_op+0x1a/0x20
> [ T93022] ? ice_adapter_put+0xef/0x100 [ice
> 33d2647ad4f6d866d41eefff1806df37c68aef0c]
> [ T93022] pci_device_remove+0x42/0xb0
> [ T93022] device_release_driver_internal+0x19f/0x200
> [ T93022] driver_detach+0x48/0x90
> [ T93022] bus_remove_driver+0x70/0xf0
> [ T93022] pci_unregister_driver+0x42/0xb0
> [ T93022] ice_module_exit+0x10/0xdb0 [ice
> 33d2647ad4f6d866d41eefff1806df37c68aef0c]
> ...
> [ T93022] ---[ end trace 0000000000000000 ]---
> [ T93022] ice: module unloaded
>
> Fixes: e800654e85b5 ("ice: Use ice_adapter for PTP shared data instead of auxdev")
> Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
> drivers/net/ethernet/intel/ice/ice_ptp.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index fb0f6365a6d6..c43a7973d70f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -3282,8 +3282,10 @@ void ice_ptp_init(struct ice_pf *pf)
> */
> void ice_ptp_release(struct ice_pf *pf)
> {
> - if (pf->ptp.state != ICE_PTP_READY)
> + if (pf->ptp.state != ICE_PTP_READY) {
> + ice_ptp_cleanup_pf(pf);
> return;
> + }
>
> pf->ptp.state = ICE_PTP_UNINIT;
>
ice_ptp_cleanup_pf() removes ptp->port.list_node, which is inited in
ice_ptp_setup_pf(), but ice_ptp_init() may fail before
ice_ptp_setup_pf() is called, and it will keep pf->ptp.state =
ICE_PTP_ERROR. the cleanup then will work on uninitialized data.
It looks like it's better to make proper clean up in ice_ptp_setup_pf()
on error path rather then modify ice_ptp_cleanup_pf().
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net] ice: fix PTP cleanup on driver removal in error path
2025-10-08 13:38 ` Vadim Fedorenko
@ 2025-10-08 15:21 ` Nitka, Grzegorz
-1 siblings, 0 replies; 6+ messages in thread
From: Nitka, Grzegorz @ 2025-10-08 15:21 UTC (permalink / raw)
To: Vadim Fedorenko, intel-wired-lan@lists.osuosl.org
Cc: netdev@vger.kernel.org, Loktionov, Aleksandr, Kitszel, Przemyslaw
> -----Original Message-----
> From: Vadim Fedorenko <vadim.fedorenko@linux.dev>
> Sent: Wednesday, October 8, 2025 3:39 PM
> To: Nitka, Grzegorz <grzegorz.nitka@intel.com>; intel-wired-
> lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>
> Subject: Re: [PATCH iwl-net] ice: fix PTP cleanup on driver removal in error
> path
>
> On 08/10/2025 12:58, Grzegorz Nitka wrote:
> > Improve PTP feature cleanup in error path by adding explicit call to
> > ice_ptp_cleanup_pf in the case in which PTP feature is not fully
> > operational at the time of driver removal (which is indicated by
> > ptp->state flag).
> > At the driver probe, if PTP feature is supported, each PF adds its own
> > port to the list of ports controlled by ice_adapter object.
> > Analogously, at the driver remove, it's expected each PF is
> > responsible for removing previously added port from the list.
> > If for some reason (like errors in reset handling, NVM update etc.), PTP
> > feature has not rebuilt successfully, the driver is still responsible for
> > proper clearing ice_adapter port list. It's done by calling
> > ice_ptp_cleanup_pf function.
> > Otherwise, the following call trace is observed when ice_adapter object
> > is freed (port list is not empty, as it is expected at this stage):
> >
> > [ T93022] ------------[ cut here ]------------
> > [ T93022] WARNING: CPU: 10 PID: 93022 at
> > ice/ice_adapter.c:67 ice_adapter_put+0xef/0x100 [ice]
> > ...
> > [ T93022] RIP: 0010:ice_adapter_put+0xef/0x100 [ice]
> > ...
> > [ T93022] Call Trace:
> > [ T93022] <TASK>
> > [ T93022] ? ice_adapter_put+0xef/0x100 [ice
> > 33d2647ad4f6d866d41eefff1806df37c68aef0c]
> > [ T93022] ? __warn.cold+0xb0/0x10e
> > [ T93022] ? ice_adapter_put+0xef/0x100 [ice
> > 33d2647ad4f6d866d41eefff1806df37c68aef0c]
> > [ T93022] ? report_bug+0xd8/0x150
> > [ T93022] ? handle_bug+0xe9/0x110
> > [ T93022] ? exc_invalid_op+0x17/0x70
> > [ T93022] ? asm_exc_invalid_op+0x1a/0x20
> > [ T93022] ? ice_adapter_put+0xef/0x100 [ice
> > 33d2647ad4f6d866d41eefff1806df37c68aef0c]
> > [ T93022] pci_device_remove+0x42/0xb0
> > [ T93022] device_release_driver_internal+0x19f/0x200
> > [ T93022] driver_detach+0x48/0x90
> > [ T93022] bus_remove_driver+0x70/0xf0
> > [ T93022] pci_unregister_driver+0x42/0xb0
> > [ T93022] ice_module_exit+0x10/0xdb0 [ice
> > 33d2647ad4f6d866d41eefff1806df37c68aef0c]
> > ...
> > [ T93022] ---[ end trace 0000000000000000 ]---
> > [ T93022] ice: module unloaded
> >
> > Fixes: e800654e85b5 ("ice: Use ice_adapter for PTP shared data instead of
> auxdev")
> > Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
> > Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> > ---
> > drivers/net/ethernet/intel/ice/ice_ptp.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c
> b/drivers/net/ethernet/intel/ice/ice_ptp.c
> > index fb0f6365a6d6..c43a7973d70f 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> > @@ -3282,8 +3282,10 @@ void ice_ptp_init(struct ice_pf *pf)
> > */
> > void ice_ptp_release(struct ice_pf *pf)
> > {
> > - if (pf->ptp.state != ICE_PTP_READY)
> > + if (pf->ptp.state != ICE_PTP_READY) {
> > + ice_ptp_cleanup_pf(pf);
> > return;
> > + }
> >
> > pf->ptp.state = ICE_PTP_UNINIT;
> >
>
> ice_ptp_cleanup_pf() removes ptp->port.list_node, which is inited in
> ice_ptp_setup_pf(), but ice_ptp_init() may fail before
> ice_ptp_setup_pf() is called, and it will keep pf->ptp.state =
> ICE_PTP_ERROR. the cleanup then will work on uninitialized data.
>
> It looks like it's better to make proper clean up in ice_ptp_setup_pf()
> on error path rather then modify ice_ptp_cleanup_pf().
Thanks Vadim for your feedback.
Of course, you're right for this specific flow.
However, the init part is not the only case when sth might go wrong.
I think we won't avoid that cleanup in ptp_release().
Anyway, I'm going to improve init error path part as you suggested
and rethink what else/how can be improved in ice_ptp_release().
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH iwl-net] ice: fix PTP cleanup on driver removal in error path
@ 2025-10-08 15:21 ` Nitka, Grzegorz
0 siblings, 0 replies; 6+ messages in thread
From: Nitka, Grzegorz @ 2025-10-08 15:21 UTC (permalink / raw)
To: Vadim Fedorenko, intel-wired-lan@lists.osuosl.org
Cc: netdev@vger.kernel.org, Loktionov, Aleksandr, Kitszel, Przemyslaw
> -----Original Message-----
> From: Vadim Fedorenko <vadim.fedorenko@linux.dev>
> Sent: Wednesday, October 8, 2025 3:39 PM
> To: Nitka, Grzegorz <grzegorz.nitka@intel.com>; intel-wired-
> lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>
> Subject: Re: [PATCH iwl-net] ice: fix PTP cleanup on driver removal in error
> path
>
> On 08/10/2025 12:58, Grzegorz Nitka wrote:
> > Improve PTP feature cleanup in error path by adding explicit call to
> > ice_ptp_cleanup_pf in the case in which PTP feature is not fully
> > operational at the time of driver removal (which is indicated by
> > ptp->state flag).
> > At the driver probe, if PTP feature is supported, each PF adds its own
> > port to the list of ports controlled by ice_adapter object.
> > Analogously, at the driver remove, it's expected each PF is
> > responsible for removing previously added port from the list.
> > If for some reason (like errors in reset handling, NVM update etc.), PTP
> > feature has not rebuilt successfully, the driver is still responsible for
> > proper clearing ice_adapter port list. It's done by calling
> > ice_ptp_cleanup_pf function.
> > Otherwise, the following call trace is observed when ice_adapter object
> > is freed (port list is not empty, as it is expected at this stage):
> >
> > [ T93022] ------------[ cut here ]------------
> > [ T93022] WARNING: CPU: 10 PID: 93022 at
> > ice/ice_adapter.c:67 ice_adapter_put+0xef/0x100 [ice]
> > ...
> > [ T93022] RIP: 0010:ice_adapter_put+0xef/0x100 [ice]
> > ...
> > [ T93022] Call Trace:
> > [ T93022] <TASK>
> > [ T93022] ? ice_adapter_put+0xef/0x100 [ice
> > 33d2647ad4f6d866d41eefff1806df37c68aef0c]
> > [ T93022] ? __warn.cold+0xb0/0x10e
> > [ T93022] ? ice_adapter_put+0xef/0x100 [ice
> > 33d2647ad4f6d866d41eefff1806df37c68aef0c]
> > [ T93022] ? report_bug+0xd8/0x150
> > [ T93022] ? handle_bug+0xe9/0x110
> > [ T93022] ? exc_invalid_op+0x17/0x70
> > [ T93022] ? asm_exc_invalid_op+0x1a/0x20
> > [ T93022] ? ice_adapter_put+0xef/0x100 [ice
> > 33d2647ad4f6d866d41eefff1806df37c68aef0c]
> > [ T93022] pci_device_remove+0x42/0xb0
> > [ T93022] device_release_driver_internal+0x19f/0x200
> > [ T93022] driver_detach+0x48/0x90
> > [ T93022] bus_remove_driver+0x70/0xf0
> > [ T93022] pci_unregister_driver+0x42/0xb0
> > [ T93022] ice_module_exit+0x10/0xdb0 [ice
> > 33d2647ad4f6d866d41eefff1806df37c68aef0c]
> > ...
> > [ T93022] ---[ end trace 0000000000000000 ]---
> > [ T93022] ice: module unloaded
> >
> > Fixes: e800654e85b5 ("ice: Use ice_adapter for PTP shared data instead of
> auxdev")
> > Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
> > Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> > ---
> > drivers/net/ethernet/intel/ice/ice_ptp.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c
> b/drivers/net/ethernet/intel/ice/ice_ptp.c
> > index fb0f6365a6d6..c43a7973d70f 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> > @@ -3282,8 +3282,10 @@ void ice_ptp_init(struct ice_pf *pf)
> > */
> > void ice_ptp_release(struct ice_pf *pf)
> > {
> > - if (pf->ptp.state != ICE_PTP_READY)
> > + if (pf->ptp.state != ICE_PTP_READY) {
> > + ice_ptp_cleanup_pf(pf);
> > return;
> > + }
> >
> > pf->ptp.state = ICE_PTP_UNINIT;
> >
>
> ice_ptp_cleanup_pf() removes ptp->port.list_node, which is inited in
> ice_ptp_setup_pf(), but ice_ptp_init() may fail before
> ice_ptp_setup_pf() is called, and it will keep pf->ptp.state =
> ICE_PTP_ERROR. the cleanup then will work on uninitialized data.
>
> It looks like it's better to make proper clean up in ice_ptp_setup_pf()
> on error path rather then modify ice_ptp_cleanup_pf().
Thanks Vadim for your feedback.
Of course, you're right for this specific flow.
However, the init part is not the only case when sth might go wrong.
I think we won't avoid that cleanup in ptp_release().
Anyway, I'm going to improve init error path part as you suggested
and rethink what else/how can be improved in ice_ptp_release().
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-10-08 15:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-08 11:58 [Intel-wired-lan] [PATCH iwl-net] ice: fix PTP cleanup on driver removal in error path Grzegorz Nitka
2025-10-08 11:58 ` Grzegorz Nitka
2025-10-08 13:38 ` [Intel-wired-lan] " Vadim Fedorenko
2025-10-08 13:38 ` Vadim Fedorenko
2025-10-08 15:21 ` [Intel-wired-lan] " Nitka, Grzegorz
2025-10-08 15:21 ` Nitka, Grzegorz
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.