On Sat, 2021-01-16 at 18:21 +0100, Jacopo Mondi wrote:

Hi Ezequiel,

On Tue, Jan 12, 2021 at 10:23:32AM -0300, Ezequiel Garcia wrote:

quoted

The use of v4l2_async_notifier_add_subdev is discouraged.
Drivers are instead encouraged to use a helper such as
v4l2_async_notifier_add_fwnode_remote_subdev.

This fixes a misuse of the API, as v4l2_async_notifier_add_subdev
should get a kmalloc'ed struct v4l2_async_subdev,
removing some boilerplate code while at it.

Signed-off-by: Ezequiel Garcia <redacted>
---
 drivers/media/platform/atmel/atmel-isc.h      |  1 +
 drivers/media/platform/atmel/atmel-isi.c      | 46 ++++++-------------
 .../media/platform/atmel/atmel-sama5d2-isc.c  | 44 ++++++------------
 3 files changed, 29 insertions(+), 62 deletions(-)

        }

[snip]

quoted

        list_for_each_entry(subdev_entity, &isc->subdev_entities, list) {
+               struct v4l2_async_subdev *asd;
+
                v4l2_async_notifier_init(&subdev_entity->notifier);

-               ret = v4l2_async_notifier_add_subdev(&subdev_entity->notifier,
-                                                    subdev_entity->asd);
-               if (ret) {
-                       fwnode_handle_put(subdev_entity->asd->match.fwnode);
-                       kfree(subdev_entity->asd);
+               asd = v4l2_async_notifier_add_fwnode_remote_subdev(
+                                       &subdev_entity->notifier,
+                                       of_fwnode_handle(subdev_entity->epn),
+                                       sizeof(*asd));
+
+               of_node_put(subdev_entity->epn);
+               subdev_entity->epn = NULL;
+
+               if (IS_ERR(asd)) {
+                       ret = PTR_ERR(asd);
                        goto cleanup_subdev;

The isc_subdev_cleanup() this label jumps to does not put the other
subdev_entity->epn

The issue was there already if I'm not mistaken. If I'm not, I think
it's worth recording it with a FIXME: comment while you're here

Although, as you've noticed I tend to break this rule myself,
I'd rather avoid adding more changes to the patch.

The OF/fwnode handling in this atmel driver might benefit
from some improvements (and the same can be said of some
other drivers) but I'd say let's stick to just improving
the v4l2-async API.

Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks a lot for the reviews!
Ezequiel