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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 24 Jan 2017 00:40:21 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     "Winkler, Tomas" <tomas.winkler@...el.com>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Usyskin, Alexander" <alexander.usyskin@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [char-misc-next] mei: simplify error handling via devres function.

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