[PATCH] i2c: saa7115: Support CJC7113 detection
From: Kevin Fitch
It's been reported that CJC7113 devices are returning
all 1s when reading register 0:
"" found @ 0x4a (stk1160)
This new device is apparently compatible with SA7113, so let's
add a quirk to allow its autodetection. Given there isn't
any known differences with SAA7113, this commit does not
introduces a new saa711x_model value.
Reported-by: Philippe Desrochers
Signed-off-by: Kevin Fitch
Signed-off-by: Ezequiel Garcia
---
drivers/media/i2c/saa7115.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/drivers/media/i2c/saa7115.c b/drivers/media/i2c/saa7115.c
index 24d2b76dbe97..04f266d0a1ef 100644
--- a/drivers/media/i2c/saa7115.c
+++ b/drivers/media/i2c/saa7115.c
@@ -1794,6 +1794,21 @@ static int saa711x_detect_chip(struct i2c_client *client,
return GM7113C;
}
+ /* Check if it is a CJC7113 */
+ if (!memcmp(name, "", CHIP_VER_SIZE)) {
+ strlcpy(name, "cjc7113", CHIP_VER_SIZE);
+
+ if (!autodetect && strcmp(name, id->name))
+ return -EINVAL;
+
+ v4l_dbg(1, debug, client,
+ "It seems to be a %s chip (%*ph) @ 0x%x.\n",
+ name, 16, chip_ver, client->addr << 1);
+
+ /* CJC7113 seems to be SAA7113-compatible */
+ return SAA7113;
+ }
+
/* Chip was not discovered. Return its ID and don't bind */
v4l_dbg(1, debug, client, "chip %*ph @ 0x%x is unknown.\n",
16, chip_ver, client->addr << 1);
--
2.7.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
cron job: media_tree daily build: OK
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Wed Mar 23 04:00:25 CET 2016 git branch: test git hash: 2705c1a96f978450377f1019d4bef34190b4ef05 gcc version:i686-linux-gcc (GCC) 5.3.0 sparse version: v0.5.0-56-g7647c77 smatch version: v0.5.0-3353-gcae47da host hardware: x86_64 host os:4.4.0-164 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-exynos: OK linux-git-arm-mx: OK linux-git-arm-omap: OK linux-git-arm-omap1: OK linux-git-arm-pxa: OK linux-git-blackfin-bf561: OK linux-git-i686: OK linux-git-m32r: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK linux-2.6.36.4-i686: OK linux-2.6.37.6-i686: OK linux-2.6.38.8-i686: OK linux-2.6.39.4-i686: OK linux-3.0.60-i686: OK linux-3.1.10-i686: OK linux-3.2.37-i686: OK linux-3.3.8-i686: OK linux-3.4.27-i686: OK linux-3.5.7-i686: OK linux-3.6.11-i686: OK linux-3.7.4-i686: OK linux-3.8-i686: OK linux-3.9.2-i686: OK linux-3.10.1-i686: OK linux-3.11.1-i686: OK linux-3.12.23-i686: OK linux-3.13.11-i686: OK linux-3.14.9-i686: OK linux-3.15.2-i686: OK linux-3.16.7-i686: OK linux-3.17.8-i686: OK linux-3.18.7-i686: OK linux-3.19-i686: OK linux-4.0-i686: OK linux-4.1.1-i686: OK linux-4.2-i686: OK linux-4.3-i686: OK linux-4.4-i686: OK linux-4.5-i686: OK linux-2.6.36.4-x86_64: OK linux-2.6.37.6-x86_64: OK linux-2.6.38.8-x86_64: OK linux-2.6.39.4-x86_64: OK linux-3.0.60-x86_64: OK linux-3.1.10-x86_64: OK linux-3.2.37-x86_64: OK linux-3.3.8-x86_64: OK linux-3.4.27-x86_64: OK linux-3.5.7-x86_64: OK linux-3.6.11-x86_64: OK linux-3.7.4-x86_64: OK linux-3.8-x86_64: OK linux-3.9.2-x86_64: OK linux-3.10.1-x86_64: OK linux-3.11.1-x86_64: OK linux-3.12.23-x86_64: OK linux-3.13.11-x86_64: OK linux-3.14.9-x86_64: OK linux-3.15.2-x86_64: OK linux-3.16.7-x86_64: OK linux-3.17.8-x86_64: OK linux-3.18.7-x86_64: OK linux-3.19-x86_64: OK linux-4.0-x86_64: OK linux-4.1.1-x86_64: OK linux-4.2-x86_64: OK linux-4.3-x86_64: OK linux-4.4-x86_64: OK linux-4.5-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS smatch: ERRORS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Wednesday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Wednesday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/media.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[sailus-media:master 9/14] DockBook: include/media/media-device.h:629: warning: No description found for parameter 'mdev'
tree: git://linuxtv.org/media_tree.git master head: 2705c1a96f978450377f1019d4bef34190b4ef05 commit: 44ff16d0b7ccb4c872de7a53196b2d3f83089607 [9/14] [media] media-device: use kref for media_device instance reproduce: make htmldocs All warnings (new ones prefixed by >>): include/linux/init.h:1: warning: no structured comments found kernel/sys.c:1: warning: no structured comments found drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found drivers/dma-buf/reservation.c:1: warning: no structured comments found include/linux/reservation.h:1: warning: no structured comments found >> include/media/media-device.h:629: warning: No description found for >> parameter 'mdev' >> include/media/media-device.h:629: warning: Excess function parameter 'dev' >> description in 'media_device_unregister_devres' >> include/media/media-device.h:629: warning: No description found for >> parameter 'mdev' >> include/media/media-device.h:629: warning: Excess function parameter 'dev' >> description in 'media_device_unregister_devres' vim +/mdev +629 include/media/media-device.h 613 * 614 * @dev: pointer to struct &device. 615 */ 616 struct media_device *media_device_find_devres(struct device *dev); 617 618 /** 619 * media_device_unregister_devres) - Unregister media device allocated as 620 * as device resource 621 * 622 * @dev: pointer to struct &device. 623 * 624 * Devices allocated via media_device_get_devres should be de-alocalted 625 * and freed via this function. Callers should not call 626 * media_device_unregister() nor media_device_cleanup() on devices 627 * allocated via media_device_get_devres(). 628 */ > 629 void media_device_unregister_devres(struct media_device *mdev); 630 631 /* Iterate over all entities. */ 632 #define media_device_for_each_entity(entity, mdev) \ 633 list_for_each_entry(entity, &(mdev)->entities, graph_obj.list) 634 635 /* Iterate over all interfaces. */ 636 #define media_device_for_each_intf(intf, mdev) \ 637 list_for_each_entry(intf, &(mdev)->interfaces, graph_obj.list) --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: Binary data
Re: [PATCH 2/2] [media] exynos4-is: FIMC port parse should fail if there's no endpoint
Hello Sylwester, On 03/11/2016 10:03 AM, Sylwester Nawrocki wrote: > On 03/04/2016 09:20 PM, Javier Martinez Canillas wrote: >> The fimc_md_parse_port_node() function return 0 if an endpoint node is >> not found but according to Documentation/devicetree/bindings/graph.txt, >> a port must always have at least one enpoint. >> >> So return an -EINVAL errno code to the caller instead, so it knows that >> the port node parse failed due an invalid Device Tree description. > > I don't think it is forbidden to have a port node in device tree > containing no endpoint nodes. Empty port node means only that, > for example, a subsystem has a port/bus for connecting external > devices but nothing is actually connected to it. > > In case of Exynos CSIS it might not be so useful to have an empty > port node specified in some top level *.dtsi file and only > the endpoints specified in a board specific dts file. Nevertheless, > I wouldn't be saying in general a port node must always have some > endpoint node defined. > You are right, I asked Laurent and he confirms what you said that it's possible to have ports with no endpoints. I still think the DT binding docs could be more clear but that's a separate issue. > I could apply this patch as it doesn't do any harm considering > existing dts files in the kernel tree (arch/arm/boot/dts/ > exynos4412-trats2.dts), but the commit description would need to > be changed. > No worries, the current code is correct if endpoints are optional and this patch is wrong so it should not be applied. Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] [media] media-device: use kref for media_device instance
On 03/18/2016 06:42 PM, Mauro Carvalho Chehab wrote:
> Now that the media_device can be used by multiple drivers,
> via devres, we need to be sure that it will be dropped only
> when all drivers stop using it.
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
>
> v2: The kref is now used only when media_device is allocated via
> the media_device*_devress. This warrants that other drivers won't be
> affected, and that we can keep media_device_cleanup() balanced with
> media_device_init().
Tested bind_unbind au0828 loop 1000 times, followed by bind_unbind
snd_usb_audio loop 1000 times. Didn't see any lock warnings on a
KASAN enabled kernel (lock testing enabled). No use-after-free errors
during these runs.
Ran device removal test and rmmod and modprobe tests on both drivers.
Generated graph after the runs and the graph looks good.
Reviewed-by: Shuah Khan
Tested-by: Shuah Khan
thanks,
-- Shuah
>
> drivers/media/media-device.c | 117
> +
> drivers/media/usb/au0828/au0828-core.c | 3 +-
> include/media/media-device.h | 28
> sound/usb/media.c | 3 +-
> 4 files changed, 118 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index c32fa15cc76e..4a97d92a7e7d 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -707,11 +707,16 @@ void media_device_init(struct media_device *mdev)
> }
> EXPORT_SYMBOL_GPL(media_device_init);
>
> -void media_device_cleanup(struct media_device *mdev)
> +static void __media_device_cleanup(struct media_device *mdev)
> {
> ida_destroy(&mdev->entity_internal_idx);
> mdev->entity_internal_idx_max = 0;
> media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
> +}
> +
> +void media_device_cleanup(struct media_device *mdev)
> +{
> + __media_device_cleanup(mdev);
> mutex_destroy(&mdev->graph_mutex);
> }
> EXPORT_SYMBOL_GPL(media_device_cleanup);
> @@ -721,6 +726,9 @@ int __must_check __media_device_register(struct
> media_device *mdev,
> {
> int ret;
>
> + /* Check if mdev was ever registered at all */
> + mutex_lock(&mdev->graph_mutex);
> +
> /* Register the device node. */
> mdev->devnode.fops = &media_device_fops;
> mdev->devnode.parent = mdev->dev;
> @@ -731,17 +739,19 @@ int __must_check __media_device_register(struct
> media_device *mdev,
>
> ret = media_devnode_register(&mdev->devnode, owner);
> if (ret < 0)
> - return ret;
> + goto err;
>
> ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
> if (ret < 0) {
> media_devnode_unregister(&mdev->devnode);
> - return ret;
> + goto err;
> }
>
> dev_dbg(mdev->dev, "Media device registered\n");
>
> - return 0;
> +err:
> + mutex_unlock(&mdev->graph_mutex);
> + return ret;
> }
> EXPORT_SYMBOL_GPL(__media_device_register);
>
> @@ -773,24 +783,13 @@ void media_device_unregister_entity_notify(struct
> media_device *mdev,
> }
> EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
>
> -void media_device_unregister(struct media_device *mdev)
> +static void __media_device_unregister(struct media_device *mdev)
> {
> struct media_entity *entity;
> struct media_entity *next;
> struct media_interface *intf, *tmp_intf;
> struct media_entity_notify *notify, *nextp;
>
> - if (mdev == NULL)
> - return;
> -
> - mutex_lock(&mdev->graph_mutex);
> -
> - /* Check if mdev was ever registered at all */
> - if (!media_devnode_is_registered(&mdev->devnode)) {
> - mutex_unlock(&mdev->graph_mutex);
> - return;
> - }
> -
> /* Remove all entities from the media device */
> list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
> __media_device_unregister_entity(entity);
> @@ -807,38 +806,98 @@ void media_device_unregister(struct media_device *mdev)
> kfree(intf);
> }
>
> - mutex_unlock(&mdev->graph_mutex);
> -
> - device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> - media_devnode_unregister(&mdev->devnode);
> + /* Check if mdev devnode was registered */
> + if (media_devnode_is_registered(&mdev->devnode)) {
> + device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> + media_devnode_unregister(&mdev->devnode);
> + }
>
> dev_dbg(mdev->dev, "Media device unregistered\n");
> }
> +
> +void media_device_unregister(struct media_device *mdev)
> +{
> + if (mdev == NULL)
> + return;
> +
> + mutex_lock(&mdev->graph_mutex);
> + __media_device_unregister(mdev);
> + mutex_unlock(&mdev->graph_mutex);
> +}
> EXPORT_SYMBOL_GPL(media_device_unregister);
>
> static void media_device_release_devres(struct device *dev, void *res)
> {
> }
>
Re: [PATCH 4/5] [media] media-device: use kref for media_device instance
On 03/22/2016 01:56 PM, Shuah Khan wrote:
> On 03/16/2016 06:04 AM, Mauro Carvalho Chehab wrote:
>> Now that the media_device can be used by multiple drivers,
>> via devres, we need to be sure that it will be dropped only
>> when all drivers stop using it.
>>
>> Signed-off-by: Mauro Carvalho Chehab
>
> Tested bind_unbind au0828 loop 1000 times, followed by bind_unbind
> snd_usb_audio loop 1000 times. Didn't see any lock warnings on a
> KASAN enabled kernel (lock testing enabled). No use-after-free errors
> during these runs.
>
> Ran device removal test and rmmod and modprobe tests on both drivers.
>
> Generated graph after the runs and the graph looks good.
>
> Reviewed-by: Shuah Khan
> Tested-by: Shuah Khan
>
Acked the wrong patch. Sorry. Will ack the v2 which is the one I tested.
thanks,
-- Shuah
>> ---
>> drivers/media/media-device.c | 48
>> +++-
>> include/media/media-device.h | 3 +++
>> 2 files changed, 37 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index c32fa15cc76e..38e6c319fe6e 100644
>> --- a/drivers/media/media-device.c
>> +++ b/drivers/media/media-device.c
>> @@ -721,6 +721,15 @@ int __must_check __media_device_register(struct
>> media_device *mdev,
>> {
>> int ret;
>>
>> +/* Check if mdev was ever registered at all */
>> +mutex_lock(&mdev->graph_mutex);
>> +if (media_devnode_is_registered(&mdev->devnode)) {
>> +kref_get(&mdev->kref);
>> +mutex_unlock(&mdev->graph_mutex);
>> +return 0;
>> +}
>> +kref_init(&mdev->kref);
>> +
>> /* Register the device node. */
>> mdev->devnode.fops = &media_device_fops;
>> mdev->devnode.parent = mdev->dev;
>> @@ -730,8 +739,10 @@ int __must_check __media_device_register(struct
>> media_device *mdev,
>> mdev->topology_version = 0;
>>
>> ret = media_devnode_register(&mdev->devnode, owner);
>> -if (ret < 0)
>> +if (ret < 0) {
>> +media_devnode_unregister(&mdev->devnode);
>> return ret;
>> +}
>>
>> ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
>> if (ret < 0) {
>> @@ -739,6 +750,7 @@ int __must_check __media_device_register(struct
>> media_device *mdev,
>> return ret;
>> }
>>
>> +mutex_unlock(&mdev->graph_mutex);
>> dev_dbg(mdev->dev, "Media device registered\n");
>>
>> return 0;
>> @@ -773,23 +785,15 @@ void media_device_unregister_entity_notify(struct
>> media_device *mdev,
>> }
>> EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
>>
>> -void media_device_unregister(struct media_device *mdev)
>> +static void do_media_device_unregister(struct kref *kref)
>> {
>> +struct media_device *mdev;
>> struct media_entity *entity;
>> struct media_entity *next;
>> struct media_interface *intf, *tmp_intf;
>> struct media_entity_notify *notify, *nextp;
>>
>> -if (mdev == NULL)
>> -return;
>> -
>> -mutex_lock(&mdev->graph_mutex);
>> -
>> -/* Check if mdev was ever registered at all */
>> -if (!media_devnode_is_registered(&mdev->devnode)) {
>> -mutex_unlock(&mdev->graph_mutex);
>> -return;
>> -}
>> +mdev = container_of(kref, struct media_device, kref);
>>
>> /* Remove all entities from the media device */
>> list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
>> @@ -807,13 +811,26 @@ void media_device_unregister(struct media_device *mdev)
>> kfree(intf);
>> }
>>
>> -mutex_unlock(&mdev->graph_mutex);
>> +/* Check if mdev devnode was registered */
>> +if (!media_devnode_is_registered(&mdev->devnode))
>> +return;
>>
>> device_remove_file(&mdev->devnode.dev, &dev_attr_model);
>> media_devnode_unregister(&mdev->devnode);
>>
>> dev_dbg(mdev->dev, "Media device unregistered\n");
>> }
>> +
>> +void media_device_unregister(struct media_device *mdev)
>> +{
>> +if (mdev == NULL)
>> +return;
>> +
>> +mutex_lock(&mdev->graph_mutex);
>> +kref_put(&mdev->kref, do_media_device_unregister);
>> +mutex_unlock(&mdev->graph_mutex);
>> +
>> +}
>> EXPORT_SYMBOL_GPL(media_device_unregister);
>>
>> static void media_device_release_devres(struct device *dev, void *res)
>> @@ -825,13 +842,16 @@ struct media_device *media_device_get_devres(struct
>> device *dev)
>> struct media_device *mdev;
>>
>> mdev = devres_find(dev, media_device_release_devres, NULL, NULL);
>> -if (mdev)
>> +if (mdev) {
>> +kref_get(&mdev->kref);
>> return mdev;
>> +}
>>
>> mdev = devres_alloc(media_device_release_devres,
>> sizeof(struct media_device), GFP_KERNEL);
>> if (!mdev)
>> return NULL;
>> +
>> return devres_get(dev, mdev, NULL, NULL);
>> }
>> EXPORT_SYMBOL_GPL(media_device_get_de
Re: [PATCH 5/5] [media] media-device: make media_device_cleanup() static
On 03/16/2016 06:04 AM, Mauro Carvalho Chehab wrote:
> When multiple drivers are sharing the media_device struct,
> one driver cannot know the right moment to cleanup the
> media_device struct, because it can happen only when the
> struct is not used by the other drivers.
>
> So, let's call media_device_cleanup() internally, and
> ensure that media_device_unregister() will do the right thing,
> if the media device is not fully initialized.
>
> Signed-off-by: Mauro Carvalho Chehab
Tested bind_unbind au0828 loop 1000 times, followed by bind_unbind
snd_usb_audio loop 1000 times. Didn't see any lock warnings on a
KASAN enabled kernel (lock testing enabled). No use-after-free errors
during these runs.
Ran device removal test and rmmod and modprobe tests on both drivers.
Generated graph after the runs and the graph looks good.
For the au0828 and snd_usb_audio change:
Reviewed-by: Shuah Khan
Tested-by: Shuah Khan
thanks,
-- Shuah
> ---
> drivers/media/common/siano/smsdvb-main.c | 1 -
> drivers/media/media-device.c | 13 ++---
> drivers/media/pci/saa7134/saa7134-core.c | 1 -
> drivers/media/platform/exynos4-is/media-dev.c | 3 +--
> drivers/media/platform/omap3isp/isp.c | 1 -
> drivers/media/platform/s3c-camif/camif-core.c | 2 --
> drivers/media/platform/vsp1/vsp1_drv.c| 1 -
> drivers/media/platform/xilinx/xilinx-vipp.c | 3 +--
> drivers/media/usb/au0828/au0828-core.c| 1 -
> drivers/media/usb/cx231xx/cx231xx-cards.c | 1 -
> drivers/media/usb/dvb-usb-v2/dvb_usb_core.c | 1 -
> drivers/media/usb/dvb-usb/dvb-usb-dvb.c | 1 -
> drivers/media/usb/em28xx/em28xx-cards.c | 1 -
> drivers/media/usb/siano/smsusb.c | 2 +-
> drivers/media/usb/uvc/uvc_driver.c| 4 +---
> include/media/media-device.h | 10 --
> 16 files changed, 10 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/media/common/siano/smsdvb-main.c
> b/drivers/media/common/siano/smsdvb-main.c
> index 9148e14c9d07..711c389c05e3 100644
> --- a/drivers/media/common/siano/smsdvb-main.c
> +++ b/drivers/media/common/siano/smsdvb-main.c
> @@ -617,7 +617,6 @@ static void smsdvb_media_device_unregister(struct
> smsdvb_client_t *client)
> if (!coredev->media_dev)
> return;
> media_device_unregister(coredev->media_dev);
> - media_device_cleanup(coredev->media_dev);
> kfree(coredev->media_dev);
> coredev->media_dev = NULL;
> #endif
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 38e6c319fe6e..0c7371eeda84 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -707,14 +707,12 @@ void media_device_init(struct media_device *mdev)
> }
> EXPORT_SYMBOL_GPL(media_device_init);
>
> -void media_device_cleanup(struct media_device *mdev)
> +static void media_device_cleanup(struct media_device *mdev)
> {
> ida_destroy(&mdev->entity_internal_idx);
> mdev->entity_internal_idx_max = 0;
> media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
> - mutex_destroy(&mdev->graph_mutex);
> }
> -EXPORT_SYMBOL_GPL(media_device_cleanup);
>
> int __must_check __media_device_register(struct media_device *mdev,
>struct module *owner)
> @@ -812,11 +810,12 @@ static void do_media_device_unregister(struct kref
> *kref)
> }
>
> /* Check if mdev devnode was registered */
> - if (!media_devnode_is_registered(&mdev->devnode))
> - return;
> + if (media_devnode_is_registered(&mdev->devnode)) {
> + device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> + media_devnode_unregister(&mdev->devnode);
> + }
>
> - device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> - media_devnode_unregister(&mdev->devnode);
> + media_device_cleanup(mdev);
>
> dev_dbg(mdev->dev, "Media device unregistered\n");
> }
> diff --git a/drivers/media/pci/saa7134/saa7134-core.c
> b/drivers/media/pci/saa7134/saa7134-core.c
> index c0e1780ec831..213dc71f09eb 100644
> --- a/drivers/media/pci/saa7134/saa7134-core.c
> +++ b/drivers/media/pci/saa7134/saa7134-core.c
> @@ -813,7 +813,6 @@ static void saa7134_unregister_media_device(struct
> saa7134_dev *dev)
> if (!dev->media_dev)
> return;
> media_device_unregister(dev->media_dev);
> - media_device_cleanup(dev->media_dev);
> kfree(dev->media_dev);
> dev->media_dev = NULL;
> #endif
> diff --git a/drivers/media/platform/exynos4-is/media-dev.c
> b/drivers/media/platform/exynos4-is/media-dev.c
> index feb521f28e14..8c106fda7b84 100644
> --- a/drivers/media/platform/exynos4-is/media-dev.c
> +++ b/drivers/media/platform/exynos4-is/media-dev.c
> @@ -1501,7 +1501,7 @@ err_clk:
> err_m_ent:
> fimc_md_unregister_entities(fmd);
> err_md:
> - media_device_cleanup(&fmd->media_dev);
> + media_device_unregister(&f
Re: [PATCH 4/5] [media] media-device: use kref for media_device instance
On 03/16/2016 06:04 AM, Mauro Carvalho Chehab wrote:
> Now that the media_device can be used by multiple drivers,
> via devres, we need to be sure that it will be dropped only
> when all drivers stop using it.
>
> Signed-off-by: Mauro Carvalho Chehab
Tested bind_unbind au0828 loop 1000 times, followed by bind_unbind
snd_usb_audio loop 1000 times. Didn't see any lock warnings on a
KASAN enabled kernel (lock testing enabled). No use-after-free errors
during these runs.
Ran device removal test and rmmod and modprobe tests on both drivers.
Generated graph after the runs and the graph looks good.
Reviewed-by: Shuah Khan
Tested-by: Shuah Khan
thanks,
-- Shuah
> ---
> drivers/media/media-device.c | 48
> +++-
> include/media/media-device.h | 3 +++
> 2 files changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index c32fa15cc76e..38e6c319fe6e 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -721,6 +721,15 @@ int __must_check __media_device_register(struct
> media_device *mdev,
> {
> int ret;
>
> + /* Check if mdev was ever registered at all */
> + mutex_lock(&mdev->graph_mutex);
> + if (media_devnode_is_registered(&mdev->devnode)) {
> + kref_get(&mdev->kref);
> + mutex_unlock(&mdev->graph_mutex);
> + return 0;
> + }
> + kref_init(&mdev->kref);
> +
> /* Register the device node. */
> mdev->devnode.fops = &media_device_fops;
> mdev->devnode.parent = mdev->dev;
> @@ -730,8 +739,10 @@ int __must_check __media_device_register(struct
> media_device *mdev,
> mdev->topology_version = 0;
>
> ret = media_devnode_register(&mdev->devnode, owner);
> - if (ret < 0)
> + if (ret < 0) {
> + media_devnode_unregister(&mdev->devnode);
> return ret;
> + }
>
> ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
> if (ret < 0) {
> @@ -739,6 +750,7 @@ int __must_check __media_device_register(struct
> media_device *mdev,
> return ret;
> }
>
> + mutex_unlock(&mdev->graph_mutex);
> dev_dbg(mdev->dev, "Media device registered\n");
>
> return 0;
> @@ -773,23 +785,15 @@ void media_device_unregister_entity_notify(struct
> media_device *mdev,
> }
> EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
>
> -void media_device_unregister(struct media_device *mdev)
> +static void do_media_device_unregister(struct kref *kref)
> {
> + struct media_device *mdev;
> struct media_entity *entity;
> struct media_entity *next;
> struct media_interface *intf, *tmp_intf;
> struct media_entity_notify *notify, *nextp;
>
> - if (mdev == NULL)
> - return;
> -
> - mutex_lock(&mdev->graph_mutex);
> -
> - /* Check if mdev was ever registered at all */
> - if (!media_devnode_is_registered(&mdev->devnode)) {
> - mutex_unlock(&mdev->graph_mutex);
> - return;
> - }
> + mdev = container_of(kref, struct media_device, kref);
>
> /* Remove all entities from the media device */
> list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
> @@ -807,13 +811,26 @@ void media_device_unregister(struct media_device *mdev)
> kfree(intf);
> }
>
> - mutex_unlock(&mdev->graph_mutex);
> + /* Check if mdev devnode was registered */
> + if (!media_devnode_is_registered(&mdev->devnode))
> + return;
>
> device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> media_devnode_unregister(&mdev->devnode);
>
> dev_dbg(mdev->dev, "Media device unregistered\n");
> }
> +
> +void media_device_unregister(struct media_device *mdev)
> +{
> + if (mdev == NULL)
> + return;
> +
> + mutex_lock(&mdev->graph_mutex);
> + kref_put(&mdev->kref, do_media_device_unregister);
> + mutex_unlock(&mdev->graph_mutex);
> +
> +}
> EXPORT_SYMBOL_GPL(media_device_unregister);
>
> static void media_device_release_devres(struct device *dev, void *res)
> @@ -825,13 +842,16 @@ struct media_device *media_device_get_devres(struct
> device *dev)
> struct media_device *mdev;
>
> mdev = devres_find(dev, media_device_release_devres, NULL, NULL);
> - if (mdev)
> + if (mdev) {
> + kref_get(&mdev->kref);
> return mdev;
> + }
>
> mdev = devres_alloc(media_device_release_devres,
> sizeof(struct media_device), GFP_KERNEL);
> if (!mdev)
> return NULL;
> +
> return devres_get(dev, mdev, NULL, NULL);
> }
> EXPORT_SYMBOL_GPL(media_device_get_devres);
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index ca3871b853ba..73c16e6e6b6b 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -
Re: [PATCH 3/5] [media] au0828: Unregister notifiers
On 03/16/2016 06:04 AM, Mauro Carvalho Chehab wrote:
> If au0828 gets removed, we need to remove the notifiers.
>
> Signed-off-by: Mauro Carvalho Chehab
Tested bind_unbind au0828 loop 1000 times, followed by bind_unbind
snd_usb_audio loop 1000 times. Didn't see any lock warnings on a
KASAN enabled kernel (lock testing enabled). No use-after-free errors
during these runs.
Ran device removal test and rmmod and modprobe tests on both drivers.
Generated graph after the runs and the graph looks good.
Reviewed-by: Shuah Khan
Tested-by: Shuah Khan
thanks,
-- Shuah
> ---
> drivers/media/usb/au0828/au0828-core.c | 34
> --
> 1 file changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/usb/au0828/au0828-core.c
> b/drivers/media/usb/au0828/au0828-core.c
> index 2fcd17d9b1a6..06da73f1ff22 100644
> --- a/drivers/media/usb/au0828/au0828-core.c
> +++ b/drivers/media/usb/au0828/au0828-core.c
> @@ -131,21 +131,35 @@ static int recv_control_msg(struct au0828_dev *dev, u16
> request, u32 value,
> return status;
> }
>
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +static void au0828_media_graph_notify(struct media_entity *new,
> + void *notify_data);
> +#endif
> +
> static void au0828_unregister_media_device(struct au0828_dev *dev)
> {
> -
> #ifdef CONFIG_MEDIA_CONTROLLER
> - if (dev->media_dev &&
> - media_devnode_is_registered(&dev->media_dev->devnode)) {
> - /* clear enable_source, disable_source */
> - dev->media_dev->source_priv = NULL;
> - dev->media_dev->enable_source = NULL;
> - dev->media_dev->disable_source = NULL;
> + struct media_device *mdev = dev->media_dev;
> + struct media_entity_notify *notify, *nextp;
>
> - media_device_unregister(dev->media_dev);
> - media_device_cleanup(dev->media_dev);
> - dev->media_dev = NULL;
> + if (!mdev || !media_devnode_is_registered(&mdev->devnode))
> + return;
> +
> + /* Remove au0828 entity_notify callbacks */
> + list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list) {
> + if (notify->notify != au0828_media_graph_notify)
> + continue;
> + media_device_unregister_entity_notify(mdev, notify);
> }
> +
> + /* clear enable_source, disable_source */
> + dev->media_dev->source_priv = NULL;
> + dev->media_dev->enable_source = NULL;
> + dev->media_dev->disable_source = NULL;
> +
> + media_device_unregister(dev->media_dev);
> + media_device_cleanup(dev->media_dev);
> + dev->media_dev = NULL;
> #endif
> }
>
>
--
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shua...@osg.samsung.com | (970) 217-8978
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] [media] media-device: get rid of the spinlock
On 03/16/2016 06:04 AM, Mauro Carvalho Chehab wrote:
> Right now, the lock schema for media_device struct is messy,
> since sometimes, it is protected via a spin lock, while, for
> media graph traversal, it is protected by a mutex.
>
> Solve this conflict by always using a mutex.
>
> As a side effect, this prevents a bug where the media notifiers
> were called at atomic context.
>
> Signed-off-by: Mauro Carvalho Chehab
Tested bind_unbind au0828 loop 1000 times, followed by bind_unbind
snd_usb_audio loop 1000 times. Didn't see any lock warnings on a
KASAN enabled kernel (lock testing enabled). No use-after-free errors
during these runs.
Ran device removal test and rmmod and modprobe tests on both drivers.
Generated graph after the runs and the graph looks good.
Reviewed-by: Shuah Khan
Tested-by: Shuah Khan
thanks,
-- Shuah
> ---
> drivers/media/media-device.c | 32 ++--
> drivers/media/media-entity.c | 16
> include/media/media-device.h | 6 +-
> 3 files changed, 23 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 6e43c95629ea..c32fa15cc76e 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -90,17 +90,17 @@ static struct media_entity *find_entity(struct
> media_device *mdev, u32 id)
>
> id &= ~MEDIA_ENT_ID_FLAG_NEXT;
>
> - spin_lock(&mdev->lock);
> + mutex_lock(&mdev->graph_mutex);
>
> media_device_for_each_entity(entity, mdev) {
> if (((media_entity_id(entity) == id) && !next) ||
> ((media_entity_id(entity) > id) && next)) {
> - spin_unlock(&mdev->lock);
> + mutex_unlock(&mdev->graph_mutex);
> return entity;
> }
> }
>
> - spin_unlock(&mdev->lock);
> + mutex_unlock(&mdev->graph_mutex);
>
> return NULL;
> }
> @@ -590,12 +590,12 @@ int __must_check media_device_register_entity(struct
> media_device *mdev,
> if (!ida_pre_get(&mdev->entity_internal_idx, GFP_KERNEL))
> return -ENOMEM;
>
> - spin_lock(&mdev->lock);
> + mutex_lock(&mdev->graph_mutex);
>
> ret = ida_get_new_above(&mdev->entity_internal_idx, 1,
> &entity->internal_idx);
> if (ret < 0) {
> - spin_unlock(&mdev->lock);
> + mutex_unlock(&mdev->graph_mutex);
> return ret;
> }
>
> @@ -615,9 +615,6 @@ int __must_check media_device_register_entity(struct
> media_device *mdev,
> (notify)->notify(entity, notify->notify_data);
> }
>
> - spin_unlock(&mdev->lock);
> -
> - mutex_lock(&mdev->graph_mutex);
> if (mdev->entity_internal_idx_max
> >= mdev->pm_count_walk.ent_enum.idx_max) {
> struct media_entity_graph new = { .top = 0 };
> @@ -680,9 +677,9 @@ void media_device_unregister_entity(struct media_entity
> *entity)
> if (mdev == NULL)
> return;
>
> - spin_lock(&mdev->lock);
> + mutex_lock(&mdev->graph_mutex);
> __media_device_unregister_entity(entity);
> - spin_unlock(&mdev->lock);
> + mutex_unlock(&mdev->graph_mutex);
> }
> EXPORT_SYMBOL_GPL(media_device_unregister_entity);
>
> @@ -703,7 +700,6 @@ void media_device_init(struct media_device *mdev)
> INIT_LIST_HEAD(&mdev->pads);
> INIT_LIST_HEAD(&mdev->links);
> INIT_LIST_HEAD(&mdev->entity_notify);
> - spin_lock_init(&mdev->lock);
> mutex_init(&mdev->graph_mutex);
> ida_init(&mdev->entity_internal_idx);
>
> @@ -752,9 +748,9 @@ EXPORT_SYMBOL_GPL(__media_device_register);
> int __must_check media_device_register_entity_notify(struct media_device
> *mdev,
> struct media_entity_notify *nptr)
> {
> - spin_lock(&mdev->lock);
> + mutex_lock(&mdev->graph_mutex);
> list_add_tail(&nptr->list, &mdev->entity_notify);
> - spin_unlock(&mdev->lock);
> + mutex_unlock(&mdev->graph_mutex);
> return 0;
> }
> EXPORT_SYMBOL_GPL(media_device_register_entity_notify);
> @@ -771,9 +767,9 @@ static void
> __media_device_unregister_entity_notify(struct media_device *mdev,
> void media_device_unregister_entity_notify(struct media_device *mdev,
> struct media_entity_notify *nptr)
> {
> - spin_lock(&mdev->lock);
> + mutex_lock(&mdev->graph_mutex);
> __media_device_unregister_entity_notify(mdev, nptr);
> - spin_unlock(&mdev->lock);
> + mutex_unlock(&mdev->graph_mutex);
> }
> EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
>
> @@ -787,11 +783,11 @@ void media_device_unregister(struct media_device *mdev)
> if (mdev == NULL)
> return;
>
> - spin_lock(&mdev->lock);
> + mutex_lock(&mdev->graph_mutex);
>
> /* Check if mdev was ever registered at all */
> if (!media_devnode_is_
Re: [PATCH] [media] au0828: Fix dev_state handling
On 03/22/2016 07:16 AM, Mauro Carvalho Chehab wrote:
> The au0828 dev_state is actually a bit mask. It should not be
> checking with "==" but, instead, with a logic and. There are some
> places where it was doing it wrong.
>
> Fix that by replacing the dev_state set/clear/test with the
> bitops.
>
> Cc: sta...@vger.kernel.org
> Signed-off-by: Mauro Carvalho Chehab
> ---
> Shuah,
>
> Please test.
Looks good. Tested running bind/unbind au0828 loop for 1000 times.
Didn't see any problems and the v4l2_querycap() problem has been
fixed with this patch.
After the above test, ran bind/unbind snd_usb_audio 1000 times.
Didn't see any problems. Generated media graph and the graph
looks good.
Reviewed-by: Shuah Khan
Tested-by: Shuah Khan
thanks,
-- Shuah
>
> I'm testing it here too.
>
> drivers/media/usb/au0828/au0828-core.c | 2 +-
> drivers/media/usb/au0828/au0828-input.c | 4 +--
> drivers/media/usb/au0828/au0828-video.c | 63
> -
> drivers/media/usb/au0828/au0828.h | 9 ++---
> 4 files changed, 39 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/media/usb/au0828/au0828-core.c
> b/drivers/media/usb/au0828/au0828-core.c
> index 060904ed8f20..85c13ca5178f 100644
> --- a/drivers/media/usb/au0828/au0828-core.c
> +++ b/drivers/media/usb/au0828/au0828-core.c
> @@ -184,7 +184,7 @@ static void au0828_usb_disconnect(struct usb_interface
> *interface)
> Set the status so poll routines can check and avoid
> access after disconnect.
> */
> - dev->dev_state = DEV_DISCONNECTED;
> + set_bit(DEV_DISCONNECTED, &dev->dev_state);
>
> au0828_rc_unregister(dev);
> /* Digital TV */
> diff --git a/drivers/media/usb/au0828/au0828-input.c
> b/drivers/media/usb/au0828/au0828-input.c
> index b0f067971979..3d6687f0407d 100644
> --- a/drivers/media/usb/au0828/au0828-input.c
> +++ b/drivers/media/usb/au0828/au0828-input.c
> @@ -130,7 +130,7 @@ static int au0828_get_key_au8522(struct au0828_rc *ir)
> bool first = true;
>
> /* do nothing if device is disconnected */
> - if (ir->dev->dev_state == DEV_DISCONNECTED)
> + if (test_bit(DEV_DISCONNECTED, &ir->dev->dev_state))
> return 0;
>
> /* Check IR int */
> @@ -260,7 +260,7 @@ static void au0828_rc_stop(struct rc_dev *rc)
> cancel_delayed_work_sync(&ir->work);
>
> /* do nothing if device is disconnected */
> - if (ir->dev->dev_state != DEV_DISCONNECTED) {
> + if (!test_bit(DEV_DISCONNECTED, &ir->dev->dev_state)) {
> /* Disable IR */
> au8522_rc_clear(ir, 0xe0, 1 << 4);
> }
> diff --git a/drivers/media/usb/au0828/au0828-video.c
> b/drivers/media/usb/au0828/au0828-video.c
> index 88dcc6e0e178..32d7db96479c 100644
> --- a/drivers/media/usb/au0828/au0828-video.c
> +++ b/drivers/media/usb/au0828/au0828-video.c
> @@ -106,14 +106,13 @@ static inline void print_err_status(struct au0828_dev
> *dev,
>
> static int check_dev(struct au0828_dev *dev)
> {
> - if (dev->dev_state & DEV_DISCONNECTED) {
> + if (test_bit(DEV_DISCONNECTED, &dev->dev_state)) {
> pr_info("v4l2 ioctl: device not present\n");
> return -ENODEV;
> }
>
> - if (dev->dev_state & DEV_MISCONFIGURED) {
> - pr_info("v4l2 ioctl: device is misconfigured; "
> -"close and open it again\n");
> + if (test_bit(DEV_MISCONFIGURED, &dev->dev_state)) {
> + pr_info("v4l2 ioctl: device is misconfigured; close and open it
> again\n");
> return -EIO;
> }
> return 0;
> @@ -521,8 +520,8 @@ static inline int au0828_isoc_copy(struct au0828_dev
> *dev, struct urb *urb)
> if (!dev)
> return 0;
>
> - if ((dev->dev_state & DEV_DISCONNECTED) ||
> - (dev->dev_state & DEV_MISCONFIGURED))
> + if (test_bit(DEV_DISCONNECTED, &dev->dev_state) ||
> + test_bit(DEV_MISCONFIGURED, &dev->dev_state))
> return 0;
>
> if (urb->status < 0) {
> @@ -824,10 +823,10 @@ static int au0828_stream_interrupt(struct au0828_dev
> *dev)
> int ret = 0;
>
> dev->stream_state = STREAM_INTERRUPT;
> - if (dev->dev_state == DEV_DISCONNECTED)
> + if (test_bit(DEV_DISCONNECTED, &dev->dev_state))
> return -ENODEV;
> else if (ret) {
> - dev->dev_state = DEV_MISCONFIGURED;
> + set_bit(DEV_MISCONFIGURED, &dev->dev_state);
> dprintk(1, "%s device is misconfigured!\n", __func__);
> return ret;
> }
> @@ -1026,7 +1025,7 @@ static int au0828_v4l2_open(struct file *filp)
> int ret;
>
> dprintk(1,
> - "%s called std_set %d dev_state %d stream users %d users %d\n",
> + "%s called std_set %d dev_state %ld stream users %d users %d\n",
> __func__, dev->std_set_in_tuner_core, dev->dev_state,
> dev->streaming_users, dev->users);
>
> @@ -1045,7 +1044,7
Re: [PATCH] sound/usb: fix to release stream resources from media_snd_device_delete()
Em Tue, 22 Mar 2016 11:29:34 -0600 Shuah Khan escreveu: > On 03/22/2016 07:03 AM, Shuah Khan wrote: > > On 03/21/2016 10:01 PM, Shuah Khan wrote: > >> On 03/19/2016 07:31 AM, Shuah Khan wrote: > >>> On 03/19/2016 06:10 AM, Mauro Carvalho Chehab wrote: > Em Fri, 18 Mar 2016 20:50:31 -0600 > Shuah Khan escreveu: > > > Fix to release stream resources from media_snd_device_delete() before > > media device is unregistered. Without this change, stream resource free > > is attempted after the media device is unregistered which would result > > in use-after-free errors. > > > > Signed-off-by: Shuah Khan > > --- > > > > - Ran bind/unbind loop (1000 iteration) test on snd-usb-audio > > while running mc_nextgen_test loop (1000 iterations) in parallel. > > - Ran bind/unbind and rmmod/modprobe tests on both drivers. Also > > generated graphs when after bind/unbind, rmmod/modprobe. Graphs > > look good. > > - Note: Please apply the following patch to fix memory leak: > > sound/usb: Fix memory leak in media_snd_stream_delete() during unbind > > https://lkml.org/lkml/2016/3/16/1050 > > Yeah, a way better! > > For normal bind/unbind, it seems to be working fine. Also > for driver's rmmod, so: > > Tested-by: Mauro Carvalho Chehab > > Takashi, > > Could please ack this patch - please see below that the problem > Mauro and I both saw ended up to a latent bug in au0828 that is > in Linux 4.5 as well. It is now fixed. FYI, the patches we're intending to send to fix the issues with au0828 and snd-usb-audio are at my experimental tree: https://git.linuxtv.org/mchehab/experimental.git/log/?h=au0828-unbind-fixes The patches are: f9dca0c46f12 [media] au0828: Fix dev_state handling d9898e2e7bb3 [media] au0828: fix au0828_v4l2_close() dev_state race condition 52a6e1f97587 [media] media-devnode: add missing mutex lock in error handler db268d4f59c5 [media] media-device: Simplify compat32 logic 105817f85b02 sound/usb: fix to release stream resources from media_snd_device_delete() 70fafd948468 sound/usb: Fix memory leak in media_snd_stream_delete() during unbind a78a4b10ecd3 sound/usb/media: use core routine to initialize media_device 9d8830150475 [media] media-device: use kref for media_device instance 544439bf084a [media] media-device: make topology_version u64 4e18ca9ce0c2 [media] media-device: Fix a comment c38077d39c7e [media] media-device: get rid of the spinlock a50d06389fdf sound/usb: fix NULL dereference in usb_audio_probe() b39950960d2b [media] media: au0828 fix to clear enable/disable/change source handlers d9f03ad36a9d [media] v4l2-mc: cleanup a warning 6a4f10cff976 [media] au0828: disable tuner links and cache tuner/decoder We're running some stress tests today, so we may need to send a few other patches later on, but I guess they'll be either at au0828 or at the media core. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sound/usb: fix to release stream resources from media_snd_device_delete()
On 03/22/2016 07:03 AM, Shuah Khan wrote: > On 03/21/2016 10:01 PM, Shuah Khan wrote: >> On 03/19/2016 07:31 AM, Shuah Khan wrote: >>> On 03/19/2016 06:10 AM, Mauro Carvalho Chehab wrote: Em Fri, 18 Mar 2016 20:50:31 -0600 Shuah Khan escreveu: > Fix to release stream resources from media_snd_device_delete() before > media device is unregistered. Without this change, stream resource free > is attempted after the media device is unregistered which would result > in use-after-free errors. > > Signed-off-by: Shuah Khan > --- > > - Ran bind/unbind loop (1000 iteration) test on snd-usb-audio > while running mc_nextgen_test loop (1000 iterations) in parallel. > - Ran bind/unbind and rmmod/modprobe tests on both drivers. Also > generated graphs when after bind/unbind, rmmod/modprobe. Graphs > look good. > - Note: Please apply the following patch to fix memory leak: > sound/usb: Fix memory leak in media_snd_stream_delete() during unbind > https://lkml.org/lkml/2016/3/16/1050 Yeah, a way better! For normal bind/unbind, it seems to be working fine. Also for driver's rmmod, so: Tested-by: Mauro Carvalho Chehab Takashi, Could please ack this patch - please see below that the problem Mauro and I both saw ended up to a latent bug in au0828 that is in Linux 4.5 as well. It is now fixed. thanks, -- Shuah PS.: === There are still some troubles if we run an infinite loop binding/unbinding au0828 and another one binding/unbinding snd-usb-audio, like this one: >>> >>> Yes. I noticed this one when I was running a loop of 1000 on au0828. >>> I couldn't reproduce this when I tested this patch. >>> >>> P.S: au08282 loops takes longer btw since au0828 initialization is lot more >>> complex. We have to look at this one. >>> [ 91.556188] [ cut here ] [ 91.556500] WARNING: CPU: 1 PID: 2920 at drivers/media/media-entity.c:392 __media_entity_pipeline_start+0x271/0xce0 [media]() [ 91.556626] Modules linked in: ir_lirc_codec lirc_dev au8522_dig xc5000 tuner au8522_decoder au8522_common au0828 videobuf2_vmalloc videobuf2_memops videobuf2_v4l2 videobuf2_core tveeprom dvb_core rc_core v4l2_common videodev snd_usb_audio snd_usbmidi_lib snd_rawmidi snd_seq_device media cpufreq_powersave cpufreq_conservative cpufreq_userspace cpufreq_stats parport_pc ppdev lp parport snd_hda_codec_hdmi intel_rapl x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel iTCO_wdt kvm iTCO_vendor_support irqbypass crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel sha256_ssse3 sha256_generic hmac drbg snd_hda_codec_realtek i915 snd_hda_codec_generic aesni_intel aes_x86_64 btusb lrw btrtl gf128mul snd_hda_intel glue_helper ablk_helper btbcm btintel cryptd psmouse snd_hda_codec bluetooth [ 91.556693] snd_hwdep i2c_algo_bit sg snd_hda_core serio_raw pcspkr evdev drm_kms_helper snd_pcm rfkill drm snd_timer mei_me snd i2c_i801 soundcore lpc_ich mei mfd_core battery video dw_dmac i2c_designware_platform dw_dmac_core i2c_designware_core acpi_pad button tpm_tis tpm ext4 crc16 mbcache jbd2 dm_mod sd_mod hid_generic usbhid ahci libahci libata e1000e xhci_pci ptp scsi_mod ehci_pci ehci_hcd pps_core xhci_hcd fan thermal sdhci_acpi sdhci mmc_core i2c_hid hid [ 91.556748] CPU: 1 PID: 2920 Comm: v4l_id Tainted: G D W 4.5.0+ #62 [ 91.556751] Hardware name: /NUC5i7RYB, BIOS RYBDWi35.86A.0350.2015.0812.1722 08/12/2015 [ 91.556754] a0e247a0 8803a3d17b08 819447c1 [ 91.556759] 88009bbe17c0 8803a3d17b48 8114fd16 a0e20651 [ 91.556763] 8803a47c9c58 8803a477d2c8 8803a5ac96f8 dc00 [ 91.556768] Call Trace: [ 91.556774] [] dump_stack+0x85/0xc4 [ 91.556778] [] warn_slowpath_common+0xc6/0x120 [ 91.556783] [] ? __media_entity_pipeline_start+0x271/0xce0 [media] [ 91.556786] [] warn_slowpath_null+0x1a/0x20 [ 91.556790] [] __media_entity_pipeline_start+0x271/0xce0 [media] [ 91.556794] [] ? __schedule+0x78a/0x2570 [ 91.556797] [] ? memset+0x28/0x30 [ 91.556802] [] ? media_entity_pipeline_stop+0x60/0x60 [media] [ 91.556806] [] ? trace_hardirqs_on+0xd/0x10 [ 91.556810] [] ? au0828_enable_source+0x55/0x9f0 [au0828] [ 91.556813] [] ? mutex_trylock+0x400/0x400 [ 91.556818] [] ? au0828_v4l2_close+0xb3/0x590 [au0828] [ 91.556822] [] au0828_enable_source+0x3f4/0x9f0 [au0828] [ 91.556824] [] ? mutex_trylock+0x400/0x400 [ 91.556834] [] v4l_enable_media_source+0x66/0x90 [videodev] [ 91.556839] [] au0828_
Re: [PATCH 2/4] media: Support Intersil/Techwell TW686x-based video capture cards
Hi Hans,
On 21 March 2016 at 08:41, Hans Verkuil wrote:
> From: Ezequiel Garcia
>
> This commit introduces the support for the Techwell TW686x video
> capture IC. This hardware supports a few DMA modes, including
> scatter-gather and frame (contiguous).
>
> This commit makes little use of the DMA engine and instead has
> a memcpy based implementation. DMA frame and scatter-gather modes
> support may be added in the future.
>
> Currently supported chips:
> - TW6864 (4 video channels),
> - TW6865 (4 video channels, not tested, second generation chip),
> - TW6868 (8 video channels but only 4 first channels using
>built-in video decoder are supported, not tested),
> - TW6869 (8 video channels, second generation chip).
>
> Cc: Krzysztof Halasa
> Signed-off-by: Ezequiel Garcia
> Signed-off-by: Hans Verkuil
> Tested-by: Hans Verkuil
[..]
> +int tw686x_video_init(struct tw686x_dev *dev)
> +{
> + unsigned int ch, val, pb;
> + int err;
> +
> + err = v4l2_device_register(&dev->pci_dev->dev, &dev->v4l2_dev);
> + if (err)
> + return err;
> +
> + for (ch = 0; ch < max_channels(dev); ch++) {
> + struct tw686x_video_channel *vc = &dev->video_channels[ch];
> + struct video_device *vdev;
> +
> + mutex_init(&vc->vb_mutex);
> + spin_lock_init(&vc->qlock);
> + INIT_LIST_HEAD(&vc->vidq_queued);
> +
> + vc->dev = dev;
> + vc->ch = ch;
> +
> + /* default settings */
> + vc->format = &formats[0];
> + vc->video_standard = V4L2_STD_NTSC;
> + vc->width = TW686X_VIDEO_WIDTH;
> + vc->height = TW686X_VIDEO_HEIGHT(vc->video_standard);
> + vc->input = 0;
> +
> + reg_write(vc->dev, SDT[ch], 0);
> + tw686x_set_framerate(vc, 30);
> +
> + reg_write(dev, VDELAY_LO[ch], 0x14);
> + reg_write(dev, HACTIVE_LO[ch], 0xd0);
> + reg_write(dev, VIDEO_SIZE[ch], 0);
> +
> + for (pb = 0; pb < 2; pb++) {
> + err = tw686x_alloc_dma(vc, pb);
> + if (err)
> + goto error;
> + }
> +
> + vc->vidq.io_modes = VB2_READ | VB2_MMAP | VB2_DMABUF;
> + vc->vidq.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> + vc->vidq.drv_priv = vc;
> + vc->vidq.buf_struct_size = sizeof(struct tw686x_v4l2_buf);
> + vc->vidq.ops = &tw686x_video_qops;
> + vc->vidq.mem_ops = &vb2_vmalloc_memops;
> + vc->vidq.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> + vc->vidq.min_buffers_needed = 2;
> + vc->vidq.lock = &vc->vb_mutex;
> +
I missed the GFP_DMA32 on vb2_queue.gfp_flags.
Feel free to amend it, unless you want me to submit a patch for you to pick.
Thanks,
--
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] [media] au0828: Fix dev_state handling
The au0828 dev_state is actually a bit mask. It should not be
checking with "==" but, instead, with a logic and. There are some
places where it was doing it wrong.
Fix that by replacing the dev_state set/clear/test with the
bitops.
Cc: sta...@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab
---
Shuah,
Please test.
I'm testing it here too.
drivers/media/usb/au0828/au0828-core.c | 2 +-
drivers/media/usb/au0828/au0828-input.c | 4 +--
drivers/media/usb/au0828/au0828-video.c | 63 -
drivers/media/usb/au0828/au0828.h | 9 ++---
4 files changed, 39 insertions(+), 39 deletions(-)
diff --git a/drivers/media/usb/au0828/au0828-core.c
b/drivers/media/usb/au0828/au0828-core.c
index 060904ed8f20..85c13ca5178f 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -184,7 +184,7 @@ static void au0828_usb_disconnect(struct usb_interface
*interface)
Set the status so poll routines can check and avoid
access after disconnect.
*/
- dev->dev_state = DEV_DISCONNECTED;
+ set_bit(DEV_DISCONNECTED, &dev->dev_state);
au0828_rc_unregister(dev);
/* Digital TV */
diff --git a/drivers/media/usb/au0828/au0828-input.c
b/drivers/media/usb/au0828/au0828-input.c
index b0f067971979..3d6687f0407d 100644
--- a/drivers/media/usb/au0828/au0828-input.c
+++ b/drivers/media/usb/au0828/au0828-input.c
@@ -130,7 +130,7 @@ static int au0828_get_key_au8522(struct au0828_rc *ir)
bool first = true;
/* do nothing if device is disconnected */
- if (ir->dev->dev_state == DEV_DISCONNECTED)
+ if (test_bit(DEV_DISCONNECTED, &ir->dev->dev_state))
return 0;
/* Check IR int */
@@ -260,7 +260,7 @@ static void au0828_rc_stop(struct rc_dev *rc)
cancel_delayed_work_sync(&ir->work);
/* do nothing if device is disconnected */
- if (ir->dev->dev_state != DEV_DISCONNECTED) {
+ if (!test_bit(DEV_DISCONNECTED, &ir->dev->dev_state)) {
/* Disable IR */
au8522_rc_clear(ir, 0xe0, 1 << 4);
}
diff --git a/drivers/media/usb/au0828/au0828-video.c
b/drivers/media/usb/au0828/au0828-video.c
index 88dcc6e0e178..32d7db96479c 100644
--- a/drivers/media/usb/au0828/au0828-video.c
+++ b/drivers/media/usb/au0828/au0828-video.c
@@ -106,14 +106,13 @@ static inline void print_err_status(struct au0828_dev
*dev,
static int check_dev(struct au0828_dev *dev)
{
- if (dev->dev_state & DEV_DISCONNECTED) {
+ if (test_bit(DEV_DISCONNECTED, &dev->dev_state)) {
pr_info("v4l2 ioctl: device not present\n");
return -ENODEV;
}
- if (dev->dev_state & DEV_MISCONFIGURED) {
- pr_info("v4l2 ioctl: device is misconfigured; "
- "close and open it again\n");
+ if (test_bit(DEV_MISCONFIGURED, &dev->dev_state)) {
+ pr_info("v4l2 ioctl: device is misconfigured; close and open it
again\n");
return -EIO;
}
return 0;
@@ -521,8 +520,8 @@ static inline int au0828_isoc_copy(struct au0828_dev *dev,
struct urb *urb)
if (!dev)
return 0;
- if ((dev->dev_state & DEV_DISCONNECTED) ||
- (dev->dev_state & DEV_MISCONFIGURED))
+ if (test_bit(DEV_DISCONNECTED, &dev->dev_state) ||
+ test_bit(DEV_MISCONFIGURED, &dev->dev_state))
return 0;
if (urb->status < 0) {
@@ -824,10 +823,10 @@ static int au0828_stream_interrupt(struct au0828_dev *dev)
int ret = 0;
dev->stream_state = STREAM_INTERRUPT;
- if (dev->dev_state == DEV_DISCONNECTED)
+ if (test_bit(DEV_DISCONNECTED, &dev->dev_state))
return -ENODEV;
else if (ret) {
- dev->dev_state = DEV_MISCONFIGURED;
+ set_bit(DEV_MISCONFIGURED, &dev->dev_state);
dprintk(1, "%s device is misconfigured!\n", __func__);
return ret;
}
@@ -1026,7 +1025,7 @@ static int au0828_v4l2_open(struct file *filp)
int ret;
dprintk(1,
- "%s called std_set %d dev_state %d stream users %d users %d\n",
+ "%s called std_set %d dev_state %ld stream users %d users %d\n",
__func__, dev->std_set_in_tuner_core, dev->dev_state,
dev->streaming_users, dev->users);
@@ -1045,7 +1044,7 @@ static int au0828_v4l2_open(struct file *filp)
au0828_analog_stream_enable(dev);
au0828_analog_stream_reset(dev);
dev->stream_state = STREAM_OFF;
- dev->dev_state |= DEV_INITIALIZED;
+ set_bit(DEV_INITIALIZED, &dev->dev_state);
}
dev->users++;
mutex_unlock(&dev->lock);
@@ -1059,7 +1058,7 @@ static int au0828_v4l2_close(struct file *filp)
struct video_device *vdev = video_devdata(filp);
dprintk(1,
- "%s cal
Re: [PATCH] [media] xilinx-vipp: remove unnecessary of_node_put
Le 22/03/2016 13:12, Laurent Pinchart a écrit :
> Hi Frank,
>
> Thank you for the patch.
>
> On Tuesday 22 Mar 2016 11:43:58 Franck Jullien wrote:
>> of_graph_get_next_endpoint(node, ep) decrements refcount on
>> ep. When next==NULL we break and refcount on ep is decremented
>> again.
>>
>> Signed-off-by: Franck Jullien
>
> Reviewed-by: Laurent Pinchart
>
> I don't have patches queued for Xilinx drivers, would you like to push this
> one to Mauro directly, or would you prefer me to take it in my tree ?
>
I don't have a strong opinion about this. Mauro can you take it ?
>> ---
>> drivers/media/platform/xilinx/xilinx-vipp.c |8 ++--
>> 1 files changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c
>> b/drivers/media/platform/xilinx/xilinx-vipp.c index e795a45..feb3b2f 100644
>> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
>> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
>> @@ -351,19 +351,15 @@ static int xvip_graph_parse_one(struct
>> xvip_composite_device *xdev, struct xvip_graph_entity *entity;
>> struct device_node *remote;
>> struct device_node *ep = NULL;
>> -struct device_node *next;
>> int ret = 0;
>>
>> dev_dbg(xdev->dev, "parsing node %s\n", node->full_name);
>>
>> while (1) {
>> -next = of_graph_get_next_endpoint(node, ep);
>> -if (next == NULL)
>> +ep = of_graph_get_next_endpoint(node, ep);
>> +if (ep == NULL)
>> break;
>>
>> -of_node_put(ep);
>> -ep = next;
>> -
>> dev_dbg(xdev->dev, "handling endpoint %s\n", ep->full_name);
>>
>> remote = of_graph_get_remote_port_parent(ep);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] [media] xilinx-vipp: remove unnecessary of_node_put
of_graph_get_next_endpoint(node, ep) decrements refcount on
ep. When next==NULL we break and refcount on ep is decremented
again.
Signed-off-by: Franck Jullien
---
drivers/media/platform/xilinx/xilinx-vipp.c |8 ++--
1 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c
b/drivers/media/platform/xilinx/xilinx-vipp.c
index e795a45..feb3b2f 100644
--- a/drivers/media/platform/xilinx/xilinx-vipp.c
+++ b/drivers/media/platform/xilinx/xilinx-vipp.c
@@ -351,19 +351,15 @@ static int xvip_graph_parse_one(struct
xvip_composite_device *xdev,
struct xvip_graph_entity *entity;
struct device_node *remote;
struct device_node *ep = NULL;
- struct device_node *next;
int ret = 0;
dev_dbg(xdev->dev, "parsing node %s\n", node->full_name);
while (1) {
- next = of_graph_get_next_endpoint(node, ep);
- if (next == NULL)
+ ep = of_graph_get_next_endpoint(node, ep);
+ if (ep == NULL)
break;
- of_node_put(ep);
- ep = next;
-
dev_dbg(xdev->dev, "handling endpoint %s\n", ep->full_name);
remote = of_graph_get_remote_port_parent(ep);
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] media: au0828 fix au0828_v4l2_close() dev_state race condition
On 03/21/2016 10:04 PM, Shuah Khan wrote: > au0828_v4l2_close() check for dev_state == DEV_DISCONNECTED will fail to > detect the device disconnected state correctly, if au0828_v4l2_open() runs > to set the DEV_INITIALIZED bit. A loop test of bind/unbind found this bug > by increasing the likelihood of au0828_v4l2_open() occurring while unbind > is in progress. When au0828_v4l2_close() fails to detect that the device > is in disconnect state, it attempts to power down the device and fails with > the following general protection fault: > > [ 260.992962] Call Trace: > [ 260.993008] [] ? xc5000_sleep+0x8f/0xd0 [xc5000] > [ 260.993095] [] ? fe_standby+0x3c/0x50 [tuner] > [ 260.993186] [] au0828_v4l2_close+0x53c/0x620 [au0828] > [ 260.993298] [] v4l2_release+0xf0/0x210 [videodev] > [ 260.993382] [] __fput+0x1fc/0x6c0 > [ 260.993449] [] fput+0xe/0x10 > [ 260.993519] [] task_work_run+0x133/0x1f0 > [ 260.993602] [] exit_to_usermode_loop+0x140/0x170 > [ 260.993681] [] syscall_return_slowpath+0x16a/0x1a0 > [ 260.993754] [] entry_SYSCALL_64_fastpath+0xa6/0xa8 > > Signed-off-by: Shuah Khan Hi Mauro, Reproduced this problem on Linux 4.5. This patch should be marked for stable releases. Mar 22 06:55:16 anduin kernel: [ 883.235413] PM: Removing info for No Bus:video1 Mar 22 06:55:16 anduin kernel: [ 883.236165] device: 'vbi0': device_unregister Mar 22 06:55:16 anduin kernel: [ 883.237883] kasan: CONFIG_KASAN_INLINE enabled Mar 22 06:55:16 anduin kernel: [ 883.237893] kasan: GPF could be caused by NULL-ptr deref or user memory accessgeneral protection fault: [#1] SMP KASAN Mar 22 06:55:16 anduin kernel: [ 883.237988] Modules linked in: nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack ipt_REJECT nf_reject_ipv4 xt_tcpudp bridge stp llc ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter ip_tables x_tables snd_usb_audio snd_usbmidi_lib snd_seq_midi snd_seq_midi_event snd_seq snd_rawmidi ir_lirc_codec snd_seq_device lirc_dev binfmt_misc rc_hauppauge au8522_dig nls_iso8859_1 xc5000 tuner au8522_decoder au8522_common hp_wmi sparse_keymap au0828 ghash_clmulni_intel aesni_intel tveeprom aes_x86_64 ablk_helper dvb_core cryptd lrw gf128mul uvcvideo glue_helper rc_core v4l2_common joydev videobuf2_vmalloc videobuf2_memops videobuf2_v4l2 videobuf2_core serio_raw videodev media k10temp i2c_piix4 snd_hda_codec_idt snd_hda_codec_generic snd_hda_codec_hdmi snd_hda_intel snd_hda_codec tpm_infineon snd_hda_core snd_hwdep snd_pcm hp_accel lis3lv02d input_polldev mac_hid snd_timer kvm_amd tpm_tis kvm irqbypass parport_pc ppdev lp parport a utofs4 p l 2303 usbserial hid_generic psmouse usbhid hid radeon i2c_algo_bit ttm drm_kms_helper syscopyarea sysfillrect sysimgblt firewire_ohci fb_sys_fops sdhci_pci firewire_core sdhci crc_itu_t r8169 drm mii wmi video Mar 22 06:55:16 anduin kernel: [ 883.239547] CPU: 1 PID: 2793 Comm: v4l_id Not tainted 4.5.0 #16 Mar 22 06:55:16 anduin kernel: [ 883.239613] Hardware name: Hewlett-Packard HP ProBook 6475b/180F, BIOS 68TTU Ver. F.04 08/03/2012 Mar 22 06:55:16 anduin kernel: [ 883.239709] task: 88009a0fcd80 ti: 8800942f task.ti: 8800942f Mar 22 06:55:16 anduin kernel: [ 883.239790] RIP: 0010:[] [] usb_set_interface+0x34/0x9d0 Mar 22 06:55:16 anduin kernel: [ 883.239896] PM: Removing info for No Bus:vbi0 Mar 22 06:55:16 anduin kernel: [ 883.239947] RSP: 0018:8800942f7d10 EFLAGS: 00010212 Mar 22 06:55:16 anduin kernel: [ 883.240007] RAX: dc00 RBX: 8801eef75338 RCX: 88009a0fd5b8 Mar 22 06:55:16 anduin kernel: [ 883.240093] RDX: 0008 RSI: RDI: 0040 Mar 22 06:55:16 anduin kernel: [ 883.240189] RBP: 8800942f7d88 R08: 0006 R09: Mar 22 06:55:16 anduin kernel: [ 883.240284] R10: R11: R12: 8801eef76874 Mar 22 06:55:16 anduin kernel: [ 883.240381] R13: R14: dc00 R15: 8801eef74000 Mar 22 06:55:16 anduin kernel: [ 883.240478] FS: 7f83985c4700() GS:8801fa88() knlGS: Mar 22 06:55:16 anduin kernel: [ 883.240586] CS: 0010 DS: ES: CR0: 8005003b Mar 22 06:55:16 anduin kernel: [ 883.240664] CR2: 55fd2e0ec000 CR3: 9426d000 CR4: 000406e0 Mar 22 06:55:16 anduin kernel: [ 883.240754] Stack: Mar 22 06:55:16 anduin kernel: [ 883.240779] dc00 8801eef74000 8800942f7d40 a0f58f0f Mar 22 06:55:16 anduin kernel: [ 883.240873] 88009a0fd5d8 88014050 8800942f7d58 a0e3803c Mar 22 06:55:16 anduin kernel: [ 883.240966] 8801 8800 8801eef75338 8801eef76874 Mar 22 06:55:16 anduin kernel: [ 883.241058] Call Trace: Mar 22 06:55:16 anduin kernel: [ 883.241095] [] ? xc5000_sleep+0x8f/0xd0 [xc5000] Mar 22 06:55:16 anduin kernel: [ 883.241166] [] ? fe_standby+0x3c/0x50 [tuner] Mar 2
Re: [PATCH] sound/usb: fix to release stream resources from media_snd_device_delete()
On 03/21/2016 10:01 PM, Shuah Khan wrote: > On 03/19/2016 07:31 AM, Shuah Khan wrote: >> On 03/19/2016 06:10 AM, Mauro Carvalho Chehab wrote: >>> Em Fri, 18 Mar 2016 20:50:31 -0600 >>> Shuah Khan escreveu: >>> Fix to release stream resources from media_snd_device_delete() before media device is unregistered. Without this change, stream resource free is attempted after the media device is unregistered which would result in use-after-free errors. Signed-off-by: Shuah Khan --- - Ran bind/unbind loop (1000 iteration) test on snd-usb-audio while running mc_nextgen_test loop (1000 iterations) in parallel. - Ran bind/unbind and rmmod/modprobe tests on both drivers. Also generated graphs when after bind/unbind, rmmod/modprobe. Graphs look good. - Note: Please apply the following patch to fix memory leak: sound/usb: Fix memory leak in media_snd_stream_delete() during unbind https://lkml.org/lkml/2016/3/16/1050 >>> >>> Yeah, a way better! >>> >>> For normal bind/unbind, it seems to be working fine. Also >>> for driver's rmmod, so: >>> >>> Tested-by: Mauro Carvalho Chehab >>> >>> PS.: >>> === >>> >>> There are still some troubles if we run an infinite loop >>> binding/unbinding au0828 and another one binding/unbinding >>> snd-usb-audio, like this one: >> >> Yes. I noticed this one when I was running a loop of 1000 on au0828. >> I couldn't reproduce this when I tested this patch. >> >> P.S: au08282 loops takes longer btw since au0828 initialization is lot more >> complex. We have to look at this one. >> >>> >>> >>> [ 91.556188] [ cut here ] >>> [ 91.556500] WARNING: CPU: 1 PID: 2920 at >>> drivers/media/media-entity.c:392 __media_entity_pipeline_start+0x271/0xce0 >>> [media]() >>> [ 91.556626] Modules linked in: ir_lirc_codec lirc_dev au8522_dig xc5000 >>> tuner au8522_decoder au8522_common au0828 videobuf2_vmalloc >>> videobuf2_memops videobuf2_v4l2 videobuf2_core tveeprom dvb_core rc_core >>> v4l2_common videodev snd_usb_audio snd_usbmidi_lib snd_rawmidi >>> snd_seq_device media cpufreq_powersave cpufreq_conservative >>> cpufreq_userspace cpufreq_stats parport_pc ppdev lp parport >>> snd_hda_codec_hdmi intel_rapl x86_pkg_temp_thermal intel_powerclamp >>> coretemp kvm_intel iTCO_wdt kvm iTCO_vendor_support irqbypass >>> crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel sha256_ssse3 >>> sha256_generic hmac drbg snd_hda_codec_realtek i915 snd_hda_codec_generic >>> aesni_intel aes_x86_64 btusb lrw btrtl gf128mul snd_hda_intel glue_helper >>> ablk_helper btbcm btintel cryptd psmouse snd_hda_codec bluetooth >>> [ 91.556693] snd_hwdep i2c_algo_bit sg snd_hda_core serio_raw pcspkr >>> evdev drm_kms_helper snd_pcm rfkill drm snd_timer mei_me snd i2c_i801 >>> soundcore lpc_ich mei mfd_core battery video dw_dmac >>> i2c_designware_platform dw_dmac_core i2c_designware_core acpi_pad button >>> tpm_tis tpm ext4 crc16 mbcache jbd2 dm_mod sd_mod hid_generic usbhid ahci >>> libahci libata e1000e xhci_pci ptp scsi_mod ehci_pci ehci_hcd pps_core >>> xhci_hcd fan thermal sdhci_acpi sdhci mmc_core i2c_hid hid >>> [ 91.556748] CPU: 1 PID: 2920 Comm: v4l_id Tainted: G D W >>> 4.5.0+ #62 >>> [ 91.556751] Hardware name: /NUC5i7RYB, BIOS >>> RYBDWi35.86A.0350.2015.0812.1722 08/12/2015 >>> [ 91.556754] a0e247a0 8803a3d17b08 819447c1 >>> >>> [ 91.556759] 88009bbe17c0 8803a3d17b48 8114fd16 >>> a0e20651 >>> [ 91.556763] 8803a47c9c58 8803a477d2c8 8803a5ac96f8 >>> dc00 >>> [ 91.556768] Call Trace: >>> [ 91.556774] [] dump_stack+0x85/0xc4 >>> [ 91.556778] [] warn_slowpath_common+0xc6/0x120 >>> [ 91.556783] [] ? >>> __media_entity_pipeline_start+0x271/0xce0 [media] >>> [ 91.556786] [] warn_slowpath_null+0x1a/0x20 >>> [ 91.556790] [] >>> __media_entity_pipeline_start+0x271/0xce0 [media] >>> [ 91.556794] [] ? __schedule+0x78a/0x2570 >>> [ 91.556797] [] ? memset+0x28/0x30 >>> [ 91.556802] [] ? media_entity_pipeline_stop+0x60/0x60 >>> [media] >>> [ 91.556806] [] ? trace_hardirqs_on+0xd/0x10 >>> [ 91.556810] [] ? au0828_enable_source+0x55/0x9f0 >>> [au0828] >>> [ 91.556813] [] ? mutex_trylock+0x400/0x400 >>> [ 91.556818] [] ? au0828_v4l2_close+0xb3/0x590 [au0828] >>> [ 91.556822] [] au0828_enable_source+0x3f4/0x9f0 >>> [au0828] >>> [ 91.556824] [] ? mutex_trylock+0x400/0x400 >>> [ 91.556834] [] v4l_enable_media_source+0x66/0x90 >>> [videodev] >>> [ 91.556839] [] au0828_v4l2_close+0x25a/0x590 [au0828] >>> [ 91.556846] [] v4l2_release+0xf0/0x210 [videodev] >>> [ 91.556849] [] __fput+0x20f/0x6d0 >>> [ 91.556853] [] fput+0xe/0x10 >>> [ 91.556856] [] task_work_run+0x137/0x200 >>> [ 91.556859] [] exit_to_usermode_loop+0x154/0x180 >>> [ 91.556863] [] ? trace_hardirqs_on_caller+0x16/0x590 >>> [ 91.55
Re: [PATCH] [media] xilinx-vipp: remove unnecessary of_node_put
Hi Frank,
Thank you for the patch.
On Tuesday 22 Mar 2016 11:43:58 Franck Jullien wrote:
> of_graph_get_next_endpoint(node, ep) decrements refcount on
> ep. When next==NULL we break and refcount on ep is decremented
> again.
>
> Signed-off-by: Franck Jullien
Reviewed-by: Laurent Pinchart
I don't have patches queued for Xilinx drivers, would you like to push this
one to Mauro directly, or would you prefer me to take it in my tree ?
> ---
> drivers/media/platform/xilinx/xilinx-vipp.c |8 ++--
> 1 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c
> b/drivers/media/platform/xilinx/xilinx-vipp.c index e795a45..feb3b2f 100644
> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> @@ -351,19 +351,15 @@ static int xvip_graph_parse_one(struct
> xvip_composite_device *xdev, struct xvip_graph_entity *entity;
> struct device_node *remote;
> struct device_node *ep = NULL;
> - struct device_node *next;
> int ret = 0;
>
> dev_dbg(xdev->dev, "parsing node %s\n", node->full_name);
>
> while (1) {
> - next = of_graph_get_next_endpoint(node, ep);
> - if (next == NULL)
> + ep = of_graph_get_next_endpoint(node, ep);
> + if (ep == NULL)
> break;
>
> - of_node_put(ep);
> - ep = next;
> -
> dev_dbg(xdev->dev, "handling endpoint %s\n", ep->full_name);
>
> remote = of_graph_get_remote_port_parent(ep);
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] media: am437x-vpfe: ensure ret is initialized
Hi Colin, On Mon, Mar 21, 2016 at 11:32 PM, Colin King wrote: > From: Colin Ian King > > ret should be initialized to 0; for example if pfe->fmt.fmt.pix.field > is V4L2_FIELD_NONE then ret will contain garbage from the > uninitialized state causing garbage to be returned if it is non-zero. > Thanks for the patch, patch [1] fixing this issue is already posted in ML. [1] http://www.mail-archive.com/linux-media@vger.kernel.org/msg95562.html Cheers, --Prabhakar Lad -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] vidioc-dv-timings-cap.xml: explicitly state that pad and reserved should be zeroed
The DV_TIMINGS_CAP documentation didn't state clearly that the pad and reserved fields should be zeroed by the application. For subdev pad can be other values as well. It also mistakenly said that only drivers would have to zero the reserved field, that's not correct. Signed-off-by: Hans Verkuil --- Documentation/DocBook/media/v4l/vidioc-dv-timings-cap.xml | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/Documentation/DocBook/media/v4l/vidioc-dv-timings-cap.xml b/Documentation/DocBook/media/v4l/vidioc-dv-timings-cap.xml index a2017bf..b6f47a6 100644 --- a/Documentation/DocBook/media/v4l/vidioc-dv-timings-cap.xml +++ b/Documentation/DocBook/media/v4l/vidioc-dv-timings-cap.xml @@ -55,8 +55,9 @@ interface and may change in the future. -To query the capabilities of the DV receiver/transmitter applications -can call the VIDIOC_DV_TIMINGS_CAP ioctl on a video node +To query the capabilities of the DV receiver/transmitter applications initialize the +pad field to 0, zero the reserved array of &v4l2-dv-timings-cap; +and call the VIDIOC_DV_TIMINGS_CAP ioctl on a video node and the driver will fill in the structure. Note that drivers may return different values after switching the video input or output. @@ -65,8 +66,8 @@ queried by calling the VIDIOC_SUBDEV_DV_TIMINGS_CAP ioctl directly on a subdevice node. The capabilities are specific to inputs (for DV receivers) or outputs (for DV transmitters), applications must specify the desired pad number in the &v4l2-dv-timings-cap; pad -field. Attempts to query capabilities on a pad that doesn't support them will -return an &EINVAL;. +field and zero the reserved array. Attempts to query +capabilities on a pad that doesn't support them will return an &EINVAL;. struct v4l2_bt_timings_cap @@ -145,7 +146,8 @@ return an &EINVAL;. __u32 reserved[2] - Reserved for future extensions. Drivers must set the array to zero. + Reserved for future extensions. Drivers and applications must + set the array to zero. union -- 2.7.0 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] vidioc-enum-dv-timings.xml: explicitly state that pad and reserved should be zeroed
The ENUM_DV_TIMINGS documentation did not clearly state that the pad and reserved fields should be zeroed (pad only when used with a video device node). Signed-off-by: Hans Verkuil --- Documentation/DocBook/media/v4l/vidioc-enum-dv-timings.xml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Documentation/DocBook/media/v4l/vidioc-enum-dv-timings.xml b/Documentation/DocBook/media/v4l/vidioc-enum-dv-timings.xml index 6e3cadd..70ca76d 100644 --- a/Documentation/DocBook/media/v4l/vidioc-enum-dv-timings.xml +++ b/Documentation/DocBook/media/v4l/vidioc-enum-dv-timings.xml @@ -61,8 +61,9 @@ of known supported timings. Call &VIDIOC-DV-TIMINGS-CAP; to check if it also sup standards or even custom timings that are not in this list. To query the available timings, applications initialize the -index field and zero the reserved array of &v4l2-enum-dv-timings; -and call the VIDIOC_ENUM_DV_TIMINGS ioctl on a video node with a +index field, set the pad field to 0, +zero the reserved array of &v4l2-enum-dv-timings; and call the +VIDIOC_ENUM_DV_TIMINGS ioctl on a video node with a pointer to this structure. Drivers fill the rest of the structure or return an &EINVAL; when the index is out of bounds. To enumerate all supported DV timings, applications shall begin at index zero, incrementing by one until the -- 2.7.0 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] vidioc-g-edid.xml: be explicit about zeroing the reserved array
The G/S_EDID documentation did not explicitly state that the reserved array should be zeroed by the application. Also add the missing VIDIOC_SUBDEV_G/S_EDID ioctl names to the header. Signed-off-by: Hans Verkuil --- Documentation/DocBook/media/v4l/vidioc-g-edid.xml | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/Documentation/DocBook/media/v4l/vidioc-g-edid.xml b/Documentation/DocBook/media/v4l/vidioc-g-edid.xml index 2702536..b7602d3 100644 --- a/Documentation/DocBook/media/v4l/vidioc-g-edid.xml +++ b/Documentation/DocBook/media/v4l/vidioc-g-edid.xml @@ -1,6 +1,6 @@ -ioctl VIDIOC_G_EDID, VIDIOC_S_EDID +ioctl VIDIOC_G_EDID, VIDIOC_S_EDID, VIDIOC_SUBDEV_G_EDID, VIDIOC_SUBDEV_S_EDID &manvol; @@ -71,7 +71,8 @@ To get the EDID data the application has to fill in the pad, start_block, blocks and edid -fields and call VIDIOC_G_EDID. The current EDID from block +fields, zero the reserved array and call +VIDIOC_G_EDID. The current EDID from block start_block and of size blocks will be placed in the memory edid points to. The edid pointer must point to memory at least blocks * 128 bytes @@ -92,8 +93,9 @@ the driver will set blocks to 0 and it returns 0. To set the EDID blocks of a receiver the application has to fill in the pad, -blocks and edid fields and set -start_block to 0. It is not possible to set part of an EDID, +blocks and edid fields, set +start_block to 0 and zero the reserved array. +It is not possible to set part of an EDID, it is always all or nothing. Setting the EDID data is only valid for receivers as it makes no sense for a transmitter. -- 2.7.0 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] EDID/DV_TIMINGS docbook fixes
Fixes a few issues I found in the documentation. Hans Verkuil (3): vidioc-g-edid.xml: be explicit about zeroing the reserved array vidioc-enum-dv-timings.xml: explicitly state that pad and reserved should be zeroed vidioc-dv-timings-cap.xml: explicitly state that pad and reserved should be zeroed Documentation/DocBook/media/v4l/vidioc-dv-timings-cap.xml | 12 +++- Documentation/DocBook/media/v4l/vidioc-enum-dv-timings.xml | 5 +++-- Documentation/DocBook/media/v4l/vidioc-g-edid.xml | 10 ++ 3 files changed, 16 insertions(+), 11 deletions(-) -- 2.7.0 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] tc358743: zero the reserved array
v4l2-compliance complained about this.
Signed-off-by: Hans Verkuil
diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index 972e0d4..73e0cef 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1551,6 +1551,8 @@ static int tc358743_g_edid(struct v4l2_subdev *sd,
{
struct tc358743_state *state = to_state(sd);
+ memset(edid->reserved, 0, sizeof(edid->reserved));
+
if (edid->pad != 0)
return -EINVAL;
@@ -1585,6 +1587,8 @@ static int tc358743_s_edid(struct v4l2_subdev *sd,
v4l2_dbg(2, debug, sd, "%s, pad %d, start block %d, blocks %d\n",
__func__, edid->pad, edid->start_block, edid->blocks);
+ memset(edid->reserved, 0, sizeof(edid->reserved));
+
if (edid->pad != 0)
return -EINVAL;
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/15] mediactl: Add media device ioctl API
Hi Sakari, On 03/21/2016 01:07 AM, Sakari Ailus wrote: Hi Jacek, On Thu, Feb 18, 2016 at 02:14:40PM +0100, Jacek Anaszewski wrote: Hi Sakari, On 02/18/2016 01:09 PM, Sakari Ailus wrote: Hi Jacek, On Mon, Feb 15, 2016 at 02:06:06PM +0100, Jacek Anaszewski wrote: Hi Sakari, Thanks for the review. On 02/15/2016 01:41 PM, Sakari Ailus wrote: Hi Jacek, Jacek Anaszewski wrote: Ioctls executed on complex media devices need special handling. For instance some ioctls need to be targeted for specific sub-devices, depending on the media device configuration. The APIs being introduced address such requirements. Signed-off-by: Jacek Anaszewski Acked-by: Kyungmin Park --- utils/media-ctl/Makefile.am |2 +- utils/media-ctl/libv4l2media_ioctl.c | 404 ++ utils/media-ctl/libv4l2media_ioctl.h | 48 3 files changed, 453 insertions(+), 1 deletion(-) create mode 100644 utils/media-ctl/libv4l2media_ioctl.c create mode 100644 utils/media-ctl/libv4l2media_ioctl.h diff --git a/utils/media-ctl/Makefile.am b/utils/media-ctl/Makefile.am index 3e883e0..7f18624 100644 --- a/utils/media-ctl/Makefile.am +++ b/utils/media-ctl/Makefile.am @@ -1,6 +1,6 @@ noinst_LTLIBRARIES = libmediactl.la libv4l2subdev.la libmediatext.la -libmediactl_la_SOURCES = libmediactl.c mediactl-priv.h +libmediactl_la_SOURCES = libmediactl.c mediactl-priv.h libv4l2media_ioctl.c libv4l2media_ioctl.h libmediactl_la_CFLAGS = -static $(LIBUDEV_CFLAGS) libmediactl_la_LDFLAGS = -static $(LIBUDEV_LIBS) diff --git a/utils/media-ctl/libv4l2media_ioctl.c b/utils/media-ctl/libv4l2media_ioctl.c new file mode 100644 index 000..b186121 --- /dev/null +++ b/utils/media-ctl/libv4l2media_ioctl.c @@ -0,0 +1,404 @@ +/* + * Copyright (c) 2015 Samsung Electronics Co., Ltd. + * http://www.samsung.com + * + * Author: Jacek Anaszewski + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation; either version 2.1 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + */ + +#include +#include +#include +#include + +#include + +#include "libv4l2media_ioctl.h" +#include "mediactl-priv.h" +#include "mediactl.h" +#include "v4l2subdev.h" + +#define VIDIOC_CTRL(type) \ + ((type) == VIDIOC_S_CTRL ? "VIDIOC_S_CTRL" : \ + "VIDIOC_G_CTRL") + +#define VIDIOC_EXT_CTRL(type) \ + ((type) == VIDIOC_S_EXT_CTRLS ? \ + "VIDIOC_S_EXT_CTRLS" : \ +((type) == VIDIOC_G_EXT_CTRLS ?\ + "VIDIOC_G_EXT_CTRLS" :\ + "VIDIOC_TRY_EXT_CTRLS")) + +#define SYS_IOCTL(fd, cmd, arg) \ + syscall(SYS_ioctl, (int)(fd), (unsigned long)(cmd), (void *)(arg)) + + +int media_ioctl_ctrl(struct media_device *media, int request, unsigned int request OK. +struct v4l2_control *arg) I wonder if it'd make sense to always use v4l2_ext_control instead. You can't access 64-bit integer controls with VIDIOC_S_CTRL for instance. This function is meant to handle VIDIOC_S_CTRL/VIDIOC_G_CTRL ioctls. For ext ctrls there is media_ioctl_ext_ctrl(). Is there any reason not to use extended control always? In other words, do we have a driver that does support Media controller but does not support extended controls? Shouldn't we support non-extended controls for backward compatibility reasons? I am not aware of the policy in this matter. To put it bluntly, supporting the non-extended controls in this use is waste of time IMHO. OK, I'll drop the non-ext controls related API then. As this is a user space library, I'd probably add a function to handle S/G/TRY control each. There is media_ioctl_ext_ctrl() that handles VIDIOC_S_EXT_CTRLS, VIDIOC_G_EXT_CTRLS and VIDIOC_TRY_EXT_CTRLS. Have you considered binding the control to a video node rather than a media device? We have many sensors on current media devices already, and e.g. exposure time control can be found in multiple sub-devices. Doesn't v4l2-ctrl-redir config entry address that? How does it work if you have, say, two video nodes where you can capture images from a different sensor? I.e. your media graph could look like this: sensor0 -> CSI-2 0 -> video0 sensor1 -> CSI-2 1 -> video1 Exemplary config settings for this case: v4l2-ctrl-redir 0x0098091f -> "sensor0" v4l2-ctrl-redir 0x0098091f -> "sensor1" In media_ioctl_ctrl the v4l2_subdev_get_pipe
Re: [PATCH 01/15] mediactl: Introduce v4l2_subdev structure
Hi Sakari,
On 03/21/2016 12:39 AM, Sakari Ailus wrote:
Hi Jacek,
On Thu, Feb 18, 2016 at 03:15:32PM +0100, Jacek Anaszewski wrote:
Hi Sakari,
Thanks for the review.
On 02/12/2016 01:42 PM, Sakari Ailus wrote:
Hi Jacek,
Thanks for continuing this work! And my apologies for reviewing only
now... please see the comments below.
Jacek Anaszewski wrote:
Add struct v4l2_subdev - a representation of the v4l2 sub-device,
related to the media entity. Add field 'sd', the pointer to
the newly introduced structure, to the struct media_entity
and move 'fd' property from struct media entity to struct v4l2_subdev.
Avoid accessing sub-device file descriptor from libmediactl and
make the v4l2_subdev_open capable of creating the v4l2_subdev
if the 'sd' pointer is uninitialized.
Signed-off-by: Jacek Anaszewski
Acked-by: Kyungmin Park
---
utils/media-ctl/libmediactl.c |4 --
utils/media-ctl/libv4l2subdev.c | 82 +++
utils/media-ctl/mediactl-priv.h |5 ++-
utils/media-ctl/v4l2subdev.h| 38 ++
4 files changed, 107 insertions(+), 22 deletions(-)
diff --git a/utils/media-ctl/libmediactl.c b/utils/media-ctl/libmediactl.c
index 4a82d24..7e98440 100644
--- a/utils/media-ctl/libmediactl.c
+++ b/utils/media-ctl/libmediactl.c
@@ -525,7 +525,6 @@ static int media_enum_entities(struct media_device *media)
entity = &media->entities[media->entities_count];
memset(entity, 0, sizeof(*entity));
- entity->fd = -1;
entity->info.id = id | MEDIA_ENT_ID_FLAG_NEXT;
entity->media = media;
@@ -719,8 +718,6 @@ void media_device_unref(struct media_device *media)
free(entity->pads);
free(entity->links);
- if (entity->fd != -1)
- close(entity->fd);
}
free(media->entities);
@@ -747,7 +744,6 @@ int media_device_add_entity(struct media_device *media,
entity = &media->entities[media->entities_count - 1];
memset(entity, 0, sizeof *entity);
- entity->fd = -1;
entity->media = media;
strncpy(entity->devname, devnode, sizeof entity->devname);
entity->devname[sizeof entity->devname - 1] = '\0';
diff --git a/utils/media-ctl/libv4l2subdev.c b/utils/media-ctl/libv4l2subdev.c
index 33c1ee6..3977ce5 100644
--- a/utils/media-ctl/libv4l2subdev.c
+++ b/utils/media-ctl/libv4l2subdev.c
@@ -39,13 +39,61 @@
#include "tools.h"
#include "v4l2subdev.h"
+int v4l2_subdev_create(struct media_entity *entity)
+{
+ if (entity->sd)
+ return 0;
+
+ entity->sd = calloc(1, sizeof(*entity->sd));
+ if (entity->sd == NULL)
+ return -ENOMEM;
+
+ entity->sd->fd = -1;
+
+ return 0;
+}
+
+int v4l2_subdev_create_with_fd(struct media_entity *entity, int fd)
+{
+ int ret;
+
+ if (entity->sd)
+ return -EEXIST;
+
+ ret = v4l2_subdev_create(entity);
+ if (ret < 0)
+ return ret;
+
+ entity->sd->fd = fd;
+
+ return 0;
+}
+
+void v4l2_subdev_release(struct media_entity *entity, bool close_fd)
+{
+ if (entity->sd == NULL)
+ return;
+
+ if (close_fd)
+ v4l2_subdev_close(entity);
+
+ free(entity->sd->v4l2_control_redir);
+ free(entity->sd);
+}
+
int v4l2_subdev_open(struct media_entity *entity)
{
- if (entity->fd != -1)
+ int ret;
+
+ ret = v4l2_subdev_create(entity);
The current users of v4l2_subdev_open() in libv4l2subdev do not
explicitly close the sub-devices they open; thus calling
v4l2_subdev_create() here creates a memory leak.
Currently in my use cases there is no memory leak since I assumed
that the one who instantiates struct media_device should take
care of releasing it properly. I added v4l2_subdev_open_pipeline()
and v4l2_subdev_release_pipeline() API that is called on plugin
init and close respectively.
I'm referring to the use of the libv4l2subdev API as it's documented; the
media-ctl test program which also serves as a good example on the API.
Any sub-device IOCTL wrapper function will call v4l2_subdev_open() which
stores the file descriptor returned by open(2) to struct media_entity.fd.
v4l2_subdev_close() is not called explicitly. This is currently not
required.
The file handle is not leaked, as it is closed by media_device_unref() in
libmediactl.
This patch allocates memory for each sub-device in v4l2_subdev_create()
which is in turn called from v4l2_subdev_open(). As the calls to
v4l2_subdev_close() (which would release memory) are lacking, the memory is
leaked.
Probably it would be good to remove v4l2_subdev_open from
v4l2_subdev_* prefixed API and return error if sd property
of passed struct media_entity is not initialized.
The purpose of the libv4l2subdev library is to make accessing sub-devices
easier, and tossing that responsibility to the user is certainly not
advancing tha
