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

[Archit Taneja <architt@codeaurora.org>]

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@codeaurora.org>
>>>> ---
>>>>    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@vger.kernel.org
>>> 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@vger.kernel.org
> 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