Hi Hans,
On Thu, Sep 07, 2017 at 10:51:21AM +0200, Hans Verkuil wrote:
> On 09/07/17 09:34, Sakari Ailus wrote:
> > Hi Hans,
> >
> > On Wed, Sep 06, 2017 at 09:41:40AM +0200, Hans Verkuil wrote:
> >> On 09/05/2017 03:05 PM, Sakari Ailus wrote:
> >>> The current practice is that drivers iterate over their endpoints and
> >>> parse each endpoint separately. This is very similar in a number of
> >>> drivers, implement a generic function for the job. Driver specific matters
> >>> can be taken into account in the driver specific callback.
> >>>
> >>> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> >>> ---
> >>> drivers/media/v4l2-core/v4l2-async.c | 19 +++++
> >>> drivers/media/v4l2-core/v4l2-fwnode.c | 140 ++++++++++++++++++++++++++++++++++
> >>> include/media/v4l2-async.h | 24 +++++-
> >>> include/media/v4l2-fwnode.h | 53 +++++++++++++
> >>> 4 files changed, 234 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> >>> index 3d81ff6a496f..7bd595c4094a 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-async.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-async.c
> >>> @@ -22,6 +22,7 @@
> >>>
> >>> #include <media/v4l2-async.h>
> >>> #include <media/v4l2-device.h>
> >>> +#include <media/v4l2-fwnode.h>
> >>> #include <media/v4l2-subdev.h>
> >>>
> >>> static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> >>> @@ -224,6 +225,24 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> >>> }
> >>> EXPORT_SYMBOL(v4l2_async_notifier_unregister);
> >>>
> >>> +void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier)
> >>> +{
> >>> + unsigned int i;
> >>> +
> >>> + if (!notifier->max_subdevs)
> >>> + return;
> >>> +
> >>> + for (i = 0; i < notifier->num_subdevs; i++)
> >>> + kfree(notifier->subdevs[i]);
> >>> +
> >>> + notifier->max_subdevs = 0;
> >>> + notifier->num_subdevs = 0;
> >>> +
> >>> + kvfree(notifier->subdevs);
> >>> + notifier->subdevs = NULL;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(v4l2_async_notifier_release);
> >>> +
> >>> int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> >>> {
> >>> struct v4l2_async_notifier *notifier;
> >>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> >>> index 706f9e7b90f1..e6932d7d47b6 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> >>> @@ -19,6 +19,7 @@
> >>> */
> >>> #include <linux/acpi.h>
> >>> #include <linux/kernel.h>
> >>> +#include <linux/mm.h>
> >>> #include <linux/module.h>
> >>> #include <linux/of.h>
> >>> #include <linux/property.h>
> >>> @@ -26,6 +27,7 @@
> >>> #include <linux/string.h>
> >>> #include <linux/types.h>
> >>>
> >>> +#include <media/v4l2-async.h>
> >>> #include <media/v4l2-fwnode.h>
> >>>
> >>> enum v4l2_fwnode_bus_type {
> >>> @@ -313,6 +315,144 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
> >>> }
> >>> EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> >>>
> >>> +static int v4l2_async_notifier_realloc(struct v4l2_async_notifier *notifier,
> >>> + unsigned int max_subdevs)
> >>> +{
> >>> + struct v4l2_async_subdev **subdevs;
> >>> +
> >>> + if (max_subdevs <= notifier->max_subdevs)
> >>> + return 0;
> >>> +
> >>> + subdevs = kvmalloc_array(
> >>> + max_subdevs, sizeof(*notifier->subdevs),
> >>> + GFP_KERNEL | __GFP_ZERO);
> >>> + if (!subdevs)
> >>> + return -ENOMEM;
> >>> +
> >>> + if (notifier->subdevs) {
> >>> + memcpy(subdevs, notifier->subdevs,
> >>> + sizeof(*subdevs) * notifier->num_subdevs);
> >>> +
> >>> + kvfree(notifier->subdevs);
> >>> + }
> >>> +
> >>> + notifier->subdevs = subdevs;
> >>> + notifier->max_subdevs = max_subdevs;
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int v4l2_async_notifier_fwnode_parse_endpoint(
> >>> + struct device *dev, struct v4l2_async_notifier *notifier,
> >>> + struct fwnode_handle *endpoint, unsigned int asd_struct_size,
> >>> + int (*parse_endpoint)(struct device *dev,
> >>> + struct v4l2_fwnode_endpoint *vep,
> >>> + struct v4l2_async_subdev *asd))
> >>> +{
> >>> + struct v4l2_async_subdev *asd;
> >>> + struct v4l2_fwnode_endpoint *vep;
> >>> + struct fwnode_endpoint ep;
> >>> + int ret = 0;
> >>> +
> >>> + asd = kzalloc(asd_struct_size, GFP_KERNEL);
> >>> + if (!asd)
> >>> + return -ENOMEM;
> >>> +
> >>> + asd->match.fwnode.fwnode =
> >>> + fwnode_graph_get_remote_port_parent(endpoint);
> >>> + if (!asd->match.fwnode.fwnode) {
> >>> + dev_warn(dev, "bad remote port parent\n");
> >>> + ret = -EINVAL;
> >>> + goto out_err;
> >>> + }
> >>> +
> >>> + /* Ignore endpoints the parsing of which failed. */
> >>> + vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
> >>> + if (IS_ERR(vep)) {
> >>> + ret = PTR_ERR(vep);
> >>> + dev_warn(dev, "unable to parse V4L2 fwnode endpoint (%d)\n",
> >>> + ret);
> >>> + goto out_err;
> >>> + }
> >>> +
> >>> + ep = vep->base;
> >>> +
> >>> + ret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;
> >>> + v4l2_fwnode_endpoint_free(vep);
> >>> + if (ret == -ENOTCONN) {
> >>> + dev_dbg(dev, "ignoring endpoint %u,%u\n", ep.port, ep.id);
> >>> + kfree(asd);
> >>
> >> Shouldn't there be a call to fwnode_handle_put()?
> >
> > Actually no. But your hunch is right in the sense that I think there are
> > issues. The fwnode_endpoint (as of_endpoint) contains the fwnode which is
> > referenced, but no reference is taken here. One couldn't be released either
> > later on, as the fwnode field is const.
> >
> > I guess this is almost fine as long as the fwnode field is used for pointer
> > comparison and nothing else but you can never be sure what drivers actually
> > do.
> >
> > This actually should be addressed for both OF and fwnode but it's mostly
> > independent of this patchset. Luckily there are not *that* many users of
> > this. The V4L2 fwnode interface that allocates and releases endpoints makes
> > this quite a bit easier actually.
>
> I think a comment or two here would be helpful. If I tripped over this, so
> will others.
Reading the code again, I noticed I didn't read far enough the last time.
While v4l2_fwnode_endpoint_alloc_parse() won't take a reference,
fwnode_graph_get_remote_port_parent() does. That reference indeed isn't
released right now.
It shouldn't be too hard to do that properly in
v4l2_async_notifier_release(). I guess I'll do just that actually. By that
time, there are no users left anymore.
Good catch.
>
> >
> >>
> >>> + return 0;
> >>> + } else if (ret < 0) {
> >>> + dev_warn(dev, "driver could not parse endpoint %u,%u (%d)\n",
> >>> + ep.port, ep.id, ret);
> >>> + goto out_err;
> >>> + }
> >>
> >> I think this would be better and it avoids the need for the ep local variable:
> >>
> >> ret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;
> >> if (ret == -ENOTCONN)
> >> dev_dbg(dev, "ignoring endpoint %u,%u\n", vep->base.port, vep->base.id);
> >> else if (ret < 0)
> >> dev_warn(dev, "driver could not parse endpoint %u,%u (%d)\n",
> >> vep->base.port, vep->base.id, ret);
> >> v4l2_fwnode_endpoint_free(vep);
> >> if (ret < 0)
> >> goto out_err;
> >>
> >> And the 'return ret;' below would become:
> >>
> >> return ret == -ENOTCONN ? 0 : ret;
> >
> > Looks good to me, I'll use it.
> >
> >>
> >>> +
> >>> + asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> >>> + notifier->subdevs[notifier->num_subdevs] = asd;
> >>> + notifier->num_subdevs++;
> >>> +
> >>> + return 0;
> >>> +
> >>> +out_err:
> >>> + fwnode_handle_put(asd->match.fwnode.fwnode);
>
> Just a reminder: this put() should now check if ret != -ENOTCONN.
Should it? Even if ret == -ENOTCONN, the reference is there and needs to be
released as the endpoint is not used (just as in the case of an error).
>
> >>> + kfree(asd);
> >>> +
> >>> + return ret;
> >>> +}
> >>> +
> >>> +int v4l2_async_notifier_parse_fwnode_endpoints(
> >>> + struct device *dev, struct v4l2_async_notifier *notifier,
> >>> + size_t asd_struct_size,
> >>> + int (*parse_endpoint)(struct device *dev,
> >>> + struct v4l2_fwnode_endpoint *vep,
> >>> + struct v4l2_async_subdev *asd))
> >>> +{
> >>> + struct fwnode_handle *fwnode = NULL;
> >>> + unsigned int max_subdevs = notifier->max_subdevs;
> >>> + int ret;
> >>> +
> >>> + if (asd_struct_size < sizeof(struct v4l2_async_subdev))
> >>
> >> I think this can be a WARN_ON or a WARN_ON_ONCE. Up to you, though.
> >
> > It'd be a driver bug, so either seems appropriate. I'll use WARN_ON().
> >
> >>
> >>> + return -EINVAL;
> >>> +
> >>> + for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> >>> + dev_fwnode(dev), fwnode)); )
> >>> + if (fwnode_device_is_available(
> >>> + fwnode_graph_get_port_parent(fwnode)))
> >>> + max_subdevs++;
> >>
> >> I think I would prefer this as a simple for (;;):
> >>
> >> for (;;) {
> >
> > I think for (;;) is misuse of the for loop. :-D
> >
> >> fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev), fwnode));
> >> if (fwnode == NULL)
> >> break;
> >> ...
> >> }
> >>
> >> Or alternatively as a do...while:
> >>
> >> do {
> >> fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev), fwnode));
> >> if (fwnode && fwnode_device_is_available(
> >> fwnode_graph_get_port_parent(fwnode)))
> >> max_subdevs++;
> >> } while (fwnode);
> >>
> >> Both are IMHO a bit more readable. I leave it up to you, though.
> >>
> >>> +
> >>> + /* No subdevs to add? Return here. */
> >>> + if (max_subdevs == notifier->max_subdevs)
> >>> + return 0;
> >>> +
> >>> + ret = v4l2_async_notifier_realloc(notifier, max_subdevs);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> >>> + dev_fwnode(dev), fwnode)); ) {
> >>
> >> Same comment as above.
> >
> > What I like in the original code is that managing looping over all
> > endpoints takes place directly in the for () statement and does not spill
> > over to where the individual endpoints are being handled.
> >
> > What would you think of adding a macro to do this? Say, we could write
> > this as:
> >
> > fwnode_graph_for_each_endpoint(dev_fwnode(dev), fwnode) {
> > ...
> > }
> >
> > That'd have to go through linux-pm tree though, and we could change the
> > code here later on.
>
> I think that's a good idea. Let's do that.
Ack. I'll submit a patch on that in the near future.
>
> >
> >>
> >>> + if (!fwnode_device_is_available(
> >>> + fwnode_graph_get_port_parent(fwnode)))
> >>> + continue;
> >>> +
> >>> + if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs)) {
> >>> + ret = -EINVAL;
> >>> + break;
> >>> + }
> >>> +
> >>> + ret = v4l2_async_notifier_fwnode_parse_endpoint(
> >>> + dev, notifier, fwnode, asd_struct_size, parse_endpoint);
> >>> + if (ret < 0)
> >>> + break;
> >>> + }
> >>> +
> >>> + fwnode_handle_put(fwnode);
> >>> +
> >>> + return ret;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);
> >>> +
> >>> MODULE_LICENSE("GPL");
> >>> MODULE_AUTHOR("Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>");
> >>> MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>");
> >>> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> >>> index c69d8c8a66d0..96fa1afc00dd 100644
> >>> --- a/include/media/v4l2-async.h
> >>> +++ b/include/media/v4l2-async.h
> >>> @@ -18,7 +18,6 @@ struct device;
> >>> struct device_node;
> >>> struct v4l2_device;
> >>> struct v4l2_subdev;
> >>> -struct v4l2_async_notifier;
> >>>
> >>> /* A random max subdevice number, used to allocate an array on stack */
> >>> #define V4L2_MAX_SUBDEVS 128U
> >>> @@ -50,6 +49,10 @@ enum v4l2_async_match_type {
> >>> * @match: union of per-bus type matching data sets
> >>> * @list: used to link struct v4l2_async_subdev objects, waiting to be
> >>> * probed, to a notifier->waiting list
> >>> + *
> >>> + * When this struct is used as a member in a driver specific struct,
> >>> + * the driver specific struct shall contain the @struct
> >>> + * v4l2_async_subdev as its first member.
> >>> */
> >>> struct v4l2_async_subdev {
> >>> enum v4l2_async_match_type match_type;
> >>> @@ -78,7 +81,8 @@ struct v4l2_async_subdev {
> >>> /**
> >>> * struct v4l2_async_notifier - v4l2_device notifier data
> >>> *
> >>> - * @num_subdevs: number of subdevices
> >>> + * @num_subdevs: number of subdevices used in the subdevs array
> >>> + * @max_subdevs: number of subdevices allocated in the subdevs array
> >>> * @subdevs: array of pointers to subdevice descriptors
> >>> * @v4l2_dev: pointer to struct v4l2_device
> >>> * @waiting: list of struct v4l2_async_subdev, waiting for their drivers
> >>> @@ -90,6 +94,7 @@ struct v4l2_async_subdev {
> >>> */
> >>> struct v4l2_async_notifier {
> >>> unsigned int num_subdevs;
> >>> + unsigned int max_subdevs;
> >>> struct v4l2_async_subdev **subdevs;
> >>> struct v4l2_device *v4l2_dev;
> >>> struct list_head waiting;
> >>> @@ -121,6 +126,21 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> >>> void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier);
> >>>
> >>> /**
> >>> + * v4l2_async_notifier_release - release notifier resources
> >>> + * @notifier: the notifier the resources of which are to be released
> >>> + *
> >>> + * Release memory resources related to a notifier, including the async
> >>> + * sub-devices allocated for the purposes of the notifier. The user is
> >>> + * responsible for releasing the notifier's resources after calling
> >>> + * @v4l2_async_notifier_parse_fwnode_endpoints.
> >>> + *
> >>> + * There is no harm from calling v4l2_async_notifier_release in other
> >>> + * cases as long as its memory has been zeroed after it has been
> >>> + * allocated.
> >>> + */
> >>> +void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier);
> >>> +
> >>> +/**
> >>> * v4l2_async_register_subdev - registers a sub-device to the asynchronous
> >>> * subdevice framework
> >>> *
> >>> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> >>> index 68eb22ba571b..6d125f26ec84 100644
> >>> --- a/include/media/v4l2-fwnode.h
> >>> +++ b/include/media/v4l2-fwnode.h
> >>> @@ -25,6 +25,8 @@
> >>> #include <media/v4l2-mediabus.h>
> >>>
> >>> struct fwnode_handle;
> >>> +struct v4l2_async_notifier;
> >>> +struct v4l2_async_subdev;
> >>>
> >>> #define V4L2_FWNODE_CSI2_MAX_DATA_LANES 4
> >>>
> >>> @@ -201,4 +203,55 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
> >>> */
> >>> void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> >>>
> >>> +/**
> >>> + * v4l2_async_notifier_parse_fwnode_endpoints - Parse V4L2 fwnode endpoints in a
> >>> + * device node
> >>> + * @dev: the device the endpoints of which are to be parsed
> >>> + * @notifier: notifier for @dev
> >>> + * @asd_struct_size: size of the driver's async sub-device struct, including
> >>> + * sizeof(struct v4l2_async_subdev). The &struct
> >>> + * v4l2_async_subdev shall be the first member of
> >>> + * the driver's async sub-device struct, i.e. both
> >>> + * begin at the same memory address.
> >>> + * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
> >>> + * endpoint. Optional.
> >>> + * Return: %0 on success
> >>> + * %-ENOTCONN if the endpoint is to be skipped but this
> >>> + * should not be considered as an error
> >>> + * %-EINVAL if the endpoint configuration is invalid
> >>> + *
> >>> + * Parse the fwnode endpoints of the @dev device and populate the async sub-
> >>> + * devices array of the notifier. The @parse_endpoint callback function is
> >>> + * called for each endpoint with the corresponding async sub-device pointer to
> >>> + * let the caller initialize the driver-specific part of the async sub-device
> >>> + * structure.
> >>> + *
> >>> + * The notifier memory shall be zeroed before this function is called on the
> >>> + * notifier.
> >>> + *
> >>> + * This function may not be called on a registered notifier and may be called on
> >>> + * a notifier only once. When using this function, the user may not access the
> >>> + * notifier's subdevs array nor change notifier's num_subdevs field, these are
> >>> + * reserved for the framework's internal use only.
> >>
> >> The rvin_digital_graph_init() function accesses the subdevs array.
> >>
> >> I still don't like this sentence, and I still think it should be dropped. Either that
> >> or completely rewritten.
> >
> > How about:
> >
> > When using this function on a notifier, the user is not allowed to change
> > the notifier's subdevs array, take references to the subdevs array itself
> > nor change the notifier's num_subdevs field. This is because the function
> > allocates and reallocates the subdevs array based on parsing endpoints.
> >
>
> How about this:
>
> "Do not change the notifier's subdevs array, take references to the subdevs
> array itself or change the notifier's num_subdevs field. This is because
> this function allocates and reallocates the subdevs array based on parsing
> endpoints."
I'll use this in the next version.
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@xxxxxx
