diff mbox

[RFC,1/3] of: make of_mutex public

Message ID 1435700651-3543-2-git-send-email-wsa@the-dreams.de (mailing list archive)
State New, archived
Headers show

Commit Message

Wolfram Sang June 30, 2015, 9:44 p.m. UTC
From: Wolfram Sang <wsa+renesas@sang-engineering.com>

If we want to use OF_DYNAMIC features outside the of framework, we need
to access this lock. If OF maintainers don't like exporting, we can
maybe create accessor functions that we can use?

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/of/of_private.h | 1 -
 include/linux/of.h      | 2 ++
 2 files changed, 2 insertions(+), 1 deletion(-)

Comments

Rob Herring July 3, 2015, 4:43 a.m. UTC | #1
+Pantelis

On Tue, Jun 30, 2015 at 4:44 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> If we want to use OF_DYNAMIC features outside the of framework, we need
> to access this lock. If OF maintainers don't like exporting, we can
> maybe create accessor functions that we can use?

It looks like you are just taking the mutex around various
of_changeset_* calls. We should either split the mutex into of_mutex
for core and changesets and an overlay mutex for overlays, or we need
to do the typical pattern of having changeset functions having locked
and unlocked versions for external and internal (overlays) use.

Rob

> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/of/of_private.h | 1 -
>  include/linux/of.h      | 2 ++
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index 8e882e706cd8c6..f92ec41efb5dfd 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -31,7 +31,6 @@ struct alias_prop {
>         char stem[0];
>  };
>
> -extern struct mutex of_mutex;
>  extern struct list_head aliases_lookup;
>  extern struct kset *of_kset;
>
> diff --git a/include/linux/of.h b/include/linux/of.h
> index b871ff9d81d720..f0464efcbdc5aa 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -32,6 +32,8 @@
>  typedef u32 phandle;
>  typedef u32 ihandle;
>
> +extern struct mutex of_mutex;
> +
>  struct property {
>         char    *name;
>         int     length;
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pantelis Antoniou July 3, 2015, 6:54 a.m. UTC | #2
+Grant

Hi Rob,

> On Jul 3, 2015, at 07:43 , Rob Herring <robherring2@gmail.com> wrote:
> 
> +Pantelis
> 
> On Tue, Jun 30, 2015 at 4:44 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> 
>> If we want to use OF_DYNAMIC features outside the of framework, we need
>> to access this lock. If OF maintainers don't like exporting, we can
>> maybe create accessor functions that we can use?
> 
> It looks like you are just taking the mutex around various
> of_changeset_* calls. We should either split the mutex into of_mutex
> for core and changesets and an overlay mutex for overlays, or we need
> to do the typical pattern of having changeset functions having locked
> and unlocked versions for external and internal (overlays) use.
> 

IIRC we had an argument with Grant just about that. The original
overlays patches did use a two level locking scheme, a changeset specific 
mutex, and a global transaction mutex.

They were dropped after Grant’s request.

>> +
>> +/**
>> + * struct of_transaction - transaction tracker structure
>> + *
>> + * @lock:	lock for accessing the te_list and state entries
>> + * @te_list:	list_head for the transaction entries
>> + * @state:	State of the transaction
>> + *
>> + * Transactions are a convenient way to apply bulk changes to the
>> + * live tree. In case of an error, changes are rolled-back.
>> + * Transactions live on after initial application, and if not
>> + * destroyed after use, they can be reverted in one single call.
>> + */
>> +struct of_transaction {
>> +	struct mutex lock;
> 
> What's the lock for? There will only ever by a single transaction in
> flight at any given time, and when a transaction is /not/ in flight, it
> is completely owned by the caller. I see no condition where there needs
> to be a lock just for the of_transaction structure.

What is it that we’re trying to do here? What is the use case?

> Rob
> 

Regards

— Pantelis

>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> ---
>> drivers/of/of_private.h | 1 -
>> include/linux/of.h      | 2 ++
>> 2 files changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
>> index 8e882e706cd8c6..f92ec41efb5dfd 100644
>> --- a/drivers/of/of_private.h
>> +++ b/drivers/of/of_private.h
>> @@ -31,7 +31,6 @@ struct alias_prop {
>>        char stem[0];
>> };
>> 
>> -extern struct mutex of_mutex;
>> extern struct list_head aliases_lookup;
>> extern struct kset *of_kset;
>> 
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index b871ff9d81d720..f0464efcbdc5aa 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -32,6 +32,8 @@
>> typedef u32 phandle;
>> typedef u32 ihandle;
>> 
>> +extern struct mutex of_mutex;
>> +
>> struct property {
>>        char    *name;
>>        int     length;
>> --
>> 2.1.4
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" 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 devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang July 3, 2015, 7:24 a.m. UTC | #3
> What is it that we’re trying to do here? What is the use case?

http://www.spinics.net/lists/devicetree/msg85462.html

I'll bounce the mails to you as well.

Thanks,

   Wolfram
diff mbox

Patch

diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 8e882e706cd8c6..f92ec41efb5dfd 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -31,7 +31,6 @@  struct alias_prop {
 	char stem[0];
 };
 
-extern struct mutex of_mutex;
 extern struct list_head aliases_lookup;
 extern struct kset *of_kset;
 
diff --git a/include/linux/of.h b/include/linux/of.h
index b871ff9d81d720..f0464efcbdc5aa 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -32,6 +32,8 @@ 
 typedef u32 phandle;
 typedef u32 ihandle;
 
+extern struct mutex of_mutex;
+
 struct property {
 	char	*name;
 	int	length;