On Tue, Jan 24, 2017 at 12:33 AM, Winkler, Tomas <tomas.wink...@intel.com> wrote: >> On Sat, 2017-01-21 at 10:12 +0000, Winkler, Tomas wrote:
>> > > > - free_irq(pdev->irq, dev); >> > > > + devm_free_irq(&pdev->dev, pdev->irq, dev); >> > > > pci_disable_msi(pdev); >> > > >> > > All three not needed >> > >> > I believe we need it on suspend as we are going over irq request >> > again in resume. Please provide more info you if you still insist. >> >> Ah, sorry, I missed that these are suspend/resume hooks. >> >> So, Can you elaborate a bit why you need to disable interrupts during system >> suspend? >> >> (Basically in this case better not to use devm_request_*irq() at all) > > MEI is used for manageability so the device might be alive also in S3 on some > platforms, > anyhow this might be reviewed more as we do disable interrupts explicitly on > suspend. > So far the current code has passed suspend/resume stress tests. OK, so, I would recommend to use old variant with plain request_threaded_irq() / free_irq(). Those (IRQ related) functions somehow mistakenly got devm_*() variation. It's not first time where devm_*irq() is inconvenient in some ways. >> Use reversed tree for definition block. >> >> The longest lines with the assignment = first; Then lines without assignment; >> Then return code variable; >> >> Flags for spin_lock -- depends. > > I haven't seen this rule in coding style doc, this is the first I'm seeing > such request. > W/o offence I prefer the current style. Yes, this is matter of style -- your choice. >> > > > @@ -256,7 +210,7 @@ static int mei_txe_pci_suspend(struct device >> > > > *device) >> > > > - free_irq(pdev->irq, dev); >> > > > + devm_free_irq(&pdev->dev, pdev->irq, dev); >> > > > pci_disable_msi(pdev); >> > > >> > > All are redundant. > Thanks for the review, will post v2, tomorrow as this will requires some more > stress testing Take your time. -- With Best Regards, Andy Shevchenko
