* Re: [PATCH v2 2/4] ide: triflex: use generic power management [not found] <CAPBsFfCdJkXO-V16yO2eTeAwnQnKJZ49yaJepbsF2dNZjLZFhw@mail.gmail.com> @ 2020-07-07 21:15 ` Bjorn Helgaas 0 siblings, 0 replies; 3+ messages in thread From: Bjorn Helgaas @ 2020-07-07 21:15 UTC (permalink / raw) To: Vaibhav Gupta Cc: Vaibhav Gupta, Bjorn Helgaas, bjorn, David S. Miller, linux-kernel, linux-kernel-mentees, skhan, linux-ide On Wed, Jul 08, 2020 at 02:23:22AM +0530, Vaibhav Gupta wrote: > On Wed, Jul 8, 2020, 2:12 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Jul 03, 2020 at 01:44:26PM +0530, Vaibhav Gupta wrote: > > > -static int triflex_ide_pci_resume(struct pci_dev *dev) > > > +/* > > > + * We must not disable or powerdown the device. > > > + * APM bios refuses to suspend if IDE is not accessible. > > > + */ > > > +static void triflex_pci_pm_cap_fixup(struct pci_dev *pdev) > > > { > > > - struct ide_host *host = pci_get_drvdata(dev); > > > - int rc; > > > - > > > - pci_set_power_state(dev, PCI_D0); > > > - > > > - rc = pci_enable_device(dev); > > > - if (rc) > > > - return rc; > > > - > > > - pci_restore_state(dev); > > > - pci_set_master(dev); > > > - > > > - if (host->init_chipset) > > > - host->init_chipset(dev); > > > - > > > - return 0; > > > + dev_info(&pdev->dev, "Disable triflex to be turned off by PCI > > CORE\n"); > > > > I would change this message to "Disabling PCI power management" to be > > more like existing messages: > > > > "PM disabled\n" > > "Disabling PCI power management to avoid bug\n" > > "Disabling PCI power management on camera ISP\n" > > > > > + pdev->pm_cap = 0; > > > } > > > -#else > > > -#define triflex_ide_pci_suspend NULL > > > -#define triflex_ide_pci_resume NULL > > > -#endif > > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_COMPAQ, > > > + PCI_DEVICE_ID_COMPAQ_TRIFLEX_IDE, > > > + triflex_pci_pm_cap_fixup); > > > > I don't think this needs to be a fixup. This could be done in the > > probe routine (triflex_init_one()). > > > > Doing it as a fixup means the PCI core will check every PCI device > > to see if it matches PCI_DEVICE_ID_COMPAQ_TRIFLEX_IDE, which is a > > little extra useless overhead and quirks are a little bit magic > > because it's not as obvious how they're called. > > > > But since triflex_init_one() is called only for the devices we care > > about, you can just do: > > > > static int triflex_init_one(struct pci_dev *dev, const struct > > pci_device_id *id) > > { > > dev->pm_cap = 0; > > dev_info(...); > > return ide_pci_init_one(dev, &triflex_device, NULL); > > } > > > Seems a better approach. And in that case I won't need this extra patch > just for triflex. I can put dev->pm_cap=0 change > in [Patch 1/1] along with other ide drivers. Hmm, I just noticed that there are actually *two* drivers (triflex.c and pata_triflex.c) for this same device, and they both have this comment about "APM BIOS refusing to suspend if IDE is not accessible" and therefore preventing suspend of IDE. At least, the comment seems to imply that calling pci_save_state() is a way to prevent suspend of the device. That seems like a strange way to do it, but ... Anyway, I wonder if a single quirk in drivers/pci/quirks.c would be better. A preliminary patch could add a quirk (keeping the comment) and remove the pci_save_state() from both triflex_ide_pci_suspend() and triflex_ata_pci_device_suspend(). Then you could proceed with these generic PM changes on top of that. Maybe make the dev_info() mention that you're disabling PM to avoid an APM BIOS suspend defect so users have a clue about why. Bjorn ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH v2 0/4] drivers: ide: use generic power management @ 2020-07-03 8:14 Vaibhav Gupta 2020-07-03 8:14 ` [PATCH v2 2/4] ide: triflex: " Vaibhav Gupta 0 siblings, 1 reply; 3+ messages in thread From: Vaibhav Gupta @ 2020-07-03 8:14 UTC (permalink / raw) To: Bjorn Helgaas, Bjorn Helgaas, bjorn, Vaibhav Gupta, David S. Miller Cc: Vaibhav Gupta, linux-kernel, linux-kernel-mentees, skhan, linux-ide Linux Kernel Mentee: Remove Legacy Power Management. The purpose of this patch series is to remove legacy power management callbacks from ide drivers. The suspend() and resume() callbacks operations are still invoking pci_save/restore_state(), pci_set_power_state(), pci_enable/disable_state(), etc. and handling the power management themselves, which is not recommended. The conversion requires the removal of the those function calls and change the callback definition accordingly and make use of dev_pm_ops structure. All patches are compile-tested only. V2: Kbuild had modpost error for undefined reference in v1. Testing by: Compiler: gcc (GCC) 10.1.0 Build: make -j$(nproc) W=1 all Vaibhav Gupta (4): ide: use generic power management ide: triflex: use generic power management ide: sc1200: use generic power management ide: delkin_cb: use generic power management drivers/ide/aec62xx.c | 3 +-- drivers/ide/alim15x3.c | 3 +-- drivers/ide/amd74xx.c | 3 +-- drivers/ide/atiixp.c | 3 +-- drivers/ide/cmd64x.c | 3 +-- drivers/ide/cs5520.c | 3 +-- drivers/ide/cs5530.c | 3 +-- drivers/ide/cs5535.c | 3 +-- drivers/ide/cs5536.c | 3 +-- drivers/ide/cy82c693.c | 3 +-- drivers/ide/delkin_cb.c | 30 +++++------------------- drivers/ide/hpt366.c | 3 +-- drivers/ide/ide-pci-generic.c | 3 +-- drivers/ide/it8172.c | 3 +-- drivers/ide/it8213.c | 3 +-- drivers/ide/it821x.c | 3 +-- drivers/ide/jmicron.c | 3 +-- drivers/ide/ns87415.c | 3 +-- drivers/ide/opti621.c | 3 +-- drivers/ide/pdc202xx_new.c | 3 +-- drivers/ide/pdc202xx_old.c | 3 +-- drivers/ide/piix.c | 3 +-- drivers/ide/sc1200.c | 43 ++++++++++++----------------------- drivers/ide/serverworks.c | 3 +-- drivers/ide/setup-pci.c | 29 +++++------------------ drivers/ide/siimage.c | 3 +-- drivers/ide/sis5513.c | 3 +-- drivers/ide/sl82c105.c | 3 +-- drivers/ide/slc90e66.c | 3 +-- drivers/ide/triflex.c | 24 +++++++++---------- drivers/ide/via82cxxx.c | 3 +-- include/linux/ide.h | 8 +------ 32 files changed, 65 insertions(+), 150 deletions(-) -- 2.27.0 ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH v2 2/4] ide: triflex: use generic power management 2020-07-03 8:14 [PATCH v2 0/4] drivers: ide: " Vaibhav Gupta @ 2020-07-03 8:14 ` Vaibhav Gupta 2020-07-07 20:42 ` Bjorn Helgaas 0 siblings, 1 reply; 3+ messages in thread From: Vaibhav Gupta @ 2020-07-03 8:14 UTC (permalink / raw) To: Bjorn Helgaas, Bjorn Helgaas, bjorn, Vaibhav Gupta, David S. Miller Cc: Vaibhav Gupta, linux-kernel, linux-kernel-mentees, skhan, linux-ide While upgrading ide_pci_suspend() and ide_pci_resume(), all other source files, using same callbacks, were also updated except drivers/ide/triflex.c. This is because the driver does not want to power off the device during suspend. A quirk was required for the same. This patch provides the fix. Another driver, drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c, makes use of a quirk for similar goal. Hence a similar quirk has been applied for triflex. Compile-tested only. Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> --- drivers/ide/triflex.c | 45 +++++++++++-------------------------------- 1 file changed, 11 insertions(+), 34 deletions(-) diff --git a/drivers/ide/triflex.c b/drivers/ide/triflex.c index 1886bbfb9e5d..f707f11c296d 100644 --- a/drivers/ide/triflex.c +++ b/drivers/ide/triflex.c @@ -100,48 +100,25 @@ static const struct pci_device_id triflex_pci_tbl[] = { }; MODULE_DEVICE_TABLE(pci, triflex_pci_tbl); -#ifdef CONFIG_PM -static int triflex_ide_pci_suspend(struct pci_dev *dev, pm_message_t state) -{ - /* - * We must not disable or powerdown the device. - * APM bios refuses to suspend if IDE is not accessible. - */ - pci_save_state(dev); - return 0; -} - -static int triflex_ide_pci_resume(struct pci_dev *dev) +/* + * We must not disable or powerdown the device. + * APM bios refuses to suspend if IDE is not accessible. + */ +static void triflex_pci_pm_cap_fixup(struct pci_dev *pdev) { - struct ide_host *host = pci_get_drvdata(dev); - int rc; - - pci_set_power_state(dev, PCI_D0); - - rc = pci_enable_device(dev); - if (rc) - return rc; - - pci_restore_state(dev); - pci_set_master(dev); - - if (host->init_chipset) - host->init_chipset(dev); - - return 0; + dev_info(&pdev->dev, "Disable triflex to be turned off by PCI CORE\n"); + pdev->pm_cap = 0; } -#else -#define triflex_ide_pci_suspend NULL -#define triflex_ide_pci_resume NULL -#endif +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_COMPAQ, + PCI_DEVICE_ID_COMPAQ_TRIFLEX_IDE, + triflex_pci_pm_cap_fixup); static struct pci_driver triflex_pci_driver = { .name = "TRIFLEX_IDE", .id_table = triflex_pci_tbl, .probe = triflex_init_one, .remove = ide_pci_remove, - .suspend = triflex_ide_pci_suspend, - .resume = triflex_ide_pci_resume, + .driver.pm = &ide_pci_pm_ops, }; static int __init triflex_ide_init(void) -- 2.27.0 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2 2/4] ide: triflex: use generic power management 2020-07-03 8:14 ` [PATCH v2 2/4] ide: triflex: " Vaibhav Gupta @ 2020-07-07 20:42 ` Bjorn Helgaas 0 siblings, 0 replies; 3+ messages in thread From: Bjorn Helgaas @ 2020-07-07 20:42 UTC (permalink / raw) To: Vaibhav Gupta Cc: Bjorn Helgaas, bjorn, Vaibhav Gupta, David S. Miller, linux-kernel, linux-kernel-mentees, skhan, linux-ide On Fri, Jul 03, 2020 at 01:44:26PM +0530, Vaibhav Gupta wrote: > While upgrading ide_pci_suspend() and ide_pci_resume(), all other source > files, using same callbacks, were also updated except > drivers/ide/triflex.c. This is because the driver does not want to power > off the device during suspend. A quirk was required for the same. > > This patch provides the fix. Another driver, > drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c, makes use of > a quirk for similar goal. Hence a similar quirk has been applied for > triflex. > > Compile-tested only. > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> > --- > drivers/ide/triflex.c | 45 +++++++++++-------------------------------- > 1 file changed, 11 insertions(+), 34 deletions(-) > > diff --git a/drivers/ide/triflex.c b/drivers/ide/triflex.c > index 1886bbfb9e5d..f707f11c296d 100644 > --- a/drivers/ide/triflex.c > +++ b/drivers/ide/triflex.c > @@ -100,48 +100,25 @@ static const struct pci_device_id triflex_pci_tbl[] = { > }; > MODULE_DEVICE_TABLE(pci, triflex_pci_tbl); > > -#ifdef CONFIG_PM > -static int triflex_ide_pci_suspend(struct pci_dev *dev, pm_message_t state) > -{ > - /* > - * We must not disable or powerdown the device. > - * APM bios refuses to suspend if IDE is not accessible. > - */ > - pci_save_state(dev); > - return 0; > -} > - > -static int triflex_ide_pci_resume(struct pci_dev *dev) > +/* > + * We must not disable or powerdown the device. > + * APM bios refuses to suspend if IDE is not accessible. > + */ > +static void triflex_pci_pm_cap_fixup(struct pci_dev *pdev) > { > - struct ide_host *host = pci_get_drvdata(dev); > - int rc; > - > - pci_set_power_state(dev, PCI_D0); > - > - rc = pci_enable_device(dev); > - if (rc) > - return rc; > - > - pci_restore_state(dev); > - pci_set_master(dev); > - > - if (host->init_chipset) > - host->init_chipset(dev); > - > - return 0; > + dev_info(&pdev->dev, "Disable triflex to be turned off by PCI CORE\n"); I would change this message to "Disabling PCI power management" to be more like existing messages: "PM disabled\n" "Disabling PCI power management to avoid bug\n" "Disabling PCI power management on camera ISP\n" > + pdev->pm_cap = 0; > } > -#else > -#define triflex_ide_pci_suspend NULL > -#define triflex_ide_pci_resume NULL > -#endif > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_COMPAQ, > + PCI_DEVICE_ID_COMPAQ_TRIFLEX_IDE, > + triflex_pci_pm_cap_fixup); I don't think this needs to be a fixup. This could be done in the probe routine (triflex_init_one()). Doing it as a fixup means the PCI core will check every PCI device to see if it matches PCI_DEVICE_ID_COMPAQ_TRIFLEX_IDE, which is a little extra useless overhead and quirks are a little bit magic because it's not as obvious how they're called. But since triflex_init_one() is called only for the devices we care about, you can just do: static int triflex_init_one(struct pci_dev *dev, const struct pci_device_id *id) { dev->pm_cap = 0; dev_info(...); return ide_pci_init_one(dev, &triflex_device, NULL); } > static struct pci_driver triflex_pci_driver = { > .name = "TRIFLEX_IDE", > .id_table = triflex_pci_tbl, > .probe = triflex_init_one, > .remove = ide_pci_remove, > - .suspend = triflex_ide_pci_suspend, > - .resume = triflex_ide_pci_resume, > + .driver.pm = &ide_pci_pm_ops, > }; > > static int __init triflex_ide_init(void) > -- > 2.27.0 > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-07-07 21:15 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CAPBsFfCdJkXO-V16yO2eTeAwnQnKJZ49yaJepbsF2dNZjLZFhw@mail.gmail.com> 2020-07-07 21:15 ` [PATCH v2 2/4] ide: triflex: use generic power management Bjorn Helgaas 2020-07-03 8:14 [PATCH v2 0/4] drivers: ide: " Vaibhav Gupta 2020-07-03 8:14 ` [PATCH v2 2/4] ide: triflex: " Vaibhav Gupta 2020-07-07 20:42 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).