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

[Winkler, Tomas]

> 
> 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