RE: [PATCH] PCI: Do not enable async suspend for JMicron chips

[Liu, Chuansheng]

> -----Original Message-----
> From: Lu, Aaron
> Sent: Thursday, November 06, 2014 1:37 PM
> To: Liu, Chuansheng; Bjorn Helgaas
> Cc: Barto; Tejun Heo (tj@xxxxxxxxxx); Rafael Wysocki;
> linux-pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] PCI: Do not enable async suspend for JMicron chips
> 
> On 11/06/2014 01:29 PM, Liu, Chuansheng wrote:
> > Hello Bjorn,
> >
> >> -----Original Message-----
> >> From: Bjorn Helgaas [mailto:bhelgaas@xxxxxxxxxx]
> >> Sent: Thursday, November 06, 2014 12:09 PM
> >> To: Liu, Chuansheng
> >> Cc: Barto; Tejun Heo (tj@xxxxxxxxxx); Lu, Aaron; Rafael Wysocki;
> >> linux-pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> >> Subject: Re: [PATCH] PCI: Do not enable async suspend for JMicron chips
> >>
> >> On Wed, Nov 5, 2014 at 6:48 PM, Liu, Chuansheng
> >> <chuansheng.liu@xxxxxxxxx> wrote:
> >>> Hello Bjorn,
> >>>
> >>>> -----Original Message-----
> >>>> From: Bjorn Helgaas [mailto:bhelgaas@xxxxxxxxxx]
> >>>> Sent: Thursday, November 06, 2014 3:04 AM
> >>>> To: Barto
> >>>> Cc: Liu, Chuansheng; Lu, Aaron; Tejun Heo; Rafael Wysocki;
> >>>> linux-pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> >>>> Subject: Re: [PATCH] PCI: Do not enable async suspend for JMicron chips
> >>>>
> >>>> On Wed, Nov 5, 2014 at 11:46 AM, Barto <mister.freeman@xxxxxxxxxxx>
> >>>> wrote:
> >>>>> this patch solves these 2 bug reports :
> >>>>>
> >>>>> https://bugzilla.kernel.org/show_bug.cgi?id=84861
> >>>>>
> >>>>> https://bugzilla.kernel.org/show_bug.cgi?id=81551
> >>>>
> >>>> Those bugs were already mentioned.  But e6b7e41cdd8c claims to solve
> >>>> https://bugzilla.kernel.org/show_bug.cgi?id=81551, and 84861 is a
> >>>> duplicate of 81551, so it should also be fixed by e6b7e41cdd8c.
> >>>>
> >>>> So the question is, why was e6b7e41cdd8c insufficient?  Presumably it
> >>>> was tested and somebody thought it did fix the problem.
> >>>
> >>> The first patch e6b7e41cdd8c which is just exclude some of JMicron
> >> chips(363/361) out of async_suspend,
> >>> then Barto found the same issue on JMicron 368, so we need one more
> >> general patch to let JMicron chips
> >>> out of async_suspend, so we make this patch.
> >>>
> >>> Bjorn, tj,
> >>> Could you kindly take this patch? As Barto said, it effected the user
> >> experience indeed, thanks.
> >>
> >> Thanks for clarifying the changelog as far as the different chips and
> >> the different bugzillas.
> >>
> >> But you haven't addressed my concerns about (1) putting a PCI vendor
> >> ID check in the generic PCI core code, and (2) applying this to *all*
> >> JMicron devices.  You might want to explore a quirk-type solution or
> >> maybe just add the JMicron 368 to the checks added by e6b7e41cdd8c.
> > Understand your point, in fact, before this patch submitted, I had written
> another patch https://lkml.org/lkml/2014/9/24/68
> > which addressed to add the quirk-type solution in ATA code, and Aaron given
> better suggestion that implemented at pci_pm_init().
> > How do you think of it? Thanks.
> 
> I think Bjorn means that we should place the code as a fixup somewhere
> in the quirks.c. I didn't take a closer look but DECLARE_PCI_FIXUP_FINAL
> for those JMicron PCI devices seems to be a proper phase.

Thanks Aaron, then how about below patch?

diff --git a/drivers/ata/pata_jmicron.c b/drivers/ata/pata_jmicron.c
index 47e418b..9e85f86 100644
--- a/drivers/ata/pata_jmicron.c
+++ b/drivers/ata/pata_jmicron.c
@@ -158,6 +158,21 @@ static int jmicron_init_one (struct pci_dev *pdev, const struct pci_device_id *i
        return ata_pci_bmdma_init_one(pdev, ppi, &jmicron_sht, NULL, 0);
 }
 
+/*
+ * For JMicron chips, we need to disable the async_suspend method, otherwise
+ * they will hit the power-on issue when doing device resume, add one quick
+ * solution to disable the async_suspend method.
+ */
+static void pci_async_suspend_fixup(struct pci_dev *pdev)
+{
+       /*
+        * disabling the async_suspend method for JMicron chips to
+        * avoid device resuming issue.
+        */
+       device_disable_async_suspend(&pdev->dev);
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_JMICRON, PCI_ANY_ID, pci_async_suspend_fixup);
+
 static const struct pci_device_id jmicron_pci_tbl[] = {
        { PCI_VENDOR_ID_JMICRON, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
          PCI_CLASS_STORAGE_IDE << 8, 0xffff00, 0 },

Barto,
Could you have a try on your side? Thanks.