On 3/8/19 12:26 PM, Laurent Pinchart wrote: > Hi Hans, > > On Thu, Mar 07, 2019 at 10:23:03AM +0100, Hans Verkuil wrote: >> On 3/5/19 8:39 PM, Laurent Pinchart wrote: >>> On Tue, Mar 05, 2019 at 10:58:42AM +0100, hverkuil-cisco@xxxxxxxxx wrote: >>>> From: Hans Verkuil <hverkuil-cisco@xxxxxxxxx> >>>> >>>> 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. >>> >>> I still think the problem lies in the vimc driver. Reusing static >>> structures is really asking for trouble. I'm however not opposed to >>> merging this patch, as you mentioned the problem will be fixed in vimc >>> too. I still wonder, though, if merging this couldn't make it easier for >>> drivers to do the wrong thing. >> >> I'm keeping this patch :-) >> >> I don't think that what vimc is doing is wrong in principle, just very unusual. > > I disagree here. We've developed the media controller (and V4L2) core > code with many assumptions that structures are zeroed on allocation. For > the structures that are meant to be registered once only, the code > assumes, explicitly and implicitly, that some of the fields are zeroed. > Removing that assumption for the odd case of vimc will require you to > chase bugs for a long time. You've caught a few of the easier ones here, > I'm sure other will linger for a much longer time before they get fixed. > In the vimc case, the best option is to zero the structure manually if > you don't want to allocate it dynamically (and I think it should be > allocated dynamically). > > For the record, I ran into a similar problem before when trying to > unregister and re-register a struct device. I reported what I considered > to be a bug, and Greg very clearly told me it was plain wrong. You will > run into similar issues due to the platform_device embedded in struct > vimc_device. Let's just allocate it dynamically. > >> Also I think it makes the mc framework more robust by properly zeroing this >> pointer. > > This patch is not wrong per-se, and I'm not opposed to it, but we should > fix issues in drivers, which would render this patch unneeded. > I posted a v4 where I drop this patch and instead zero the global media_device struct in the vimc probe() function. Regards, Hans
