Re: [PATCH 1/3] net: qrtr: Turn QRTR into a bus

Yassine Oudjana
2025-06-25T22:20:19Z

On Sunday, April 6th, 2025 at 5:01 PM, Jonathan Cameron <jic23@kernel.org> wrote:

> On Sun, 06 Apr 2025 14:07:43 +0000
> Yassine Oudjana y.oudjana@protonmail.com wrote:
>
> > Implement a QRTR bus to allow for creating drivers for individual QRTR
> > services. With this in place, devices are dynamically registered for QRTR
> > services as they become available, and drivers for these devices are
> > matched using service and instance IDs.
> >
> > In smd.c, replace all current occurences of qdev with qsdev in order to
> > distinguish between the newly added QRTR device which represents a QRTR
> > service with the existing QRTR SMD device which represents the endpoint
> > through which services are provided.
> >
> > Signed-off-by: Yassine Oudjana y.oudjana@protonmail.com
>
> Hi Yassine
>
> Just took a quick look through.
>
> It might make more sense to do this with an auxiliary_bus rather
> than defining a new bus.
>
> I'd also split out the renames as a precursor patch.
>
> Various other comments inline.
>
> Jonathan

<...>

> > + if (!del_server)
> > + return -ENOMEM;
> > +
> > + del_server->parent = qsdev;
> > + del_server->port = port;
> > +
> > + INIT_WORK(&del_server->work, qcom_smd_qrtr_del_device_worker);
> > + schedule_work(&del_server->work);
> > +
> > + return 0;
> > +}
> > +
> > +static int qcom_smd_qrtr_device_unregister(struct device *dev, void *data)
> > +{
> > + device_unregister(dev);
>
>
> One option that may simplify this is to do the device_unregister() handling
> a devm_action_or_reset() handler that is using the parent device as it's dev
> but unregistering the children. That way the unregister is called in the
> reverse order of setup and you only register a handler for those devices
> registered (rather walking children). I did this in the CXL pmu driver
> for instance.

Not sure I understand this correctly. This function is called for all children when
the parent (the bus) is removed in order to unregister them, so its called for all
registered devices under the parent. It's just a wrapper for device_unregister so
that it can be used with device_for_each_child. If I register a handler with
devm_add_action_or_reset using the parent device then it seems to me like I will
have to add a new function used as handler for that which in turn goes over the
children and unregisters them (we always unregister all children since the parent
will be no more) then I will only be adding an extra layer. I checked the CXL PMU
driver but I only found devm_add_action_or_reset used for cleaning up objects
associated with the device, not removing child devices.