| Message ID | 20190221142148.3412-5-hverkuil-cisco@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Various core and virtual driver fixes | expand |
Hi Hans, Thank you for the patch. On Thu, Feb 21, 2019 at 03:21:45PM +0100, Hans Verkuil wrote: > Ensure that this pointer is set to NULL after it is freed. > The vimc driver has a static media_entity and after > unbinding and rebinding the vimc device the media code will > try to free this pointer again since it wasn't set to NULL. As this fixes a problem in vimc, should you add a Fixes: tag ? To avoid other similar problems, I think the vimc driver should allocate the media_device and other device data dynamically at probe time. Bundling them with the platform_device in struct vimc_device isn't a good idea. > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/media-entity.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c > index 0b1cb3559140..7b2a2cc95530 100644 > --- a/drivers/media/media-entity.c > +++ b/drivers/media/media-entity.c > @@ -88,6 +88,7 @@ EXPORT_SYMBOL_GPL(__media_entity_enum_init); > void media_entity_enum_cleanup(struct media_entity_enum *ent_enum) > { > kfree(ent_enum->bmap); > + ent_enum->bmap = NULL; > } > EXPORT_SYMBOL_GPL(media_entity_enum_cleanup); >
On 2/22/19 12:17 PM, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Thu, Feb 21, 2019 at 03:21:45PM +0100, Hans Verkuil wrote: >> Ensure that this pointer is set to NULL after it is freed. >> The vimc driver has a static media_entity and after >> unbinding and rebinding the vimc device the media code will >> try to free this pointer again since it wasn't set to NULL. > > As this fixes a problem in vimc, should you add a Fixes: tag ? To avoid The Fixes tag is really for cases where a patch introduces a bug, whereas this was always wrong. > other similar problems, I think the vimc driver should allocate the > media_device and other device data dynamically at probe time. Bundling > them with the platform_device in struct vimc_device isn't a good idea. It's not actually wrong as such, just unusual. Which actually makes it a good test case. Anyway, the upcoming "vimc: add configfs API to configure the topology" patch makes this dynamic. Waiting for a v2 of that patch. Regards, Hans > >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> --- >> drivers/media/media-entity.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c >> index 0b1cb3559140..7b2a2cc95530 100644 >> --- a/drivers/media/media-entity.c >> +++ b/drivers/media/media-entity.c >> @@ -88,6 +88,7 @@ EXPORT_SYMBOL_GPL(__media_entity_enum_init); >> void media_entity_enum_cleanup(struct media_entity_enum *ent_enum) >> { >> kfree(ent_enum->bmap); >> + ent_enum->bmap = NULL; >> } >> EXPORT_SYMBOL_GPL(media_entity_enum_cleanup); >> >
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c index 0b1cb3559140..7b2a2cc95530 100644 --- a/drivers/media/media-entity.c +++ b/drivers/media/media-entity.c @@ -88,6 +88,7 @@ EXPORT_SYMBOL_GPL(__media_entity_enum_init); void media_entity_enum_cleanup(struct media_entity_enum *ent_enum) { kfree(ent_enum->bmap); + ent_enum->bmap = NULL; } EXPORT_SYMBOL_GPL(media_entity_enum_cleanup);
Ensure that this pointer is set to NULL after it is freed. The vimc driver has a static media_entity and after unbinding and rebinding the vimc device the media code will try to free this pointer again since it wasn't set to NULL. Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- drivers/media/media-entity.c | 1 + 1 file changed, 1 insertion(+)