Hi, On Fri, Sep 11, 2015 at 03:32:56PM +0530, Sudip Mukherjee wrote: > On Thu, Sep 10, 2015 at 11:03:34AM -0700, David Cohen wrote: > > Hi Sudip, > > > > On Fri, Sep 04, 2015 at 05:12:23PM +0530, Sudip Mukherjee wrote: > > > We were checking if dev->regs is NULL but it was done after > > > dereferencing it. Lets reset the controller and iounmap dev->regs only > > > if it is not NULL. > > > free_irq() does not need dev->regs, so unmaping it before freeing the > > > irq should not matter. > > > > > > Signed-off-by: Sudip Mukherjee <sudip@xxxxxxxxxxxxxxx> > > > --- > > > drivers/usb/gadget/udc/amd5536udc.c | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/usb/gadget/udc/amd5536udc.c b/drivers/usb/gadget/udc/amd5536udc.c > > > index fdacddb..26066d3 100644 > > > --- a/drivers/usb/gadget/udc/amd5536udc.c > > > +++ b/drivers/usb/gadget/udc/amd5536udc.c > > > @@ -3135,11 +3135,12 @@ static void udc_pci_remove(struct pci_dev *pdev) > <snip> > > > > I'm not familiar with the driver, but you're iounmap'ing before freeing > > irq. Looks fishy to me. > Well, I thought you will be able to give me some idea about how fix it. :) > Then I guess we should be on the safe side and what about the following: > > > diff --git a/drivers/usb/gadget/udc/amd5536udc.c b/drivers/usb/gadget/udc/amd5536udc.c > index fdacddb..82f36f6 100644 > --- a/drivers/usb/gadget/udc/amd5536udc.c > +++ b/drivers/usb/gadget/udc/amd5536udc.c > @@ -3134,8 +3134,9 @@ static void udc_pci_remove(struct pci_dev *pdev) > pci_pool_destroy(dev->stp_requests); > } > > - /* reset controller */ > - writel(AMD_BIT(UDC_DEVCFG_SOFTRESET), &dev->regs->cfg); > + if (dev->regs) > + /* reset controller */ > + writel(AMD_BIT(UDC_DEVCFG_SOFTRESET), &dev->regs->cfg); no, the check is pointless. Most of these are. Just look at your probe() and you'll see that if dev->virt_addr is NULL (meaning ioremap_nocache() failed) you exit from probe() with error. The driver doesn't probe at all. So you can be sure that by remove, dev->regs is valid. BTW, if probe fails, you have a TON of leaked resources!! You don't kfree() dev, you don't pci_disable_device(), you don't release_mem_region(), you don't iounmap() virt_addr, you don't free_irq(). Also the iounmap() call in remove is wrong. You ioremapped dev->virt_addr but iounmap() dev->regs. Are you SURE that's ok ? Man, what a mess! You gotta fix that up. > if (dev->irq_registered) > free_irq(pdev->irq, dev); > if (dev->regs) > > And just for my information: for a device what might happen if I iounmap > before I free the irq? One thing I can think of is that after iounmap what happens if an IRQ fires after you iounmap() but before you free_irq() ? > just at that moment one interrupt comes and the driver tries to access > the io memory while servicing the irq. there you go -- balbi
Attachment:
signature.asc
Description: Digital signature
