Message ID | 1586919463-30542-1-git-send-email-skomatineni@nvidia.com (mailing list archive) |
---|---|
Headers | show |
Series | Add Tegra driver for video capture | expand |
15.04.2020 05:57, Sowjanya Komatineni пишет: > +static int tegra_csi_remove(struct platform_device *pdev) > +{ > + struct tegra_csi *csi = platform_get_drvdata(pdev); > + int err; > + > + err = host1x_client_unregister(&csi->client); > + if (err < 0) { > + dev_err(csi->dev, > + "failed to unregister host1x client: %d\n", err); > + return err; > + } > + > + pm_runtime_disable(csi->dev); > + kfree(csi); IIRC, the driver removal is invoked on the unbinding. Hence, I'm not sure how moving away from the resource-managed API helps here. Could you please explain in a more details? Have you tried to test this driver under KASAN? I suspect that you just masked the problem, instead of fixing it.
On 4/15/20 7:22 AM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 15.04.2020 05:57, Sowjanya Komatineni пишет: >> +static int tegra_csi_remove(struct platform_device *pdev) >> +{ >> + struct tegra_csi *csi = platform_get_drvdata(pdev); >> + int err; >> + >> + err = host1x_client_unregister(&csi->client); >> + if (err < 0) { >> + dev_err(csi->dev, >> + "failed to unregister host1x client: %d\n", err); >> + return err; >> + } >> + >> + pm_runtime_disable(csi->dev); >> + kfree(csi); > IIRC, the driver removal is invoked on the unbinding. Hence, I'm not > sure how moving away from the resource-managed API helps here. Could you > please explain in a more details? > > Have you tried to test this driver under KASAN? I suspect that you just > masked the problem, instead of fixing it. Using devm_kzalloc for vi/csi structures based on prior feedback request to switch to use kzalloc all over this driver. Hi Hans, video devices lifetime is till video device nodes are released. So, v4l2 device release callback does the release of tegra channel allocation which hold video device. Below are the 3 possible cases of unbind/unload, 1. during tegra-video module unload, if v4l2 device refcnt is not 0 which is the case when any of video device node handle is kept opened then unloading module will not happen and module refcnt is also non-zero and unloading tegra-video module reports module in use. 2. during tegra-video driver unbind, tegra-video driver removal will do vi/csi clients exit ops which unregisters video device allocated memory during release callback of v4l2 device. vi/csi structure allocation remains same as vi/csi driver removal will not happen in this case. 3. during direct host1x client drivers vi/csi unbind, both host1x_clients vi/csi gets unregistered, deletes host1x logical device which executes tegra-video driver removal() -> vi/csi exit() before vi/csi memory gets freed in vi/csi driver remove(). So, any active streaming will stop and video devices are unregistered during direct client driver unbind prior to freeing vi/csi memory. Also vi/csi driver remove does explicit free vi/csi as its allocated with kzalloc. So not sure how using kzalloc is different to devm_kzalloc for vi/csi structure in terms of when vi/csi memory gets freed? Except for channel allocation which holds video device and as video device life time is beyond tegra-video module unbind->vi exit(), looks like we can use devm_kzalloc for vi/csi. Can you please comment if you still think we need to use kzalloc rather than devm_kzalloc for vi/csi structure allocation? Thanks Sowjanya
On 4/15/20 9:54 AM, Sowjanya Komatineni wrote: > > On 4/15/20 7:22 AM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 15.04.2020 05:57, Sowjanya Komatineni пишет: >>> +static int tegra_csi_remove(struct platform_device *pdev) >>> +{ >>> + struct tegra_csi *csi = platform_get_drvdata(pdev); >>> + int err; >>> + >>> + err = host1x_client_unregister(&csi->client); >>> + if (err < 0) { >>> + dev_err(csi->dev, >>> + "failed to unregister host1x client: %d\n", err); >>> + return err; >>> + } >>> + >>> + pm_runtime_disable(csi->dev); >>> + kfree(csi); >> IIRC, the driver removal is invoked on the unbinding. Hence, I'm not >> sure how moving away from the resource-managed API helps here. Could you >> please explain in a more details? >> >> Have you tried to test this driver under KASAN? I suspect that you just >> masked the problem, instead of fixing it. > Using devm_kzalloc for vi/csi structures based on prior feedback > request to switch to use kzalloc all over this driver. > > Hi Hans, > > video devices lifetime is till video device nodes are released. So, > v4l2 device release callback does the release of tegra channel > allocation which hold video device. > > Below are the 3 possible cases of unbind/unload, > > 1. during tegra-video module unload, if v4l2 device refcnt is not 0 > which is the case when any of video device node handle is kept opened > then unloading module will not happen and module refcnt is also > non-zero and unloading tegra-video module reports module in use. > > 2. during tegra-video driver unbind, tegra-video driver removal will > do vi/csi clients exit ops which unregisters video device allocated > memory during release callback of v4l2 device. vi/csi structure > allocation remains same as vi/csi driver removal will not happen in > this case. > > > 3. during direct host1x client drivers vi/csi unbind, both > host1x_clients vi/csi gets unregistered, deletes host1x logical device > which executes tegra-video driver removal() -> vi/csi exit() before > vi/csi memory gets freed in vi/csi driver remove(). > > So, any active streaming will stop and video devices are unregistered > during direct client driver unbind prior to freeing vi/csi memory. > > Also vi/csi driver remove does explicit free vi/csi as its allocated > with kzalloc. So not sure how using kzalloc is different to > devm_kzalloc for vi/csi structure in terms of when vi/csi memory gets > freed? > > Except for channel allocation which holds video device and as video > device life time is beyond tegra-video module unbind->vi exit(), looks > like we can use devm_kzalloc for vi/csi. > > > Can you please comment if you still think we need to use kzalloc > rather than devm_kzalloc for vi/csi structure allocation? > > Thanks > > Sowjanya > One more case is when video device node is kept opened with v4l2-ctl sleep (rather than streaming), where it will keep device node open for specified time and if direct vi client driver unbind happens then vi driver remove() will free vi memory before v4l2 device release happens. But I don't see any crash or errors with this case. Also if we allow direct client driver unbind, then vi structure memory lifetime should also be till v4l2 device release happens. But we can free vi in v4l2 device release callback as in case when device node is not kept opened, video device release happens immediate and we cant free vi that early. Hans/Thierry, Can you please comment on this case? Thanks Sowjanya
On 4/15/20 10:21 AM, Sowjanya Komatineni wrote: > > On 4/15/20 9:54 AM, Sowjanya Komatineni wrote: >> >> On 4/15/20 7:22 AM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 15.04.2020 05:57, Sowjanya Komatineni пишет: >>>> +static int tegra_csi_remove(struct platform_device *pdev) >>>> +{ >>>> + struct tegra_csi *csi = platform_get_drvdata(pdev); >>>> + int err; >>>> + >>>> + err = host1x_client_unregister(&csi->client); >>>> + if (err < 0) { >>>> + dev_err(csi->dev, >>>> + "failed to unregister host1x client: %d\n", >>>> err); >>>> + return err; >>>> + } >>>> + >>>> + pm_runtime_disable(csi->dev); >>>> + kfree(csi); >>> IIRC, the driver removal is invoked on the unbinding. Hence, I'm not >>> sure how moving away from the resource-managed API helps here. Could >>> you >>> please explain in a more details? >>> >>> Have you tried to test this driver under KASAN? I suspect that you just >>> masked the problem, instead of fixing it. >> Using devm_kzalloc for vi/csi structures based on prior feedback >> request to switch to use kzalloc all over this driver. >> >> Hi Hans, >> >> video devices lifetime is till video device nodes are released. So, >> v4l2 device release callback does the release of tegra channel >> allocation which hold video device. >> >> Below are the 3 possible cases of unbind/unload, >> >> 1. during tegra-video module unload, if v4l2 device refcnt is not 0 >> which is the case when any of video device node handle is kept opened >> then unloading module will not happen and module refcnt is also >> non-zero and unloading tegra-video module reports module in use. >> >> 2. during tegra-video driver unbind, tegra-video driver removal will >> do vi/csi clients exit ops which unregisters video device allocated >> memory during release callback of v4l2 device. vi/csi structure >> allocation remains same as vi/csi driver removal will not happen in >> this case. >> >> >> 3. during direct host1x client drivers vi/csi unbind, both >> host1x_clients vi/csi gets unregistered, deletes host1x logical >> device which executes tegra-video driver removal() -> vi/csi exit() >> before vi/csi memory gets freed in vi/csi driver remove(). >> >> So, any active streaming will stop and video devices are unregistered >> during direct client driver unbind prior to freeing vi/csi memory. >> >> Also vi/csi driver remove does explicit free vi/csi as its allocated >> with kzalloc. So not sure how using kzalloc is different to >> devm_kzalloc for vi/csi structure in terms of when vi/csi memory gets >> freed? >> >> Except for channel allocation which holds video device and as video >> device life time is beyond tegra-video module unbind->vi exit(), >> looks like we can use devm_kzalloc for vi/csi. >> >> >> Can you please comment if you still think we need to use kzalloc >> rather than devm_kzalloc for vi/csi structure allocation? >> >> Thanks >> >> Sowjanya >> > One more case is when video device node is kept opened with v4l2-ctl > sleep (rather than streaming), where it will keep device node open for > specified time and if direct vi client driver unbind happens then vi > driver remove() will free vi memory before v4l2 device release happens. > > But I don't see any crash or errors with this case. > > Also if we allow direct client driver unbind, then vi structure memory > lifetime should also be till v4l2 device release happens. > > But we can free vi in v4l2 device release callback as in case when > device node is not kept opened, video device release happens immediate > and we cant free vi that early. typo fix: But we can't free vi structure memory allocation in v4l2 device release callback as in case when device node is not kept opened, device release happens immediate and we can't free vi structure memory that early. > > Hans/Thierry, Can you please comment on this case? > > Thanks > > Sowjanya >
On 4/15/20 10:47 AM, Sowjanya Komatineni wrote: > > On 4/15/20 10:21 AM, Sowjanya Komatineni wrote: >> >> On 4/15/20 9:54 AM, Sowjanya Komatineni wrote: >>> >>> On 4/15/20 7:22 AM, Dmitry Osipenko wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> 15.04.2020 05:57, Sowjanya Komatineni пишет: >>>>> +static int tegra_csi_remove(struct platform_device *pdev) >>>>> +{ >>>>> + struct tegra_csi *csi = platform_get_drvdata(pdev); >>>>> + int err; >>>>> + >>>>> + err = host1x_client_unregister(&csi->client); >>>>> + if (err < 0) { >>>>> + dev_err(csi->dev, >>>>> + "failed to unregister host1x client: %d\n", >>>>> err); >>>>> + return err; >>>>> + } >>>>> + >>>>> + pm_runtime_disable(csi->dev); >>>>> + kfree(csi); >>>> IIRC, the driver removal is invoked on the unbinding. Hence, I'm not >>>> sure how moving away from the resource-managed API helps here. >>>> Could you >>>> please explain in a more details? >>>> >>>> Have you tried to test this driver under KASAN? I suspect that you >>>> just >>>> masked the problem, instead of fixing it. >>> Using devm_kzalloc for vi/csi structures based on prior feedback >>> request to switch to use kzalloc all over this driver. >>> >>> Hi Hans, >>> >>> video devices lifetime is till video device nodes are released. So, >>> v4l2 device release callback does the release of tegra channel >>> allocation which hold video device. >>> >>> Below are the 3 possible cases of unbind/unload, >>> >>> 1. during tegra-video module unload, if v4l2 device refcnt is not 0 >>> which is the case when any of video device node handle is kept >>> opened then unloading module will not happen and module refcnt is >>> also non-zero and unloading tegra-video module reports module in use. >>> >>> 2. during tegra-video driver unbind, tegra-video driver removal will >>> do vi/csi clients exit ops which unregisters video device allocated >>> memory during release callback of v4l2 device. vi/csi structure >>> allocation remains same as vi/csi driver removal will not happen in >>> this case. >>> >>> >>> 3. during direct host1x client drivers vi/csi unbind, both >>> host1x_clients vi/csi gets unregistered, deletes host1x logical >>> device which executes tegra-video driver removal() -> vi/csi exit() >>> before vi/csi memory gets freed in vi/csi driver remove(). >>> >>> So, any active streaming will stop and video devices are >>> unregistered during direct client driver unbind prior to freeing >>> vi/csi memory. >>> >>> Also vi/csi driver remove does explicit free vi/csi as its allocated >>> with kzalloc. So not sure how using kzalloc is different to >>> devm_kzalloc for vi/csi structure in terms of when vi/csi memory >>> gets freed? >>> >>> Except for channel allocation which holds video device and as video >>> device life time is beyond tegra-video module unbind->vi exit(), >>> looks like we can use devm_kzalloc for vi/csi. >>> >>> >>> Can you please comment if you still think we need to use kzalloc >>> rather than devm_kzalloc for vi/csi structure allocation? >>> >>> Thanks >>> >>> Sowjanya >>> >> One more case is when video device node is kept opened with v4l2-ctl >> sleep (rather than streaming), where it will keep device node open >> for specified time and if direct vi client driver unbind happens then >> vi driver remove() will free vi memory before v4l2 device release >> happens. >> >> But I don't see any crash or errors with this case. >> >> Also if we allow direct client driver unbind, then vi structure >> memory lifetime should also be till v4l2 device release happens. >> >> But we can free vi in v4l2 device release callback as in case when >> device node is not kept opened, video device release happens >> immediate and we cant free vi that early. > > typo fix: > > But we can't free vi structure memory allocation in v4l2 device > release callback as in case when device node is not kept opened, > device release happens immediate and we can't free vi structure memory > that early. > >> >> Hans/Thierry, Can you please comment on this case? >> >> Thanks >> >> Sowjanya >> Also, Can you please help explain on cases where we do/need direct host1x clients vi/csi drivers unbind?
On 4/15/20 10:48 AM, Sowjanya Komatineni wrote: > > On 4/15/20 10:47 AM, Sowjanya Komatineni wrote: >> >> On 4/15/20 10:21 AM, Sowjanya Komatineni wrote: >>> >>> On 4/15/20 9:54 AM, Sowjanya Komatineni wrote: >>>> >>>> On 4/15/20 7:22 AM, Dmitry Osipenko wrote: >>>>> External email: Use caution opening links or attachments >>>>> >>>>> >>>>> 15.04.2020 05:57, Sowjanya Komatineni пишет: >>>>>> +static int tegra_csi_remove(struct platform_device *pdev) >>>>>> +{ >>>>>> + struct tegra_csi *csi = platform_get_drvdata(pdev); >>>>>> + int err; >>>>>> + >>>>>> + err = host1x_client_unregister(&csi->client); >>>>>> + if (err < 0) { >>>>>> + dev_err(csi->dev, >>>>>> + "failed to unregister host1x client: %d\n", >>>>>> err); >>>>>> + return err; >>>>>> + } >>>>>> + >>>>>> + pm_runtime_disable(csi->dev); >>>>>> + kfree(csi); >>>>> IIRC, the driver removal is invoked on the unbinding. Hence, I'm not >>>>> sure how moving away from the resource-managed API helps here. >>>>> Could you >>>>> please explain in a more details? >>>>> >>>>> Have you tried to test this driver under KASAN? I suspect that you >>>>> just >>>>> masked the problem, instead of fixing it. >>>> Using devm_kzalloc for vi/csi structures based on prior feedback >>>> request to switch to use kzalloc all over this driver. >>>> >>>> Hi Hans, >>>> >>>> video devices lifetime is till video device nodes are released. So, >>>> v4l2 device release callback does the release of tegra channel >>>> allocation which hold video device. >>>> >>>> Below are the 3 possible cases of unbind/unload, >>>> >>>> 1. during tegra-video module unload, if v4l2 device refcnt is not 0 >>>> which is the case when any of video device node handle is kept >>>> opened then unloading module will not happen and module refcnt is >>>> also non-zero and unloading tegra-video module reports module in use. v4l2 device is associated with host1x device where during v4l2_device_register get_device causes refcnt of tegra video host1x device to increase and prevents allowing module unload/load till v4l2 device release happens. >>>> 2. during tegra-video driver unbind, tegra-video driver removal >>>> will do vi/csi clients exit ops which unregisters video device >>>> allocated memory during release callback of v4l2 device. vi/csi >>>> structure allocation remains same as vi/csi driver removal will not >>>> happen in this case. >>>> >>>> >>>> 3. during direct host1x client drivers vi/csi unbind, both >>>> host1x_clients vi/csi gets unregistered, deletes host1x logical >>>> device which executes tegra-video driver removal() -> vi/csi exit() >>>> before vi/csi memory gets freed in vi/csi driver remove(). >>>> >>>> So, any active streaming will stop and video devices are >>>> unregistered during direct client driver unbind prior to freeing >>>> vi/csi memory. >>>> >>>> Also vi/csi driver remove does explicit free vi/csi as its >>>> allocated with kzalloc. So not sure how using kzalloc is different >>>> to devm_kzalloc for vi/csi structure in terms of when vi/csi memory >>>> gets freed? >>>> >>>> Except for channel allocation which holds video device and as video >>>> device life time is beyond tegra-video module unbind->vi exit(), >>>> looks like we can use devm_kzalloc for vi/csi. >>>> >>>> >>>> Can you please comment if you still think we need to use kzalloc >>>> rather than devm_kzalloc for vi/csi structure allocation? >>>> >>>> Thanks >>>> >>>> Sowjanya >>>> >>> One more case is when video device node is kept opened with v4l2-ctl >>> sleep (rather than streaming), where it will keep device node open >>> for specified time and if direct vi client driver unbind happens >>> then vi driver remove() will free vi memory before v4l2 device >>> release happens. >>> >>> But I don't see any crash or errors with this case. In the above case, channels allocated memory release may not happen in this case as list head pointer will be gone when vi memory is freed during direct client unbind and by the time v4l2 device release callback gets executed vi channels list head is gone. Also, freeing vi structure memory can't be done in v4l2 device release callback either. >>> >>> Also if we allow direct client driver unbind, then vi structure >>> memory lifetime should also be till v4l2 device release happens. >>> >>> But we can free vi in v4l2 device release callback as in case when >>> device node is not kept opened, video device release happens >>> immediate and we cant free vi that early. >> >> typo fix: >> >> But we can't free vi structure memory allocation in v4l2 device >> release callback as in case when device node is not kept opened, >> device release happens immediate and we can't free vi structure >> memory that early. >> >>> Hans/Thierry, Can you please comment on this case? >>> >>> Thanks >>> >>> Sowjanya >>> > Also, Can you please help explain on cases where we do/need direct > host1x clients vi/csi drivers unbind?
On 4/15/20 11:39 AM, Sowjanya Komatineni wrote: > > On 4/15/20 10:48 AM, Sowjanya Komatineni wrote: >> >> On 4/15/20 10:47 AM, Sowjanya Komatineni wrote: >>> >>> On 4/15/20 10:21 AM, Sowjanya Komatineni wrote: >>>> >>>> On 4/15/20 9:54 AM, Sowjanya Komatineni wrote: >>>>> >>>>> On 4/15/20 7:22 AM, Dmitry Osipenko wrote: >>>>>> External email: Use caution opening links or attachments >>>>>> >>>>>> >>>>>> 15.04.2020 05:57, Sowjanya Komatineni пишет: >>>>>>> +static int tegra_csi_remove(struct platform_device *pdev) >>>>>>> +{ >>>>>>> + struct tegra_csi *csi = platform_get_drvdata(pdev); >>>>>>> + int err; >>>>>>> + >>>>>>> + err = host1x_client_unregister(&csi->client); >>>>>>> + if (err < 0) { >>>>>>> + dev_err(csi->dev, >>>>>>> + "failed to unregister host1x client: >>>>>>> %d\n", err); >>>>>>> + return err; >>>>>>> + } >>>>>>> + >>>>>>> + pm_runtime_disable(csi->dev); >>>>>>> + kfree(csi); >>>>>> IIRC, the driver removal is invoked on the unbinding. Hence, I'm not >>>>>> sure how moving away from the resource-managed API helps here. >>>>>> Could you >>>>>> please explain in a more details? >>>>>> >>>>>> Have you tried to test this driver under KASAN? I suspect that >>>>>> you just >>>>>> masked the problem, instead of fixing it. Tested with kmemleak scan and did not see any memory leaks >>>>> Using devm_kzalloc for vi/csi structures based on prior feedback >>>>> request to switch to use kzalloc all over this driver. >>>>> >>>>> Hi Hans, >>>>> >>>>> video devices lifetime is till video device nodes are released. >>>>> So, v4l2 device release callback does the release of tegra channel >>>>> allocation which hold video device. >>>>> >>>>> Below are the 3 possible cases of unbind/unload, >>>>> >>>>> 1. during tegra-video module unload, if v4l2 device refcnt is not >>>>> 0 which is the case when any of video device node handle is kept >>>>> opened then unloading module will not happen and module refcnt is >>>>> also non-zero and unloading tegra-video module reports module in use. > v4l2 device is associated with host1x device where during > v4l2_device_register get_device causes refcnt of tegra video host1x > device to increase and prevents allowing module unload/load till v4l2 > device release happens. > > >>>>> 2. during tegra-video driver unbind, tegra-video driver removal >>>>> will do vi/csi clients exit ops which unregisters video device >>>>> allocated memory during release callback of v4l2 device. vi/csi >>>>> structure allocation remains same as vi/csi driver removal will >>>>> not happen in this case. >>>>> >>>>> >>>>> 3. during direct host1x client drivers vi/csi unbind, both >>>>> host1x_clients vi/csi gets unregistered, deletes host1x logical >>>>> device which executes tegra-video driver removal() -> vi/csi >>>>> exit() before vi/csi memory gets freed in vi/csi driver remove(). >>>>> >>>>> So, any active streaming will stop and video devices are >>>>> unregistered during direct client driver unbind prior to freeing >>>>> vi/csi memory. >>>>> >>>>> Also vi/csi driver remove does explicit free vi/csi as its >>>>> allocated with kzalloc. So not sure how using kzalloc is different >>>>> to devm_kzalloc for vi/csi structure in terms of when vi/csi >>>>> memory gets freed? >>>>> >>>>> Except for channel allocation which holds video device and as >>>>> video device life time is beyond tegra-video module unbind->vi >>>>> exit(), looks like we can use devm_kzalloc for vi/csi. >>>>> >>>>> >>>>> Can you please comment if you still think we need to use kzalloc >>>>> rather than devm_kzalloc for vi/csi structure allocation? >>>>> >>>>> Thanks >>>>> >>>>> Sowjanya >>>>> >>>> One more case is when video device node is kept opened with >>>> v4l2-ctl sleep (rather than streaming), where it will keep device >>>> node open for specified time and if direct vi client driver unbind >>>> happens then vi driver remove() will free vi memory before v4l2 >>>> device release happens. >>>> >>>> But I don't see any crash or errors with this case. > > In the above case, channels allocated memory release may not happen in > this case as list head pointer will be gone when vi memory is freed > during direct client unbind and by the time v4l2 device release > callback gets executed vi channels list head is gone. > > Also, freeing vi structure memory can't be done in v4l2 device release > callback either. > >>>> >>>> Also if we allow direct client driver unbind, then vi structure >>>> memory lifetime should also be till v4l2 device release happens. >>>> >>>> But we can free vi in v4l2 device release callback as in case when >>>> device node is not kept opened, video device release happens >>>> immediate and we cant free vi that early. >>> >>> typo fix: >>> >>> But we can't free vi structure memory allocation in v4l2 device >>> release callback as in case when device node is not kept opened, >>> device release happens immediate and we can't free vi structure >>> memory that early. >>> > >>>> Hans/Thierry, Can you please comment on this case? >>>> >>>> Thanks >>>> >>>> Sowjanya >>>> >> Also, Can you please help explain on cases where we do/need direct >> host1x clients vi/csi drivers unbind?
15.04.2020 21:53, Sowjanya Komatineni пишет: ... >>>>>>> Have you tried to test this driver under KASAN? I suspect that >>>>>>> you just >>>>>>> masked the problem, instead of fixing it. > Tested with kmemleak scan and did not see any memory leaks You should get use-after-free and not memleak.
On 4/15/20 12:21 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 15.04.2020 21:53, Sowjanya Komatineni пишет: > ... >>>>>>>> Have you tried to test this driver under KASAN? I suspect that >>>>>>>> you just >>>>>>>> masked the problem, instead of fixing it. >> Tested with kmemleak scan and did not see any memory leaks > You should get use-after-free and not memleak. I don't see use-after-free bugs during the testing. But as mentioned when direct vi/csi client driver unbind happens while video device node is kept opened, vi driver remove will free vi structure memory but actual video device memory which is part of channels remains but list head gets lost when vi structure is freed. So, when device node is released and executes release callback as list head is lost it can't free allocated channels which is not good. This happens only with direct host1x client vi/csi driver unbind. Need to find better place to free host1x client driver data structure to allow direct client driver unbind->bind.
With minor change of not using vi reference after host1x_client_unregister and freeing vi during v4l2 device release works. For csi, we can use devm_kzalloc for now untill we decide later if we want to expose async subdev nodes during sensor support. Will have this fix in v8 with a comment in vi_remove to make sure not to use vi reference after host1x_client_unregister. Will test more and will release v8 with above fix to allow direct host1x client driver unbind. Thanks sowjanya On 4/15/20 12:51 PM, Sowjanya Komatineni wrote: > > On 4/15/20 12:21 PM, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 15.04.2020 21:53, Sowjanya Komatineni пишет: >> ... >>>>>>>>> Have you tried to test this driver under KASAN? I suspect that >>>>>>>>> you just >>>>>>>>> masked the problem, instead of fixing it. >>> Tested with kmemleak scan and did not see any memory leaks >> You should get use-after-free and not memleak. > I don't see use-after-free bugs during the testing. > > But as mentioned when direct vi/csi client driver unbind happens while > video device node is kept opened, vi driver remove will free vi > structure memory but actual video device memory which is part of > channels remains but list head gets lost when vi structure is freed. > > So, when device node is released and executes release callback as list > head is lost it can't free allocated channels which is not good. > > This happens only with direct host1x client vi/csi driver unbind. > > Need to find better place to free host1x client driver data structure > to allow direct client driver unbind->bind. >
Sorry please ignore. We can't free vi during v4l2 device release as when no device nodes are opened, vi free happens right away during host1x_video_remove. With this tegra-video driver unbind ->bind will not work as vi memory allocated during vi_probe gets freed during v4l2 device release so during bind init() callback execution will crash as vi got freed while vi driver is still bound to device. Will wait for Hans/Thierry comments as I see dependency depending on where unbind/bind happens. On 4/15/20 4:08 PM, Sowjanya Komatineni wrote: > With minor change of not using vi reference after > host1x_client_unregister and freeing vi during v4l2 device release works. > > For csi, we can use devm_kzalloc for now untill we decide later if we > want to expose async subdev nodes during sensor support. > > Will have this fix in v8 with a comment in vi_remove to make sure not > to use vi reference after host1x_client_unregister. > > Will test more and will release v8 with above fix to allow direct > host1x client driver unbind. > > Thanks > > sowjanya > > > On 4/15/20 12:51 PM, Sowjanya Komatineni wrote: >> >> On 4/15/20 12:21 PM, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 15.04.2020 21:53, Sowjanya Komatineni пишет: >>> ... >>>>>>>>>> Have you tried to test this driver under KASAN? I suspect that >>>>>>>>>> you just >>>>>>>>>> masked the problem, instead of fixing it. >>>> Tested with kmemleak scan and did not see any memory leaks >>> You should get use-after-free and not memleak. >> I don't see use-after-free bugs during the testing. >> >> But as mentioned when direct vi/csi client driver unbind happens >> while video device node is kept opened, vi driver remove will free vi >> structure memory but actual video device memory which is part of >> channels remains but list head gets lost when vi structure is freed. >> >> So, when device node is released and executes release callback as >> list head is lost it can't free allocated channels which is not good. >> >> This happens only with direct host1x client vi/csi driver unbind. >> >> Need to find better place to free host1x client driver data structure >> to allow direct client driver unbind->bind. >>
tegra-video module unload->load and tegra-video driver unbind->bind are good. Will have v8 to switch to use devm_kzalloc for vi/csi and will revisit direct host1x client driver unbind->bind later. Thanks Sowjanya On 4/15/20 4:28 PM, Sowjanya Komatineni wrote: > Sorry please ignore. > > We can't free vi during v4l2 device release as when no device nodes > are opened, vi free happens right away during host1x_video_remove. > > With this tegra-video driver unbind ->bind will not work as vi memory > allocated during vi_probe gets freed during v4l2 device release so > during bind init() callback execution will crash as vi got freed while > vi driver is still bound to device. > > Will wait for Hans/Thierry comments as I see dependency depending on > where unbind/bind happens. > > > On 4/15/20 4:08 PM, Sowjanya Komatineni wrote: >> With minor change of not using vi reference after >> host1x_client_unregister and freeing vi during v4l2 device release >> works. >> >> For csi, we can use devm_kzalloc for now untill we decide later if we >> want to expose async subdev nodes during sensor support. >> >> Will have this fix in v8 with a comment in vi_remove to make sure not >> to use vi reference after host1x_client_unregister. >> >> Will test more and will release v8 with above fix to allow direct >> host1x client driver unbind. >> >> Thanks >> >> sowjanya >> >> >> On 4/15/20 12:51 PM, Sowjanya Komatineni wrote: >>> >>> On 4/15/20 12:21 PM, Dmitry Osipenko wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> 15.04.2020 21:53, Sowjanya Komatineni пишет: >>>> ... >>>>>>>>>>> Have you tried to test this driver under KASAN? I suspect that >>>>>>>>>>> you just >>>>>>>>>>> masked the problem, instead of fixing it. >>>>> Tested with kmemleak scan and did not see any memory leaks >>>> You should get use-after-free and not memleak. >>> I don't see use-after-free bugs during the testing. >>> >>> But as mentioned when direct vi/csi client driver unbind happens >>> while video device node is kept opened, vi driver remove will free >>> vi structure memory but actual video device memory which is part of >>> channels remains but list head gets lost when vi structure is freed. >>> >>> So, when device node is released and executes release callback as >>> list head is lost it can't free allocated channels which is not good. >>> >>> This happens only with direct host1x client vi/csi driver unbind. >>> >>> Need to find better place to free host1x client driver data >>> structure to allow direct client driver unbind->bind. >>>