Re: [PATCH v2 2/4] ide: triflex: use generic power management

[Bjorn Helgaas <helgaas@kernel.org>]

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