| Message ID | 1444399896-78599-1-git-send-email-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Fri, Oct 09, 2015 at 05:11:32PM +0300, Andy Shevchenko wrote: > The error handling is broken right now since it leaves resources unfreed. > Convert the code to use managed resources to fix the error handling. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Thanks Andriy, Please always explicitly Cc LKML. Is this hardware you able to test explicitly? > --- > drivers/platform/x86/intel_scu_ipc.c | 26 ++++++++------------------ > 1 file changed, 8 insertions(+), 18 deletions(-) > > diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c > index 187d108..7148535 100644 > --- a/drivers/platform/x86/intel_scu_ipc.c > +++ b/drivers/platform/x86/intel_scu_ipc.c ... > - err = pci_request_regions(dev, "intel_scu_ipc"); > + err = pcim_iomap_regions(dev, 1 << 0, pci_name(dev)); > if (err) > return err; > > - base = pci_resource_start(dev, 0); > - if (!base) > - return -ENOMEM; > - > init_completion(&ipcdev.cmd_complete); > > - if (request_irq(dev->irq, ioc, 0, "intel_scu_ipc", &ipcdev)) > - return -EBUSY; > + err = devm_request_irq(&dev->dev, dev->irq, ioc, 0, "intel_scu_ipc", You switched to using pci_name(dev) above, seems to me the same rationale should apply here. Any reason not to use pci_name(dev) here instead of "intel_scu_ipc"? Thanks,
On Sat, 2015-10-10 at 21:21 -0700, Darren Hart wrote: > On Fri, Oct 09, 2015 at 05:11:32PM +0300, Andy Shevchenko wrote: > > The error handling is broken right now since it leaves resources > > unfreed. > > Convert the code to use managed resources to fix the error > > handling. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Thanks Andriy, > > Please always explicitly Cc LKML. > > Is this hardware you able to test explicitly? Yes, the patch series has been tested on Intel Medfield and Intel Edison (Merrifield) boards. > > > --- > > drivers/platform/x86/intel_scu_ipc.c | 26 ++++++++---------------- > > -- > > 1 file changed, 8 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/platform/x86/intel_scu_ipc.c > > b/drivers/platform/x86/intel_scu_ipc.c > > index 187d108..7148535 100644 > > --- a/drivers/platform/x86/intel_scu_ipc.c > > +++ b/drivers/platform/x86/intel_scu_ipc.c > > ... > > > - err = pci_request_regions(dev, "intel_scu_ipc"); > > + err = pcim_iomap_regions(dev, 1 << 0, pci_name(dev)); > > if (err) > > return err; > > > > - base = pci_resource_start(dev, 0); > > - if (!base) > > - return -ENOMEM; > > - > > init_completion(&ipcdev.cmd_complete); > > > > - if (request_irq(dev->irq, ioc, 0, "intel_scu_ipc", > > &ipcdev)) > > - return -EBUSY; > > + err = devm_request_irq(&dev->dev, dev->irq, ioc, 0, > > "intel_scu_ipc", > > You switched to using pci_name(dev) above, seems to me the same > rationale should > apply here. Any reason not to use pci_name(dev) here instead of > "intel_scu_ipc"? > > Thanks, >
diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c index 187d108..7148535 100644 --- a/drivers/platform/x86/intel_scu_ipc.c +++ b/drivers/platform/x86/intel_scu_ipc.c @@ -563,7 +563,6 @@ static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id) { int err; struct intel_scu_ipc_pdata_t *pdata; - resource_size_t base; if (ipcdev.pdev) /* We support only one SCU */ return -EBUSY; @@ -573,32 +572,26 @@ static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id) ipcdev.pdev = pci_dev_get(dev); ipcdev.irq_mode = pdata->irq_mode; - err = pci_enable_device(dev); + err = pcim_enable_device(dev); if (err) return err; - err = pci_request_regions(dev, "intel_scu_ipc"); + err = pcim_iomap_regions(dev, 1 << 0, pci_name(dev)); if (err) return err; - base = pci_resource_start(dev, 0); - if (!base) - return -ENOMEM; - init_completion(&ipcdev.cmd_complete); - if (request_irq(dev->irq, ioc, 0, "intel_scu_ipc", &ipcdev)) - return -EBUSY; + err = devm_request_irq(&dev->dev, dev->irq, ioc, 0, "intel_scu_ipc", + &ipcdev); + if (err) + return err; - ipcdev.ipc_base = ioremap_nocache(base, pci_resource_len(dev, 0)); - if (!ipcdev.ipc_base) - return -ENOMEM; + ipcdev.ipc_base = pcim_iomap_table(dev)[0]; ipcdev.i2c_base = ioremap_nocache(pdata->i2c_base, pdata->i2c_len); - if (!ipcdev.i2c_base) { - iounmap(ipcdev.ipc_base); + if (!ipcdev.i2c_base) return -ENOMEM; - } intel_scu_devices_create(); @@ -617,10 +610,7 @@ static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id) */ static void ipc_remove(struct pci_dev *pdev) { - free_irq(pdev->irq, &ipcdev); - pci_release_regions(pdev); pci_dev_put(ipcdev.pdev); - iounmap(ipcdev.ipc_base); iounmap(ipcdev.i2c_base); ipcdev.pdev = NULL; intel_scu_devices_destroy();
The error handling is broken right now since it leaves resources unfreed. Convert the code to use managed resources to fix the error handling. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/platform/x86/intel_scu_ipc.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-)