LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/1] device core: Add flag to autoremove device link on supplier unbind
@ 2018-05-30  9:57 Vivek Gautam
  2018-05-30 10:59 ` Rafael J. Wysocki
  0 siblings, 1 reply; 6+ messages in thread
From: Vivek Gautam @ 2018-05-30  9:57 UTC (permalink / raw)
  To: gregkh, rafael.j.wysocki, lukas
  Cc: dmitry.torokhov, aspriel, robin.murphy, linux-kernel,
	linux-arm-msm, Vivek Gautam

When using the device links without the consumers or
suppliers maintaining pointers to these links, a flag can
help in autoremoving the links on supplier driver unbind.
We remove these links only when the supplier's link to its
consumers has gone in DL_STATE_SUPPLIER_UNBIND state.

Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
Suggested-by: Lukas Wunner <lukas@wunner.de>
---

Lukas, as suggested in the thread [1] this change adds additional flag
to autoremove device links on supplier unbind.
For arm-smmu, we want to _not_ keep references to the device links
added between arm-smmu, and consumer devices.
Robin also pointed to [2] the need to autoremove the device link on
supplier unbind rather than consumer unbind.

[1] https://lkml.org/lkml/2018/3/14/390
[2] https://lkml.org/lkml/2018/5/21/381

 drivers/base/core.c    | 10 ++++++++++
 include/linux/device.h |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index b610816eb887..52c7222bb3c4 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -490,6 +490,16 @@ void device_links_driver_cleanup(struct device *dev)
 
 		WARN_ON(link->flags & DL_FLAG_AUTOREMOVE);
 		WARN_ON(link->status != DL_STATE_SUPPLIER_UNBIND);
+
+		/*
+		 * autoremove the links between this @dev and its consumer
+		 * devices that are not active, i.e. where the link state
+		 * has moved to DL_STATE_SUPPLIER_UNBIND.
+		 */
+		if (link->status == DL_STATE_SUPPLIER_UNBIND &&
+		    link->flags & DL_FLAG_AUTOREMOVE_S)
+			kref_put(&link->kref, __device_link_del);
+
 		WRITE_ONCE(link->status, DL_STATE_DORMANT);
 	}
 
diff --git a/include/linux/device.h b/include/linux/device.h
index 477956990f5e..6033bf58453d 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -779,11 +779,13 @@ enum device_link_state {
  * AUTOREMOVE: Remove this link automatically on consumer driver unbind.
  * PM_RUNTIME: If set, the runtime PM framework will use this link.
  * RPM_ACTIVE: Run pm_runtime_get_sync() on the supplier during link creation.
+ * AUTOREMOVE_S: Remove this link automatically on supplier driver unbind.
  */
 #define DL_FLAG_STATELESS	BIT(0)
 #define DL_FLAG_AUTOREMOVE	BIT(1)
 #define DL_FLAG_PM_RUNTIME	BIT(2)
 #define DL_FLAG_RPM_ACTIVE	BIT(3)
+#define DL_FLAG_AUTOREMOVE_S	BIT(4)
 
 /**
  * struct device_link - Device link representation.
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] device core: Add flag to autoremove device link on supplier unbind
  2018-05-30  9:57 [PATCH 1/1] device core: Add flag to autoremove device link on supplier unbind Vivek Gautam
@ 2018-05-30 10:59 ` Rafael J. Wysocki
  2018-05-30 12:51   ` Vivek Gautam
  0 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2018-05-30 10:59 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: gregkh, lukas, dmitry.torokhov, aspriel, robin.murphy,
	linux-kernel, linux-pm

On 5/30/2018 11:57 AM, Vivek Gautam wrote:
> When using the device links without the consumers or
> suppliers maintaining pointers to these links, a flag can
> help in autoremoving the links on supplier driver unbind.
> We remove these links only when the supplier's link to its
> consumers has gone in DL_STATE_SUPPLIER_UNBIND state.
>
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> ---
>
> Lukas, as suggested in the thread [1] this change adds additional flag
> to autoremove device links on supplier unbind.
> For arm-smmu, we want to _not_ keep references to the device links
> added between arm-smmu, and consumer devices.
> Robin also pointed to [2] the need to autoremove the device link on
> supplier unbind rather than consumer unbind.

Please CC device links patches to linux-pm.

> [1] https://lkml.org/lkml/2018/3/14/390
> [2] https://lkml.org/lkml/2018/5/21/381
>
>   drivers/base/core.c    | 10 ++++++++++
>   include/linux/device.h |  2 ++
>   2 files changed, 12 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index b610816eb887..52c7222bb3c4 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -490,6 +490,16 @@ void device_links_driver_cleanup(struct device *dev)
>   
>   		WARN_ON(link->flags & DL_FLAG_AUTOREMOVE);
>   		WARN_ON(link->status != DL_STATE_SUPPLIER_UNBIND);
> +
> +		/*
> +		 * autoremove the links between this @dev and its consumer
> +		 * devices that are not active, i.e. where the link state
> +		 * has moved to DL_STATE_SUPPLIER_UNBIND.
> +		 */
> +		if (link->status == DL_STATE_SUPPLIER_UNBIND &&
> +		    link->flags & DL_FLAG_AUTOREMOVE_S)
> +			kref_put(&link->kref, __device_link_del);
> +
>   		WRITE_ONCE(link->status, DL_STATE_DORMANT);
>   	}
>   
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 477956990f5e..6033bf58453d 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -779,11 +779,13 @@ enum device_link_state {
>    * AUTOREMOVE: Remove this link automatically on consumer driver unbind.
>    * PM_RUNTIME: If set, the runtime PM framework will use this link.
>    * RPM_ACTIVE: Run pm_runtime_get_sync() on the supplier during link creation.
> + * AUTOREMOVE_S: Remove this link automatically on supplier driver unbind.
>    */
>   #define DL_FLAG_STATELESS	BIT(0)
>   #define DL_FLAG_AUTOREMOVE	BIT(1)
>   #define DL_FLAG_PM_RUNTIME	BIT(2)
>   #define DL_FLAG_RPM_ACTIVE	BIT(3)
> +#define DL_FLAG_AUTOREMOVE_S	BIT(4)

Couldn't you invent a better name for this one?

>   
>   /**
>    * struct device_link - Device link representation.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] device core: Add flag to autoremove device link on supplier unbind
  2018-05-30 10:59 ` Rafael J. Wysocki
@ 2018-05-30 12:51   ` Vivek Gautam
  2018-06-26 10:03     ` Vivek Gautam
  0 siblings, 1 reply; 6+ messages in thread
From: Vivek Gautam @ 2018-05-30 12:51 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: gregkh, lukas, dmitry.torokhov, aspriel, robin.murphy,
	linux-kernel, linux-pm

Hi Rafael,


On 5/30/2018 4:29 PM, Rafael J. Wysocki wrote:
> On 5/30/2018 11:57 AM, Vivek Gautam wrote:
>> When using the device links without the consumers or
>> suppliers maintaining pointers to these links, a flag can
>> help in autoremoving the links on supplier driver unbind.
>> We remove these links only when the supplier's link to its
>> consumers has gone in DL_STATE_SUPPLIER_UNBIND state.
>>
>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>> Suggested-by: Lukas Wunner <lukas@wunner.de>
>> ---
>>
>> Lukas, as suggested in the thread [1] this change adds additional flag
>> to autoremove device links on supplier unbind.
>> For arm-smmu, we want to _not_ keep references to the device links
>> added between arm-smmu, and consumer devices.
>> Robin also pointed to [2] the need to autoremove the device link on
>> supplier unbind rather than consumer unbind.
>
> Please CC device links patches to linux-pm.

Thanks for the quick review. Sure, will keep this noted from now on.

>
>> [1] https://lkml.org/lkml/2018/3/14/390
>> [2] https://lkml.org/lkml/2018/5/21/381
>>
>>   drivers/base/core.c    | 10 ++++++++++
>>   include/linux/device.h |  2 ++
>>   2 files changed, 12 insertions(+)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index b610816eb887..52c7222bb3c4 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -490,6 +490,16 @@ void device_links_driver_cleanup(struct device 
>> *dev)
>>             WARN_ON(link->flags & DL_FLAG_AUTOREMOVE);
>>           WARN_ON(link->status != DL_STATE_SUPPLIER_UNBIND);
>> +
>> +        /*
>> +         * autoremove the links between this @dev and its consumer
>> +         * devices that are not active, i.e. where the link state
>> +         * has moved to DL_STATE_SUPPLIER_UNBIND.
>> +         */
>> +        if (link->status == DL_STATE_SUPPLIER_UNBIND &&
>> +            link->flags & DL_FLAG_AUTOREMOVE_S)
>> +            kref_put(&link->kref, __device_link_del);
>> +
>>           WRITE_ONCE(link->status, DL_STATE_DORMANT);
>>       }
>>   diff --git a/include/linux/device.h b/include/linux/device.h
>> index 477956990f5e..6033bf58453d 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -779,11 +779,13 @@ enum device_link_state {
>>    * AUTOREMOVE: Remove this link automatically on consumer driver 
>> unbind.
>>    * PM_RUNTIME: If set, the runtime PM framework will use this link.
>>    * RPM_ACTIVE: Run pm_runtime_get_sync() on the supplier during 
>> link creation.
>> + * AUTOREMOVE_S: Remove this link automatically on supplier driver 
>> unbind.
>>    */
>>   #define DL_FLAG_STATELESS    BIT(0)
>>   #define DL_FLAG_AUTOREMOVE    BIT(1)
>>   #define DL_FLAG_PM_RUNTIME    BIT(2)
>>   #define DL_FLAG_RPM_ACTIVE    BIT(3)
>> +#define DL_FLAG_AUTOREMOVE_S    BIT(4)
>
> Couldn't you invent a better name for this one?

Frankly, I wanted to have something like DL_FLAG_AUTOREMOVE_SUPPLIER, 
but that
felt a bit too long considering other flags.
Can you please consider suggesting a concise name?

Regards
Vivek
>
>>     /**
>>    * struct device_link - Device link representation.
>
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] device core: Add flag to autoremove device link on supplier unbind
  2018-05-30 12:51   ` Vivek Gautam
@ 2018-06-26 10:03     ` Vivek Gautam
  2018-06-26 10:19       ` Rafael J. Wysocki
  0 siblings, 1 reply; 6+ messages in thread
From: Vivek Gautam @ 2018-06-26 10:03 UTC (permalink / raw)
  To: Rafael J. Wysocki, Ulf Hansson, Marek Szyprowski
  Cc: Greg KH, lukas, dmitry.torokhov, aspriel, Robin Murphy,
	open list, Linux PM

On Wed, May 30, 2018 at 6:21 PM, Vivek Gautam
<vivek.gautam@codeaurora.org> wrote:

Adding Ulf and Marek for their side of comments.
In summary, we do not want to not maintain a device link pointer
when adding a device link in supplier's driver, to delete the link later.
An autoremove flag from the suppliers side (similar to what we already
have for consumer side) can help autoremove the device link
when supplier driver goes away.

Hi Rafael, Lukas,
Gentle ping. Can you please consider reviewing this patch.

Best regards
Vivek

> Hi Rafael,
>
>
> On 5/30/2018 4:29 PM, Rafael J. Wysocki wrote:
>>
>> On 5/30/2018 11:57 AM, Vivek Gautam wrote:
>>>
>>> When using the device links without the consumers or
>>> suppliers maintaining pointers to these links, a flag can
>>> help in autoremoving the links on supplier driver unbind.
>>> We remove these links only when the supplier's link to its
>>> consumers has gone in DL_STATE_SUPPLIER_UNBIND state.
>>>
>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>>> Suggested-by: Lukas Wunner <lukas@wunner.de>
>>> ---
>>>
>>> Lukas, as suggested in the thread [1] this change adds additional flag
>>> to autoremove device links on supplier unbind.
>>> For arm-smmu, we want to _not_ keep references to the device links
>>> added between arm-smmu, and consumer devices.
>>> Robin also pointed to [2] the need to autoremove the device link on
>>> supplier unbind rather than consumer unbind.
>>
>>
>> Please CC device links patches to linux-pm.
>
>
> Thanks for the quick review. Sure, will keep this noted from now on.
>
>
>>
>>> [1] https://lkml.org/lkml/2018/3/14/390
>>> [2] https://lkml.org/lkml/2018/5/21/381
>>>
>>>   drivers/base/core.c    | 10 ++++++++++
>>>   include/linux/device.h |  2 ++
>>>   2 files changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>>> index b610816eb887..52c7222bb3c4 100644
>>> --- a/drivers/base/core.c
>>> +++ b/drivers/base/core.c
>>> @@ -490,6 +490,16 @@ void device_links_driver_cleanup(struct device *dev)
>>>             WARN_ON(link->flags & DL_FLAG_AUTOREMOVE);
>>>           WARN_ON(link->status != DL_STATE_SUPPLIER_UNBIND);
>>> +
>>> +        /*
>>> +         * autoremove the links between this @dev and its consumer
>>> +         * devices that are not active, i.e. where the link state
>>> +         * has moved to DL_STATE_SUPPLIER_UNBIND.
>>> +         */
>>> +        if (link->status == DL_STATE_SUPPLIER_UNBIND &&
>>> +            link->flags & DL_FLAG_AUTOREMOVE_S)
>>> +            kref_put(&link->kref, __device_link_del);
>>> +
>>>           WRITE_ONCE(link->status, DL_STATE_DORMANT);
>>>       }
>>>   diff --git a/include/linux/device.h b/include/linux/device.h
>>> index 477956990f5e..6033bf58453d 100644
>>> --- a/include/linux/device.h
>>> +++ b/include/linux/device.h
>>> @@ -779,11 +779,13 @@ enum device_link_state {
>>>    * AUTOREMOVE: Remove this link automatically on consumer driver
>>> unbind.
>>>    * PM_RUNTIME: If set, the runtime PM framework will use this link.
>>>    * RPM_ACTIVE: Run pm_runtime_get_sync() on the supplier during link
>>> creation.
>>> + * AUTOREMOVE_S: Remove this link automatically on supplier driver
>>> unbind.
>>>    */
>>>   #define DL_FLAG_STATELESS    BIT(0)
>>>   #define DL_FLAG_AUTOREMOVE    BIT(1)
>>>   #define DL_FLAG_PM_RUNTIME    BIT(2)
>>>   #define DL_FLAG_RPM_ACTIVE    BIT(3)
>>> +#define DL_FLAG_AUTOREMOVE_S    BIT(4)
>>
>>
>> Couldn't you invent a better name for this one?
>
>
> Frankly, I wanted to have something like DL_FLAG_AUTOREMOVE_SUPPLIER, but
> that
> felt a bit too long considering other flags.
> Can you please consider suggesting a concise name?
>
> Regards
> Vivek
>
>>
>>>     /**
>>>    * struct device_link - Device link representation.
>>
>>
>>
>



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] device core: Add flag to autoremove device link on supplier unbind
  2018-06-26 10:03     ` Vivek Gautam
@ 2018-06-26 10:19       ` Rafael J. Wysocki
  2018-06-26 11:48         ` Vivek Gautam
  0 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2018-06-26 10:19 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: Rafael J. Wysocki, Ulf Hansson, Marek Szyprowski, Greg KH, lukas,
	dmitry.torokhov, aspriel, Robin Murphy, open list, Linux PM

On Tuesday, June 26, 2018 12:03:44 PM CEST Vivek Gautam wrote:
> On Wed, May 30, 2018 at 6:21 PM, Vivek Gautam
> <vivek.gautam@codeaurora.org> wrote:
> 
> Adding Ulf and Marek for their side of comments.
> In summary, we do not want to not maintain a device link pointer
> when adding a device link in supplier's driver, to delete the link later.
> An autoremove flag from the suppliers side (similar to what we already
> have for consumer side) can help autoremove the device link
> when supplier driver goes away.
> 
> Hi Rafael, Lukas,
> Gentle ping. Can you please consider reviewing this patch.

ISTR telling you that I didn't like the DL_FLAG_AUTOREMOVE_S name
as it is too similar to another existing flag.  That hasn't changed.

BTW, please CC the patch to linux-pm when you resend it.

Thanks,
Rafael


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] device core: Add flag to autoremove device link on supplier unbind
  2018-06-26 10:19       ` Rafael J. Wysocki
@ 2018-06-26 11:48         ` Vivek Gautam
  0 siblings, 0 replies; 6+ messages in thread
From: Vivek Gautam @ 2018-06-26 11:48 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Ulf Hansson, Marek Szyprowski, Greg KH, lukas,
	dmitry.torokhov, aspriel, Robin Murphy, open list, Linux PM

Hi Rafael,

On Tue, Jun 26, 2018 at 3:49 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, June 26, 2018 12:03:44 PM CEST Vivek Gautam wrote:
>> On Wed, May 30, 2018 at 6:21 PM, Vivek Gautam
>> <vivek.gautam@codeaurora.org> wrote:
>>
>> Adding Ulf and Marek for their side of comments.
>> In summary, we do not want to not maintain a device link pointer
>> when adding a device link in supplier's driver, to delete the link later.
>> An autoremove flag from the suppliers side (similar to what we already
>> have for consumer side) can help autoremove the device link
>> when supplier driver goes away.
>>
>> Hi Rafael, Lukas,
>> Gentle ping. Can you please consider reviewing this patch.
>
> ISTR telling you that I didn't like the DL_FLAG_AUTOREMOVE_S name
> as it is too similar to another existing flag.  That hasn't changed.

Right. I thought to gather more comments about the patch overall
before attempting to respin with the changed name.
I will send the next version.

>
> BTW, please CC the patch to linux-pm when you resend it.

Sure. I will do that. Thanks.

Best regards
Vivek

>
> Thanks,
> Rafael
>



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-06-26 11:48 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-30  9:57 [PATCH 1/1] device core: Add flag to autoremove device link on supplier unbind Vivek Gautam
2018-05-30 10:59 ` Rafael J. Wysocki
2018-05-30 12:51   ` Vivek Gautam
2018-06-26 10:03     ` Vivek Gautam
2018-06-26 10:19       ` Rafael J. Wysocki
2018-06-26 11:48         ` Vivek Gautam

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).