<div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jul 8, 2020, 2:12 AM Bjorn Helgaas <<a href="mailto:helgaas@kernel.org">helgaas@kernel.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri, Jul 03, 2020 at 01:44:26PM +0530, Vaibhav Gupta wrote:<br>
> While upgrading ide_pci_suspend() and ide_pci_resume(), all other source<br>
> files, using same callbacks, were also updated except<br>
> drivers/ide/triflex.c. This is because the driver does not want to power<br>
> off the device during suspend. A quirk was required for the same.<br>
> <br>
> This patch provides the fix. Another driver,<br>
> drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c, makes use of<br>
> a quirk for similar goal. Hence a similar quirk has been applied for<br>
> triflex.<br>
> <br>
> Compile-tested only.<br>
> <br>
> Signed-off-by: Vaibhav Gupta <<a href="mailto:vaibhavgupta40@gmail.com" target="_blank" rel="noreferrer">vaibhavgupta40@gmail.com</a>><br>
> ---<br>
>  drivers/ide/triflex.c | 45 +++++++++++--------------------------------<br>
>  1 file changed, 11 insertions(+), 34 deletions(-)<br>
> <br>
> diff --git a/drivers/ide/triflex.c b/drivers/ide/triflex.c<br>
> index 1886bbfb9e5d..f707f11c296d 100644<br>
> --- a/drivers/ide/triflex.c<br>
> +++ b/drivers/ide/triflex.c<br>
> @@ -100,48 +100,25 @@ static const struct pci_device_id triflex_pci_tbl[] = {<br>
>  };<br>
>  MODULE_DEVICE_TABLE(pci, triflex_pci_tbl);<br>
>  <br>
> -#ifdef CONFIG_PM<br>
> -static int triflex_ide_pci_suspend(struct pci_dev *dev, pm_message_t state)<br>
> -{<br>
> -     /*<br>
> -      * We must not disable or powerdown the device.<br>
> -      * APM bios refuses to suspend if IDE is not accessible.<br>
> -      */<br>
> -     pci_save_state(dev);<br>
> -     return 0;<br>
> -}<br>
> -<br>
> -static int triflex_ide_pci_resume(struct pci_dev *dev)<br>
> +/*<br>
> + * We must not disable or powerdown the device.<br>
> + * APM bios refuses to suspend if IDE is not accessible.<br>
> + */<br>
> +static void triflex_pci_pm_cap_fixup(struct pci_dev *pdev)<br>
>  {<br>
> -     struct ide_host *host = pci_get_drvdata(dev);<br>
> -     int rc;<br>
> -<br>
> -     pci_set_power_state(dev, PCI_D0);<br>
> -<br>
> -     rc = pci_enable_device(dev);<br>
> -     if (rc)<br>
> -             return rc;<br>
> -<br>
> -     pci_restore_state(dev);<br>
> -     pci_set_master(dev);<br>
> -<br>
> -     if (host->init_chipset)<br>
> -             host->init_chipset(dev);<br>
> -<br>
> -     return 0;<br>
> +     dev_info(&pdev->dev, "Disable triflex to be turned off by PCI CORE\n");<br>
<br>
I would change this message to "Disabling PCI power management" to be<br>
more like existing messages:<br>
<br>
  "PM disabled\n"<br>
  "Disabling PCI power management to avoid bug\n"<br>
  "Disabling PCI power management on camera ISP\n"<br>
<br>
> +     pdev->pm_cap = 0;<br>
>  }<br>
> -#else<br>
> -#define triflex_ide_pci_suspend NULL<br>
> -#define triflex_ide_pci_resume NULL<br>
> -#endif<br>
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_COMPAQ,<br>
> +                     PCI_DEVICE_ID_COMPAQ_TRIFLEX_IDE,<br>
> +                     triflex_pci_pm_cap_fixup);<br>
<br>
I don't think this needs to be a fixup.  This could be done in the<br>
probe routine (triflex_init_one()).<br>
<br>
Doing it as a fixup means the PCI core will check every PCI device<br>
to see if it matches PCI_DEVICE_ID_COMPAQ_TRIFLEX_IDE, which is a<br>
little extra useless overhead and quirks are a little bit magic<br>
because it's not as obvious how they're called.<br>
<br>
But since triflex_init_one() is called only for the devices we care<br>
about, you can just do:<br>
<br>
  static int triflex_init_one(struct pci_dev *dev, const struct pci_device_id *id)<br>
  {<br>
        dev->pm_cap = 0;<br>
        dev_info(...);<br>
        return ide_pci_init_one(dev, &triflex_device, NULL);<br>
  }<br></blockquote></div></div><div dir="auto">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</div><div dir="auto">in [Patch 1/1] along with other ide drivers.</div><div dir="auto"><br></div><div dir="auto">--Vaibhav Gupta</div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
>  static struct pci_driver triflex_pci_driver = {<br>
>       .name           = "TRIFLEX_IDE",<br>
>       .id_table       = triflex_pci_tbl,<br>
>       .probe          = triflex_init_one,<br>
>       .remove         = ide_pci_remove,<br>
> -     .suspend        = triflex_ide_pci_suspend,<br>
> -     .resume         = triflex_ide_pci_resume,<br>
> +     .<a href="http://driver.pm" rel="noreferrer noreferrer" target="_blank">driver.pm</a>      = &ide_pci_pm_ops,<br>
>  };<br>
>  <br>
>  static int __init triflex_ide_init(void)<br>
> -- <br>
> 2.27.0<br>
> <br>
</blockquote></div></div></div>