linux-kernel-mentees.lists.linuxfoundation.org archive mirror
 help / color / mirror / Atom feed
From: Dhiraj Sharma <dhiraj.sharma0024@gmail.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Vaibhav Gupta <vaibhavgupta40@gmail.com>,
	linux-kernel@vger.kernel.org, linux-ide@vger.kernel.org,
	Bjorn Helgaas <bhelgaas@google.com>,
	linux-kernel-mentees@lists.linuxfoundation.org,
	"David S. Miller" <davem@davemloft.net>
Subject: Re: [Linux-kernel-mentees] [PATCH v2 2/4] ide: triflex: use generic power management
Date: Wed, 8 Jul 2020 02:19:19 +0530	[thread overview]
Message-ID: <CAPRy4h2FB3rQ5pni+XaceMtBDyHDxADfj00ZZCWXcdpv1ozPTg@mail.gmail.com> (raw)
In-Reply-To: <20200707204201.GA382915@bjorn-Precision-5520>


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

Hello all,

I have applied to Linux Mentorship on community bridge.

May I know further updates on whether passed Skill evaluation and when can
i start with application contribution.


Hoping for a positive response
Dhiraj Sharma

On Jul 8, 2020 02:12, "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);
>   }
>
> >  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
> >
> _______________________________________________
> Linux-kernel-mentees mailing list
> Linux-kernel-mentees@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
>

[-- Attachment #1.2: Type: text/html, Size: 6548 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

  reply	other threads:[~2020-07-07 20:49 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-03  8:14 [Linux-kernel-mentees] [PATCH v2 0/4] drivers: ide: use generic power management Vaibhav Gupta
2020-07-03  8:14 ` [Linux-kernel-mentees] [PATCH v2 1/4] " Vaibhav Gupta
2020-07-03  8:14 ` [Linux-kernel-mentees] [PATCH v2 2/4] ide: triflex: " Vaibhav Gupta
2020-07-07 20:42   ` Bjorn Helgaas
2020-07-07 20:49     ` Dhiraj Sharma [this message]
2020-07-07 20:53     ` Vaibhav Gupta
2020-07-07 21:15       ` Bjorn Helgaas
2020-07-03  8:14 ` [Linux-kernel-mentees] [PATCH v2 3/4] ide: sc1200: " Vaibhav Gupta
2020-07-03  8:14 ` [Linux-kernel-mentees] [PATCH v2 4/4] ide: delkin_cb: " Vaibhav Gupta

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=CAPRy4h2FB3rQ5pni+XaceMtBDyHDxADfj00ZZCWXcdpv1ozPTg@mail.gmail.com \
    --to=dhiraj.sharma0024@gmail.com \
    --cc=bhelgaas@google.com \
    --cc=davem@davemloft.net \
    --cc=helgaas@kernel.org \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-kernel-mentees@lists.linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).