Linux-ide Archive on lore.kernel.org
 help / color / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: Vaibhav Gupta <vaibhav.varodek@gmail.com>
Cc: Vaibhav Gupta <vaibhavgupta40@gmail.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	bjorn@helgaas.com, "David S. Miller" <davem@davemloft.net>,
	linux-kernel@vger.kernel.org,
	linux-kernel-mentees@lists.linuxfoundation.org,
	skhan@linuxfoundation.org, linux-ide@vger.kernel.org
Subject: Re: [PATCH v2 2/4] ide: triflex: use generic power management
Date: Tue, 7 Jul 2020 16:15:16 -0500
Message-ID: <20200707211516.GA385934@bjorn-Precision-5520> (raw)
In-Reply-To: <CAPBsFfCdJkXO-V16yO2eTeAwnQnKJZ49yaJepbsF2dNZjLZFhw@mail.gmail.com>

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

       reply index

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CAPBsFfCdJkXO-V16yO2eTeAwnQnKJZ49yaJepbsF2dNZjLZFhw@mail.gmail.com>
2020-07-07 21:15 ` Bjorn Helgaas [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200707211516.GA385934@bjorn-Precision-5520 \
    --to=helgaas@kernel.org \
    --cc=bhelgaas@google.com \
    --cc=bjorn@helgaas.com \
    --cc=davem@davemloft.net \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-kernel-mentees@lists.linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=skhan@linuxfoundation.org \
    --cc=vaibhav.varodek@gmail.com \
    --cc=vaibhavgupta40@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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