Re: [PATCH 1/1] device core: Add flag to autoremove device link on supplier unbind

Re: [PATCH 1/1] device core: Add flag to autoremove device link on supplier unbind

[Rafael J. Wysocki]

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.

Re: [PATCH 1/1] device core: Add flag to autoremove device link on supplier unbind

[Vivek Gautam]

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

Re: [PATCH 1/1] device core: Add flag to autoremove device link on supplier unbind

[Vivek Gautam]

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

Re: [PATCH 1/1] device core: Add flag to autoremove device link on supplier unbind

[Rafael J. Wysocki]

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

Re: [PATCH 1/1] device core: Add flag to autoremove device link on supplier unbind

[Vivek Gautam]

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