Re: [PATCH 02/10] rpmsg: core: Add channel creation internal API

From: Arnaud POULIQUEN <arnaud.pouliquen@st.com>
To: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>,
	Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: "ohad@wizery.com" <ohad@wizery.com>,
	"bjorn.andersson@linaro.org" <bjorn.andersson@linaro.org>,
	Loic PALLARDY <loic.pallardy@st.com>,
	"linux-remoteproc@vger.kernel.org"
	<linux-remoteproc@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 02/10] rpmsg: core: Add channel creation internal API
Date: Thu, 1 Oct 2020 16:46:52 +0200	[thread overview]
Message-ID: <beb0b4d9-e661-43d2-c5d5-b90eccf11036@st.com> (raw)
In-Reply-To: <20200930063553.GB20683@ubuntu>

On 9/30/20 8:35 AM, Guennadi Liakhovetski wrote:
> On Mon, Sep 21, 2020 at 06:09:52PM -0600, Mathieu Poirier wrote:
>> From: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>>
>> Add the channel creation API as a first step to be able to define the
>> name service announcement as a rpmsg driver independent from the RPMsg
>> virtio bus.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>> ---
>>  drivers/rpmsg/rpmsg_core.c     | 45 ++++++++++++++++++++++++++++++++++
>>  drivers/rpmsg/rpmsg_internal.h | 12 +++++++++
>>  2 files changed, 57 insertions(+)
>>
>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>> index 91de940896e3..50a835eaf1ba 100644
>> --- a/drivers/rpmsg/rpmsg_core.c
>> +++ b/drivers/rpmsg/rpmsg_core.c
>> @@ -20,6 +20,51 @@
>>  
>>  #include "rpmsg_internal.h"
>>  
>> +/**
>> + * rpmsg_create_channel() - create a new rpmsg channel
>> + * using its name and address info.
>> + * @rpdev: rpmsg driver
> 
> device
> 
>> + * @chinfo: channel_info to bind
>> + *
>> + * Returns a pointer to the new rpmsg device on success, or NULL on error.
>> + */
>> +struct rpmsg_device *
>> +	rpmsg_create_channel(struct rpmsg_device *rpdev,
> 
> You might call this nitpicking, but we already have two indentation styles for 
> functions:
> 
> return_type function(type1 arg1,
> 			...)
> 
> (my personal preference, it also has sub-variants - depending on the aligning 
> of the second line and any following lines, and one more moving "type1 arg1" 
> to the second line)
> 
> return_type
> function(...
> 
> and now you're also introducing the third style - with "function" indented... 
> Maybe we don't need more of those, particularly since now with 100 chars per 
> line in most cases the very first style can be used.

Right, bad coding style.

> 
>> +			     struct rpmsg_channel_info *chinfo)
>> +{
>> +	if (WARN_ON(!rpdev))
>> +		return NULL;
>> +	if (!rpdev->ops || !rpdev->ops->create_channel) {
>> +		dev_err(&rpdev->dev, "no create_channel ops found\n");
>> +		return NULL;
>> +	}
>> +
>> +	return rpdev->ops->create_channel(rpdev, chinfo);
>> +}
>> +EXPORT_SYMBOL(rpmsg_create_channel);
>> +
>> +/**
>> + * rpmsg_release_channel() - release a rpmsg channel
>> + * using its name and address info.
>> + * @rpdev: rpmsg driver
> 
> device
> 
>> + * @chinfo: channel_info to bind
>> + *
>> + * Returns 0 on success or an appropriate error value.
>> + */
>> +int rpmsg_release_channel(struct rpmsg_device *rpdev,
>> +			  struct rpmsg_channel_info *chinfo)
>> +{
>> +	if (WARN_ON(!rpdev))
>> +		return -EINVAL;
>> +	if (!rpdev->ops || !rpdev->ops->release_channel) {
>> +		dev_err(&rpdev->dev, "no release_channel ops found\n");
>> +		return -EPERM;
> 
> ENOSYS or EOPNOTSUPP? I'm never sure which one is appropriate for 
> this kind of errors.

For coherency with the other RPMsg API functions, could be ENXIO...

> 
>> +	}
>> +
>> +	return rpdev->ops->release_channel(rpdev, chinfo);
>> +}
>> +EXPORT_SYMBOL(rpmsg_release_channel);
>> +
>>  /**
>>   * rpmsg_create_ept() - create a new rpmsg_endpoint
>>   * @rpdev: rpmsg channel device
>> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
>> index 3fc83cd50e98..587f723757d4 100644
>> --- a/drivers/rpmsg/rpmsg_internal.h
>> +++ b/drivers/rpmsg/rpmsg_internal.h
>> @@ -20,6 +20,8 @@
>>  
>>  /**
>>   * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
>> + * @create_channel:	create backend-specific channel, optional
>> + * @release_channel:	release backend-specific channel, optional
> 
> Are they really optional? You return errors if they aren't available.

I think they are optional, the back-end might not support the operation. 
In this case, activating an RPMsg client that uses the interface should result in an
error because the service is not compatible with the back-end implementation.

Regards,
Arnaud

> 
>>   * @create_ept:		create backend-specific endpoint, required
>>   * @announce_create:	announce presence of new channel, optional
>>   * @announce_destroy:	announce destruction of channel, optional
>> @@ -29,6 +31,11 @@
>>   * advertise new channels implicitly by creating the endpoints.
>>   */
>>  struct rpmsg_device_ops {
>> +	struct rpmsg_device *(*create_channel)(struct rpmsg_device *rpdev,
>> +					     struct rpmsg_channel_info *chinfo);
>> +	int (*release_channel)(struct rpmsg_device *rpdev,
>> +			       struct rpmsg_channel_info *chinfo);
>> +
>>  	struct rpmsg_endpoint *(*create_ept)(struct rpmsg_device *rpdev,
>>  					    rpmsg_rx_cb_t cb, void *priv,
>>  					    struct rpmsg_channel_info chinfo);
>> @@ -75,6 +82,11 @@ int rpmsg_unregister_device(struct device *parent,
>>  struct device *rpmsg_find_device(struct device *parent,
>>  				 struct rpmsg_channel_info *chinfo);
>>  
>> +struct rpmsg_device *
>> +rpmsg_create_channel(struct rpmsg_device *rpdev,
>> +		     struct rpmsg_channel_info *chinfo);
>> +int rpmsg_release_channel(struct rpmsg_device *rpdev,
>> +			  struct rpmsg_channel_info *chinfo);
>>  /**
>>   * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
>>   * @rpdev:	prepared rpdev to be used for creating endpoints
>> -- 
>> 2.25.1
>>