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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 23 Jan 2017 22:33:36 +0000
From:   "Winkler, Tomas" <tomas.winkler@...el.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>
CC:     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 Sat, 2017-01-21 at 10:12 +0000, Winkler, Tomas wrote:
> 
> > >
> > > > -struct mei_device *mei_txe_dev_init(struct pci_dev *pdev)
> > > > +struct mei_device *devm_mei_txe_init(struct pci_dev *pdev)
> > >
> > > Ditto.
> > >
> > > >  end:
> > > > +       pci_set_drvdata(pdev, NULL);
> > >
> > > Not needed.
> >
> > Please explain, we rely on pci_get_drvdata() returning NULL when
> > unregistered.
> 
> PCI core will take care about this one. Actually device core for any of user of
> struct device.
> See __device_release_driver() for the details.

Okay, got it. 
> 
> > >
> > > > -       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.

> > >
> > > >         return 0;
> > > > @@ -75,22 +64,22 @@ static int mei_txe_probe(struct pci_dev *pdev,
> > > > const struct pci_device_id *ent)  {
> > > >         struct mei_device *dev;
> > > >         struct mei_txe_hw *hw;
> > > > +       const int mask = BIT(SEC_BAR) | BIT(BRIDGE_BAR);
> > >
> > > First line?
> >
> > Please be more verbose.
> 
> 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. 

> 
> > >
> > > > +       memcpy(hw->mem_addr, pcim_iomap_table(pdev),
> > > > +sizeof(hw->mem_addr));
> > >
> > > Why?
> > > It is kept by PCI core, you don't need a copy.
> >
> > There is no simple accessor for that, it's easier to copy the two
> > dwords then going over the function calls.
> 
> I'm not sure you need a copy. That function call just return the pointer to the
> table.
> 
> I remember 8250_pci used to have similar approach, now it's using whatever is
> kept by PCI core.
> 
> It's less error prone.

Yep, we  definitely can use a pointer here . 

> 
> > > > @@ -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.
> 
> Yeah, same clarification as for above case with system sleep.
> 

Thanks for the review, will post v2, tomorrow as this will requires some more stress testing
Thanks
Tomas