| Message ID | 151001140261.77201.8823780763771880199.stgit@alans-desktop (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Alan, On Mon, Nov 06, 2017 at 11:36:45PM +0000, Alan wrote: > We load the firmware once, set pointers to it and then at some point release > it. We should not be doing a vfree() on the pointers into the firmware. > > Signed-off-by: Alan Cox <alan@linux.intel.com> > --- > .../atomisp/pci/atomisp2/css2400/sh_css_firmware.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c > index 8158ea40d069..f181bd8fcee2 100644 > --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c > +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c > @@ -288,8 +288,6 @@ void sh_css_unload_firmware(void) > for (i = 0; i < sh_css_num_binaries; i++) { > if (fw_minibuffer[i].name) > kfree((void *)fw_minibuffer[i].name); > - if (fw_minibuffer[i].buffer) > - vfree((void *)fw_minibuffer[i].buffer); You shouldn't end up here if the firmware is just loaded once. If multiple times, then yes. The memory appears to have been allocated using kmalloc() in some cases. How about kvfree(), or changing that kmalloc() to vmalloc()? > } > kfree(fw_minibuffer); > fw_minibuffer = NULL; >
On Tue, 14 Nov 2017 00:05:48 +0200 Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > Hi Alan, > > On Mon, Nov 06, 2017 at 11:36:45PM +0000, Alan wrote: > > We load the firmware once, set pointers to it and then at some point release > > it. We should not be doing a vfree() on the pointers into the firmware. > > > > Signed-off-by: Alan Cox <alan@linux.intel.com> > > --- > > .../atomisp/pci/atomisp2/css2400/sh_css_firmware.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c > > index 8158ea40d069..f181bd8fcee2 100644 > > --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c > > +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c > > @@ -288,8 +288,6 @@ void sh_css_unload_firmware(void) > > for (i = 0; i < sh_css_num_binaries; i++) { > > if (fw_minibuffer[i].name) > > kfree((void *)fw_minibuffer[i].name); > > - if (fw_minibuffer[i].buffer) > > - vfree((void *)fw_minibuffer[i].buffer); > > You shouldn't end up here if the firmware is just loaded once. If multiple > times, then yes. You end up there when unloading the module. > The memory appears to have been allocated using kmalloc() in some cases. > How about kvfree(), or changing that kmalloc() to vmalloc() I'll take a deeper look at what is going on. Alan
On Tue, Nov 14, 2017 at 12:16:01AM +0000, Alan Cox wrote: > On Tue, 14 Nov 2017 00:05:48 +0200 > Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > > Hi Alan, > > > > On Mon, Nov 06, 2017 at 11:36:45PM +0000, Alan wrote: > > > We load the firmware once, set pointers to it and then at some point release > > > it. We should not be doing a vfree() on the pointers into the firmware. > > > > > > Signed-off-by: Alan Cox <alan@linux.intel.com> > > > --- > > > .../atomisp/pci/atomisp2/css2400/sh_css_firmware.c | 2 -- > > > 1 file changed, 2 deletions(-) > > > > > > diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c > > > index 8158ea40d069..f181bd8fcee2 100644 > > > --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c > > > +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c > > > @@ -288,8 +288,6 @@ void sh_css_unload_firmware(void) > > > for (i = 0; i < sh_css_num_binaries; i++) { > > > if (fw_minibuffer[i].name) > > > kfree((void *)fw_minibuffer[i].name); > > > - if (fw_minibuffer[i].buffer) > > > - vfree((void *)fw_minibuffer[i].buffer); > > > > You shouldn't end up here if the firmware is just loaded once. If multiple > > times, then yes. > > You end up there when unloading the module. Ah, that's for sure indeed. I thought loading would be already a challenge. :-) > > > The memory appears to have been allocated using kmalloc() in some cases. > > How about kvfree(), or changing that kmalloc() to vmalloc() > > I'll take a deeper look at what is going on. Look for minibuffer in sh_css_load_blob_info(). The buffer field of the struct is allocated using kmalloc, I wonder if changing that to vmalloc would just address this. The buffer is elsewhere allocated using vmalloc. I suspect that some of the cleanup patches changed how this works but missed changing the other one.
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c index 8158ea40d069..f181bd8fcee2 100644 --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_firmware.c @@ -288,8 +288,6 @@ void sh_css_unload_firmware(void) for (i = 0; i < sh_css_num_binaries; i++) { if (fw_minibuffer[i].name) kfree((void *)fw_minibuffer[i].name); - if (fw_minibuffer[i].buffer) - vfree((void *)fw_minibuffer[i].buffer); } kfree(fw_minibuffer); fw_minibuffer = NULL;
We load the firmware once, set pointers to it and then at some point release it. We should not be doing a vfree() on the pointers into the firmware. Signed-off-by: Alan Cox <alan@linux.intel.com> --- .../atomisp/pci/atomisp2/css2400/sh_css_firmware.c | 2 -- 1 file changed, 2 deletions(-)