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
| ||
|
Date: Sat, 21 Jan 2017 10:12:24 +0000 From: "Winkler, Tomas" <tomas.winkler@...el.com> To: 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>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com> Subject: RE: [char-misc-next] mei: simplify error handling via devres function. > > On Fri, Jan 20, 2017 at 7:22 PM, Tomas Winkler <tomas.winkler@...el.com> > wrote: > > Use devm_ and pcim_ functions to make error handling simpler and code > > smaller and tidier. > > > > Based on original patch by > > mei: me: use managed functions pcim_* and devm_* Andy Shevchenko > > <andriy.shevchenko@...ux.intel.com> > > https://lkml.org/lkml/2016/2/1/339 > > Perhaps you may leave SoB. > But let me review the code. No problem, though we also had some internal code before, you were just first to send it out ;) > > > Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com> > > Signed-off-by: Tomas Winkler <tomas.winkler@...el.com> > > Signed-off-by: Alexander Usyskin <alexander.usyskin@...el.com> > > > - * Return: The mei_device_device pointer on success, NULL on failure. > > + * Return: The mei_device pointer on success, NULL on failure. > > This is not related. > > > -struct mei_device *mei_me_dev_init(struct pci_dev *pdev, > > - const struct mei_cfg *cfg) > > +struct mei_device *devm_mei_me_init(struct pci_dev *pdev, > > + const struct mei_cfg *cfg) > > It's not exactly in devm_ namespace. (They have no corresponding > devm_*_fini, etc.) Better to leave same name in spite of calling devm_* inside. Okay, got it. > > > -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. > > > static void mei_me_remove(struct pci_dev *pdev) { > > struct mei_device *dev; > > - struct mei_me_hw *hw; > > > > dev = pci_get_drvdata(pdev); > > if (!dev) > > @@ -276,33 +260,19 @@ static void mei_me_remove(struct pci_dev *pdev) > > if (mei_pg_is_enabled(dev)) > > pm_runtime_get_noresume(&pdev->dev); > > > + pci_set_drvdata(pdev, NULL); > > Ditto. > > > - 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. > > > 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. > > > + 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. > > end: > > > + pci_set_drvdata(pdev, NULL); > > Redundant. > > > static void mei_txe_remove(struct pci_dev *pdev) { > > > + pci_set_drvdata(pdev, NULL); > > Ditto. > > > @@ -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 Tomas
Powered by blists - more mailing lists