[RFC,v2,4/5] drm/dsi: Add routine to unregister dsi device
diff mbox

Message ID 1444123482-25579-5-git-send-email-architt@codeaurora.org
State New
Headers show

Commit Message

Archit Taneja Oct. 6, 2015, 9:24 a.m. UTC
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.

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(+)

Comments

Andrzej Hajda Oct. 30, 2015, 2:21 p.m. UTC | #1
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:
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?

>
> 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.

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
Archit Taneja Nov. 2, 2015, 6:28 a.m. UTC | #2
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.

>
>>
>> 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
>
Andrzej Hajda Nov. 2, 2015, 10:42 a.m. UTC | #3
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

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
>>
Archit Taneja Nov. 3, 2015, 7:18 a.m. UTC | #4
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
>

Patch
diff mbox

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);
+
 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