[FYI, your reply was a multipart message (not a simple plain text message), so I think the mailing lists discarded it and it doesn't appear in the archives] On Thu, Jan 23, 2020 at 08:05:04PM +0530, Prabhakar Kushwaha wrote: > Thanks Bjorn for review. Reply is in-lined. > > On Thu, Jan 23, 2020 at 7:24 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > > On Thu, Jan 23, 2020 at 10:41:57AM +0000, Prabhakar Kushwaha wrote: > > > device_shutdown() called from reboot/power_shutdown expect all > > > devices to be shutdown. Same is true for ahci pci driver. > > > As no shutdown function was implemented ata subsystem remains > > > always alive and DMA/interrupt still active. > > > > > > It creates problem during kexec, here "M" bit is cleared to stop > > > DMA usage. > > > > Maybe this is obvious to AHCI folks, but I don't know what "M" bit you > > are referring to, and I don't see anything in the patch itself that > > looks like an "M" bit. I thought maybe you meant the "Bus Master > > Enable" bit (PCI_COMMAND_MASTER), but I don't see an obvious > > connection to that either. > > > I missed to explain M bit. Thanks for pointing it out. > M bit is PCI_COMMAND_MASTER bit i.e. Bus Master Enable. Please don't call it the "M" bit. Neither the Conventional PCI nor the PCIe specs call it that, and it could just as easily refer to the "Memory Space Enable" bit. Just call it the "Bus Master Enable" bit like the specs do. > There are 2 flow in kernel which calls device_shutdown() which inherently > call pci_device_shutdown()(). > a) reboot flow > b) kexec -e flow > > issue seen in kexec -e flow where hard-disk is not getting detected in > second kernel. > There is special code in pci_device_shutdown() which clear M bit during > kexec -e i.e. kexec_in_progress flag is set. > > if (kexec_in_progress && (pci_dev->current_state <= PCI_D3hot)) > pci_clear_master(pci_dev); > > There should not be any transaction after pci_clear_master() but i am > seeing filesystem sync function calls after this. > this is the reason shutdown function is added(this patch) which disable > interrupt, stop DMA and freeze SATA port before execution of > pci_clear_master() I don't see where your patch adds anything that clears Bus Master Enable. You're adding ahci_shutdown_one(), so pci_device_shutdown() will now call it, and maybe something inside ahci_shutdown_one() will clear Bus Master Enable, but I couldn't find it with a quick look. pci_device_shutdown() *does* clear it, of course, but only when kexec_in_progress, and it does that even without this patch. Just to be clear, I don't object to the patch itself -- that's an AHCI thing that I don't know much about. I'm just complaining because it's not obvious that your commit log describes what the patch actually does. ahci_shutdown_one() might indeed stop the AHCI port DMA, but I suspect it's doing that via the AHCI programming model, not by clearing Bus Master Enable. So maybe all you need to do is remove the references to BME. > After adding shutdown function, i can see hard-disk getting detected in > second kernel. > > > > Any further DMA transaction may cause instability and > > > the hard-disk may even not get detected for second kernel. > > > One of possible case is periodic file system sync. > > > > I think "may cause instability" and "disk may even not get detected" > > is too vague and hand-wavy to really add useful information to the > > commit log. > > > let me rework on this.. Thanks > > > > So defining ahci pci driver shutdown to freeze hardware (mask > > > interrupt, stop DMA engine and free DMA resources).
