From: Alex Elder <hidden> Date: 2021-08-04 15:36:36
The first two patches in this series are basically bug fixes, but in
practice I don't think we've seen the problems they might cause.
The third patch moves clock and interconnect related error messages
around a bit, reporting better information and doing so in the
functions where they are enabled or disabled (rather than those
functions' callers).
The last three patches move power-related code into "ipa_clock.c",
as a step toward generalizing the purpose of that source file.
-Alex
Alex Elder (6):
net: ipa: don't suspend/resume modem if not up
net: ipa: reorder netdev pointer assignments
net: ipa: improve IPA clock error messages
net: ipa: move IPA power operations to ipa_clock.c
net: ipa: move ipa_suspend_handler()
net: ipa: move IPA flags field
drivers/net/ipa/ipa.h | 12 ---
drivers/net/ipa/ipa_clock.c | 147 +++++++++++++++++++++++++++++++-----
drivers/net/ipa/ipa_clock.h | 15 ++++
drivers/net/ipa/ipa_main.c | 97 ++----------------------
drivers/net/ipa/ipa_modem.c | 30 +++++---
5 files changed, 172 insertions(+), 129 deletions(-)
--
2.27.0
From: Alex Elder <hidden> Date: 2021-08-04 15:36:34
The modem network device is set up by ipa_modem_start(). But its
TX queue is not actually started and endpoints enabled until it is
opened.
So avoid stopping the modem network device TX queue and disabling
endpoints on suspend or stop unless the netdev is marked UP. And
skip attempting to resume unless it is UP.
Signed-off-by: Alex Elder <redacted>
---
drivers/net/ipa/ipa_modem.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
@@ -265,9 +271,11 @@ int ipa_modem_stop(struct ipa *ipa)/* Prevent the modem from triggering a call to ipa_setup() */ipa_smp2p_disable(ipa);-/* Stop the queue and disable the endpoints if it's open */+/* Clean up the netdev and endpoints if it was started */if(netdev){-(void)ipa_stop(netdev);+/* If it was opened, stop it first */+if(netdev->flags&IFF_UP)+(void)ipa_stop(netdev);ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev=NULL;ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev=NULL;ipa->modem_netdev=NULL;
From: Alex Elder <hidden> Date: 2021-08-04 15:36:38
Assign the ipa->modem_netdev and endpoint->netdev pointers *before*
registering the network device. As soon as the device is
registered it can be opened, and by that time we'll want those
pointers valid.
Similarly, don't make those pointers NULL until *after* the modem
network device is unregistered in ipa_modem_stop().
Signed-off-by: Alex Elder <redacted>
---
drivers/net/ipa/ipa_modem.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
@@ -231,13 +231,15 @@ int ipa_modem_start(struct ipa *ipa)SET_NETDEV_DEV(netdev,&ipa->pdev->dev);priv=netdev_priv(netdev);priv->ipa=ipa;+ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev=netdev;+ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev=netdev;+ipa->modem_netdev=netdev;ret=register_netdev(netdev);-if(!ret){-ipa->modem_netdev=netdev;-ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev=netdev;-ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev=netdev;-}else{+if(ret){+ipa->modem_netdev=NULL;+ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev=NULL;+ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev=NULL;free_netdev(netdev);}
@@ -276,10 +278,10 @@ int ipa_modem_stop(struct ipa *ipa)/* If it was opened, stop it first */if(netdev->flags&IFF_UP)(void)ipa_stop(netdev);-ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev=NULL;-ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev=NULL;-ipa->modem_netdev=NULL;unregister_netdev(netdev);+ipa->modem_netdev=NULL;+ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev=NULL;+ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev=NULL;free_netdev(netdev);}
From: Alex Elder <hidden> Date: 2021-08-04 15:36:41
Move ipa_suspend_handler() into "ipa_clock.c" from "ipa_main.c", to
group with the reset of the suspend/resume code. This IPA interrupt
is triggered if an IPA RX endpoint is suspended but has a packet to
be delivered.
Introduce ipa_power_setup() and ipa_power_teardown() to add and
remove the handler for the IPA SUSPEND interrupt at the same place
as before, while allowing the handler to remain private.
The "power" naming convention will be adopted elsewhere in this
file as well (soon).
Signed-off-by: Alex Elder <redacted>
---
drivers/net/ipa/ipa_clock.c | 34 +++++++++++++++++++++++++++++++++
drivers/net/ipa/ipa_clock.h | 12 ++++++++++++
drivers/net/ipa/ipa_main.c | 38 +++++++------------------------------
3 files changed, 53 insertions(+), 31 deletions(-)
@@ -279,6 +279,40 @@ u32 ipa_clock_rate(struct ipa *ipa)returnipa->clock?(u32)clk_get_rate(ipa->clock->core):0;}+/**+*ipa_suspend_handler()-HandlethesuspendIPAinterrupt+*@ipa:IPApointer+*@irq_id:IPAinterrupttype(unused)+*+*IfanRXendpointissuspended,andtheIPAhasapacketdestinedfor+*thatendpoint,theIPAgeneratesaSUSPENDinterrupttoinformtheAP+*thatitshouldresumetheendpoint.Ifwegetoneoftheseinterrupts+*wejustwakeupthesystem.+*/+staticvoidipa_suspend_handler(structipa*ipa,enumipa_irq_idirq_id)+{+/* Just report the event, and let system resume handle the rest.+*Morethanoneendpointcouldsignalthis;ifso,ignore+*allbutthefirst.+*/+if(!test_and_set_bit(IPA_FLAG_RESUMED,ipa->flags))+pm_wakeup_dev_event(&ipa->pdev->dev,0,true);++/* Acknowledge/clear the suspend interrupt on all endpoints */+ipa_interrupt_suspend_clear_all(ipa->interrupt);+}++voidipa_power_setup(structipa*ipa)+{+ipa_interrupt_add(ipa->interrupt,IPA_IRQ_TX_SUSPEND,+ipa_suspend_handler);+}++voidipa_power_teardown(structipa*ipa)+{+ipa_interrupt_remove(ipa->interrupt,IPA_IRQ_TX_SUSPEND);+}+/* Initialize IPA clocking */structipa_clock*ipa_clock_init(structdevice*dev,conststructipa_clock_data*data)
@@ -79,29 +79,6 @@/* Divider for 19.2 MHz crystal oscillator clock to get common timer clock */#define IPA_XO_CLOCK_DIVIDER 192 /* 1 is subtracted where used */-/**-*ipa_suspend_handler()-HandlethesuspendIPAinterrupt-*@ipa:IPApointer-*@irq_id:IPAinterrupttype(unused)-*-*IfanRXendpointisinsuspendstate,andtheIPAhasapacket-*destinedforthatendpoint,theIPAgeneratesaSUSPENDinterrupt-*toinformtheAPthatitshouldresumetheendpoint.Ifweget-*oneoftheseinterruptswejustresumeeverything.-*/-staticvoidipa_suspend_handler(structipa*ipa,enumipa_irq_idirq_id)-{-/* Just report the event, and let system resume handle the rest.-*Morethanoneendpointcouldsignalthis;ifso,ignore-*allbutthefirst.-*/-if(!test_and_set_bit(IPA_FLAG_RESUMED,ipa->flags))-pm_wakeup_dev_event(&ipa->pdev->dev,0,true);--/* Acknowledge/clear the suspend interrupt on all endpoints */-ipa_interrupt_suspend_clear_all(ipa->interrupt);-}-/***ipa_setup()-SetupIPAhardware*@ipa:IPApointer
@@ -124,12 +101,11 @@ int ipa_setup(struct ipa *ipa)if(ret)returnret;-ipa_interrupt_add(ipa->interrupt,IPA_IRQ_TX_SUSPEND,-ipa_suspend_handler);+ipa_power_setup(ipa);ret=device_init_wakeup(dev,true);if(ret)-gotoerr_interrupt_remove;+gotoerr_gsi_teardown;ipa_endpoint_setup(ipa);
@@ -177,9 +153,9 @@ int ipa_setup(struct ipa *ipa)ipa_endpoint_disable_one(command_endpoint);err_endpoint_teardown:ipa_endpoint_teardown(ipa);+ipa_power_teardown(ipa);(void)device_init_wakeup(dev,false);-err_interrupt_remove:-ipa_interrupt_remove(ipa->interrupt,IPA_IRQ_TX_SUSPEND);+err_gsi_teardown:gsi_teardown(&ipa->gsi);returnret;
From: Alex Elder <hidden> Date: 2021-08-04 15:36:46
The ipa->flags field is only ever used in "ipa_clock.c", related to
suspend/resume activity.
Move the definition of the ipa_flag enumerated type to "ipa_clock.c".
And move the flags field from the ipa structure and to the ipa_clock
structure. Rename the type and its values to include "power" or
"POWER" in the name.
Signed-off-by: Alex Elder <redacted>
---
drivers/net/ipa/ipa.h | 12 ------------
drivers/net/ipa/ipa_clock.c | 16 ++++++++++++++--
2 files changed, 14 insertions(+), 14 deletions(-)
@@ -27,20 +27,9 @@ struct ipa_clock;structipa_smp2p;structipa_interrupt;-/**-*enumipa_flag-IPAstateflags-*@IPA_FLAG_RESUMED:Whetherresumefromsuspendhasbeensignaled-*@IPA_FLAG_COUNT:NumberofdefinedIPAflags-*/-enumipa_flag{-IPA_FLAG_RESUMED,-IPA_FLAG_COUNT,/* Last; not a flag */-};-/***structipa-IPAinformation*@gsi:EmbeddedGSIstructure-*@flags:Booleanstateflags*@version:IPAhardwareversion*@pdev:Platformdevice*@completion:Usedtosignalpipelinecleartransfercomplete
@@ -45,11 +45,22 @@ struct ipa_interconnect {u32peak_bandwidth;};+/**+*enumipa_power_flag-IPApowerflags+*@IPA_POWER_FLAG_RESUMED:Whetherresumefromsuspendhasbeensignaled+*@IPA_POWER_FLAG_COUNT:Numberofdefinedpowerflags+*/+enumipa_power_flag{+IPA_POWER_FLAG_RESUMED,+IPA_POWER_FLAG_COUNT,/* Last; not a flag */+};+/***structipa_clock-IPAclockinginformation*@count:Clockingreferencecount*@mutex:Protectsclockenable/disable*@core:IPAcoreclock+*@flags:Booleanstateflags*@interconnect_count:Numberofelementsininterconnect[]*@interconnect:Interconnectarray*/
From: Alex Elder <hidden> Date: 2021-08-04 15:36:49
Rearrange messages reported when errors occur in the IPA clock code,
so that the specific interconnect is identified when an error occurs
enabling or disabling it, or the core clock is indicated when an
error occurs enabling it.
Have ipa_interconnect_disable() return zero or the negative error
value returned by the first interconnect that produced an error
when disabled. For now, the callers ignore the returned value.
Signed-off-by: Alex Elder <redacted>
---
drivers/net/ipa/ipa_clock.c | 39 +++++++++++++++++++++----------------
1 file changed, 22 insertions(+), 17 deletions(-)
@@ -159,10 +163,11 @@ static int ipa_interconnect_enable(struct ipa *ipa)}/* To disable an interconnect, we just its bandwidth to 0 */-staticvoidipa_interconnect_disable(structipa*ipa)+staticintipa_interconnect_disable(structipa*ipa){structipa_interconnect*interconnect;structipa_clock*clock=ipa->clock;+structdevice*dev=&ipa->pdev->dev;intresult=0;u32count;intret;
@@ -172,13 +177,16 @@ static void ipa_interconnect_disable(struct ipa *ipa)while(count--){interconnect--;ret=icc_set_bw(interconnect->path,0,0);-if(ret&&!result)-result=ret;+if(ret){+dev_err(dev,"error %d disabling %s interconnect\n",+ret,icc_get_name(interconnect->path));+/* Try to disable all; record only the first error */+if(!result)+result=ret;+}}-if(result)-dev_err(&ipa->pdev->dev,-"error %d disabling IPA interconnects\n",ret);+returnresult;}/* Turn on IPA clocks, including interconnects */
@@ -201,7 +211,7 @@ static int ipa_clock_enable(struct ipa *ipa)staticvoidipa_clock_disable(structipa*ipa){clk_disable_unprepare(ipa->clock->core);-ipa_interconnect_disable(ipa);+(void)ipa_interconnect_disable(ipa);}/* Get an IPA clock reference, but only if the reference count is
From: Alex Elder <hidden> Date: 2021-08-04 15:36:53
Move ipa_suspend() and ipa_resume(), as well as the definition of
the ipa_pm_ops structure into "ipa_clock.c". Make ipa_pm_ops public
and declare it as extern in "ipa_clock.h".
This is part of centralizing IPA power management functionality into
"ipa_clock.c" (the file will eventually get a name change).
Signed-off-by: Alex Elder <redacted>
---
drivers/net/ipa/ipa_clock.c | 62 +++++++++++++++++++++++++++++++++++++
drivers/net/ipa/ipa_clock.h | 3 ++
drivers/net/ipa/ipa_main.c | 59 -----------------------------------
3 files changed, 65 insertions(+), 59 deletions(-)
@@ -334,3 +337,62 @@ void ipa_clock_exit(struct ipa_clock *clock)kfree(clock);clk_put(clk);}++/**+*ipa_suspend()-Powermanagementsystemsuspendcallback+*@dev:IPAdevicestructure+*+*Return:Alwaysreturnszero+*+*CalledbythePMframeworkwhenasystemsuspendoperationisinvoked.+*Suspendsendpointsandreleasestheclockreferenceheldtokeep+*theIPAclockrunninguntilthispoint.+*/+staticintipa_suspend(structdevice*dev)+{+structipa*ipa=dev_get_drvdata(dev);++/* Endpoints aren't usable until setup is complete */+if(ipa->setup_complete){+__clear_bit(IPA_FLAG_RESUMED,ipa->flags);+ipa_endpoint_suspend(ipa);+gsi_suspend(&ipa->gsi);+}++ipa_clock_put(ipa);++return0;+}++/**+*ipa_resume()-Powermanagementsystemresumecallback+*@dev:IPAdevicestructure+*+*Return:Alwaysreturns0+*+*CalledbythePMframeworkwhenasystemresumeoperationisinvoked.+*TakesanIPAclockreferencetokeeptheclockrunninguntilsuspend,+*andresumesendpoints.+*/+staticintipa_resume(structdevice*dev)+{+structipa*ipa=dev_get_drvdata(dev);++/* This clock reference will keep the IPA out of suspend+*untilwegetapowermanagementsuspendrequest.+*/+ipa_clock_get(ipa);++/* Endpoints aren't usable until setup is complete */+if(ipa->setup_complete){+gsi_resume(&ipa->gsi);+ipa_endpoint_resume(ipa);+}++return0;+}++conststructdev_pm_opsipa_pm_ops={+.suspend=ipa_suspend,+.resume=ipa_resume,+};
@@ -874,65 +874,6 @@ static void ipa_shutdown(struct platform_device *pdev)dev_err(&pdev->dev,"shutdown: remove returned %d\n",ret);}-/**-*ipa_suspend()-Powermanagementsystemsuspendcallback-*@dev:IPAdevicestructure-*-*Return:Alwaysreturnszero-*-*CalledbythePMframeworkwhenasystemsuspendoperationisinvoked.-*Suspendsendpointsandreleasestheclockreferenceheldtokeep-*theIPAclockrunninguntilthispoint.-*/-staticintipa_suspend(structdevice*dev)-{-structipa*ipa=dev_get_drvdata(dev);--/* Endpoints aren't usable until setup is complete */-if(ipa->setup_complete){-__clear_bit(IPA_FLAG_RESUMED,ipa->flags);-ipa_endpoint_suspend(ipa);-gsi_suspend(&ipa->gsi);-}--ipa_clock_put(ipa);--return0;-}--/**-*ipa_resume()-Powermanagementsystemresumecallback-*@dev:IPAdevicestructure-*-*Return:Alwaysreturns0-*-*CalledbythePMframeworkwhenasystemresumeoperationisinvoked.-*TakesanIPAclockreferencetokeeptheclockrunninguntilsuspend,-*andresumesendpoints.-*/-staticintipa_resume(structdevice*dev)-{-structipa*ipa=dev_get_drvdata(dev);--/* This clock reference will keep the IPA out of suspend-*untilwegetapowermanagementsuspendrequest.-*/-ipa_clock_get(ipa);--/* Endpoints aren't usable until setup is complete */-if(ipa->setup_complete){-gsi_resume(&ipa->gsi);-ipa_endpoint_resume(ipa);-}--return0;-}--staticconststructdev_pm_opsipa_pm_ops={-.suspend=ipa_suspend,-.resume=ipa_resume,-};-staticconststructattribute_group*ipa_attribute_groups[]={&ipa_attribute_group,&ipa_feature_attribute_group,
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-08-06 01:26:32
On Wed, 4 Aug 2021 10:36:21 -0500 Alex Elder wrote:
The modem network device is set up by ipa_modem_start(). But its
TX queue is not actually started and endpoints enabled until it is
opened.
So avoid stopping the modem network device TX queue and disabling
endpoints on suspend or stop unless the netdev is marked UP. And
skip attempting to resume unless it is UP.
Signed-off-by: Alex Elder <redacted>
You said in the cover letter that in practice this fix doesn't matter.
It seems trivial to test so perhaps it doesn't and we should leave the
code be? Looking at dev->flags without holding rtnl_lock() seems
suspicious, drivers commonly put the relevant portion of suspend/resume
routines under rtnl_lock()/rtnl_unlock() (although to be completely
frank IDK if it's actually possible for concurrent suspend +
open/close to happen).
Are there any callers of ipa_modem_stop() which don't hold rtnl_lock()?
@@ -265,9 +271,11 @@ int ipa_modem_stop(struct ipa *ipa)/* Prevent the modem from triggering a call to ipa_setup() */ipa_smp2p_disable(ipa);-/* Stop the queue and disable the endpoints if it's open */+/* Clean up the netdev and endpoints if it was started */if(netdev){-(void)ipa_stop(netdev);+/* If it was opened, stop it first */+if(netdev->flags&IFF_UP)+(void)ipa_stop(netdev);ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev=NULL;ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev=NULL;ipa->modem_netdev=NULL;
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-08-06 01:27:17
On Wed, 4 Aug 2021 10:36:22 -0500 Alex Elder wrote:
Assign the ipa->modem_netdev and endpoint->netdev pointers *before*
registering the network device. As soon as the device is
registered it can be opened, and by that time we'll want those
pointers valid.
Similarly, don't make those pointers NULL until *after* the modem
network device is unregistered in ipa_modem_stop().
Signed-off-by: Alex Elder <redacted>
This one seems like a pretty legit race, net would be better if you
don't mind.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-08-06 01:41:52
On Thu, 5 Aug 2021 18:27:12 -0700 Jakub Kicinski wrote:
On Wed, 4 Aug 2021 10:36:22 -0500 Alex Elder wrote:
quoted
Assign the ipa->modem_netdev and endpoint->netdev pointers *before*
registering the network device. As soon as the device is
registered it can be opened, and by that time we'll want those
pointers valid.
Similarly, don't make those pointers NULL until *after* the modem
network device is unregistered in ipa_modem_stop().
Signed-off-by: Alex Elder <redacted>
This one seems like a pretty legit race, net would be better if you
don't mind.
Ah, this set was already applied, don't mind me :)
From: Alex Elder <hidden> Date: 2021-08-06 11:39:54
On 8/5/21 8:26 PM, Jakub Kicinski wrote:
On Wed, 4 Aug 2021 10:36:21 -0500 Alex Elder wrote:
quoted
The modem network device is set up by ipa_modem_start(). But its
TX queue is not actually started and endpoints enabled until it is
opened.
So avoid stopping the modem network device TX queue and disabling
endpoints on suspend or stop unless the netdev is marked UP. And
skip attempting to resume unless it is UP.
Signed-off-by: Alex Elder <redacted>
You said in the cover letter that in practice this fix doesn't matter.
I don't think we've seen this problem with system suspend, but
with runtime suspend we could get a forced suspend request at
any time (and frequently), so if there is a problem, it will be
much more likely to occur.
For suspend, I don't think it's actually a "problem". Disabling
the TX queue if it wasn't open is harmless--it just sets the
DRV_XOFF bit in the TX queue state field. And we have a
separate "enabled endpoints" mask that prevents stopping or
suspending the endpoint if it wasn't opened.
But for resume, waking the queue schedules it. I'm not sure
what exactly ensues in that case, but it's not correct if the
network device hasn't been opened. For endpoints, again, they
won't be resumed if they weren't enabled, so that part's OK.
It seems trivial to test so perhaps it doesn't and we should leave the
code be? Looking at dev->flags without holding rtnl_lock() seems
suspicious, drivers commonly put the relevant portion of suspend/resume
routines under rtnl_lock()/rtnl_unlock() (although to be completely
I don't use rtnl_lock()/rtnl_unlock() *anywhere* in the driver.
It has no netlink interface (yet), and therefore I didn't even
think about using rtnl_lock(). Do I need it?
frank IDK if it's actually possible for concurrent suspend +
open/close to happen).
I think it isn't possible, but I'm less than 100% sure. I've
been thinking a lot about exactly this sort of question lately...
Are there any callers of ipa_modem_stop() which don't hold rtnl_lock()?
None of them take that lock. It is called in the driver ->remove
callback, and is called during cleanup if the modem crashes.
I think this fix is good, but as I said in the cover letter I'm
not aware of ever having hit it to date.
Thank you very much for your review and comments.
-Alex
@@ -265,9 +271,11 @@ int ipa_modem_stop(struct ipa *ipa)/* Prevent the modem from triggering a call to ipa_setup() */ipa_smp2p_disable(ipa);-/* Stop the queue and disable the endpoints if it's open */+/* Clean up the netdev and endpoints if it was started */if(netdev){-(void)ipa_stop(netdev);+/* If it was opened, stop it first */+if(netdev->flags&IFF_UP)+(void)ipa_stop(netdev);ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev=NULL;ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev=NULL;ipa->modem_netdev=NULL;
From: Alex Elder <hidden> Date: 2021-08-06 11:39:57
On 8/5/21 8:27 PM, Jakub Kicinski wrote:
On Wed, 4 Aug 2021 10:36:22 -0500 Alex Elder wrote:
quoted
Assign the ipa->modem_netdev and endpoint->netdev pointers *before*
registering the network device. As soon as the device is
registered it can be opened, and by that time we'll want those
pointers valid.
Similarly, don't make those pointers NULL until *after* the modem
network device is unregistered in ipa_modem_stop().
Signed-off-by: Alex Elder <redacted>
This one seems like a pretty legit race, net would be better if you
don't mind.
I don't mind at all. But now that it's accepted, I'm not sure how
to go about getting it back-ported. Mainly I don't want to interfere
with any interaction between net/master and net-next/master... Maybe
you're in a better position to do that. And if so:
Fixes: 57f63faf0562 ("net: ipa: only set endpoint netdev pointer when in
use")
I'll happily do it if you can tell me the best way how. Thanks.
-Alex
From: Alex Elder <hidden> Date: 2021-08-06 11:40:03
On 8/5/21 8:41 PM, Jakub Kicinski wrote:
On Thu, 5 Aug 2021 18:27:12 -0700 Jakub Kicinski wrote:
quoted
On Wed, 4 Aug 2021 10:36:22 -0500 Alex Elder wrote:
quoted
Assign the ipa->modem_netdev and endpoint->netdev pointers *before*
registering the network device. As soon as the device is
registered it can be opened, and by that time we'll want those
pointers valid.
Similarly, don't make those pointers NULL until *after* the modem
network device is unregistered in ipa_modem_stop().
Signed-off-by: Alex Elder <redacted>
This one seems like a pretty legit race, net would be better if you
don't mind.
Ah, this set was already applied, don't mind me :)
I really appreciate your review and feedback. Applied or not,
it's valuable to me.
-Alex
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-08-06 13:00:07
On Fri, 6 Aug 2021 06:39:46 -0500 Alex Elder wrote:
On 8/5/21 8:26 PM, Jakub Kicinski wrote:
quoted
On Wed, 4 Aug 2021 10:36:21 -0500 Alex Elder wrote:
quoted
The modem network device is set up by ipa_modem_start(). But its
TX queue is not actually started and endpoints enabled until it is
opened.
So avoid stopping the modem network device TX queue and disabling
endpoints on suspend or stop unless the netdev is marked UP. And
skip attempting to resume unless it is UP.
Signed-off-by: Alex Elder <redacted>
You said in the cover letter that in practice this fix doesn't matter.
I don't think we've seen this problem with system suspend, but
with runtime suspend we could get a forced suspend request at
any time (and frequently), so if there is a problem, it will be
much more likely to occur.
For suspend, I don't think it's actually a "problem". Disabling
the TX queue if it wasn't open is harmless--it just sets the
DRV_XOFF bit in the TX queue state field. And we have a
separate "enabled endpoints" mask that prevents stopping or
suspending the endpoint if it wasn't opened.
But for resume, waking the queue schedules it. I'm not sure
what exactly ensues in that case, but it's not correct if the
network device hasn't been opened. For endpoints, again, they
won't be resumed if they weren't enabled, so that part's OK.
quoted
It seems trivial to test so perhaps it doesn't and we should leave the
code be? Looking at dev->flags without holding rtnl_lock() seems
suspicious, drivers commonly put the relevant portion of suspend/resume
routines under rtnl_lock()/rtnl_unlock() (although to be completely
I don't use rtnl_lock()/rtnl_unlock() *anywhere* in the driver.
It has no netlink interface (yet), and therefore I didn't even
think about using rtnl_lock(). Do I need it?
Runtime PM interactions with rtnl_lock get really tricky, if there are
callers which will wake the device up while holding rtnl then taking
rtnl in .resume will cause an obvious deadlock, right?
I'm starting to feel like driver's RPM-related code has to be under it's
own lock, and interrogating higher layer's (e.g. network stack's) state
from RPM code should be avoided...
Long story short I don't think we have a good handle on this,
I certainly don't so maybe let's leave your code be, for now.
quoted
frank IDK if it's actually possible for concurrent suspend +
open/close to happen).
I think it isn't possible, but I'm less than 100% sure. I've
been thinking a lot about exactly this sort of question lately...
quoted
Are there any callers of ipa_modem_stop() which don't hold rtnl_lock()?
None of them take that lock. It is called in the driver ->remove
callback, and is called during cleanup if the modem crashes.
I think this fix is good, but as I said in the cover letter I'm
not aware of ever having hit it to date.
Thank you very much for your review and comments.