Re: [RFC v2 4/5] drm/dsi: Add routine to unregister dsi device

[Archit Taneja]

On 11/02/2015 04:12 PM, Andrzej Hajda wrote:
> On 11/02/2015 07:28 AM, Archit Taneja wrote:
> > On 10/30/2015 07:51 PM, Andrzej Hajda wrote:
> > > On 10/06/2015 11:24 AM, Archit Taneja wrote:
> > > > A driver calling mipi_dsi_device_new might want to unregister the device
> > > > once it's done. It might also require it in an error handling path in
> > > > case something didn't go right.
> > > >
> > > > When the dsi host driver calls mipi_dsi_host_unregister, the devices
> > > > created by both DT and and without DT will be removed. This does leave
> > > > the possibility of the host removing the dsi device without the
> > > > peripheral driver being aware of it. I don't know a good way to solve
> > > > this. Some suggestions here would be of help too.
> > > The 2nd paragraph is not relevant here. It is another issue. Some comments
> > > about it:
> > Yes, it's probably not the best to put it in the commit message of this
> > patch.
> >
> > > I am not sure, but I guess device should not be removed if it is refcounted
> > > properly, it will be just detached from the driver, bus and system (whatever it
> > > means:) ).
> > > It does not mean it will be usable and probably some races can occur anyway.
> > > I guess i2c and other buses have the same problem, am I right?
> > I was concerned about one particular sequence:
> >
> > 1) DSI host driver calls mipi_dsi_host_unregister: All dsi devices would
> > be unregistered.
> >
> > 2) dsi device driver calls mipi_dsi_device_unregister: This will try to
> > unregister our dsi device
> >
> > The problem here is that the device will cease to exist after step (1)
> > itself, because the refcount of our device will never be 2.
> >
> > mipi_dsi_host_register() will only register devices represented in DT,
> > not the one the drivers register manually.
> >
> > In other words, the dsi pointer in our driver will point to nothing
> > valid after mipi_dsi_host_unregister is called.
> >
> > As you said, I guess this exists in other buses too, and it's the
> > drivers job to not use them.
>
> I think the whole problem is due to fact we try to use devices
> as interfaces to some bus hosts (DSI in our case), these devices
> are owned by bus host and we cannot control their lifetime from other code.
> The best solution IMO would be to create separate lightweight framework
> as I suggested in previous discussion[1]. It should be cleaner solution
> without any 'dummy' proxy devices.
> But even in this case we would need some callbacks to notify that the provider
> is about to be removed.
>
> 2nd 'solution' is to leave it as is and pretend everything is OK,
> as in case of other frameworks :)
>
> Maybe it would be possible 3rd solution - we could use probe and remove callbacks
> from dsi driver to notify clients about adding/removal of dsi device to/from bus.
> So during dummy dsi dev creation we would provide some callbacks which would be
> called
> by dummy dsi driver probe/remove to notifiy client it can start/stop using dsi
> device.
> This crazy construction probably can work but looks insane :)
>
> [1]: http://www.spinics.net/lists/linux-arm-msm/msg16945.html

I'm okay with the 2nd solution :). We can add callbacks or a
notification mechanism if anyone needs it in the future.

Thanks,
Archit

> Regards
> Andrzej
>
> > > > Signed-off-by: Archit Taneja <architt@xxxxxxxxxxxxxx>
> > > > ---
> > > >    drivers/gpu/drm/drm_mipi_dsi.c | 7 +++++++
> > > >    include/drm/drm_mipi_dsi.h     | 2 ++
> > > >    2 files changed, 9 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> > > > index db6130a..cbb7373 100644
> > > > --- a/drivers/gpu/drm/drm_mipi_dsi.c
> > > > +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> > > > @@ -183,6 +183,13 @@ err:
> > > >    }
> > > >    EXPORT_SYMBOL(mipi_dsi_device_new);
> > > >
> > > > +void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi)
> > > > +{
> > > > +	if (dsi)
> > > > +		device_unregister(&dsi->dev);
> > > > +}
> > > > +EXPORT_SYMBOL(mipi_dsi_device_unregister);
> > > > +
> > > I guess NULL check can be removed and the whole function can be inlined.
> > Yeah, this check won't help anyway.
> >
> > I think I'll mention that drivers should use this only in error
> > handling paths, and not in the driver's remove() op.
> >
> > I'll also change this to inlined.
> >
> > Archit
> >
> > > Regards
> > > Andrzej
> > > >    static struct mipi_dsi_device *
> > > >    of_mipi_dsi_device_add(struct mipi_dsi_host *host, struct device_node *node)
> > > >    {
> > > > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> > > > index 93dec7b..68f49f4 100644
> > > > --- a/include/drm/drm_mipi_dsi.h
> > > > +++ b/include/drm/drm_mipi_dsi.h
> > > > @@ -197,6 +197,8 @@ ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,
> > > >
> > > >    struct mipi_dsi_device *mipi_dsi_device_new(struct mipi_dsi_host *host,
> > > >    					    struct mipi_dsi_device_info *info);
> > > > +void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi);
> > > > +
> > > >    /**
> > > >     * enum mipi_dsi_dcs_tear_mode - Tearing Effect Output Line mode
> > > >     * @MIPI_DSI_DCS_TEAR_MODE_VBLANK: the TE output line consists of V-Blanking
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> > > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/