Re: [Linux-kernel-mentees] [PATCH v2 2/4] ide: triflex: use generic power management

[Vaibhav Gupta <vaibhav.varodek@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4124 bytes --]

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:
> > 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);
>   }
>
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.

--Vaibhav Gupta

>
> >  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
> >
>

[-- Attachment #1.2: Type: text/html, Size: 5684 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees