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

[Andy Shevchenko]

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