Re: [PATCH 03/11] firmware: arm_scmi: introduce common protocol interface

2020-10-28 Thread Cristian Marussi 
On Mon, Oct 26, 2020 at 09:07:12AM -0400, Thara Gopinath wrote:

Hi Thara,

> 
> 
> On 10/21/20 6:27 AM, Cristian Marussi wrote:
> > On Tue, Oct 20, 2020 at 10:47:10PM -0400, Thara Gopinath wrote:
> > > 
> > > 
> > > On 10/14/20 11:05 AM, Cristian Marussi wrote:
> > > > Introduce generic get_ops/put_ops handle operations: any protocol, both
> > > > standard or custom, now exposes its operations through this common
> > > > interface which internally takes care to account for protocols' usage:
> > > > protocols' initialization is now performed on demand as soon as the 
> > > > first
> > > > user shows up while deinitialization (if any) is performed once
> > > > the last user of a protocol has gone.
> > > > Registered events' notifier are tracked too against the related 
> > > > protocol.
> > > > Convert all SCMI drivers to the new interface too.
> > > > 
> > > > Signed-off-by: Cristian Marussi 
> > > > ---
> > > 
> > > [...]
> > > 
> > > 
> > > > diff --git a/drivers/firmware/arm_scmi/driver.c 
> > > > b/drivers/firmware/arm_scmi/driver.c
> > > > index bad1d0130e96..049220efd227 100644
> > > > --- a/drivers/firmware/arm_scmi/driver.c
> > > > +++ b/drivers/firmware/arm_scmi/driver.c
> > > > @@ -585,7 +585,7 @@ void *scmi_get_proto_priv(const struct scmi_handle 
> > > > *handle, u8 protocol_id)
> > > > * Return: A reference to an initialized protocol instance or error 
> > > > on failure.
> > > > */
> > > >static struct scmi_protocol_instance * __must_check
> > > > -scmi_get_protocol_instance(struct scmi_handle *handle, u8 protocol_id)
> > > > +scmi_get_protocol_instance(const struct scmi_handle *handle, u8 
> > > > protocol_id)
> > > >{
> > > > int ret = -ENOMEM;
> > > > void *gid;
> > > > @@ -655,7 +655,7 @@ scmi_get_protocol_instance(struct scmi_handle 
> > > > *handle, u8 protocol_id)
> > > > *
> > > > * Return: 0 if protocol was acquired successfully.
> > > > */
> > > > -int scmi_acquire_protocol(struct scmi_handle *handle, u8 protocol_id)
> > > > +int scmi_acquire_protocol(const struct scmi_handle *handle, u8 
> > > > protocol_id)
> > > >{
> > > > return IS_ERR(scmi_get_protocol_instance(handle, protocol_id));
> > > >}
> > > > @@ -668,7 +668,7 @@ int scmi_acquire_protocol(struct scmi_handle 
> > > > *handle, u8 protocol_id)
> > > > * Remove one user for the specified protocol and triggers 
> > > > de-initialization
> > > > * and resources de-allocation once the last user has gone.
> > > > */
> > > > -void scmi_release_protocol(struct scmi_handle *handle, u8 protocol_id)
> > > > +void scmi_release_protocol(const struct scmi_handle *handle, u8 
> > > > protocol_id)
> > > >{
> > > > struct scmi_info *info = handle_to_scmi_info(handle);
> > > > struct scmi_protocol_instance *pi;
> > > > @@ -700,6 +700,29 @@ void scmi_release_protocol(struct scmi_handle 
> > > > *handle, u8 protocol_id)
> > > > mutex_unlock(>protocols_mtx);
> > > >}
> > > > +/**
> > > > + * scmi_get_protocol_operations  - Get protocol operations
> > > > + * @handle: A reference to the SCMI platform instance.
> > > > + * @protocol_id: The protocol being requested.
> > > > + *
> > > > + * Get hold of a protocol accounting for its usage, eventually 
> > > > triggering its
> > > > + * initialization, and returning the protocol specific operations.
> > > > + *
> > > > + * Return: A reference to the requested protocol operations or error.
> > > > + *Must be checked for errors by caller.
> > > > + */
> > > > +static const void __must_check
> > > > +*scmi_get_protocol_operations(const struct scmi_handle *handle, u8 
> > > > protocol_id)
> > > > +{
> > > > +   struct scmi_protocol_instance *pi;
> > > > +
> > > > +   pi = scmi_get_protocol_instance(handle, protocol_id);
> > > > +   if (IS_ERR(pi))
> > > > +   return pi;
> > > > +
> > > > +   return pi->proto->ops;
> > > > +} > +
> > > >void scmi_setup_protocol_implemented(const struct scmi_handle 
> > > > *handle,
> > > >  u8 *prot_imp)
> > > >{
> > > > @@ -975,6 +998,8 @@ static int scmi_probe(struct platform_device *pdev)
> > > > handle = >handle;
> > > > handle->dev = info->dev;
> > > > handle->version = >version;
> > > > +   handle->get_ops = scmi_get_protocol_operations;
> > > > +   handle->put_ops = scmi_release_protocol;
> > > 
> > > 
> > > Why do you need get_ops and put_ops? Why not have the drivers call
> > > scmi_acquire_protocol and scmi_release_protocol directly and get the ops
> > > from retrieved scmi_get_protocol_instance ? IMHO, this makes it more
> > > readable. Also, this will make the usage of scmi_acquire_protocol and
> > > scmi_release_protocol more consistent. Right now, notify.c uses
> > > scmi_acquire_protocol to acquire protocol because there is no need for ops
> > > and other drivers use get_ops to acquire protocol. Kind of confusing..
> > > 
> > 
> > Trying to

Re: [PATCH 03/11] firmware: arm_scmi: introduce common protocol interface

2020-10-26 Thread Thara Gopinath 




On 10/21/20 6:27 AM, Cristian Marussi wrote:

On Tue, Oct 20, 2020 at 10:47:10PM -0400, Thara Gopinath wrote:



On 10/14/20 11:05 AM, Cristian Marussi wrote:

Introduce generic get_ops/put_ops handle operations: any protocol, both
standard or custom, now exposes its operations through this common
interface which internally takes care to account for protocols' usage:
protocols' initialization is now performed on demand as soon as the first
user shows up while deinitialization (if any) is performed once
the last user of a protocol has gone.
Registered events' notifier are tracked too against the related protocol.
Convert all SCMI drivers to the new interface too.

Signed-off-by: Cristian Marussi 
---


[...]



diff --git a/drivers/firmware/arm_scmi/driver.c 
b/drivers/firmware/arm_scmi/driver.c
index bad1d0130e96..049220efd227 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -585,7 +585,7 @@ void *scmi_get_proto_priv(const struct scmi_handle *handle, 
u8 protocol_id)
* Return: A reference to an initialized protocol instance or error on 
failure.
*/
   static struct scmi_protocol_instance * __must_check
-scmi_get_protocol_instance(struct scmi_handle *handle, u8 protocol_id)
+scmi_get_protocol_instance(const struct scmi_handle *handle, u8 protocol_id)
   {
int ret = -ENOMEM;
void *gid;
@@ -655,7 +655,7 @@ scmi_get_protocol_instance(struct scmi_handle *handle, u8 
protocol_id)
*
* Return: 0 if protocol was acquired successfully.
*/
-int scmi_acquire_protocol(struct scmi_handle *handle, u8 protocol_id)
+int scmi_acquire_protocol(const struct scmi_handle *handle, u8 protocol_id)
   {
return IS_ERR(scmi_get_protocol_instance(handle, protocol_id));
   }
@@ -668,7 +668,7 @@ int scmi_acquire_protocol(struct scmi_handle *handle, u8 
protocol_id)
* Remove one user for the specified protocol and triggers de-initialization
* and resources de-allocation once the last user has gone.
*/
-void scmi_release_protocol(struct scmi_handle *handle, u8 protocol_id)
+void scmi_release_protocol(const struct scmi_handle *handle, u8 protocol_id)
   {
struct scmi_info *info = handle_to_scmi_info(handle);
struct scmi_protocol_instance *pi;
@@ -700,6 +700,29 @@ void scmi_release_protocol(struct scmi_handle *handle, u8 
protocol_id)
mutex_unlock(>protocols_mtx);
   }
+/**
+ * scmi_get_protocol_operations  - Get protocol operations
+ * @handle: A reference to the SCMI platform instance.
+ * @protocol_id: The protocol being requested.
+ *
+ * Get hold of a protocol accounting for its usage, eventually triggering its
+ * initialization, and returning the protocol specific operations.
+ *
+ * Return: A reference to the requested protocol operations or error.
+ *Must be checked for errors by caller.
+ */
+static const void __must_check
+*scmi_get_protocol_operations(const struct scmi_handle *handle, u8 protocol_id)
+{
+   struct scmi_protocol_instance *pi;
+
+   pi = scmi_get_protocol_instance(handle, protocol_id);
+   if (IS_ERR(pi))
+   return pi;
+
+   return pi->proto->ops;
+} > +
   void scmi_setup_protocol_implemented(const struct scmi_handle *handle,
 u8 *prot_imp)
   {
@@ -975,6 +998,8 @@ static int scmi_probe(struct platform_device *pdev)
handle = >handle;
handle->dev = info->dev;
handle->version = >version;
+   handle->get_ops = scmi_get_protocol_operations;
+   handle->put_ops = scmi_release_protocol;



Why do you need get_ops and put_ops? Why not have the drivers call
scmi_acquire_protocol and scmi_release_protocol directly and get the ops
from retrieved scmi_get_protocol_instance ? IMHO, this makes it more
readable. Also, this will make the usage of scmi_acquire_protocol and
scmi_release_protocol more consistent. Right now, notify.c uses
scmi_acquire_protocol to acquire protocol because there is no need for ops
and other drivers use get_ops to acquire protocol. Kind of confusing..



Trying to avoid exporting new synbols if not strictly needed, I exposed
get_ops()/put_ops() via handle for the usage of the SCMI drivers users,
while keeping scmi_acquire/release as internal non-exported wrappers used
only by the SCMI core itself like notifications.
You cannot call acquire/release from a loadable module as of now.

Additionally I thougt to add these wrappers for cases in which like
notifications you don't need the ops really (like notif or base) nor
the related forced __must_check(like notif), but just to get hold of
the protocol to avoid it being possibly unloaded.

I would antyway keep the get_ops/put_ops and I could just drop
acquire/release if confusing and use the raw ops methods also internally,
properly checking for the result everytime: currently notifications core
takes care to acquire a protocol only once the requested event has been
registered by some protocol (i.e. event handler is NOT

Re: [PATCH 03/11] firmware: arm_scmi: introduce common protocol interface

2020-10-21 Thread Cristian Marussi 
On Tue, Oct 20, 2020 at 10:47:10PM -0400, Thara Gopinath wrote:
> 
> 
> On 10/14/20 11:05 AM, Cristian Marussi wrote:
> > Introduce generic get_ops/put_ops handle operations: any protocol, both
> > standard or custom, now exposes its operations through this common
> > interface which internally takes care to account for protocols' usage:
> > protocols' initialization is now performed on demand as soon as the first
> > user shows up while deinitialization (if any) is performed once
> > the last user of a protocol has gone.
> > Registered events' notifier are tracked too against the related protocol.
> > Convert all SCMI drivers to the new interface too.
> > 
> > Signed-off-by: Cristian Marussi 
> > ---
> 
> [...]
> 
> 
> > diff --git a/drivers/firmware/arm_scmi/driver.c 
> > b/drivers/firmware/arm_scmi/driver.c
> > index bad1d0130e96..049220efd227 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -585,7 +585,7 @@ void *scmi_get_proto_priv(const struct scmi_handle 
> > *handle, u8 protocol_id)
> >* Return: A reference to an initialized protocol instance or error on 
> > failure.
> >*/
> >   static struct scmi_protocol_instance * __must_check
> > -scmi_get_protocol_instance(struct scmi_handle *handle, u8 protocol_id)
> > +scmi_get_protocol_instance(const struct scmi_handle *handle, u8 
> > protocol_id)
> >   {
> > int ret = -ENOMEM;
> > void *gid;
> > @@ -655,7 +655,7 @@ scmi_get_protocol_instance(struct scmi_handle *handle, 
> > u8 protocol_id)
> >*
> >* Return: 0 if protocol was acquired successfully.
> >*/
> > -int scmi_acquire_protocol(struct scmi_handle *handle, u8 protocol_id)
> > +int scmi_acquire_protocol(const struct scmi_handle *handle, u8 protocol_id)
> >   {
> > return IS_ERR(scmi_get_protocol_instance(handle, protocol_id));
> >   }
> > @@ -668,7 +668,7 @@ int scmi_acquire_protocol(struct scmi_handle *handle, 
> > u8 protocol_id)
> >* Remove one user for the specified protocol and triggers 
> > de-initialization
> >* and resources de-allocation once the last user has gone.
> >*/
> > -void scmi_release_protocol(struct scmi_handle *handle, u8 protocol_id)
> > +void scmi_release_protocol(const struct scmi_handle *handle, u8 
> > protocol_id)
> >   {
> > struct scmi_info *info = handle_to_scmi_info(handle);
> > struct scmi_protocol_instance *pi;
> > @@ -700,6 +700,29 @@ void scmi_release_protocol(struct scmi_handle *handle, 
> > u8 protocol_id)
> > mutex_unlock(>protocols_mtx);
> >   }
> > +/**
> > + * scmi_get_protocol_operations  - Get protocol operations
> > + * @handle: A reference to the SCMI platform instance.
> > + * @protocol_id: The protocol being requested.
> > + *
> > + * Get hold of a protocol accounting for its usage, eventually triggering 
> > its
> > + * initialization, and returning the protocol specific operations.
> > + *
> > + * Return: A reference to the requested protocol operations or error.
> > + *Must be checked for errors by caller.
> > + */
> > +static const void __must_check
> > +*scmi_get_protocol_operations(const struct scmi_handle *handle, u8 
> > protocol_id)
> > +{
> > +   struct scmi_protocol_instance *pi;
> > +
> > +   pi = scmi_get_protocol_instance(handle, protocol_id);
> > +   if (IS_ERR(pi))
> > +   return pi;
> > +
> > +   return pi->proto->ops;
> > +} > +
> >   void scmi_setup_protocol_implemented(const struct scmi_handle *handle,
> >  u8 *prot_imp)
> >   {
> > @@ -975,6 +998,8 @@ static int scmi_probe(struct platform_device *pdev)
> > handle = >handle;
> > handle->dev = info->dev;
> > handle->version = >version;
> > +   handle->get_ops = scmi_get_protocol_operations;
> > +   handle->put_ops = scmi_release_protocol;
> 
> 
> Why do you need get_ops and put_ops? Why not have the drivers call
> scmi_acquire_protocol and scmi_release_protocol directly and get the ops
> from retrieved scmi_get_protocol_instance ? IMHO, this makes it more
> readable. Also, this will make the usage of scmi_acquire_protocol and
> scmi_release_protocol more consistent. Right now, notify.c uses
> scmi_acquire_protocol to acquire protocol because there is no need for ops
> and other drivers use get_ops to acquire protocol. Kind of confusing..
> 

Trying to avoid exporting new synbols if not strictly needed, I exposed
get_ops()/put_ops() via handle for the usage of the SCMI drivers users,
while keeping scmi_acquire/release as internal non-exported wrappers used
only by the SCMI core itself like notifications.
You cannot call acquire/release from a loadable module as of now.

Additionally I thougt to add these wrappers for cases in which like
notifications you don't need the ops really (like notif or base) nor 
the related forced __must_check(like notif), but just to get hold of
the protocol to avoid it being possibly unloaded.

I would antyway keep the get_ops/put_ops and I could just drop
acquire/release if confusing and use

Re: [PATCH 03/11] firmware: arm_scmi: introduce common protocol interface

2020-10-20 Thread Thara Gopinath 




On 10/14/20 11:05 AM, Cristian Marussi wrote:

Introduce generic get_ops/put_ops handle operations: any protocol, both
standard or custom, now exposes its operations through this common
interface which internally takes care to account for protocols' usage:
protocols' initialization is now performed on demand as soon as the first
user shows up while deinitialization (if any) is performed once
the last user of a protocol has gone.
Registered events' notifier are tracked too against the related protocol.
Convert all SCMI drivers to the new interface too.

Signed-off-by: Cristian Marussi 
---


[...]



diff --git a/drivers/firmware/arm_scmi/driver.c 
b/drivers/firmware/arm_scmi/driver.c
index bad1d0130e96..049220efd227 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -585,7 +585,7 @@ void *scmi_get_proto_priv(const struct scmi_handle *handle, 
u8 protocol_id)
   * Return: A reference to an initialized protocol instance or error on 
failure.
   */
  static struct scmi_protocol_instance * __must_check
-scmi_get_protocol_instance(struct scmi_handle *handle, u8 protocol_id)
+scmi_get_protocol_instance(const struct scmi_handle *handle, u8 protocol_id)
  {
int ret = -ENOMEM;
void *gid;
@@ -655,7 +655,7 @@ scmi_get_protocol_instance(struct scmi_handle *handle, u8 
protocol_id)
   *
   * Return: 0 if protocol was acquired successfully.
   */
-int scmi_acquire_protocol(struct scmi_handle *handle, u8 protocol_id)
+int scmi_acquire_protocol(const struct scmi_handle *handle, u8 protocol_id)
  {
return IS_ERR(scmi_get_protocol_instance(handle, protocol_id));
  }
@@ -668,7 +668,7 @@ int scmi_acquire_protocol(struct scmi_handle *handle, u8 
protocol_id)
   * Remove one user for the specified protocol and triggers de-initialization
   * and resources de-allocation once the last user has gone.
   */
-void scmi_release_protocol(struct scmi_handle *handle, u8 protocol_id)
+void scmi_release_protocol(const struct scmi_handle *handle, u8 protocol_id)
  {
struct scmi_info *info = handle_to_scmi_info(handle);
struct scmi_protocol_instance *pi;
@@ -700,6 +700,29 @@ void scmi_release_protocol(struct scmi_handle *handle, u8 
protocol_id)
mutex_unlock(>protocols_mtx);
  }
  
+/**

+ * scmi_get_protocol_operations  - Get protocol operations
+ * @handle: A reference to the SCMI platform instance.
+ * @protocol_id: The protocol being requested.
+ *
+ * Get hold of a protocol accounting for its usage, eventually triggering its
+ * initialization, and returning the protocol specific operations.
+ *
+ * Return: A reference to the requested protocol operations or error.
+ *Must be checked for errors by caller.
+ */
+static const void __must_check
+*scmi_get_protocol_operations(const struct scmi_handle *handle, u8 protocol_id)
+{
+   struct scmi_protocol_instance *pi;
+
+   pi = scmi_get_protocol_instance(handle, protocol_id);
+   if (IS_ERR(pi))
+   return pi;
+
+   return pi->proto->ops;
+} > +
  void scmi_setup_protocol_implemented(const struct scmi_handle *handle,
 u8 *prot_imp)
  {
@@ -975,6 +998,8 @@ static int scmi_probe(struct platform_device *pdev)
handle = >handle;
handle->dev = info->dev;
handle->version = >version;
+   handle->get_ops = scmi_get_protocol_operations;
+   handle->put_ops = scmi_release_protocol;



Why do you need get_ops and put_ops? Why not have the drivers call
scmi_acquire_protocol and scmi_release_protocol directly and get the ops
from retrieved scmi_get_protocol_instance ? IMHO, this makes it more 
readable. Also, this will make the usage of scmi_acquire_protocol and 
scmi_release_protocol more consistent. Right now, notify.c uses
scmi_acquire_protocol to acquire protocol because there is no need for 
ops and other drivers use get_ops to acquire protocol. Kind of confusing..


--
Warm Regards
Thara

  
  	ret = scmi_txrx_setup(info, dev, SCMI_PROTOCOL_BASE);

if (ret)
diff --git a/drivers/firmware/arm_scmi/notify.c 
b/drivers/firmware/arm_scmi/notify.c
index eae58b2a92cc..02b00af9b08f 100644
--- a/drivers/firmware/arm_scmi/notify.c
+++ b/drivers/firmware/arm_scmi/notify.c
@@ -367,7 +367,7 @@ static struct scmi_event_handler *
  scmi_get_active_handler(struct scmi_notify_instance *ni, u32 evt_key);
  static void scmi_put_active_handler(struct scmi_notify_instance *ni,
struct scmi_event_handler *hndl);
-static void scmi_put_handler_unlocked(struct scmi_notify_instance *ni,
+static bool scmi_put_handler_unlocked(struct scmi_notify_instance *ni,
  struct scmi_event_handler *hndl);
  
  /**

@@ -899,9 +899,21 @@ static inline int scmi_bind_event_handler(struct 
scmi_notify_instance *ni,
if (!r_evt)
return -EINVAL;
  
-	/* Remove from pending and insert into registered */

+   /*
+* Remove from pending and