Re: Requirements for a shutdown function?
On 05/10/2017 03:11 PM, Timur Tabi wrote: > On 05/10/2017 04:47 PM, Florian Fainelli wrote: >> AFAIR kexec takes care of shutting down network devices explicitly >> (unless instructed otherwise with -x/--no-ifdown) so this may be where >> this is coming from. >> >> Reading through drivers/base/core.c it does not appear that ->remove() >> is called and then ->shutdown() gets called, only ->shutdown() gets >> called from device_shutdown() called from kernel/reboot.c. It seems to >> me like if you want to be on the safe side you would want to implement a >> shutdown function that is identical to what your remove function does. > > I finally found a testcase where the shutdown function is useful. If you do > a "reboot -f", it will call shutdown but not close. Correct yes. Sorry, I did not recall which one of kexec or reboot would call it, but both would actually now that I looked at what happens on one of my systems again. -- Florian
Re: Requirements for a shutdown function?
On 05/10/2017 04:47 PM, Florian Fainelli wrote: > AFAIR kexec takes care of shutting down network devices explicitly > (unless instructed otherwise with -x/--no-ifdown) so this may be where > this is coming from. > > Reading through drivers/base/core.c it does not appear that ->remove() > is called and then ->shutdown() gets called, only ->shutdown() gets > called from device_shutdown() called from kernel/reboot.c. It seems to > me like if you want to be on the safe side you would want to implement a > shutdown function that is identical to what your remove function does. I finally found a testcase where the shutdown function is useful. If you do a "reboot -f", it will call shutdown but not close. -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: Requirements for a shutdown function?
On 05/10/2017 01:17 PM, Timur Tabi wrote: > On 05/09/2017 02:06 PM, Florian Fainelli wrote: >> On 05/09/2017 11:51 AM, Timur Tabi wrote: > >>> Is it possible that the network stack detects a kexec and automatically >>> stops all network devices? >> >> No. why would it? However the device driver model does call into your >> driver's remove function and that one does a right job already because >> it does an network device unregister, and so on. > > I ran some more tests. When I launch kexec, the driver's > net_device_ops.ndo_stop function is called, which already stops the interface. > > So it looks to me as if the network stack does close the interface during a > kexec. With the interface closed, there's no point in having a shutdown > function, is there? AFAIR kexec takes care of shutting down network devices explicitly (unless instructed otherwise with -x/--no-ifdown) so this may be where this is coming from. Reading through drivers/base/core.c it does not appear that ->remove() is called and then ->shutdown() gets called, only ->shutdown() gets called from device_shutdown() called from kernel/reboot.c. It seems to me like if you want to be on the safe side you would want to implement a shutdown function that is identical to what your remove function does. > >>> My in-house driver stops the RX and TX queues. I'm guessing that's good >>> enough, but I don't have a failing test case to prove it. >>> >> >> That's probably good enough, yes. > > Except that it turns out that the queues are already stopped by then because > the emac_close() function has already been called. -- Florian
Re: Requirements for a shutdown function?
On 05/09/2017 02:06 PM, Florian Fainelli wrote: > On 05/09/2017 11:51 AM, Timur Tabi wrote: >> Is it possible that the network stack detects a kexec and automatically >> stops all network devices? > > No. why would it? However the device driver model does call into your > driver's remove function and that one does a right job already because > it does an network device unregister, and so on. I ran some more tests. When I launch kexec, the driver's net_device_ops.ndo_stop function is called, which already stops the interface. So it looks to me as if the network stack does close the interface during a kexec. With the interface closed, there's no point in having a shutdown function, is there? >> My in-house driver stops the RX and TX queues. I'm guessing that's good >> enough, but I don't have a failing test case to prove it. >> > > That's probably good enough, yes. Except that it turns out that the queues are already stopped by then because the emac_close() function has already been called. -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: Requirements for a shutdown function?
On 05/09/2017 11:51 AM, Timur Tabi wrote: > On 05/09/2017 01:46 PM, Florian Fainelli wrote: >> A good test case for exercising a .shutdown() function is kexec'ing a >> new kernel for instance. > > I tried that. I run iperf in one window while launching kexec in another. > Even without a shutdown function, network traffic appear to halt on its own > and the kexec succeeds. > > Is it possible that the network stack detects a kexec and automatically > stops all network devices? No. why would it? However the device driver model does call into your driver's remove function and that one does a right job already because it does an network device unregister, and so on. There is no strict requirement for implementing a .shutdown() function AFAICT and it does not necessarily make sense to have one depending on the bus type. For platform/MMIO devices, it hardly has any value, but on e.g: PCI, it could be added as an additional step to perform a full device shutdown. > >> You should put your HW in a state where it won't be doing DMA, or have >> any adverse side effects to the system, putting it in a low power state >> is also a good approach. > > My in-house driver stops the RX and TX queues. I'm guessing that's good > enough, but I don't have a failing test case to prove it. > That's probably good enough, yes. -- Florian
Re: Requirements for a shutdown function?
On 05/09/2017 01:46 PM, Florian Fainelli wrote: > A good test case for exercising a .shutdown() function is kexec'ing a > new kernel for instance. I tried that. I run iperf in one window while launching kexec in another. Even without a shutdown function, network traffic appear to halt on its own and the kexec succeeds. Is it possible that the network stack detects a kexec and automatically stops all network devices? > You should put your HW in a state where it won't be doing DMA, or have > any adverse side effects to the system, putting it in a low power state > is also a good approach. My in-house driver stops the RX and TX queues. I'm guessing that's good enough, but I don't have a failing test case to prove it. -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: Requirements for a shutdown function?
On 05/09/2017 09:58 AM, Timur Tabi wrote: > I'm trying to add a platform_driver.shutdown function to my Ethernet driver > (drivers/net/ethernet/qualcomm/emac/*), but I can't find any definitive > information as to what a network driver shutdown callback is supposed to do. > I also don't know what testcase I should use to verify that my function is > working. A good test case for exercising a .shutdown() function is kexec'ing a new kernel for instance. > > I see only four instances of a platform_driver.shutdown function in > drivers/net/ethernet: > > $ git grep -A 20 -w platform_driver | grep '\.shutdown' > apm/xgene-v2/main.c- .shutdown = xge_shutdown, > apm/xgene/xgene_enet_main.c- .shutdown = xgene_enet_shutdown, > marvell/mv643xx_eth.c-.shutdown = mv643xx_eth_shutdown, > marvell/pxa168_eth.c- .shutdown = pxa168_eth_shutdown, > > (Other shutdown functions are for pci_driver.shutdown). > > For the xgene drivers, the shutdown function just calls the 'remove' > function. Isn't that overkill? Why bother with a shutdown function if it's > just the same thing as removing the driver outright? Yes, that appears unnecessary. > > mv643xx_eth_shutdown() seems more reasonable. All it does is halt the TX > and RX queues. > > pxa168_eth_shutdown() is a little more heavyweight: halts the queues, and > stops the DMA and calls phy_stop(). > > Can anyone help me figure out what my driver really should do? You should put your HW in a state where it won't be doing DMA, or have any adverse side effects to the system, putting it in a low power state is also a good approach. -- Florian
Requirements for a shutdown function?
I'm trying to add a platform_driver.shutdown function to my Ethernet driver (drivers/net/ethernet/qualcomm/emac/*), but I can't find any definitive information as to what a network driver shutdown callback is supposed to do. I also don't know what testcase I should use to verify that my function is working. I see only four instances of a platform_driver.shutdown function in drivers/net/ethernet: $ git grep -A 20 -w platform_driver | grep '\.shutdown' apm/xgene-v2/main.c-.shutdown = xge_shutdown, apm/xgene/xgene_enet_main.c-.shutdown = xgene_enet_shutdown, marvell/mv643xx_eth.c- .shutdown = mv643xx_eth_shutdown, marvell/pxa168_eth.c- .shutdown = pxa168_eth_shutdown, (Other shutdown functions are for pci_driver.shutdown). For the xgene drivers, the shutdown function just calls the 'remove' function. Isn't that overkill? Why bother with a shutdown function if it's just the same thing as removing the driver outright? mv643xx_eth_shutdown() seems more reasonable. All it does is halt the TX and RX queues. pxa168_eth_shutdown() is a little more heavyweight: halts the queues, and stops the DMA and calls phy_stop(). Can anyone help me figure out what my driver really should do? -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.