Re: [PATCH 1/6] software node: Provide replacement for device_add_properties()

[Rafael J. Wysocki]

On Tue, Feb 2, 2021 at 4:01 PM Heikki Krogerus
<heikki.krogerus@xxxxxxxxxxxxxxx> wrote:
>
> Hi Rafael,
>
> On Tue, Feb 02, 2021 at 03:44:05PM +0100, Rafael J. Wysocki wrote:
> > > +/**
> > > + * device_create_managed_software_node - Create a software node for a device
> > > + * @dev: The device the software node is assigned to.
> > > + * @properties: Device properties for the software node.
> > > + * @parent: Parent of the software node.
> > > + *
> > > + * Creates a software node as a managed resource for @dev, which means the
> > > + * lifetime of the newly created software node is tied to the lifetime of @dev.
> > > + * Software nodes created with this function should not be reused or shared
> > > + * because of that. The function takes a deep copy of @properties for the
> > > + * software node.
> > > + *
> > > + * Since the new software node is assigned directly to @dev, and since it should
> > > + * not be shared, it is not returned to the caller. The function returns 0 on
> > > + * success, and errno in case of an error.
> > > + */
> > > +int device_create_managed_software_node(struct device *dev,
> > > +                                       const struct property_entry *properties,
> > > +                                       const struct software_node *parent)
> > > +{
> > > +       struct fwnode_handle *p = software_node_fwnode(parent);
> > > +       struct fwnode_handle *fwnode;
> > > +
> > > +       if (parent && !p)
> > > +               return -EINVAL;
> > > +
> > > +       fwnode = fwnode_create_software_node(properties, p);
>                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> To answer your question below: here.
>
> > > +       if (IS_ERR(fwnode))
> > > +               return PTR_ERR(fwnode);
> > > +
> > > +       to_swnode(fwnode)->managed = true;
> > > +       set_secondary_fwnode(dev, fwnode);
> > > +
> > > +       return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(device_create_managed_software_node);
> > > +
> > >  int software_node_notify(struct device *dev, unsigned long action)
> > >  {
> > >         struct swnode *swnode;
> > > @@ -1073,6 +1111,11 @@ int software_node_notify(struct device *dev, unsigned long action)
> > >                 sysfs_remove_link(&swnode->kobj, dev_name(dev));
> > >                 sysfs_remove_link(&dev->kobj, "software_node");
> > >                 kobject_put(&swnode->kobj);
> > > +
> > > +               if (swnode->managed) {
> > > +                       set_secondary_fwnode(dev, NULL);
> > > +                       kobject_put(&swnode->kobj);
> >
> > Where does the corresponding kobject_get() get called?
>
> So in function fwnode_create_software_node() we use
> kobject_init_and_add().

OK

It looks like there is a use case that cannot be addressed by using
device_add_properties() and that's why you need this new function.

Can you describe that use case, please, and explain what the problem
with using device_add_properties() in it is?