Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1565342

Re: [char-misc-next] mei: simplify error handling via devres function.

Show all headers | View raw

On Tue, Jan 24, 2017 at 12:33 AM, Winkler, Tomas
<tomas.winkler@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

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

[char-misc-next] mei: simplify error handling via devres function. Tomas Winkler <tomas.winkler@intel.com> - 2017-01-20 17:30 +0100
  Re: [char-misc-next] mei: simplify error handling via devres  function. Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-20 17:40 +0100
  Re: [char-misc-next] mei: simplify error handling via devres function. Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-01-20 23:10 +0100
    RE: [char-misc-next] mei: simplify error handling via devres  function. "Winkler, Tomas" <tomas.winkler@intel.com> - 2017-01-21 11:20 +0100
      Re: [char-misc-next] mei: simplify error handling via devres  function. Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-23 15:50 +0100
        RE: [char-misc-next] mei: simplify error handling via devres  function. "Winkler, Tomas" <tomas.winkler@intel.com> - 2017-01-23 23:40 +0100
          Re: [char-misc-next] mei: simplify error handling via devres function. Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-01-23 23:50 +0100

csiph-web