linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Cristian Marussi <cristian.marussi@arm.com>
To: Peter Hilber <peter.hilber@opensynergy.com>
Cc: linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	virtualization@lists.linux-foundation.org,
	virtio-dev@lists.oasis-open.org, sudeep.holla@arm.com,
	james.quinlan@broadcom.com, Jonathan.Cameron@Huawei.com,
	f.fainelli@gmail.com, etienne.carriere@linaro.org,
	vincent.guittot@linaro.org, souvik.chakravarty@arm.com,
	igor.skalkin@opensynergy.com, alex.bennee@linaro.org,
	jean-philippe@linaro.org, mikhail.golubev@opensynergy.com,
	anton.yakovlev@opensynergy.com, Vasyl.Vavrychuk@opensynergy.com,
	Andriy.Tryshnivskyy@opensynergy.com
Subject: Re: [PATCH v4 05/16] firmware: arm_scmi: Introduce delegated xfers support
Date: Thu, 1 Jul 2021 11:24:35 +0100	[thread overview]
Message-ID: <20210701102435.GB17807@e120937-lin> (raw)
In-Reply-To: <1675b521-8164-4daa-baa1-592268474a56@opensynergy.com>

Hi,

On Thu, Jul 01, 2021 at 10:42:51AM +0200, Peter Hilber wrote:
> On 11.06.21 18:59, Cristian Marussi wrote:
> > Introduce optional support for delegated xfers allocation.
> > 
> > An SCMI transport can optionally declare to support delegated xfers and
> > then use a few helper functions exposed by the core SCMI transport layer to
> > query the core for existing in-flight transfers matching a provided message
> > header or alternatively and transparently obtain a brand new xfer to handle
> > a freshly received notification message.
> > In both cases the obtained xfer is uniquely mapped into a specific xfer
> > through the means of the message header acting as key.
> > 
> > In this way such a transport can properly store its own transport specific
> > payload into the xfer uniquely associated to the message header before
> > even calling into the core scmi_rx_callback() in the usual way, so that
> > the transport specific message envelope structures can be freed early
> > and there is no more need to keep track of their status till the core
> > fully processes the xfer to completion or times out.
> > 
> > The scmi_rx_callbak() does not need to be modified to carry additional
> > transport-specific ancillary data related to such message envelopes since
> > an unique natural association is established between the xfer and the
> > related message header.
> > 
> > Existing transports that do not need anything of the above will continue
> > to work as before without any change.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> >  drivers/firmware/arm_scmi/common.h |  14 +++
> >  drivers/firmware/arm_scmi/driver.c | 132 ++++++++++++++++++++++++++++-
> >  2 files changed, 143 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> > index e64c5ca9ee7c..0edc04bc434c 100644
> > --- a/drivers/firmware/arm_scmi/common.h
> > +++ b/drivers/firmware/arm_scmi/common.h
> > @@ -80,6 +80,7 @@ struct scmi_msg_resp_prot_version {
> >   * @status: Status of the transfer once it's complete
> >   * @poll_completion: Indicate if the transfer needs to be polled for
> >   *	completion or interrupt mode is used
> > + * @saved_hdr: A copy of the original msg_hdr
> >   */
> >  struct scmi_msg_hdr {
> >  	u8 id;
> > @@ -88,6 +89,7 @@ struct scmi_msg_hdr {
> >  	u16 seq;
> >  	u32 status;
> >  	bool poll_completion;
> > +	u32 saved_hdr;
> >  };
> >  /**
> > @@ -154,6 +156,9 @@ struct scmi_msg {
> >   * @rx: Receive message, the buffer should be pre-allocated to store
> >   *	message. If request-ACK protocol is used, we can reuse the same
> >   *	buffer for the rx path as we use for the tx path.
> > + * @rx_raw_len: A field which can be optionally used by a specific transport
> > + *		to save transport specific message length
> > + *		It is not used by the SCMI transport core
> >   * @done: command message transmit completion event
> >   * @async_done: pointer to delayed response message received event completion
> >   * @users: A refcount to track the active users for this xfer
> > @@ -165,6 +170,7 @@ struct scmi_xfer {
> >  	struct scmi_msg_hdr hdr;
> >  	struct scmi_msg tx;
> >  	struct scmi_msg rx;
> > +	size_t rx_raw_len;
> >  	struct completion done;
> >  	struct completion *async_done;
> >  	refcount_t users;
> > @@ -355,6 +361,9 @@ struct scmi_device *scmi_child_dev_find(struct device *parent,
> >   * @max_msg: Maximum number of messages that can be pending
> >   *	simultaneously in the system
> >   * @max_msg_size: Maximum size of data per message that can be handled.
> > + * @support_xfers_delegation: A flag to indicate if the described transport
> > + *			      will handle delegated xfers, so the core can
> > + *			      derive proper related assumptions.
> >   */
> >  struct scmi_desc {
> >  	int (*init)(void);
> > @@ -363,6 +372,7 @@ struct scmi_desc {
> >  	int max_rx_timeout_ms;
> >  	int max_msg;
> >  	int max_msg_size;
> > +	bool support_xfers_delegation;
> >  };
> >  extern const struct scmi_desc scmi_mailbox_desc;
> > @@ -391,4 +401,8 @@ void scmi_notification_instance_data_set(const struct scmi_handle *handle,
> >  					 void *priv);
> >  void *scmi_notification_instance_data_get(const struct scmi_handle *handle);
> > +int scmi_transfer_acquire(struct scmi_chan_info *cinfo, u32 *msg_hdr,
> > +			  struct scmi_xfer **xfer);
> > +void scmi_transfer_release(struct scmi_chan_info *cinfo,
> > +			   struct scmi_xfer *xfer);
> >  #endif /* _SCMI_COMMON_H */
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index f0b20ddb24f4..371d3804cd79 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -432,6 +432,124 @@ scmi_xfer_lookup_unlocked(struct scmi_xfers_info *minfo, u16 xfer_id)
> >  	return xfer ?: ERR_PTR(-EINVAL);
> >  }
> > +/**
> > + * scmi_xfer_acquire  -  Helper to lookup and acquire an xfer
> > + *
> > + * @minfo: Pointer to Tx/Rx Message management info based on channel type
> > + * @xfer_id: Token ID to lookup in @pending_xfers
> > + *
> > + * When a valid xfer is found for the provided @xfer_id, reference counting is
> > + * properly updated.
> > + *
> > + * Return: A valid @xfer on Success or error otherwise.
> > + */
> > +static struct scmi_xfer *
> > +scmi_xfer_acquire(struct scmi_xfers_info *minfo, u16 xfer_id)
> > +{
> > +	unsigned long flags;
> > +	struct scmi_xfer *xfer;
> > +
> > +	spin_lock_irqsave(&minfo->xfer_lock, flags);
> > +	xfer = scmi_xfer_lookup_unlocked(minfo, xfer_id);
> > +	if (!IS_ERR(xfer))
> > +		refcount_inc(&xfer->users);
> > +	spin_unlock_irqrestore(&minfo->xfer_lock, flags);
> > +
> > +	return xfer;
> > +}
> > +
> > +/**
> > + * scmi_transfer_acquire  -  Lookup for an existing xfer or freshly allocate a
> > + * new one depending on the type of the message
> > + *
> > + * @cinfo: A reference to the channel descriptor.
> > + * @msg_hdr: A pointer to the message header to lookup.
> > + * @xfer: A reference to the pre-existent or freshly allocated xfer
> > + *	  associated with the provided *msg_hdr.
> > + *
> > + * This function can be used by transports supporting delegated xfers to obtain
> > + * a valid @xfer associated with the provided @msg_hdr param.
> > + *
> > + * The nature of the resulting @xfer depends on the type of message specified in
> > + * @msg_hdr:
> > + *  - for responses and delayed responses a pre-existent/pre-allocated in-flight
> > + *    xfer descriptor will be returned (properly refcounted)
> > + *  - for notifications a brand new xfer will be allocated; in this case the
> > + *    provided message header sequence number will also be mangled to match
> > + *    the token in the freshly allocated xfer: this is needed to establish a
> > + *    link between the picked xfer and the msg_hdr that will be subsequently
> > + *    passed back via usual scmi_rx_callback().
> > + *
> > + * Return: 0 if a valid xfer is returned in @xfer, error otherwise.
> > + */
> > +int scmi_transfer_acquire(struct scmi_chan_info *cinfo, u32 *msg_hdr,
> > +			  struct scmi_xfer **xfer)
> > +{
> > +	u8 msg_type;
> > +	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
> > +
> > +	if (!xfer || !msg_hdr || !info->desc->support_xfers_delegation)
> > +		return -EINVAL;
> > +
> > +	msg_type = MSG_XTRACT_TYPE(*msg_hdr);
> > +	switch (msg_type) {
> > +	case MSG_TYPE_COMMAND:
> > +	case MSG_TYPE_DELAYED_RESP:
> > +		/* Grab an existing xfer for xfer_id */
> > +		*xfer = scmi_xfer_acquire(&info->tx_minfo,
> > +					  MSG_XTRACT_TOKEN(*msg_hdr));
> > +		break;
> > +	case MSG_TYPE_NOTIFICATION:
> > +		/* Get a brand new RX xfer */
> > +		*xfer = scmi_xfer_get(cinfo->handle, &info->rx_minfo);
> > +		if (!IS_ERR(*xfer)) {
> > +			/* Save original msg_hdr and fix sequence number */
> > +			(*xfer)->hdr.saved_hdr = *msg_hdr;
> 
> The saved header isn't used anywhere.
> 

Yes, in fact it's just stored just in case in the future for some reason
the spec would change somehow to require that the original msg sequence
number of a notification has to be used.

> > +			*msg_hdr &= ~MSG_TOKEN_ID_MASK;
> > +			*msg_hdr |= FIELD_PREP(MSG_TOKEN_ID_MASK,
> > +					       (*xfer)->hdr.seq);
> 
> This will invalidate the token set by the platform in
> scmi_dump_header_dbg(). Maybe it would have been more elegant to introduce a
> dedicated hash table key field?
> 

Not sure to have understood this thing about invalidation.

Anyway, I removed scmi_dump_header_dbg( ) in V5 since we have traces since a
while and those have anyway a dedicated xfer->transfer_id.

> > +		}
> > +		break;
> > +	default:
> > +		*xfer = ERR_PTR(-EINVAL);
> > +		break;
> > +	}
> > +
> > +	if (IS_ERR(*xfer)) {
> > +		dev_err(cinfo->dev,
> > +			"Failed to acquire a valid xfer for hdr:0x%X\n",
> > +			*msg_hdr);
> > +		return PTR_ERR(*xfer);
> > +	}
> > +
> > +	/* Fix xfer->hdr.type with actual msg_hdr carried type */
> > +	unpack_scmi_header(*msg_hdr, &((*xfer)->hdr));
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * scmi_transfer_release  - Release an previously acquired xfer
> > + *
> > + * @cinfo: A reference to the channel descriptor.
> > + * @xfer: A reference to the xfer to release.
> > + */
> > +void scmi_transfer_release(struct scmi_chan_info *cinfo, struct scmi_xfer *xfer)
> > +{
> > +	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
> > +	struct scmi_xfers_info *minfo;
> > +
> > +	if (!xfer || !info->desc->support_xfers_delegation)
> > +		return;
> > +
> > +	if (xfer->hdr.type == MSG_TYPE_NOTIFICATION)
> > +		minfo = &info->rx_minfo;
> > +	else
> > +		minfo = &info->tx_minfo;
> > +
> > +	__scmi_xfer_put(minfo, xfer);
> > +}
> > +
> >  static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr)
> >  {
> >  	struct scmi_xfer *xfer;
> > @@ -441,7 +559,11 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr)
> >  	ktime_t ts;
> >  	ts = ktime_get_boottime();
> > -	xfer = scmi_xfer_get(cinfo->handle, minfo);
> > +
> > +	if (!info->desc->support_xfers_delegation)
> > +		xfer = scmi_xfer_get(cinfo->handle, minfo);
> > +	else
> > +		xfer = scmi_xfer_acquire(minfo, MSG_XTRACT_TOKEN(msg_hdr));
> >  	if (IS_ERR(xfer)) {
> >  		dev_err(dev, "failed to get free message slot (%ld)\n",
> >  			PTR_ERR(xfer));
> > @@ -449,8 +571,11 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr)
> >  		return;
> >  	}
> > -	unpack_scmi_header(msg_hdr, &xfer->hdr);
> >  	scmi_dump_header_dbg(dev, &xfer->hdr);
> > +
> > +	if (!info->desc->support_xfers_delegation)
> > +		unpack_scmi_header(msg_hdr, &xfer->hdr);
> > +
> 
> Why dump the header before unpacking?
> 

Right, but as I said above I drop scmi_dump_header_dbg() in V5.

Thanks,
Cristian


  reply	other threads:[~2021-07-01 10:24 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-11 16:59 [PATCH v4 00/16] Introduce SCMI VirtIO transport Cristian Marussi
2021-06-11 16:59 ` [PATCH v4 01/16] firmware: arm_scmi: Fix max pending messages boundary check Cristian Marussi
2021-07-01  8:42   ` Peter Hilber
2021-07-01 10:04     ` Cristian Marussi
2021-06-11 16:59 ` [PATCH v4 02/16] firmware: arm_scmi: Add support for type handling in common functions Cristian Marussi
2021-06-11 16:59 ` [PATCH v4 03/16] firmware: arm_scmi: Add transport optional init/exit support Cristian Marussi
2021-06-14 13:29   ` Jonathan Cameron
2021-06-16  9:04     ` Cristian Marussi
2021-06-11 16:59 ` [PATCH v4 04/16] firmware: arm_scmi: Introduce monotonically increasing tokens Cristian Marussi
2021-06-14 13:53   ` Jonathan Cameron
2021-06-16  9:11     ` Cristian Marussi
2021-07-01  8:42   ` Peter Hilber
2021-07-01 10:16     ` Cristian Marussi
2021-06-11 16:59 ` [PATCH v4 05/16] firmware: arm_scmi: Introduce delegated xfers support Cristian Marussi
2021-07-01  8:42   ` Peter Hilber
2021-07-01 10:24     ` Cristian Marussi [this message]
2021-06-11 16:59 ` [PATCH v4 06/16] firmware: arm_scmi, smccc, mailbox: Make shmem based transports optional Cristian Marussi
2021-07-01  8:42   ` Peter Hilber
2021-07-01 10:27     ` Cristian Marussi
2021-06-11 16:59 ` [PATCH v4 07/16] firmware: arm_scmi: Add op to override max message # Cristian Marussi
2021-06-14 14:04   ` Jonathan Cameron
2021-06-16  9:13     ` Cristian Marussi
2021-06-11 16:59 ` [PATCH v4 08/16] [RFC][REWORK] " Cristian Marussi
2021-06-11 16:59 ` [PATCH v4 09/16] firmware: arm_scmi: Add optional link_supplier() transport op Cristian Marussi
2021-06-11 16:59 ` [PATCH v4 10/16] firmware: arm_scmi: Add per-device transport private info Cristian Marussi
2021-06-11 16:59 ` [PATCH v4 11/16] firmware: arm_scmi: Add is_scmi_protocol_device() Cristian Marussi
2021-06-11 16:59 ` [PATCH v4 12/16] firmware: arm_scmi: Add message passing abstractions for transports Cristian Marussi
2021-06-14 14:10   ` Jonathan Cameron
2021-06-16  9:14     ` Cristian Marussi
2021-06-11 16:59 ` [PATCH v4 13/16] dt-bindings: arm: Add virtio transport for SCMI Cristian Marussi
2021-06-24 19:22   ` Rob Herring
2021-07-01  8:43   ` Peter Hilber
2021-07-01 10:31     ` Cristian Marussi
2021-06-11 16:59 ` [PATCH v4 14/16] firmware: arm_scmi: Add virtio transport Cristian Marussi
2021-06-14 14:23   ` Jonathan Cameron
2021-06-16 10:18     ` Cristian Marussi
2021-07-01  8:43   ` Peter Hilber
2021-07-01 10:34     ` Cristian Marussi
2021-06-11 16:59 ` [PATCH v4 15/16] [RFC][REWORK] firmware: arm_scmi: make virtio-scmi use delegated xfers Cristian Marussi
2021-07-01  8:43   ` Peter Hilber
2021-07-01 11:26     ` Cristian Marussi
2021-06-11 16:59 ` [PATCH v4 16/16] firmware: arm_scmi: Add polling mode to virtio transport Cristian Marussi
2021-06-14 11:43 ` [PATCH v4 00/16] Introduce SCMI VirtIO transport Christoph Hellwig
2021-06-14 14:03   ` Cristian Marussi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210701102435.GB17807@e120937-lin \
    --to=cristian.marussi@arm.com \
    --cc=Andriy.Tryshnivskyy@opensynergy.com \
    --cc=Jonathan.Cameron@Huawei.com \
    --cc=Vasyl.Vavrychuk@opensynergy.com \
    --cc=alex.bennee@linaro.org \
    --cc=anton.yakovlev@opensynergy.com \
    --cc=etienne.carriere@linaro.org \
    --cc=f.fainelli@gmail.com \
    --cc=igor.skalkin@opensynergy.com \
    --cc=james.quinlan@broadcom.com \
    --cc=jean-philippe@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mikhail.golubev@opensynergy.com \
    --cc=peter.hilber@opensynergy.com \
    --cc=souvik.chakravarty@arm.com \
    --cc=sudeep.holla@arm.com \
    --cc=vincent.guittot@linaro.org \
    --cc=virtio-dev@lists.oasis-open.org \
    --cc=virtualization@lists.linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).