| Message ID | 20240303152635.2762696-4-maarten@rmail.be (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | bcm2835-codec: driver for HW codecs | expand |
Hi Maarten, W dniu 3.03.2024 o 16:09, Maarten Vanraes pisze: > From: Dave Stevenson <dave.stevenson@raspberrypi.org> > > On error, vchiq_mmal_component_init could leave the > event context allocated for ports. > Clean them up in the error path. > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org> > > staging: mmal-vchiq: Free the event context for control ports > > vchiq_mmal_component_init calls init_event_context for the > control port, but vchiq_mmal_component_finalise didn't free > it, causing a memory leak.. > > Add the free call. > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org> > Signed-off-by: Maarten Vanraes <maarten@rmail.be> > --- > .../vc04_services/vchiq-mmal/mmal-vchiq.c | 29 ++++++++++++++----- > 1 file changed, 22 insertions(+), 7 deletions(-) > > diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c > index 2e616604943d..1209b7db8f30 100644 > --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c > +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c > @@ -1825,9 +1825,26 @@ static void free_event_context(struct vchiq_mmal_port *port) > { > struct mmal_msg_context *ctx = port->event_context; > > + if (!ctx) > + return; > + > kfree(ctx->u.bulk.buffer->buffer); > kfree(ctx->u.bulk.buffer); > release_msg_context(ctx); > + port->event_context = NULL; > +} > + > +static void release_all_event_contexts(struct vchiq_mmal_component *component) > +{ > + int idx; > + > + for (idx = 0; idx < component->inputs; idx++) > + free_event_context(&component->input[idx]); > + for (idx = 0; idx < component->outputs; idx++) > + free_event_context(&component->output[idx]); > + for (idx = 0; idx < component->clocks; idx++) > + free_event_context(&component->clock[idx]); > + free_event_context(&component->control); > } > > /* Initialise a mmal component and its ports > @@ -1925,6 +1942,7 @@ int vchiq_mmal_component_init(struct vchiq_mmal_instance *instance, > > release_component: > destroy_component(instance, component); > + release_all_event_contexts(component); > unlock: > if (component) > component->in_use = false; > @@ -1940,7 +1958,7 @@ EXPORT_SYMBOL_GPL(vchiq_mmal_component_init); > int vchiq_mmal_component_finalise(struct vchiq_mmal_instance *instance, > struct vchiq_mmal_component *component) > { > - int ret, idx; > + int ret; > > if (mutex_lock_interruptible(&instance->vchiq_mutex)) > return -EINTR; > @@ -1952,12 +1970,9 @@ int vchiq_mmal_component_finalise(struct vchiq_mmal_instance *instance, > > component->in_use = false; > > - for (idx = 0; idx < component->inputs; idx++) > - free_event_context(&component->input[idx]); > - for (idx = 0; idx < component->outputs; idx++) > - free_event_context(&component->output[idx]); > - for (idx = 0; idx < component->clocks; idx++) > - free_event_context(&component->clock[idx]); > + release_all_event_contexts(component); The way I understand this chunk is that you factor out the 3 "for" loops into the new function "release_all_event_contexts()", because it is then reused elsewhere. "release_all_event_contexts()" already contains invocation of "free_event_context(&component->control)"... > + > + free_event_context(&component->control); ... but it is repeated here. Why? Regards, Andrzej > > mutex_unlock(&instance->vchiq_mutex); >
Andrzej Pietrasiewicz schreef op 2024-03-04 08:38: > Hi Maarten, > > W dniu 3.03.2024 o 16:09, Maarten Vanraes pisze: >> From: Dave Stevenson <dave.stevenson@raspberrypi.org> >> >> On error, vchiq_mmal_component_init could leave the >> event context allocated for ports. >> Clean them up in the error path. >> >> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org> >> >> staging: mmal-vchiq: Free the event context for control ports >> >> vchiq_mmal_component_init calls init_event_context for the >> control port, but vchiq_mmal_component_finalise didn't free >> it, causing a memory leak.. >> >> Add the free call. >> >> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org> >> Signed-off-by: Maarten Vanraes <maarten@rmail.be> >> --- >> .../vc04_services/vchiq-mmal/mmal-vchiq.c | 29 >> ++++++++++++++----- >> 1 file changed, 22 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c >> b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c >> index 2e616604943d..1209b7db8f30 100644 >> --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c >> +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c >> @@ -1825,9 +1825,26 @@ static void free_event_context(struct >> vchiq_mmal_port *port) >> { >> struct mmal_msg_context *ctx = port->event_context; >> + if (!ctx) >> + return; >> + >> kfree(ctx->u.bulk.buffer->buffer); >> kfree(ctx->u.bulk.buffer); >> release_msg_context(ctx); >> + port->event_context = NULL; >> +} >> + >> +static void release_all_event_contexts(struct vchiq_mmal_component >> *component) >> +{ >> + int idx; >> + >> + for (idx = 0; idx < component->inputs; idx++) >> + free_event_context(&component->input[idx]); >> + for (idx = 0; idx < component->outputs; idx++) >> + free_event_context(&component->output[idx]); >> + for (idx = 0; idx < component->clocks; idx++) >> + free_event_context(&component->clock[idx]); >> + free_event_context(&component->control); >> } >> /* Initialise a mmal component and its ports >> @@ -1925,6 +1942,7 @@ int vchiq_mmal_component_init(struct >> vchiq_mmal_instance *instance, >> release_component: >> destroy_component(instance, component); >> + release_all_event_contexts(component); >> unlock: >> if (component) >> component->in_use = false; >> @@ -1940,7 +1958,7 @@ EXPORT_SYMBOL_GPL(vchiq_mmal_component_init); >> int vchiq_mmal_component_finalise(struct vchiq_mmal_instance >> *instance, >> struct vchiq_mmal_component *component) >> { >> - int ret, idx; >> + int ret; >> if (mutex_lock_interruptible(&instance->vchiq_mutex)) >> return -EINTR; >> @@ -1952,12 +1970,9 @@ int vchiq_mmal_component_finalise(struct >> vchiq_mmal_instance *instance, >> component->in_use = false; >> - for (idx = 0; idx < component->inputs; idx++) >> - free_event_context(&component->input[idx]); >> - for (idx = 0; idx < component->outputs; idx++) >> - free_event_context(&component->output[idx]); >> - for (idx = 0; idx < component->clocks; idx++) >> - free_event_context(&component->clock[idx]); >> + release_all_event_contexts(component); > > The way I understand this chunk is that you factor out the 3 "for" > loops into > the new function "release_all_event_contexts()", because it is then > reused elsewhere. "release_all_event_contexts()" already contains > invocation of > "free_event_context(&component->control)"... > >> + >> + free_event_context(&component->control); > > ... but it is repeated here. Why? I'm sorry, I messed up in squashing 2 patches, that definately does not belong there, will fix in v2. Thanks!
diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c index 2e616604943d..1209b7db8f30 100644 --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c @@ -1825,9 +1825,26 @@ static void free_event_context(struct vchiq_mmal_port *port) { struct mmal_msg_context *ctx = port->event_context; + if (!ctx) + return; + kfree(ctx->u.bulk.buffer->buffer); kfree(ctx->u.bulk.buffer); release_msg_context(ctx); + port->event_context = NULL; +} + +static void release_all_event_contexts(struct vchiq_mmal_component *component) +{ + int idx; + + for (idx = 0; idx < component->inputs; idx++) + free_event_context(&component->input[idx]); + for (idx = 0; idx < component->outputs; idx++) + free_event_context(&component->output[idx]); + for (idx = 0; idx < component->clocks; idx++) + free_event_context(&component->clock[idx]); + free_event_context(&component->control); } /* Initialise a mmal component and its ports @@ -1925,6 +1942,7 @@ int vchiq_mmal_component_init(struct vchiq_mmal_instance *instance, release_component: destroy_component(instance, component); + release_all_event_contexts(component); unlock: if (component) component->in_use = false; @@ -1940,7 +1958,7 @@ EXPORT_SYMBOL_GPL(vchiq_mmal_component_init); int vchiq_mmal_component_finalise(struct vchiq_mmal_instance *instance, struct vchiq_mmal_component *component) { - int ret, idx; + int ret; if (mutex_lock_interruptible(&instance->vchiq_mutex)) return -EINTR; @@ -1952,12 +1970,9 @@ int vchiq_mmal_component_finalise(struct vchiq_mmal_instance *instance, component->in_use = false; - for (idx = 0; idx < component->inputs; idx++) - free_event_context(&component->input[idx]); - for (idx = 0; idx < component->outputs; idx++) - free_event_context(&component->output[idx]); - for (idx = 0; idx < component->clocks; idx++) - free_event_context(&component->clock[idx]); + release_all_event_contexts(component); + + free_event_context(&component->control); mutex_unlock(&instance->vchiq_mutex);