Linux-ide Archive on lore.kernel.org
 help / color / Atom feed
* 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

* 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

* [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	[flat|nested] 3+ messages in thread

end of thread, back to index

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

Linux-ide Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-ide/0 linux-ide/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-ide linux-ide/ https://lore.kernel.org/linux-ide \
		linux-ide@vger.kernel.org
	public-inbox-index linux-ide

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-ide


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git