Hi all,
While reworking this series starting from the work done up to V3 by
OpenSynergy, I am keeping the original autorship and list distribution
unchanged.
The main aim of this rework, as said, is to simplify where possible the
SCMI VirtIO support added in V3 by adding at first some new general
mechanisms in the SCMI Transport layer.
Indeed, after some initial small fixes, patches 03/04/05 add such new
additional mechanisms to the SCMI core to ease implementation of more
complex transports like virtio, while also addressing a few general issues
already potentially affecting existing transports.
In terms of rework I dropped original V3 patches 05/06/07/08/12 as no more
needed, and modified where needed the remaining original patches to take
advantage of the above mentioned new SCMI transport features.
DT bindings patch has been ported on top of freshly YAML converted arm,scmi
bindings.
Moreover, since V5 I dropped support for polling mode from the virtio-scmi
transport, since it is an optional general mechanism provided by the core
to allow transports lacking a completion IRQ to work and it seemed a
needless addition/complication in the context of virtio transport.
Additionally, since V5 I simplified a bit the virtio-scmi transport probing
sequence observing that, as of now, only one single SCMI VirtIO device can
be possibly used, since the SCMI VirtIO devices are not identifiable from
the VirtIO layer and neither they are currently identifiable from the DT
config; as a consequence only one single SCMI transport channel is
currently supported when using virtio-scmi transport.
The series has been tested using an emulated fake SCMI device and also a
proper SCP-fw SCMI stack running through QEMU vhost-users, with the SCMI
stack compiled, in both cases, as builtin and as a loadable module, running
tests against mocked SCMI Sensors using HWMON and IIO interfaces to check
the functionality of notifications and sync/async commands.
Virtio-scmi support has been exercised in the following testing scenario
on a JUNO board:
- normal sync/async command transfers
- notifications
- concurrent delivery of correlated response and delayed responses
- out-of-order delivery of delayed responses before related responses
- unexpected delayed response delivery for sync commands
- late delivery of timed-out responses and delayed responses
Some basic regression testing against mailbox transport has been performed
for commands and notifications too.
No sensible overhead in total handling time of commands and notifications
has been observed, even though this series do indeed add a considerable
amount of code to execute on TX path.
This series is based on sudeep/for-next/scmi [1] on top of
commit bdb8742dc6f7 ("firmware: arm_scmi: Fix range check for the maximum
number of pending messages")
Any feedback/testing is welcome :D
Thanks,
Cristian
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/log/?h=for-next/scmi
---
v6 --> v7
- rebased on sudeep/for-next/scmi (v5.14-rc1)
- added Cc: for maintainers regarding Virtio SCMI device number addition
in include/uapi/linux/virtio_ids.h (previously only list was Cc'ed)
- V6 patches 01 and 02 have been removed from V7 since already queued on
sudeep/for-next/scmi
- renamed scmi_desc .init/.exit to .transport_init/.exit
- moved "firmware: arm_scmi: Add priv parameter to scmi_rx_callback" later
in the series
- simplified new SCMI Kconfig layout
- refactored/simplified support for non-polling transports
- moved introduction of xfer refcounting from "firmware: arm_scmi:
Introduce monotonically increasing tokens" to "firmware: arm_scmi:
Handle concurrent and out-of-order messages"
- renamed scmi_xfer_is_free to scmi_xfer_acquired
- moved scmi_xfer_state_update() call inside scmi_xfer_command_acquire
- added missing barrier in do_xfer()
- added proper comment to justify pending_xfers sizing and relocated such
#define directive
- removed last_token atomic counter, now generating monotonic seqnums
based on transfer_id
V5 --> V6:
- removed delegated xfers and its usage
- add and use *priv optional parameter in scmi_rx_callback()
- made .poll_done and .clear_channel ops optional
V4 --> V5:
- removed msg raw_payload helpers
- reworked msg helpers to use xfer->priv reference
- simplified SCMI device probe sequence (one static device)
- added new SCMI Kconfig layout
- removed SCMI virtio polling support
V3 --> V4:
- using new delegated xfers support and monotonically increasing tokens
in virtio transport
- ported SCMI virtio transport DT bindings to YAML format
- added virtio-scmi polling support
- added delegated xfers support
Cristian Marussi (9):
firmware: arm_scmi: Add support for type handling in common functions
firmware: arm_scmi: Remove scmi_dump_header_dbg() helper
firmware: arm_scmi: Add optional transport_init/exit support
firmware: arm_scmi: Introduce monotonically increasing tokens
firmware: arm_scmi: Handle concurrent and out-of-order messages
firmware: arm_scmi: Make .clear_channel optional
firmware: arm_scmi: Make polling mode optional
firmware: arm_scmi: Make SCMI transports configurable
firmware: arm_scmi: Add priv parameter to scmi_rx_callback
Igor Skalkin (4):
firmware: arm_scmi: Make shmem support optional for transports
firmware: arm_scmi: Add method to override max message number
dt-bindings: arm: Add virtio transport for SCMI
firmware: arm_scmi: Add virtio transport
Peter Hilber (2):
firmware: arm_scmi: Add message passing abstractions for transports
firmware: arm_scmi: Add optional link_supplier() transport op
.../bindings/firmware/arm,scmi.yaml | 8 +-
MAINTAINERS | 1 +
drivers/firmware/Kconfig | 34 +-
drivers/firmware/arm_scmi/Kconfig | 95 +++
drivers/firmware/arm_scmi/Makefile | 8 +-
drivers/firmware/arm_scmi/common.h | 113 ++-
drivers/firmware/arm_scmi/driver.c | 649 +++++++++++++++---
drivers/firmware/arm_scmi/mailbox.c | 2 +-
drivers/firmware/arm_scmi/msg.c | 111 +++
drivers/firmware/arm_scmi/smc.c | 3 +-
drivers/firmware/arm_scmi/virtio.c | 491 +++++++++++++
include/uapi/linux/virtio_ids.h | 1 +
include/uapi/linux/virtio_scmi.h | 24 +
13 files changed, 1403 insertions(+), 137 deletions(-)
create mode 100644 drivers/firmware/arm_scmi/Kconfig
create mode 100644 drivers/firmware/arm_scmi/msg.c
create mode 100644 drivers/firmware/arm_scmi/virtio.c
create mode 100644 include/uapi/linux/virtio_scmi.h
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Add SCMI type handling to pack/unpack_scmi_header common helper functions.
Initialize hdr.type properly when initializing a command xfer.
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
Needed later in the series to support serialization
---
drivers/firmware/arm_scmi/common.h | 6 +++++-
drivers/firmware/arm_scmi/driver.c | 1 +
2 files changed, 6 insertions(+), 1 deletion(-)
Being a while that we have SCMI trace events in the SCMI stack, remove
this debug helper and its call sites.
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
drivers/firmware/arm_scmi/driver.c | 16 ----------------
1 file changed, 16 deletions(-)
Some SCMI transport could need to perform some transport specific setup
before they can be used by the SCMI core transport layer: typically this
early setup consists in registering with some other kernel subsystem.
Add the optional capability for a transport to provide a couple of init
and exit functions that are assured to be called early during the SCMI
core initialization phase, well before the SCMI core probing step.
[ Peter: Adapted RFC patch by Cristian for submission to upstream. ]
Signed-off-by: Peter Hilber <redacted>
[ Cristian: Fixed scmi_transports_exit point of invocation ]
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v6 --> v7
- renamed to transport_init/exit
v4 --> V5
- removed useless pr_debug
- moved scmi_transport_exit() invocation
---
drivers/firmware/arm_scmi/common.h | 8 +++++
drivers/firmware/arm_scmi/driver.c | 57 ++++++++++++++++++++++++++++++
2 files changed, 65 insertions(+)
@@ -1581,10 +1581,65 @@ static struct platform_driver scmi_driver = {.remove=scmi_remove,};+/**+*__scmi_transports_setup-Commonhelpertocalltransport-specific+*.init/.exitcodeifprovided.+*+*@init:Aflagtodistinguishbetweeninitandexit.+*+*Notethat,ifprovided,weinvoke.init/.exitfunctionsforallthe+*transportscurrentlycompiledin.+*+*Return:0onSuccess.+*/+staticinlineint__scmi_transports_setup(boolinit)+{+intret=0;+conststructof_device_id*trans;++for(trans=scmi_of_match;trans->data;trans++){+conststructscmi_desc*tdesc=trans->data;++if((init&&!tdesc->transport_init)||+(!init&&!tdesc->transport_exit))+continue;++if(init)+ret=tdesc->transport_init();+else+tdesc->transport_exit();++if(ret){+pr_err("SCMI transport %s FAILED initialization!\n",+trans->compatible);+break;+}+}++returnret;+}++staticint__initscmi_transports_init(void)+{+return__scmi_transports_setup(true);+}++staticvoid__exitscmi_transports_exit(void)+{+__scmi_transports_setup(false);+}+staticint__initscmi_driver_init(void){+intret;+scmi_bus_init();+/* Initialize any compiled-in transport which provided an init/exit */+ret=scmi_transports_init();+if(ret)+returnret;+scmi_base_register();scmi_clock_register();
Tokens are sequence numbers embedded in the each SCMI message header: they
are used to correlate commands with responses (and delayed responses), but
their usage and policy of selection is entirely up to the caller (usually
the OSPM agent), while they are completely opaque to the callee (i.e. SCMI
platform) which merely copies them back from the command into the response
message header.
This also means that the platform does not, can not and should not enforce
any kind of policy on received messages depending on the contained sequence
number: platform can perfectly handle concurrent requests carrying the same
identifiying token if that should happen.
Moreover the platform is not required to produce in-order responses to
agent requests, the only constraint in these regards is that in case of
an asynchronous message the delayed response must be sent after the
immediate response for the synchronous part of the command transaction.
Currenly the SCMI stack of the OSPM agent selects a token for the egressing
commands picking the lowest possible number which is not already in use by
an existing in-flight transaction, which means, in other words, that we
immediately reuse any token after its transaction has completed or it has
timed out: this policy indeed does simplify management and lookup of tokens
and associated xfers.
Under the above assumptions and constraints, since there is really no state
shared between the agent and the platform to let the platform know when a
token and its associated message has timed out, the current policy of early
reuse of tokens can easily lead to the situation in which a spurious or
late received response (or delayed_response), related to an old stale and
timed out transaction, can be wrongly associated to a newer valid in-flight
xfer that just happens to have reused the same token.
This misbehaviour on such late/spurious responses is more easily exposed on
those transports that naturally have an higher level of parallelism in
processing multiple concurrent in-flight messages.
This commit introduces a new policy of selection of tokens for the OSPM
agent: each new command transfer now gets the next available, monotonically
increasing token, until tokens are exhausted and the counter rolls over.
Such new policy mitigates the above issues with late/spurious responses
since the tokens are now reused as late as possible (when they roll back
ideally) and so it is much easier to identify such late/spurious responses
to stale timed out transactions: this also helps in simplifying the
specific transports implementation since stale transport messages can be
easily identified and discarded early on in the rx path without the need
to cross check their actual state with the core transport layer.
This mitigation is even more effective when, as is usually the case, the
maximum number of pending messages is capped by the platform to a much
lower number than the whole possible range of tokens values (2^10).
This internal policy change in the core SCMI transport layer is fully
transparent to the specific transports so it has not and should not have
any impact on the transports implementation.
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v6 --> v7
- avoid 'ghost' naming in commit message
- removed xfer->users refcounting (it will be added later in the series
where it is a better fit
- removed redundant check in scmi_xfer_lookup_unlocked
- added proper comment to justify pending_xfers sizing and moved
such define away into common.h
- removed last_token atomic counters, now generating monotonic seqnums
based on transfer_id
v4 --> V5
- removed empirical profiling info from commit msg
- do NOT use monotonic tokens and pending HT for notifications (not needed)
- release xfer_lock later in scmi_xfer_get
---
drivers/firmware/arm_scmi/common.h | 32 ++++
drivers/firmware/arm_scmi/driver.c | 254 +++++++++++++++++++++++++----
2 files changed, 251 insertions(+), 35 deletions(-)
@@ -191,46 +197,185 @@ void *scmi_notification_instance_data_get(const struct scmi_handle *handle)returninfo->notify_priv;}+/**+*scmi_xfer_token_set-Reserveandsetnewtokenforthexferathand+*+*@minfo:PointertoTx/RxMessagemanagementinfobasedonchanneltype+*@xfer:Thexfertoactupon+*+*Pickthenextunusedmonotonicallyincreasingtokenandsetitinto+*xfer->hdr.seq:pickingamonotonicallyincreasingvalueavoidsimmediate+*reuseoffreshlycompletedortimed-outxfers,thusmitigatingtherisk+*ofincorrectassociationofalateandexpiredxferwithalivein-flight+*transaction,bothhappeningtore-usethesametokenidentifier.+*+*SinceplatformisNOTrequiredtoanswerourrequestin-orderweshould+*accountforafewrarebutpossiblescenarios:+*+*-exactly'next_token'maybeNOTavailablesopickxfer_id>=next_token+*usingfind_next_zero_bit()startingfromcandidatenext_tokenbit+*+*-alltokensaheadupto(MSG_TOKEN_ID_MASK-1)areusedin-flightbutwe+*areplentyoffreetokensatstart,sotryasecondpassusing+*find_next_zero_bit()andstartingfrom0.+*+*X=usedin-flight+*+*Normal+*------+*+*|-xfer_idpicked+*-----------+----------------------------------------------------------+*|||X|X|X||||||.................................|X|X|+*----------------------------------------------------------------------+*^+*|-next_token+*+*Out-of-orderpendingatstart+*-----------------------------+*+*|-xfer_idpicked,last_tokenfixed+*-----+----------------------------------------------------------------+*|X|X|||||X|X|....................................|X||+*----------------------------------------------------------------------+*^+*|-next_token+*+*+*Out-of-orderpendingatend+*---------------------------+*+*|-xfer_idpicked,last_tokenfixed+*-----+----------------------------------------------------------------+*|X|X|||||X|X|..............................|X|X|X||X|X|+*----------------------------------------------------------------------+*^+*|-next_token+*+*Context:Assumestobecalledwith@xfer_lockalreadyacquired.+*+*Return:0onSuccessorerror+*/+staticintscmi_xfer_token_set(structscmi_xfers_info*minfo,+structscmi_xfer*xfer)+{+unsignedlongxfer_id,next_token;++/*+*Pickacandidatemonotonictokeninrange[0,MSG_TOKEN_MAX-1]+*usingthepre-allocatedtransfer_idasabase.+*Notethattheglobaltransfer_idissharedacrossallmessagetypes+*sotherecouldbeholesintheallocatedsetofmonotonicsequence+*numbers,butthatisgoingtolimittheeffectivenessofthe+*mitigationonlyinveryrarelimitconditions.+*/+next_token=(xfer->transfer_id&(MSG_TOKEN_MAX-1));++/* Pick the next available xfer_id >= next_token */+xfer_id=find_next_zero_bit(minfo->xfer_alloc_table,+MSG_TOKEN_MAX,next_token);+if(xfer_id==MSG_TOKEN_MAX){+/*+*Afterheavilyout-of-orderresponses,therearenofree+*tokensahead,butonlyatstartofxfer_alloc_tableso+*tryagainfromthebeginning.+*/+xfer_id=find_next_zero_bit(minfo->xfer_alloc_table,+MSG_TOKEN_MAX,0);+/*+*Somethingiswrongifwegotheresincetherecanbea+*maximumnumberof(MSG_TOKEN_MAX-1)in-flightmessages+*butwehavenotfoundanyfreetoken[0,MSG_TOKEN_MAX-1].+*/+if(WARN_ON_ONCE(xfer_id==MSG_TOKEN_MAX))+return-ENOMEM;+}++/* Update +/- last_token accordingly if we skipped some hole */+if(xfer_id!=next_token)+atomic_add((int)(xfer_id-next_token),&transfer_last_id);++/* Set in-flight */+set_bit(xfer_id,minfo->xfer_alloc_table);+xfer->hdr.seq=(u16)xfer_id;++return0;+}++/**+*scmi_xfer_token_clear-Releasethetoken+*+*@minfo:PointertoTx/RxMessagemanagementinfobasedonchanneltype+*@xfer:Thexfertoactupon+*/+staticinlinevoidscmi_xfer_token_clear(structscmi_xfers_info*minfo,+structscmi_xfer*xfer)+{+clear_bit(xfer->hdr.seq,minfo->xfer_alloc_table);+}+/***scmi_xfer_get()-Allocateonemessage**@handle:PointertoSCMIentityhandle*@minfo:PointertoTx/RxMessagemanagementinfobasedonchanneltype+*@set_pending:Iftrueamonotonictokenispickedandthexferisaddedto+*thependinghashtable.**Helperfunctionwhichisusedbyvariousmessagefunctionsthatare*exposedtoclientsofthisdriverforallocatingamessagetrafficevent.*-*Thisfunctioncansleepdependingonpendingrequestsalreadyinthesystem-*fortheSCMIentity.Further,thisalsoholdsaspinlocktomaintain-*integrityofinternaldatastructures.+*Picksanxferfromthefreelist@free_xfers(ifanyavailable)and,if+*required,setsamonotonicallyincreasingtokenandstorestheinflightxfer+*intothe@pending_xfershashtableforlaterretrieval.+*+*Thesuccessfullyinitializedxferisrefcounted.+*+*Context:Holds@xfer_lockwhilemanipulating@xfer_alloc_tableand+*@free_xfers.**Return:0ifallwentfine,elsecorrespondingerror.*/staticstructscmi_xfer*scmi_xfer_get(conststructscmi_handle*handle,-structscmi_xfers_info*minfo)+structscmi_xfers_info*minfo,+boolset_pending){-u16xfer_id;+intret;+unsignedlongflags;structscmi_xfer*xfer;-unsignedlongflags,bit_pos;-structscmi_info*info=handle_to_scmi_info(handle);-/* Keep the locked section as small as possible */spin_lock_irqsave(&minfo->xfer_lock,flags);-bit_pos=find_first_zero_bit(minfo->xfer_alloc_table,-info->desc->max_msg);-if(bit_pos==info->desc->max_msg){+if(hlist_empty(&minfo->free_xfers)){spin_unlock_irqrestore(&minfo->xfer_lock,flags);returnERR_PTR(-ENOMEM);}-set_bit(bit_pos,minfo->xfer_alloc_table);-spin_unlock_irqrestore(&minfo->xfer_lock,flags);-xfer_id=bit_pos;+/* grab an xfer from the free_list */+xfer=hlist_entry(minfo->free_xfers.first,structscmi_xfer,node);+hlist_del_init(&xfer->node);-xfer=&minfo->xfer_block[xfer_id];-xfer->hdr.seq=xfer_id;+/*+*Allocatetransfer_idearlysothatcanbeusedalsoasbasefor+*monotonicsequencenumbergenerationifneeded.+*/xfer->transfer_id=atomic_inc_return(&transfer_last_id);+if(set_pending){+/* Pick and set monotonic token */+ret=scmi_xfer_token_set(minfo,xfer);+if(!ret){+hash_add(minfo->pending_xfers,&xfer->node,+xfer->hdr.seq);+xfer->pending=true;+}else{+dev_err(handle->dev,+"Failed to get monotonic token %d\n",ret);+hlist_add_head(&xfer->node,&minfo->free_xfers);+xfer=ERR_PTR(ret);+}+}+spin_unlock_irqrestore(&minfo->xfer_lock,flags);+returnxfer;}
@@ -266,7 +437,7 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr)ktime_tts;ts=ktime_get_boottime();-xfer=scmi_xfer_get(cinfo->handle,minfo);+xfer=scmi_xfer_get(cinfo->handle,minfo,false);if(IS_ERR(xfer)){dev_err(dev,"failed to get free message slot (%ld)\n",PTR_ERR(xfer));
@@ -292,19 +463,22 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr)staticvoidscmi_handle_response(structscmi_chan_info*cinfo,u16xfer_id,u8msg_type){+unsignedlongflags;structscmi_xfer*xfer;structdevice*dev=cinfo->dev;structscmi_info*info=handle_to_scmi_info(cinfo->handle);structscmi_xfers_info*minfo=&info->tx_minfo;/* Are we even expecting this? */-if(!test_bit(xfer_id,minfo->xfer_alloc_table)){+spin_lock_irqsave(&minfo->xfer_lock,flags);+xfer=scmi_xfer_lookup_unlocked(minfo,xfer_id);+spin_unlock_irqrestore(&minfo->xfer_lock,flags);+if(IS_ERR(xfer)){dev_err(dev,"message for %d is not expected!\n",xfer_id);info->desc->ops->clear_channel(cinfo);return;}-xfer=&minfo->xfer_block[xfer_id];/**Evenifaresponsewasindeedexpectedonthisslotatthispoint,*abuggyplatformcouldwronglyreplyfeedingusanunexpected
@@ -541,7 +715,7 @@ static int xfer_get_init(const struct scmi_protocol_handle *ph,tx_size>info->desc->max_msg_size)return-ERANGE;-xfer=scmi_xfer_get(pi->handle,minfo);+xfer=scmi_xfer_get(pi->handle,minfo,true);if(IS_ERR(xfer)){ret=PTR_ERR(xfer);dev_err(dev,"failed to get free message slot(%d)\n",ret);
@@ -1018,18 +1192,25 @@ static int __scmi_xfer_info_init(struct scmi_info *sinfo,return-EINVAL;}-info->xfer_block=devm_kcalloc(dev,desc->max_msg,-sizeof(*info->xfer_block),GFP_KERNEL);-if(!info->xfer_block)-return-ENOMEM;+hash_init(info->pending_xfers);-info->xfer_alloc_table=devm_kcalloc(dev,BITS_TO_LONGS(desc->max_msg),+/* Allocate a bitmask sized to hold MSG_TOKEN_MAX tokens */+info->xfer_alloc_table=devm_kcalloc(dev,BITS_TO_LONGS(MSG_TOKEN_MAX),sizeof(long),GFP_KERNEL);if(!info->xfer_alloc_table)return-ENOMEM;-/* Pre-initialize the buffer pointer to pre-allocated buffers */-for(i=0,xfer=info->xfer_block;i<desc->max_msg;i++,xfer++){+/*+*Preallocateanumberofxfersequaltomaxinflightmessages,+*pre-initializethebufferpointertopre-allocatedbuffersand+*attachallofthemtothefreelist+*/+INIT_HLIST_HEAD(&info->free_xfers);+for(i=0;i<desc->max_msg;i++){+xfer=devm_kzalloc(dev,sizeof(*xfer),GFP_KERNEL);+if(!xfer)+return-ENOMEM;+xfer->rx.buf=devm_kcalloc(dev,sizeof(u8),desc->max_msg_size,GFP_KERNEL);if(!xfer->rx.buf)
@@ -1037,6 +1218,9 @@ static int __scmi_xfer_info_init(struct scmi_info *sinfo,xfer->tx.buf=xfer->rx.buf;init_completion(&xfer->done);++/* Add initialized xfer to the free list */+hlist_add_head(&xfer->node,&info->free_xfers);}spin_lock_init(&info->xfer_lock);
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Even though in case of asynchronous commands an SCMI platform is
constrained to emit the delayed response message only after the related
message response has been sent, the configured underlying transport could
still deliver such messages together or in inverted order, causing races
due to the concurrent or out-of-order access to the underlying xfer.
Introduce a mechanism to grant exclusive access to an xfer in order to
properly serialize concurrent accesses to the same xfer originating from
multiple correlated messages.
Add additional state information to xfer descriptors so as to be able to
identify out-of-order message deliveries and act accordingly:
- when a delayed response is expected but delivered before the related
response, the synchronous response is considered as successfully
received and the delayed response processing is carried on as usual.
- when/if the missing synchronous response is subsequently received, it
is discarded as not congruent with the current state of the xfer, or
simply, because the xfer has been already released and so, now, the
monotonically increasing sequence number carried by the late response
is stale.
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v6 --> v7
- re-added xfer refcounting moved out of previous 'monotonic' patch
- removed un-needed 2-way switch in scmi_xfer_state_update()
- renamed scmi_xfer_is_free to scmi_xfer_acquired
- moved scmi_xfer_state_update() call inside scmi_xfer_command_acquire
to avoid races between msg validation and xfer state update
- added smp_mb() after xfer->state init before .send_message
v5 --> v6
- added spinlock comment
---
drivers/firmware/arm_scmi/common.h | 30 +++-
drivers/firmware/arm_scmi/driver.c | 257 ++++++++++++++++++++++++-----
2 files changed, 246 insertions(+), 41 deletions(-)
@@ -428,6 +435,171 @@ scmi_xfer_lookup_unlocked(struct scmi_xfers_info *minfo, u16 xfer_id)returnxfer?:ERR_PTR(-EINVAL);}+/**+*scmi_msg_response_validate-Validatemessagetypeagainststateofrelated+*xfer+*+*@cinfo:Areferencetothechanneldescriptor.+*@msg_type:Messagetypetocheck+*@xfer:Areferencetothexfertovalidateagainst@msg_type+*+*Thisfunctionchecksif@msg_typeiscongruentwiththecurrentstateof+*apending@xfer;ifanasynchronousdelayedresponseisreceivedbeforethe+*relatedsynchronousresponse(Out-of-OrderDelayedResponse)themissing+*synchronousresponseisassumedtobeOKandcompleted,carryingonwiththe+*DelayedResponse:thisisdonetoaddressthecaseinwhichtheunderlying+*SCMItransportcandeliversuchout-of-orderresponses.+*+*Context:Assumestobecalledwithxfer->lockalreadyacquired.+*+*Return:0onSuccess,errorotherwise+*/+staticinlineintscmi_msg_response_validate(structscmi_chan_info*cinfo,+u8msg_type,+structscmi_xfer*xfer)+{+/*+*Evenifaresponsewasindeedexpectedonthisslotatthispoint,+*abuggyplatformcouldwronglyreplyfeedingusanunexpected+*delayedresponsewe'renotpreparedtohandle:bail-outsafely+*blamingfirmware.+*/+if(msg_type==MSG_TYPE_DELAYED_RESP&&!xfer->async_done){+dev_err(cinfo->dev,+"Delayed Response for %d not expected! Buggy F/W ?\n",+xfer->hdr.seq);+return-EINVAL;+}++switch(xfer->state){+caseSCMI_XFER_SENT_OK:+if(msg_type==MSG_TYPE_DELAYED_RESP){+/*+*DelayedResponseexpectedbutdeliveredearlier.+*AssumemessageRESPONSEwasOKandskipstate.+*/+xfer->hdr.status=SCMI_SUCCESS;+xfer->state=SCMI_XFER_RESP_OK;+complete(&xfer->done);+dev_warn(cinfo->dev,+"Received valid OoO Delayed Response for %d\n",+xfer->hdr.seq);+}+break;+caseSCMI_XFER_RESP_OK:+if(msg_type!=MSG_TYPE_DELAYED_RESP)+return-EINVAL;+break;+caseSCMI_XFER_DRESP_OK:+/* No further message expected once in SCMI_XFER_DRESP_OK */+return-EINVAL;+}++return0;+}++/**+*scmi_xfer_state_update-Updatexferstate+*+*@xfer:Areferencetothexfertoupdate+*@msg_type:Typeofmessagebeingprocessed.+*+*Notethatthismessageisassumedtohavebeenalreadysuccessfullyvalidated+*by@scmi_msg_response_validate(),soherewejustupdatethestate.+*+*Context:Assumestobecalledonanxferexclusivelyacquiredusingthe+*busyflag.+*/+staticinlinevoidscmi_xfer_state_update(structscmi_xfer*xfer,u8msg_type)+{+xfer->hdr.type=msg_type;++/* Unknown command types were already discarded earlier */+if(xfer->hdr.type==MSG_TYPE_COMMAND)+xfer->state=SCMI_XFER_RESP_OK;+else+xfer->state=SCMI_XFER_DRESP_OK;+}++staticboolscmi_xfer_acquired(structscmi_xfer*xfer)+{+intret;++ret=atomic_cmpxchg(&xfer->busy,SCMI_XFER_FREE,SCMI_XFER_BUSY);++returnret==SCMI_XFER_FREE;+}++/**+*scmi_xfer_command_acquire-Helpertolookupandacquireacommandxfer+*+*@cinfo:Areferencetothechanneldescriptor.+*@msg_hdr:Amessageheadertouseaslookupkey+*+*Whenavalidxferisfoundforthesequencenumberembeddedintheprovided+*msg_hdr,referencecountingisproperlyupdatedandexclusiveaccesstothis+*xferisgrantedtillreleasedwith@scmi_xfer_command_release.+*+*Return:Avalid@xferonSuccessorerrorotherwise.+*/+staticinlinestructscmi_xfer*+scmi_xfer_command_acquire(structscmi_chan_info*cinfo,u32msg_hdr)+{+intret;+unsignedlongflags;+structscmi_xfer*xfer;+structscmi_info*info=handle_to_scmi_info(cinfo->handle);+structscmi_xfers_info*minfo=&info->tx_minfo;+u8msg_type=MSG_XTRACT_TYPE(msg_hdr);+u16xfer_id=MSG_XTRACT_TOKEN(msg_hdr);++/* Are we even expecting this? */+spin_lock_irqsave(&minfo->xfer_lock,flags);+xfer=scmi_xfer_lookup_unlocked(minfo,xfer_id);+if(IS_ERR(xfer)){+dev_err(cinfo->dev,+"Message for %d type %d is not expected!\n",+xfer_id,msg_type);+spin_unlock_irqrestore(&minfo->xfer_lock,flags);+returnxfer;+}+refcount_inc(&xfer->users);+spin_unlock_irqrestore(&minfo->xfer_lock,flags);++spin_lock_irqsave(&xfer->lock,flags);+ret=scmi_msg_response_validate(cinfo,msg_type,xfer);+/*+*Ifapendingxferwasfoundwhichwasalsoinacongruentstatewith+*thereceivedmessage,acquireexclusiveaccesstoitsettingthebusy+*flag.+*Spinsonlyontherarelimitconditionofconcurrentreceptionof+*RESPandDRESPforthesamexfer.+*/+if(!ret){+spin_until_cond(scmi_xfer_acquired(xfer));+scmi_xfer_state_update(xfer,msg_type);+}+spin_unlock_irqrestore(&xfer->lock,flags);++if(ret){+dev_err(cinfo->dev,+"Invalid message type:%d for %d - HDR:0x%X state:%d\n",+msg_type,xfer_id,msg_hdr,xfer->state);+/* On error the refcount incremented above has to be dropped */+__scmi_xfer_put(minfo,xfer);+xfer=ERR_PTR(-EINVAL);+}++returnxfer;+}++staticinlinevoidscmi_xfer_command_release(structscmi_info*info,+structscmi_xfer*xfer)+{+atomic_set(&xfer->busy,SCMI_XFER_FREE);+__scmi_xfer_put(&info->tx_minfo,xfer);+}+staticvoidscmi_handle_notification(structscmi_chan_info*cinfo,u32msg_hdr){structscmi_xfer*xfer;
@@ -460,57 +632,35 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr)info->desc->ops->clear_channel(cinfo);}-staticvoidscmi_handle_response(structscmi_chan_info*cinfo,-u16xfer_id,u8msg_type)+staticvoidscmi_handle_response(structscmi_chan_info*cinfo,u32msg_hdr){-unsignedlongflags;structscmi_xfer*xfer;-structdevice*dev=cinfo->dev;structscmi_info*info=handle_to_scmi_info(cinfo->handle);-structscmi_xfers_info*minfo=&info->tx_minfo;-/* Are we even expecting this? */-spin_lock_irqsave(&minfo->xfer_lock,flags);-xfer=scmi_xfer_lookup_unlocked(minfo,xfer_id);-spin_unlock_irqrestore(&minfo->xfer_lock,flags);+xfer=scmi_xfer_command_acquire(cinfo,msg_hdr);if(IS_ERR(xfer)){-dev_err(dev,"message for %d is not expected!\n",xfer_id);info->desc->ops->clear_channel(cinfo);return;}-/*-*Evenifaresponsewasindeedexpectedonthisslotatthispoint,-*abuggyplatformcouldwronglyreplyfeedingusanunexpected-*delayedresponsewe'renotpreparedtohandle:bail-outsafely-*blamingfirmware.-*/-if(unlikely(msg_type==MSG_TYPE_DELAYED_RESP&&!xfer->async_done)){-dev_err(dev,-"Delayed Response for %d not expected! Buggy F/W ?\n",-xfer_id);-info->desc->ops->clear_channel(cinfo);-/* It was unexpected, so nobody will clear the xfer if not us */-__scmi_xfer_put(minfo,xfer);-return;-}-/* rx.len could be shrunk in the sync do_xfer, so reset to maxsz */-if(msg_type==MSG_TYPE_DELAYED_RESP)+if(xfer->hdr.type==MSG_TYPE_DELAYED_RESP)xfer->rx.len=info->desc->max_msg_size;info->desc->ops->fetch_response(cinfo,xfer);trace_scmi_rx_done(xfer->transfer_id,xfer->hdr.id,xfer->hdr.protocol_id,xfer->hdr.seq,-msg_type);+xfer->hdr.type);-if(msg_type==MSG_TYPE_DELAYED_RESP){+if(xfer->hdr.type==MSG_TYPE_DELAYED_RESP){info->desc->ops->clear_channel(cinfo);complete(xfer->async_done);}else{complete(&xfer->done);}++scmi_xfer_command_release(info,xfer);}/**
@@ -606,6 +760,16 @@ static int do_xfer(const struct scmi_protocol_handle *ph,xfer->hdr.protocol_id,xfer->hdr.seq,xfer->hdr.poll_completion);+xfer->state=SCMI_XFER_SENT_OK;+/*+*Eventhoughspinlockingisnotneededheresincenoraceispossible+*onxfer->stateduetothemonotonicallyincreasingtokensallocation,+*wemustanywayensurexfer->stateinitializationisnotre-ordered+*afterthe.send_message()tobesurethatontheRXpathanearly+*ISRcallingscmi_rx_callback()cannotseeanoldstalexfer->state.+*/+smp_mb();+ret=info->desc->ops->send_message(cinfo,xfer);if(ret<0){dev_dbg(dev,"Failed to send message %d\n",ret);
@@ -617,10 +781,22 @@ static int do_xfer(const struct scmi_protocol_handle *ph,spin_until_cond(scmi_xfer_done_no_timeout(cinfo,xfer,stop));-if(ktime_before(ktime_get(),stop))-info->desc->ops->fetch_response(cinfo,xfer);-else+if(ktime_before(ktime_get(),stop)){+unsignedlongflags;++/*+*Donotfetch_responseifanout-of-orderdelayed+*responseisbeingprocessed.+*/+spin_lock_irqsave(&xfer->lock,flags);+if(xfer->state==SCMI_XFER_SENT_OK){+info->desc->ops->fetch_response(cinfo,xfer);+xfer->state=SCMI_XFER_RESP_OK;+}+spin_unlock_irqrestore(&xfer->lock,flags);+}else{ret=-ETIMEDOUT;+}}else{/* And we wait for the response. */timeout=msecs_to_jiffies(info->desc->max_rx_timeout_ms);
@@ -1218,6 +1394,7 @@ static int __scmi_xfer_info_init(struct scmi_info *sinfo,xfer->tx.buf=xfer->rx.buf;init_completion(&xfer->done);+spin_lock_init(&xfer->lock);/* Add initialized xfer to the free list */hlist_add_head(&xfer->node,&info->free_xfers);
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Make transport operation .clear_channel optional since some transports
do not need it and so avoid to have them implement dummy callbacks.
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
drivers/firmware/arm_scmi/driver.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
Add configuration options to be able to select which SCMI transports have
to be compiled into the SCMI stack.
Mailbox and SMC are by default enabled if their related dependencies are
satisfied.
While doing that move all SCMI related config options in their own
dedicated submenu.
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
Used a BUILD_BUG_ON() to avoid the scenario where SCMI is configured
without any transport. Coul dnot do in any other way in Kconfig due to
circular dependencies.
This will be neeed later on to add new Virtio based transport and
optionally exclude other transports.
v6 --> v7
- using if ARM_SCMI_PROTOCOL to simplify depends
---
drivers/firmware/Kconfig | 34 +--------------
drivers/firmware/arm_scmi/Kconfig | 70 ++++++++++++++++++++++++++++++
drivers/firmware/arm_scmi/Makefile | 4 +-
drivers/firmware/arm_scmi/common.h | 4 +-
drivers/firmware/arm_scmi/driver.c | 6 ++-
5 files changed, 80 insertions(+), 38 deletions(-)
create mode 100644 drivers/firmware/arm_scmi/Kconfig
@@ -6,39 +6,7 @@menu"Firmware Drivers"-configARM_SCMI_PROTOCOL-tristate"ARM System Control and Management Interface (SCMI) Message Protocol"-depends onARM||ARM64||COMPILE_TEST-depends onMAILBOX||HAVE_ARM_SMCCC_DISCOVERY-help-ARMSystemControlandManagementInterface(SCMI)protocolisa-setofoperatingsystem-independentsoftwareinterfacesthatare-usedinsystemmanagement.SCMIisextensibleandcurrentlyprovides-interfacesfor:Discoveryandself-descriptionoftheinterfaces-itsupports,Powerdomainmanagementwhichistheabilitytoplace-agivendeviceordomainintothevariouspower-savingstatesthat-itsupports,Performancemanagementwhichistheabilitytocontrol-theperformanceofadomainthatiscomposedofcomputeengines-suchasapplicationprocessorsandotheraccelerators,Clock-managementwhichistheabilitytosetandinquireratesonplatform-managedclocksandSensormanagementwhichistheabilitytoread-sensordata,andbenotifiedofsensorvalue.--Thisprotocollibraryprovidesinterfaceforalltheclientdrivers-makinguseofthefeaturesofferedbytheSCMI.--configARM_SCMI_POWER_DOMAIN-tristate"SCMI power domain driver"-depends onARM_SCMI_PROTOCOL||(COMPILE_TEST&&OF)-defaulty-selectPM_GENERIC_DOMAINSifPM-help-ThisenablessupportfortheSCMIpowerdomainswhichcanbe-enabledordisabledviatheSCPfirmware--Thisdrivercanalsobebuiltasamodule.Ifso,themodule-willbecalledscmi_pm_domain.Notethismayneededearlyinboot-beforerootfsmaybeavailable.+source"drivers/firmware/arm_scmi/Kconfig"configARM_SCPI_PROTOCOLtristate"ARM System Control and Power Interface (SCPI) Message Protocol"
@@ -0,0 +1,70 @@+# SPDX-License-Identifier: GPL-2.0-only+menu"ARM System Control and Management Interface Protocol"++configARM_SCMI_PROTOCOL+tristate"ARM System Control and Management Interface (SCMI) Message Protocol"+depends onARM||ARM64||COMPILE_TEST+help+ARMSystemControlandManagementInterface(SCMI)protocolisa+setofoperatingsystem-independentsoftwareinterfacesthatare+usedinsystemmanagement.SCMIisextensibleandcurrentlyprovides+interfacesfor:Discoveryandself-descriptionoftheinterfaces+itsupports,Powerdomainmanagementwhichistheabilitytoplace+agivendeviceordomainintothevariouspower-savingstatesthat+itsupports,Performancemanagementwhichistheabilitytocontrol+theperformanceofadomainthatiscomposedofcomputeengines+suchasapplicationprocessorsandotheraccelerators,Clock+managementwhichistheabilitytosetandinquireratesonplatform+managedclocksandSensormanagementwhichistheabilitytoread+sensordata,andbenotifiedofsensorvalue.++Thisprotocollibraryprovidesinterfaceforalltheclientdrivers+makinguseofthefeaturesofferedbytheSCMI.++ifARM_SCMI_PROTOCOL++configARM_SCMI_HAVE_TRANSPORT+bool+help+ThisdeclareswhetheratleastoneSCMItransporthasbeenconfigured.+UsedtotriggerabuildbugwhentryingtobuildSCMIwithoutany+configuredtransport.++configARM_SCMI_TRANSPORT_MAILBOX+bool"SCMI transport based on Mailbox"+depends onMAILBOX+selectARM_SCMI_HAVE_TRANSPORT+defaulty+help+EnablemailboxbasedtransportforSCMI.++IfyouwanttheARMSCMIPROTOCOLstacktoincludesupportfora+transportbasedonmailboxes,answerY.++configARM_SCMI_TRANSPORT_SMC+bool"SCMI transport based on SMC"+depends onHAVE_ARM_SMCCC_DISCOVERY+selectARM_SCMI_HAVE_TRANSPORT+defaulty+help+EnableSMCbasedtransportforSCMI.++IfyouwanttheARMSCMIPROTOCOLstacktoincludesupportfora+transportbasedonSMC,answerY.++endif#ARM_SCMI_PROTOCOL++configARM_SCMI_POWER_DOMAIN+tristate"SCMI power domain driver"+depends onARM_SCMI_PROTOCOL||(COMPILE_TEST&&OF)+defaulty+selectPM_GENERIC_DOMAINSifPM+help+ThisenablessupportfortheSCMIpowerdomainswhichcanbe+enabledordisabledviatheSCPfirmware++Thisdrivercanalsobebuiltasamodule.Ifso,themodule+willbecalledscmi_pm_domain.Notethismayneededearlyinboot+beforerootfsmaybeavailable.++endmenu
@@ -1933,10 +1933,10 @@ ATTRIBUTE_GROUPS(versions);/* Each compatible listed below must have descriptor associated with it */staticconststructof_device_idscmi_of_match[]={-#ifdef CONFIG_MAILBOX+#ifdef CONFIG_ARM_SCMI_TRANSPORT_MAILBOX{.compatible="arm,scmi",.data=&scmi_mailbox_desc},#endif-#ifdef CONFIG_HAVE_ARM_SMCCC_DISCOVERY+#ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC{.compatible="arm,scmi-smc",.data=&scmi_smc_desc},#endif{/* Sentinel */},
@@ -2008,6 +2008,8 @@ static int __init scmi_driver_init(void)scmi_bus_init();+BUILD_BUG_ON(!IS_ENABLED(CONFIG_ARM_SCMI_HAVE_TRANSPORT));+/* Initialize any compiled-in transport which provided an init/exit */ret=scmi_transports_init();if(ret)
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Add a check for the presence of .poll_done transport operation so that
transports that do not need to support polling mode have no need to provide
a dummy .poll_done callback either and polling mode can be disabled in the
SCMI core for that tranport.
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v6 --> v7
- s/EINVAL/-EINVAL/
- refactored to bail out early when polling mode not supported
---
drivers/firmware/arm_scmi/driver.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
@@ -751,6 +751,12 @@ static int do_xfer(const struct scmi_protocol_handle *ph,structdevice*dev=info->dev;structscmi_chan_info*cinfo;+if(xfer->hdr.poll_completion&&!info->desc->ops->poll_done){+dev_warn_once(dev,+"Polling mode is not supported by transport.\n");+return-EINVAL;+}+/**Initialiseprotocolidnowfromprotocolhandletoavoiditbeing*overriddenbymistake(ormalice)bytheprotocolcodemanglingwith
@@ -787,7 +793,6 @@ static int do_xfer(const struct scmi_protocol_handle *ph,ktime_tstop=ktime_add_ns(ktime_get(),SCMI_MAX_POLL_TO_NS);spin_until_cond(scmi_xfer_done_no_timeout(cinfo,xfer,stop));-if(ktime_before(ktime_get(),stop)){unsignedlongflags;
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Igor Skalkin <redacted>
Upcoming new SCMI transports won't need any kind of shared memory support.
Compile shmem.c only if a shmem based transport is selected.
Signed-off-by: Igor Skalkin <redacted>
[ Peter: Adapted patch for submission to upstream. ]
Co-developed-by: Peter Hilber <redacted>
Signed-off-by: Peter Hilber <redacted>
[ Cristian: Adapted patch/commit_msg to new SCMI Kconfig layout ]
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v4 --> v5
- Adapted to new SCMI Kconfig layout
---
drivers/firmware/arm_scmi/Kconfig | 8 ++++++++
drivers/firmware/arm_scmi/Makefile | 2 +-
2 files changed, 9 insertions(+), 1 deletion(-)
@@ -30,10 +30,17 @@ config ARM_SCMI_HAVE_TRANSPORTUsedtotriggerabuildbugwhentryingtobuildSCMIwithoutanyconfiguredtransport.+configARM_SCMI_HAVE_SHMEM+bool+help+ThisdeclareswhetherasharedmemorybasedtransportforSCMIis+available.+configARM_SCMI_TRANSPORT_MAILBOXbool"SCMI transport based on Mailbox"depends onMAILBOXselectARM_SCMI_HAVE_TRANSPORT+selectARM_SCMI_HAVE_SHMEMdefaultyhelpEnablemailboxbasedtransportforSCMI.
@@ -45,6 +52,7 @@ config ARM_SCMI_TRANSPORT_SMCbool"SCMI transport based on SMC"depends onHAVE_ARM_SMCCC_DISCOVERYselectARM_SCMI_HAVE_TRANSPORT+selectARM_SCMI_HAVE_SHMEMdefaultyhelpEnableSMCbasedtransportforSCMI.
From: Igor Skalkin <redacted>
The maximum number of simultaneously pending messages is a transport
specific quantity that is usually described statically in struct scmi_desc.
Some transports, though, can calculate such number only at run-time after
some initial transport specific setup and probing is completed; moreover
the resulting max message numbers could also be different between rx and
tx channels.
Add an optional get_max_msg() operation so that a transport can report more
accurate max message numbers for each channel type.
The value in scmi_desc.max_msg is still used as default when transport does
not provide any get_max_msg() method.
Signed-off-by: Igor Skalkin <redacted>
[ Peter: Adapted patch for submission to upstream. ]
Co-developed-by: Peter Hilber <redacted>
Signed-off-by: Peter Hilber <redacted>
[ Cristian: refactored how get_max_msg() is used to minimize core changes ]
Co-developed-by: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v4 --> v5
- refactored usage of get_max_msg()
---
drivers/firmware/arm_scmi/common.h | 9 +++++--
drivers/firmware/arm_scmi/driver.c | 40 +++++++++++++++++++++++++++---
2 files changed, 43 insertions(+), 6 deletions(-)
@@ -1373,10 +1375,10 @@ static int __scmi_xfer_info_init(struct scmi_info *sinfo,conststructscmi_desc*desc=sinfo->desc;/* Pre-allocated messages, no more than what hdr.seq can support */-if(WARN_ON(!desc->max_msg||desc->max_msg>MSG_TOKEN_MAX)){+if(WARN_ON(!info->max_msg||info->max_msg>MSG_TOKEN_MAX)){dev_err(dev,"Invalid maximum messages %d, not in range [1 - %lu]\n",-desc->max_msg,MSG_TOKEN_MAX);+info->max_msg,MSG_TOKEN_MAX);return-EINVAL;}
@@ -1394,7 +1396,7 @@ static int __scmi_xfer_info_init(struct scmi_info *sinfo,*attachallofthemtothefreelist*/INIT_HLIST_HEAD(&info->free_xfers);-for(i=0;i<desc->max_msg;i++){+for(i=0;i<info->max_msg;i++){xfer=devm_kzalloc(dev,sizeof(*xfer),GFP_KERNEL);if(!xfer)return-ENOMEM;
@@ -1417,10 +1419,40 @@ static int __scmi_xfer_info_init(struct scmi_info *sinfo,return0;}+staticintscmi_channels_max_msg_configure(structscmi_info*sinfo)+{+conststructscmi_desc*desc=sinfo->desc;++if(!desc->ops->get_max_msg){+sinfo->tx_minfo.max_msg=desc->max_msg;+sinfo->rx_minfo.max_msg=desc->max_msg;+}else{+structscmi_chan_info*base_cinfo;++base_cinfo=idr_find(&sinfo->tx_idr,SCMI_PROTOCOL_BASE);+if(!base_cinfo)+return-EINVAL;+sinfo->tx_minfo.max_msg=desc->ops->get_max_msg(base_cinfo);++/* RX channel is optional so can be skipped */+base_cinfo=idr_find(&sinfo->rx_idr,SCMI_PROTOCOL_BASE);+if(base_cinfo)+sinfo->rx_minfo.max_msg=+desc->ops->get_max_msg(base_cinfo);+}++return0;+}+staticintscmi_xfer_info_init(structscmi_info*sinfo){-intret=__scmi_xfer_info_init(sinfo,&sinfo->tx_minfo);+intret;++ret=scmi_channels_max_msg_configure(sinfo);+if(ret)+returnret;+ret=__scmi_xfer_info_init(sinfo,&sinfo->tx_minfo);if(!ret&&idr_find(&sinfo->rx_idr,SCMI_PROTOCOL_BASE))ret=__scmi_xfer_info_init(sinfo,&sinfo->rx_minfo);
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Peter Hilber <redacted>
Add abstractions for future transports using message passing, such as
virtio. Derive the abstractions from the shared memory abstractions.
Abstract the transport SDU through the opaque struct scmi_msg_payld.
Also enable the transport to determine all other required information
about the transport SDU.
Signed-off-by: Peter Hilber <redacted>
[ Cristian: Adapted to new SCMI Kconfig layout, updated Copyrights ]
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v6 --> v7
- removed uneeded includes
v4 --> v5
- adapted to new SCMI Kconfig
- removed raw_payload msg helpers
v3 --> v4
- added raw_payload msg helpers
---
drivers/firmware/arm_scmi/Kconfig | 6 ++
drivers/firmware/arm_scmi/Makefile | 1 +
drivers/firmware/arm_scmi/common.h | 15 ++++
drivers/firmware/arm_scmi/msg.c | 111 +++++++++++++++++++++++++++++
4 files changed, 133 insertions(+)
create mode 100644 drivers/firmware/arm_scmi/msg.c
@@ -36,6 +36,12 @@ config ARM_SCMI_HAVE_SHMEMThisdeclareswhetherasharedmemorybasedtransportforSCMIisavailable.+configARM_SCMI_HAVE_MSG+bool+help+ThisdeclareswhetheramessagepassingbasedtransportforSCMIis+available.+configARM_SCMI_TRANSPORT_MAILBOXbool"SCMI transport based on Mailbox"depends onMAILBOX
@@ -0,0 +1,111 @@+// SPDX-License-Identifier: GPL-2.0+/*+*Fortransportsusingmessagepassing.+*+*Derivedfromshm.c.+*+*Copyright(C)2019-2021ARMLtd.+*Copyright(C)2020-2021OpenSynergyGmbH+*/++#include<linux/types.h>++#include"common.h"++/*+*structscmi_msg_payld-TransportSDUlayout+*+*TheSCMIspecificationrequiresallparameters,messageheaders,return+*argumentsoranyprotocoldatatobeexpressedinlittleendianformatonly.+*/+structscmi_msg_payld{+__le32msg_header;+__le32msg_payload[];+};++/**+*msg_command_size()-ActualsizeoftransportSDUforcommand.+*+*@xfer:messagewhichcorehaspreparedforsending+*+*Return:transportSDUsize.+*/+size_tmsg_command_size(structscmi_xfer*xfer)+{+returnsizeof(structscmi_msg_payld)+xfer->tx.len;+}++/**+*msg_response_size()-MaximumsizeoftransportSDUforresponse.+*+*@xfer:messagewhichcorehaspreparedforsending+*+*Return:transportSDUsize.+*/+size_tmsg_response_size(structscmi_xfer*xfer)+{+returnsizeof(structscmi_msg_payld)+sizeof(__le32)+xfer->rx.len;+}++/**+*msg_tx_prepare()-SetuptransportSDUforcommand.+*+*@msg:transportSDUforcommand+*@xfer:messagewhichisbeingsent+*/+voidmsg_tx_prepare(structscmi_msg_payld*msg,structscmi_xfer*xfer)+{+msg->msg_header=cpu_to_le32(pack_scmi_header(&xfer->hdr));+if(xfer->tx.buf)+memcpy(msg->msg_payload,xfer->tx.buf,xfer->tx.len);+}++/**+*msg_read_header()-ReadSCMIheaderfromtransportSDU.+*+*@msg:transportSDU+*+*Return:SCMIheader+*/+u32msg_read_header(structscmi_msg_payld*msg)+{+returnle32_to_cpu(msg->msg_header);+}++/**+*msg_fetch_response()-FetchresponseSCMIpayloadfromtransportSDU.+*+*@msg:transportSDUwithresponse+*@len:transportSDUsize+*@xfer:messagebeingrespondedto+*/+voidmsg_fetch_response(structscmi_msg_payld*msg,size_tlen,+structscmi_xfer*xfer)+{+size_tprefix_len=sizeof(*msg)+sizeof(msg->msg_payload[0]);++xfer->hdr.status=le32_to_cpu(msg->msg_payload[0]);+xfer->rx.len=min_t(size_t,xfer->rx.len,+len>=prefix_len?len-prefix_len:0);++/* Take a copy to the rx buffer.. */+memcpy(xfer->rx.buf,&msg->msg_payload[1],xfer->rx.len);+}++/**+*msg_fetch_notification()-FetchnotificationpayloadfromtransportSDU.+*+*@msg:transportSDUwithnotification+*@len:transportSDUsize+*@max_len:maximumSCMIpayloadsizetofetch+*@xfer:notificationmessage+*/+voidmsg_fetch_notification(structscmi_msg_payld*msg,size_tlen,+size_tmax_len,structscmi_xfer*xfer)+{+xfer->rx.len=min_t(size_t,max_len,+len>=sizeof(*msg)?len-sizeof(*msg):0);++/* Take a copy to the rx buffer.. */+memcpy(xfer->rx.buf,msg->msg_payload,xfer->rx.len);+}
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Peter Hilber <redacted>
Some transports are also effectively registered with other kernel subsystem
in order to be properly probed and initialized; as a consequence such kind
of transports, and their related devices, might still not have been probed
and initialized at the time the main SCMI core driver is probed.
Add an optional .link_supplier() transport operation which can be used by
the core SCMI stack to dynamically check if the transport is ready and
dynamically link its device to the SCMI platform instance device.
Signed-off-by: Peter Hilber <redacted>
[ Cristian: reworded commit message ]
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v6 --> v7
- fixed commit message
v5 --> v6
- reworded commit message
---
drivers/firmware/arm_scmi/common.h | 2 ++
drivers/firmware/arm_scmi/driver.c | 6 ++++++
2 files changed, 8 insertions(+)
From: Igor Skalkin <redacted>
Document the properties for arm,scmi-virtio compatible nodes.
The backing virtio SCMI device is described in patch [1].
While doing that, make shmem property required only for pre-existing
mailbox and smc transports, since virtio-scmi does not need it.
[1] https://lists.oasis-open.org/archives/virtio-comment/202102/msg00018.html
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Igor Skalkin <redacted>
[ Peter: Adapted patch for submission to upstream. ]
Co-developed-by: Peter Hilber <redacted>
Signed-off-by: Peter Hilber <redacted>
[ Cristian: converted to yaml format, moved shmen required property. ]
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v3 --> V4
- convertd to YAML
- make shmem required only for pre-existing mailbox and smc transport
- updated VirtIO specification patch message reference
- dropped virtio-mmio SCMI device example since really not pertinent to
virtio-scmi dt bindings transport: it is not even referenced in SCMI
virtio DT node since they are enumerated by VirtIO subsystem and there
could be PCI based SCMI devices anyway.
---
Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
@@ -34,6 +34,10 @@ properties:-description:SCMI compliant firmware with ARM SMC/HVC transportitems:-const:arm,scmi-smc+-description:SCMI compliant firmware with SCMI Virtio transport.+The virtio transport only supports a single device.+items:+-const:arm,scmi-virtiointerrupts:description:
@@ -172,6 +176,7 @@ patternProperties:Each sub-node represents a protocol supported. If the platformsupports a dedicated communication channel for a particular protocol,then the corresponding transport properties must be present.+The virtio transport does not support a dedicated communication channel.properties:reg:
Add a new opaque void *priv parameter to scmi_rx_callback which can be
optionally provided by the transport layer when invoking scmi_rx_callback
and that will be passed back to the transport layer in xfer->priv.
This can be used by transports that needs to keep track of their specific
data structures together with the valid xfers.
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v6 --> v7
- moved this patch later in the series right before virtio support
that directly needs it.
---
drivers/firmware/arm_scmi/common.h | 4 +++-
drivers/firmware/arm_scmi/driver.c | 17 ++++++++++++-----
drivers/firmware/arm_scmi/mailbox.c | 2 +-
drivers/firmware/arm_scmi/smc.c | 3 ++-
4 files changed, 18 insertions(+), 8 deletions(-)
From: Igor Skalkin <redacted>
This transport enables communications with an SCMI platform through virtio;
the SCMI platform will be represented by a virtio device.
Implement an SCMI virtio driver according to the virtio SCMI device spec
[1]. Virtio device id 32 has been reserved for the SCMI device [2].
The virtio transport has one Tx channel (virtio cmdq, A2P channel) and
at most one Rx channel (virtio eventq, P2A channel).
The following feature bit defined in [1] is not implemented:
VIRTIO_SCMI_F_SHARED_MEMORY.
The number of messages which can be pending simultaneously is restricted
according to the virtqueue capacity negotiated at probing time.
As soon as Rx channel message buffers are allocated or have been read
out by the arm-scmi driver, feed them back to the virtio device.
Since some virtio devices may not have the short response time exhibited
by SCMI platforms using other transports, set a generous response
timeout.
SCMI polling mode is not supported by this virtio transport since deemed
meaningless: polling mode operation is offered by the SCMI core to those
transports that could not provide a completion interrupt on the TX path,
which is never the case for virtio whose core callbacks can easily call
into core scmi_rx_callback upon messages reception.
[1] https://github.com/oasis-tcs/virtio-spec/blob/master/virtio-scmi.tex
[2] https://www.oasis-open.org/committees/ballot.php?id=3496
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <redacted>
Signed-off-by: Igor Skalkin <redacted>
[ Peter: Adapted patch for submission to upstream. ]
Co-developed-by: Peter Hilber <redacted>
Signed-off-by: Peter Hilber <redacted>
[ Cristian: simplified driver logic, changed link_supplier and channel
available/setup logic, removed dummy callbacks ]
Co-developed-by: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
NOTE THAT VIRTIO TRANSPORT IS ADDED AS default=n
V6 --> V7
- renamed desc init/exit to transport_init/exit
- reviewed Kconfig option to fit V7 ARM_SCMI Kconfig
V5 --> V6
- removed usage of delegated xfers
- using new scmi_rx_callback with priv argument
- removed .dummy clear_channel/.poll_done callbacks
- added missing spinlock comments
- updated Copyrights
V4 --> V5
- adapted Virtio transport config to new SCMI Kconfig layout
- removed support for polling
- added validate virtio method support
- removed usage of raw_payload helpers
- removed dynamic search of matching devices
- added one single statically configured device
V3 --> V4
- using delegated xfers
- using raw_payload msg helpers
---
MAINTAINERS | 1 +
drivers/firmware/arm_scmi/Kconfig | 11 +
drivers/firmware/arm_scmi/Makefile | 1 +
drivers/firmware/arm_scmi/common.h | 3 +
drivers/firmware/arm_scmi/driver.c | 3 +
drivers/firmware/arm_scmi/virtio.c | 491 +++++++++++++++++++++++++++++
include/uapi/linux/virtio_ids.h | 1 +
include/uapi/linux/virtio_scmi.h | 24 ++
8 files changed, 535 insertions(+)
create mode 100644 drivers/firmware/arm_scmi/virtio.c
create mode 100644 include/uapi/linux/virtio_scmi.h
@@ -66,6 +66,17 @@ config ARM_SCMI_TRANSPORT_SMCIfyouwanttheARMSCMIPROTOCOLstacktoincludesupportforatransportbasedonSMC,answerY.+configARM_SCMI_TRANSPORT_VIRTIO+bool"SCMI transport based on VirtIO"+depends onVIRTIO+selectARM_SCMI_HAVE_TRANSPORT+selectARM_SCMI_HAVE_MSG+help+ThisenablesthevirtiobasedtransportforSCMI.++IfyouwanttheARMSCMIPROTOCOLstacktoincludesupportfora+transportbasedonVirtIO,answerY.+endif#ARM_SCMI_PROTOCOLconfigARM_SCMI_POWER_DOMAIN
@@ -0,0 +1,491 @@+// SPDX-License-Identifier: GPL-2.0+/*+*VirtioTransportdriverforArmSystemControlandManagementInterface+*(SCMI).+*+*Copyright(C)2020-2021OpenSynergy.+*Copyright(C)2021ARMLtd.+*/++/**+*DOC:TheoryofOperation+*+*Thescmi-virtiotransportimplementsadriverforthevirtioSCMIdevice.+*+*ThereisoneTxchannel(virtiocmdq,A2Pchannel)andatmostoneRx+*channel(virtioeventq,P2Achannel).Eachchannelisimplementedthrougha+*virtqueue.Accesstoeachvirtqueueisprotectedbyspinlocks.+*/++#include<linux/errno.h>+#include<linux/slab.h>+#include<linux/virtio.h>+#include<linux/virtio_config.h>++#include<uapi/linux/virtio_ids.h>+#include<uapi/linux/virtio_scmi.h>++#include"common.h"++#define VIRTIO_SCMI_MAX_MSG_SIZE 128 /* Value may be increased. */+#define VIRTIO_SCMI_MAX_PDU_SIZE \+(VIRTIO_SCMI_MAX_MSG_SIZE+SCMI_MSG_MAX_PROT_OVERHEAD)+#define DESCRIPTORS_PER_TX_MSG 2++/**+*structscmi_vio_channel-Transportchannelinformation+*+*@vqueue:Associatedvirtqueue+*@cinfo:SCMITxorRxchannel+*@free_list:Listofunusedscmi_vio_msg,maintainedforTxchannelsonly+*@is_rx:WhetherchannelisanRxchannel+*@ready:Whethertransportuserisreadytohearaboutchannel+*@max_msg:Maximumnumberofpendingmessagesforthischannel.+*@lock:Protectsaccesstoallmembersexceptready.+*@ready_lock:Protectsaccesstoready.Ifrequired,itmustbetakenbefore+*lock.+*/+structscmi_vio_channel{+structvirtqueue*vqueue;+structscmi_chan_info*cinfo;+structlist_headfree_list;+boolis_rx;+boolready;+unsignedintmax_msg;+/* lock to protect access to all members except ready. */+spinlock_tlock;+/* lock to rotects access to ready flag. */+spinlock_tready_lock;+};++/**+*structscmi_vio_msg-TransportPDUinformation+*+*@request:SDUusedforcommands+*@input:SDUusedfor(delayed)responsesandnotifications+*@list:Listwhichscmi_vio_msgmaybepartof+*@rx_len:InputSDUsizeinbytes,onceinputhasbeenreceived+*/+structscmi_vio_msg{+structscmi_msg_payld*request;+structscmi_msg_payld*input;+structlist_headlist;+unsignedintrx_len;+};++/* Only one SCMI VirtIO device can possibly exist */+staticstructvirtio_device*scmi_vdev;++staticboolscmi_vio_have_vq_rx(structvirtio_device*vdev)+{+returnvirtio_has_feature(vdev,VIRTIO_SCMI_F_P2A_CHANNELS);+}++staticintscmi_vio_feed_vq_rx(structscmi_vio_channel*vioch,+structscmi_vio_msg*msg)+{+structscatterlistsg_in;+intrc;+unsignedlongflags;++sg_init_one(&sg_in,msg->input,VIRTIO_SCMI_MAX_PDU_SIZE);++spin_lock_irqsave(&vioch->lock,flags);++rc=virtqueue_add_inbuf(vioch->vqueue,&sg_in,1,msg,GFP_ATOMIC);+if(rc)+dev_err_once(vioch->cinfo->dev,+"failed to add to virtqueue (%d)\n",rc);+else+virtqueue_kick(vioch->vqueue);++spin_unlock_irqrestore(&vioch->lock,flags);++returnrc;+}++staticvoidscmi_finalize_message(structscmi_vio_channel*vioch,+structscmi_vio_msg*msg)+{+if(vioch->is_rx){+scmi_vio_feed_vq_rx(vioch,msg);+}else{+unsignedlongflags;++spin_lock_irqsave(&vioch->lock,flags);+list_add(&msg->list,&vioch->free_list);+spin_unlock_irqrestore(&vioch->lock,flags);+}+}++staticvoidscmi_vio_complete_cb(structvirtqueue*vqueue)+{+unsignedlongready_flags;+unsignedlongflags;+unsignedintlength;+structscmi_vio_channel*vioch;+structscmi_vio_msg*msg;+boolcb_enabled=true;++if(WARN_ON_ONCE(!vqueue->vdev->priv))+return;+vioch=&((structscmi_vio_channel*)vqueue->vdev->priv)[vqueue->index];++for(;;){+spin_lock_irqsave(&vioch->ready_lock,ready_flags);++if(!vioch->ready){+if(!cb_enabled)+(void)virtqueue_enable_cb(vqueue);+gotounlock_ready_out;+}++spin_lock_irqsave(&vioch->lock,flags);+if(cb_enabled){+virtqueue_disable_cb(vqueue);+cb_enabled=false;+}+msg=virtqueue_get_buf(vqueue,&length);+if(!msg){+if(virtqueue_enable_cb(vqueue))+gotounlock_out;+cb_enabled=true;+}+spin_unlock_irqrestore(&vioch->lock,flags);++if(msg){+msg->rx_len=length;+scmi_rx_callback(vioch->cinfo,+msg_read_header(msg->input),msg);++scmi_finalize_message(vioch,msg);+}++spin_unlock_irqrestore(&vioch->ready_lock,ready_flags);+}++unlock_out:+spin_unlock_irqrestore(&vioch->lock,flags);+unlock_ready_out:+spin_unlock_irqrestore(&vioch->ready_lock,ready_flags);+}++staticconstchar*constscmi_vio_vqueue_names[]={"tx","rx"};++staticvq_callback_t*scmi_vio_complete_callbacks[]={+scmi_vio_complete_cb,+scmi_vio_complete_cb+};++staticunsignedintvirtio_get_max_msg(structscmi_chan_info*base_cinfo)+{+structscmi_vio_channel*vioch=base_cinfo->transport_info;++returnvioch->max_msg;+}++staticintvirtio_link_supplier(structdevice*dev)+{+if(!scmi_vdev){+dev_notice_once(dev,+"Deferring probe after not finding a bound scmi-virtio device\n");+return-EPROBE_DEFER;+}++if(!device_link_add(dev,&scmi_vdev->dev,+DL_FLAG_AUTOREMOVE_CONSUMER)){+dev_err(dev,"Adding link to supplier virtio device failed\n");+return-ECANCELED;+}++return0;+}++staticboolvirtio_chan_available(structdevice*dev,intidx)+{+structscmi_vio_channel*channels,*vioch=NULL;++if(WARN_ON_ONCE(!scmi_vdev))+returnfalse;++channels=(structscmi_vio_channel*)scmi_vdev->priv;++switch(idx){+caseVIRTIO_SCMI_VQ_TX:+vioch=&channels[VIRTIO_SCMI_VQ_TX];+break;+caseVIRTIO_SCMI_VQ_RX:+if(scmi_vio_have_vq_rx(scmi_vdev))+vioch=&channels[VIRTIO_SCMI_VQ_RX];+break;+default:+returnfalse;+}++returnvioch&&!vioch->cinfo?true:false;+}++staticintvirtio_chan_setup(structscmi_chan_info*cinfo,structdevice*dev,+booltx)+{+unsignedlongflags;+structscmi_vio_channel*vioch;+intindex=tx?VIRTIO_SCMI_VQ_TX:VIRTIO_SCMI_VQ_RX;+inti;++if(!scmi_vdev)+return-EPROBE_DEFER;++vioch=&((structscmi_vio_channel*)scmi_vdev->priv)[index];++for(i=0;i<vioch->max_msg;i++){+structscmi_vio_msg*msg;++msg=devm_kzalloc(cinfo->dev,sizeof(*msg),GFP_KERNEL);+if(!msg)+return-ENOMEM;++if(tx){+msg->request=devm_kzalloc(cinfo->dev,+VIRTIO_SCMI_MAX_PDU_SIZE,+GFP_KERNEL);+if(!msg->request)+return-ENOMEM;+}++msg->input=devm_kzalloc(cinfo->dev,VIRTIO_SCMI_MAX_PDU_SIZE,+GFP_KERNEL);+if(!msg->input)+return-ENOMEM;++if(tx){+spin_lock_irqsave(&vioch->lock,flags);+list_add_tail(&msg->list,&vioch->free_list);+spin_unlock_irqrestore(&vioch->lock,flags);+}else{+scmi_vio_feed_vq_rx(vioch,msg);+}+}++spin_lock_irqsave(&vioch->lock,flags);+cinfo->transport_info=vioch;+/* Indirectly setting channel not available any more */+vioch->cinfo=cinfo;+spin_unlock_irqrestore(&vioch->lock,flags);++spin_lock_irqsave(&vioch->ready_lock,flags);+vioch->ready=true;+spin_unlock_irqrestore(&vioch->ready_lock,flags);++return0;+}++staticintvirtio_chan_free(intid,void*p,void*data)+{+unsignedlongflags;+structscmi_chan_info*cinfo=p;+structscmi_vio_channel*vioch=cinfo->transport_info;++spin_lock_irqsave(&vioch->ready_lock,flags);+vioch->ready=false;+spin_unlock_irqrestore(&vioch->ready_lock,flags);++scmi_free_channel(cinfo,data,id);++spin_lock_irqsave(&vioch->lock,flags);+vioch->cinfo=NULL;+spin_unlock_irqrestore(&vioch->lock,flags);++return0;+}++staticintvirtio_send_message(structscmi_chan_info*cinfo,+structscmi_xfer*xfer)+{+structscmi_vio_channel*vioch=cinfo->transport_info;+structscatterlistsg_out;+structscatterlistsg_in;+structscatterlist*sgs[DESCRIPTORS_PER_TX_MSG]={&sg_out,&sg_in};+unsignedlongflags;+intrc;+structscmi_vio_msg*msg;++spin_lock_irqsave(&vioch->lock,flags);++if(list_empty(&vioch->free_list)){+spin_unlock_irqrestore(&vioch->lock,flags);+return-EBUSY;+}++msg=list_first_entry(&vioch->free_list,typeof(*msg),list);+list_del(&msg->list);++msg_tx_prepare(msg->request,xfer);++sg_init_one(&sg_out,msg->request,msg_command_size(xfer));+sg_init_one(&sg_in,msg->input,msg_response_size(xfer));++rc=virtqueue_add_sgs(vioch->vqueue,sgs,1,1,msg,GFP_ATOMIC);+if(rc){+list_add(&msg->list,&vioch->free_list);+dev_err_once(vioch->cinfo->dev,+"%s() failed to add to virtqueue (%d)\n",__func__,+rc);+}else{+virtqueue_kick(vioch->vqueue);+}++spin_unlock_irqrestore(&vioch->lock,flags);++returnrc;+}++staticvoidvirtio_fetch_response(structscmi_chan_info*cinfo,+structscmi_xfer*xfer)+{+structscmi_vio_msg*msg=xfer->priv;++if(msg){+msg_fetch_response(msg->input,msg->rx_len,xfer);+xfer->priv=NULL;+}+}++staticvoidvirtio_fetch_notification(structscmi_chan_info*cinfo,+size_tmax_len,structscmi_xfer*xfer)+{+structscmi_vio_msg*msg=xfer->priv;++if(msg){+msg_fetch_notification(msg->input,msg->rx_len,max_len,xfer);+xfer->priv=NULL;+}+}++staticconststructscmi_transport_opsscmi_virtio_ops={+.link_supplier=virtio_link_supplier,+.chan_available=virtio_chan_available,+.chan_setup=virtio_chan_setup,+.chan_free=virtio_chan_free,+.get_max_msg=virtio_get_max_msg,+.send_message=virtio_send_message,+.fetch_response=virtio_fetch_response,+.fetch_notification=virtio_fetch_notification,+};++staticintscmi_vio_probe(structvirtio_device*vdev)+{+structdevice*dev=&vdev->dev;+structscmi_vio_channel*channels;+boolhave_vq_rx;+intvq_cnt;+inti;+intret;+structvirtqueue*vqs[VIRTIO_SCMI_VQ_MAX_CNT];++/* Only one SCMI VirtiO device allowed */+if(scmi_vdev)+return-EINVAL;++have_vq_rx=scmi_vio_have_vq_rx(vdev);+vq_cnt=have_vq_rx?VIRTIO_SCMI_VQ_MAX_CNT:1;++channels=devm_kcalloc(dev,vq_cnt,sizeof(*channels),GFP_KERNEL);+if(!channels)+return-ENOMEM;++if(have_vq_rx)+channels[VIRTIO_SCMI_VQ_RX].is_rx=true;++ret=virtio_find_vqs(vdev,vq_cnt,vqs,scmi_vio_complete_callbacks,+scmi_vio_vqueue_names,NULL);+if(ret){+dev_err(dev,"Failed to get %d virtqueue(s)\n",vq_cnt);+returnret;+}++for(i=0;i<vq_cnt;i++){+unsignedintsz;++spin_lock_init(&channels[i].lock);+spin_lock_init(&channels[i].ready_lock);+INIT_LIST_HEAD(&channels[i].free_list);+channels[i].vqueue=vqs[i];++sz=virtqueue_get_vring_size(channels[i].vqueue);+/* Tx messages need multiple descriptors. */+if(!channels[i].is_rx)+sz/=DESCRIPTORS_PER_TX_MSG;++if(sz>MSG_TOKEN_MAX){+dev_info_once(dev,+"%s virtqueue could hold %d messages. Only %ld allowed to be pending.\n",+channels[i].is_rx?"rx":"tx",+sz,MSG_TOKEN_MAX);+sz=MSG_TOKEN_MAX;+}+channels[i].max_msg=sz;+}++vdev->priv=channels;+scmi_vdev=vdev;++return0;+}++staticvoidscmi_vio_remove(structvirtio_device*vdev)+{+vdev->config->reset(vdev);+vdev->config->del_vqs(vdev);+scmi_vdev=NULL;+}++staticintscmi_vio_validate(structvirtio_device*vdev)+{+if(!virtio_has_feature(vdev,VIRTIO_F_VERSION_1)){+dev_err(&vdev->dev,+"device does not comply with spec version 1.x\n");+return-EINVAL;+}++return0;+}++staticunsignedintfeatures[]={+VIRTIO_SCMI_F_P2A_CHANNELS,+};++staticconststructvirtio_device_idid_table[]={+{VIRTIO_ID_SCMI,VIRTIO_DEV_ANY_ID},+{0}+};++staticstructvirtio_drivervirtio_scmi_driver={+.driver.name="scmi-virtio",+.driver.owner=THIS_MODULE,+.feature_table=features,+.feature_table_size=ARRAY_SIZE(features),+.id_table=id_table,+.probe=scmi_vio_probe,+.remove=scmi_vio_remove,+.validate=scmi_vio_validate,+};++staticint__initvirtio_scmi_init(void)+{+returnregister_virtio_driver(&virtio_scmi_driver);+}++staticvoid__exitvirtio_scmi_exit(void)+{+unregister_virtio_driver(&virtio_scmi_driver);+}++conststructscmi_descscmi_virtio_desc={+.transport_init=virtio_scmi_init,+.transport_exit=virtio_scmi_exit,+.ops=&scmi_virtio_ops,+.max_rx_timeout_ms=60000,/* for non-realtime virtio devices */+.max_msg=0,/* overridden by virtio_get_max_msg() */+.max_msg_size=VIRTIO_SCMI_MAX_MSG_SIZE,+};
On Tue, 3 Aug 2021 14:10:09 +0100, Cristian Marussi wrote:
While reworking this series starting from the work done up to V3 by
OpenSynergy, I am keeping the original autorship and list distribution
unchanged.
The main aim of this rework, as said, is to simplify where possible the
SCMI VirtIO support added in V3 by adding at first some new general
mechanisms in the SCMI Transport layer.
[...]
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2021-09-04 13:04:07
On Tue, Aug 03, 2021 at 02:10:24PM +0100, Cristian Marussi wrote:
quoted hunk
From: Igor Skalkin <redacted>
This transport enables communications with an SCMI platform through virtio;
the SCMI platform will be represented by a virtio device.
Implement an SCMI virtio driver according to the virtio SCMI device spec
[1]. Virtio device id 32 has been reserved for the SCMI device [2].
The virtio transport has one Tx channel (virtio cmdq, A2P channel) and
at most one Rx channel (virtio eventq, P2A channel).
The following feature bit defined in [1] is not implemented:
VIRTIO_SCMI_F_SHARED_MEMORY.
The number of messages which can be pending simultaneously is restricted
according to the virtqueue capacity negotiated at probing time.
As soon as Rx channel message buffers are allocated or have been read
out by the arm-scmi driver, feed them back to the virtio device.
Since some virtio devices may not have the short response time exhibited
by SCMI platforms using other transports, set a generous response
timeout.
SCMI polling mode is not supported by this virtio transport since deemed
meaningless: polling mode operation is offered by the SCMI core to those
transports that could not provide a completion interrupt on the TX path,
which is never the case for virtio whose core callbacks can easily call
into core scmi_rx_callback upon messages reception.
[1] https://github.com/oasis-tcs/virtio-spec/blob/master/virtio-scmi.tex
[2] https://www.oasis-open.org/committees/ballot.php?id=3496
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <redacted>
Signed-off-by: Igor Skalkin <redacted>
[ Peter: Adapted patch for submission to upstream. ]
Co-developed-by: Peter Hilber <redacted>
Signed-off-by: Peter Hilber <redacted>
[ Cristian: simplified driver logic, changed link_supplier and channel
available/setup logic, removed dummy callbacks ]
Co-developed-by: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
NOTE THAT VIRTIO TRANSPORT IS ADDED AS default=n
V6 --> V7
- renamed desc init/exit to transport_init/exit
- reviewed Kconfig option to fit V7 ARM_SCMI Kconfig
V5 --> V6
- removed usage of delegated xfers
- using new scmi_rx_callback with priv argument
- removed .dummy clear_channel/.poll_done callbacks
- added missing spinlock comments
- updated Copyrights
V4 --> V5
- adapted Virtio transport config to new SCMI Kconfig layout
- removed support for polling
- added validate virtio method support
- removed usage of raw_payload helpers
- removed dynamic search of matching devices
- added one single statically configured device
V3 --> V4
- using delegated xfers
- using raw_payload msg helpers
---
MAINTAINERS | 1 +
drivers/firmware/arm_scmi/Kconfig | 11 +
drivers/firmware/arm_scmi/Makefile | 1 +
drivers/firmware/arm_scmi/common.h | 3 +
drivers/firmware/arm_scmi/driver.c | 3 +
drivers/firmware/arm_scmi/virtio.c | 491 +++++++++++++++++++++++++++++
include/uapi/linux/virtio_ids.h | 1 +
include/uapi/linux/virtio_scmi.h | 24 ++
8 files changed, 535 insertions(+)
create mode 100644 drivers/firmware/arm_scmi/virtio.c
create mode 100644 include/uapi/linux/virtio_scmi.h
@@ -66,6 +66,17 @@ config ARM_SCMI_TRANSPORT_SMCIfyouwanttheARMSCMIPROTOCOLstacktoincludesupportforatransportbasedonSMC,answerY.+configARM_SCMI_TRANSPORT_VIRTIO+bool"SCMI transport based on VirtIO"+depends onVIRTIO+selectARM_SCMI_HAVE_TRANSPORT+selectARM_SCMI_HAVE_MSG+help+ThisenablesthevirtiobasedtransportforSCMI.++IfyouwanttheARMSCMIPROTOCOLstacktoincludesupportfora+transportbasedonVirtIO,answerY.+endif#ARM_SCMI_PROTOCOLconfigARM_SCMI_POWER_DOMAIN
@@ -0,0 +1,491 @@+// SPDX-License-Identifier: GPL-2.0+/*+*VirtioTransportdriverforArmSystemControlandManagementInterface+*(SCMI).+*+*Copyright(C)2020-2021OpenSynergy.+*Copyright(C)2021ARMLtd.+*/++/**+*DOC:TheoryofOperation+*+*Thescmi-virtiotransportimplementsadriverforthevirtioSCMIdevice.+*+*ThereisoneTxchannel(virtiocmdq,A2Pchannel)andatmostoneRx+*channel(virtioeventq,P2Achannel).Eachchannelisimplementedthrougha+*virtqueue.Accesstoeachvirtqueueisprotectedbyspinlocks.+*/++#include<linux/errno.h>+#include<linux/slab.h>+#include<linux/virtio.h>+#include<linux/virtio_config.h>++#include<uapi/linux/virtio_ids.h>+#include<uapi/linux/virtio_scmi.h>++#include"common.h"++#define VIRTIO_SCMI_MAX_MSG_SIZE 128 /* Value may be increased. */+#define VIRTIO_SCMI_MAX_PDU_SIZE \+(VIRTIO_SCMI_MAX_MSG_SIZE+SCMI_MSG_MAX_PROT_OVERHEAD)+#define DESCRIPTORS_PER_TX_MSG 2++/**+*structscmi_vio_channel-Transportchannelinformation+*+*@vqueue:Associatedvirtqueue+*@cinfo:SCMITxorRxchannel+*@free_list:Listofunusedscmi_vio_msg,maintainedforTxchannelsonly+*@is_rx:WhetherchannelisanRxchannel+*@ready:Whethertransportuserisreadytohearaboutchannel+*@max_msg:Maximumnumberofpendingmessagesforthischannel.+*@lock:Protectsaccesstoallmembersexceptready.+*@ready_lock:Protectsaccesstoready.Ifrequired,itmustbetakenbefore+*lock.+*/+structscmi_vio_channel{+structvirtqueue*vqueue;+structscmi_chan_info*cinfo;+structlist_headfree_list;+boolis_rx;+boolready;+unsignedintmax_msg;+/* lock to protect access to all members except ready. */+spinlock_tlock;+/* lock to rotects access to ready flag. */+spinlock_tready_lock;+};++/**+*structscmi_vio_msg-TransportPDUinformation+*+*@request:SDUusedforcommands+*@input:SDUusedfor(delayed)responsesandnotifications+*@list:Listwhichscmi_vio_msgmaybepartof+*@rx_len:InputSDUsizeinbytes,onceinputhasbeenreceived+*/+structscmi_vio_msg{+structscmi_msg_payld*request;+structscmi_msg_payld*input;+structlist_headlist;+unsignedintrx_len;+};++/* Only one SCMI VirtIO device can possibly exist */+staticstructvirtio_device*scmi_vdev;++staticboolscmi_vio_have_vq_rx(structvirtio_device*vdev)+{+returnvirtio_has_feature(vdev,VIRTIO_SCMI_F_P2A_CHANNELS);+}++staticintscmi_vio_feed_vq_rx(structscmi_vio_channel*vioch,+structscmi_vio_msg*msg)+{+structscatterlistsg_in;+intrc;+unsignedlongflags;++sg_init_one(&sg_in,msg->input,VIRTIO_SCMI_MAX_PDU_SIZE);++spin_lock_irqsave(&vioch->lock,flags);++rc=virtqueue_add_inbuf(vioch->vqueue,&sg_in,1,msg,GFP_ATOMIC);+if(rc)+dev_err_once(vioch->cinfo->dev,+"failed to add to virtqueue (%d)\n",rc);+else+virtqueue_kick(vioch->vqueue);++spin_unlock_irqrestore(&vioch->lock,flags);++returnrc;+}++staticvoidscmi_finalize_message(structscmi_vio_channel*vioch,+structscmi_vio_msg*msg)+{+if(vioch->is_rx){+scmi_vio_feed_vq_rx(vioch,msg);+}else{+unsignedlongflags;++spin_lock_irqsave(&vioch->lock,flags);+list_add(&msg->list,&vioch->free_list);+spin_unlock_irqrestore(&vioch->lock,flags);+}+}++staticvoidscmi_vio_complete_cb(structvirtqueue*vqueue)+{+unsignedlongready_flags;+unsignedlongflags;+unsignedintlength;+structscmi_vio_channel*vioch;+structscmi_vio_msg*msg;+boolcb_enabled=true;++if(WARN_ON_ONCE(!vqueue->vdev->priv))+return;+vioch=&((structscmi_vio_channel*)vqueue->vdev->priv)[vqueue->index];++for(;;){+spin_lock_irqsave(&vioch->ready_lock,ready_flags);
maybe take the lock out of the loop for speed?
+
+ if (!vioch->ready) {
+ if (!cb_enabled)
+ (void)virtqueue_enable_cb(vqueue);
+ goto unlock_ready_out;
+ }
+
+ spin_lock_irqsave(&vioch->lock, flags);
no reason to re-save interrupts here - saved already ...
So this is read in parallel, unless there's a lock I think you need an
smp_wmb here and WRITE_ONCE. Or maybe add locking instead so we
do not worry about ordering ...
On Sat, Sep 04, 2021 at 09:03:53AM -0400, Michael S. Tsirkin wrote:
On Tue, Aug 03, 2021 at 02:10:24PM +0100, Cristian Marussi wrote:
quoted
From: Igor Skalkin <redacted>
This transport enables communications with an SCMI platform through virtio;
the SCMI platform will be represented by a virtio device.
Implement an SCMI virtio driver according to the virtio SCMI device spec
[1]. Virtio device id 32 has been reserved for the SCMI device [2].
The virtio transport has one Tx channel (virtio cmdq, A2P channel) and
at most one Rx channel (virtio eventq, P2A channel).
The following feature bit defined in [1] is not implemented:
VIRTIO_SCMI_F_SHARED_MEMORY.
The number of messages which can be pending simultaneously is restricted
according to the virtqueue capacity negotiated at probing time.
As soon as Rx channel message buffers are allocated or have been read
out by the arm-scmi driver, feed them back to the virtio device.
Since some virtio devices may not have the short response time exhibited
by SCMI platforms using other transports, set a generous response
timeout.
SCMI polling mode is not supported by this virtio transport since deemed
meaningless: polling mode operation is offered by the SCMI core to those
transports that could not provide a completion interrupt on the TX path,
which is never the case for virtio whose core callbacks can easily call
into core scmi_rx_callback upon messages reception.
[1] https://github.com/oasis-tcs/virtio-spec/blob/master/virtio-scmi.tex
[2] https://www.oasis-open.org/committees/ballot.php?id=3496
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <redacted>
Signed-off-by: Igor Skalkin <redacted>
[ Peter: Adapted patch for submission to upstream. ]
Co-developed-by: Peter Hilber <redacted>
Signed-off-by: Peter Hilber <redacted>
[ Cristian: simplified driver logic, changed link_supplier and channel
available/setup logic, removed dummy callbacks ]
Co-developed-by: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
NOTE THAT VIRTIO TRANSPORT IS ADDED AS default=n
V6 --> V7
- renamed desc init/exit to transport_init/exit
- reviewed Kconfig option to fit V7 ARM_SCMI Kconfig
V5 --> V6
- removed usage of delegated xfers
- using new scmi_rx_callback with priv argument
- removed .dummy clear_channel/.poll_done callbacks
- added missing spinlock comments
- updated Copyrights
V4 --> V5
- adapted Virtio transport config to new SCMI Kconfig layout
- removed support for polling
- added validate virtio method support
- removed usage of raw_payload helpers
- removed dynamic search of matching devices
- added one single statically configured device
V3 --> V4
- using delegated xfers
- using raw_payload msg helpers
---
MAINTAINERS | 1 +
drivers/firmware/arm_scmi/Kconfig | 11 +
drivers/firmware/arm_scmi/Makefile | 1 +
drivers/firmware/arm_scmi/common.h | 3 +
drivers/firmware/arm_scmi/driver.c | 3 +
drivers/firmware/arm_scmi/virtio.c | 491 +++++++++++++++++++++++++++++
include/uapi/linux/virtio_ids.h | 1 +
include/uapi/linux/virtio_scmi.h | 24 ++
8 files changed, 535 insertions(+)
create mode 100644 drivers/firmware/arm_scmi/virtio.c
create mode 100644 include/uapi/linux/virtio_scmi.h
Hi Michael,
thanks for your comments, replies inline.
For all virtio bits please include
L: virtualization@lists.linux-foundation.org
as well.
Thanks!
Apologies, I'm not sure what was the issue but this series was supposed
to have been posted also To: virtualization@lists.linux-foundation.org
even for the previous versions of the series: in this last v7 I also added
you in Cc.
Have you not received any of the previous v* on the virtualization list ?
quoted
SYSTEM RESET/SHUTDOWN DRIVERS
M: Sebastian Reichel [off-list ref]
@@ -66,6 +66,17 @@ config ARM_SCMI_TRANSPORT_SMCIfyouwanttheARMSCMIPROTOCOLstacktoincludesupportforatransportbasedonSMC,answerY.+configARM_SCMI_TRANSPORT_VIRTIO+bool"SCMI transport based on VirtIO"+depends onVIRTIO+selectARM_SCMI_HAVE_TRANSPORT+selectARM_SCMI_HAVE_MSG+help+ThisenablesthevirtiobasedtransportforSCMI.++IfyouwanttheARMSCMIPROTOCOLstacktoincludesupportfora+transportbasedonVirtIO,answerY.+endif#ARM_SCMI_PROTOCOLconfigARM_SCMI_POWER_DOMAIN
@@ -0,0 +1,491 @@+// SPDX-License-Identifier: GPL-2.0+/*+*VirtioTransportdriverforArmSystemControlandManagementInterface+*(SCMI).+*+*Copyright(C)2020-2021OpenSynergy.+*Copyright(C)2021ARMLtd.+*/++/**+*DOC:TheoryofOperation+*+*Thescmi-virtiotransportimplementsadriverforthevirtioSCMIdevice.+*+*ThereisoneTxchannel(virtiocmdq,A2Pchannel)andatmostoneRx+*channel(virtioeventq,P2Achannel).Eachchannelisimplementedthrougha+*virtqueue.Accesstoeachvirtqueueisprotectedbyspinlocks.+*/++#include<linux/errno.h>+#include<linux/slab.h>+#include<linux/virtio.h>+#include<linux/virtio_config.h>++#include<uapi/linux/virtio_ids.h>+#include<uapi/linux/virtio_scmi.h>++#include"common.h"++#define VIRTIO_SCMI_MAX_MSG_SIZE 128 /* Value may be increased. */+#define VIRTIO_SCMI_MAX_PDU_SIZE \+(VIRTIO_SCMI_MAX_MSG_SIZE+SCMI_MSG_MAX_PROT_OVERHEAD)+#define DESCRIPTORS_PER_TX_MSG 2++/**+*structscmi_vio_channel-Transportchannelinformation+*+*@vqueue:Associatedvirtqueue+*@cinfo:SCMITxorRxchannel+*@free_list:Listofunusedscmi_vio_msg,maintainedforTxchannelsonly+*@is_rx:WhetherchannelisanRxchannel+*@ready:Whethertransportuserisreadytohearaboutchannel+*@max_msg:Maximumnumberofpendingmessagesforthischannel.+*@lock:Protectsaccesstoallmembersexceptready.+*@ready_lock:Protectsaccesstoready.Ifrequired,itmustbetakenbefore+*lock.+*/+structscmi_vio_channel{+structvirtqueue*vqueue;+structscmi_chan_info*cinfo;+structlist_headfree_list;+boolis_rx;+boolready;+unsignedintmax_msg;+/* lock to protect access to all members except ready. */+spinlock_tlock;+/* lock to rotects access to ready flag. */+spinlock_tready_lock;+};++/**+*structscmi_vio_msg-TransportPDUinformation+*+*@request:SDUusedforcommands+*@input:SDUusedfor(delayed)responsesandnotifications+*@list:Listwhichscmi_vio_msgmaybepartof+*@rx_len:InputSDUsizeinbytes,onceinputhasbeenreceived+*/+structscmi_vio_msg{+structscmi_msg_payld*request;+structscmi_msg_payld*input;+structlist_headlist;+unsignedintrx_len;+};++/* Only one SCMI VirtIO device can possibly exist */+staticstructvirtio_device*scmi_vdev;++staticboolscmi_vio_have_vq_rx(structvirtio_device*vdev)+{+returnvirtio_has_feature(vdev,VIRTIO_SCMI_F_P2A_CHANNELS);+}++staticintscmi_vio_feed_vq_rx(structscmi_vio_channel*vioch,+structscmi_vio_msg*msg)+{+structscatterlistsg_in;+intrc;+unsignedlongflags;++sg_init_one(&sg_in,msg->input,VIRTIO_SCMI_MAX_PDU_SIZE);++spin_lock_irqsave(&vioch->lock,flags);++rc=virtqueue_add_inbuf(vioch->vqueue,&sg_in,1,msg,GFP_ATOMIC);+if(rc)+dev_err_once(vioch->cinfo->dev,+"failed to add to virtqueue (%d)\n",rc);+else+virtqueue_kick(vioch->vqueue);++spin_unlock_irqrestore(&vioch->lock,flags);++returnrc;+}++staticvoidscmi_finalize_message(structscmi_vio_channel*vioch,+structscmi_vio_msg*msg)+{+if(vioch->is_rx){+scmi_vio_feed_vq_rx(vioch,msg);+}else{+unsignedlongflags;++spin_lock_irqsave(&vioch->lock,flags);+list_add(&msg->list,&vioch->free_list);+spin_unlock_irqrestore(&vioch->lock,flags);+}+}++staticvoidscmi_vio_complete_cb(structvirtqueue*vqueue)+{+unsignedlongready_flags;+unsignedlongflags;+unsignedintlength;+structscmi_vio_channel*vioch;+structscmi_vio_msg*msg;+boolcb_enabled=true;++if(WARN_ON_ONCE(!vqueue->vdev->priv))+return;+vioch=&((structscmi_vio_channel*)vqueue->vdev->priv)[vqueue->index];++for(;;){+spin_lock_irqsave(&vioch->ready_lock,ready_flags);
maybe take the lock out of the loop for speed?
I'll review this, but it seems to me that moving the spinlock out of the
loop you won't be able to set the vioch NOT ready when shutting down,
until all message are processed, while acquiring/releasing the lock
inside the loop enables the possibility to check for ready flag between
each message processing. (more on this later down below...)
quoted
+
+ if (!vioch->ready) {
+ if (!cb_enabled)
+ (void)virtqueue_enable_cb(vqueue);
+ goto unlock_ready_out;
+ }
+
+ spin_lock_irqsave(&vioch->lock, flags);
no reason to re-save interrupts here - saved already ...
Or EBUSY maybe? And print an error message pls so users
can figure out what is going on.
Yes of course, I'll fix.
quoted
+
+ have_vq_rx = scmi_vio_have_vq_rx(vdev);
+ vq_cnt = have_vq_rx ? VIRTIO_SCMI_VQ_MAX_CNT : 1;
+
+ channels = devm_kcalloc(dev, vq_cnt, sizeof(*channels), GFP_KERNEL);
+ if (!channels)
+ return -ENOMEM;
+
+ if (have_vq_rx)
+ channels[VIRTIO_SCMI_VQ_RX].is_rx = true;
+
+ ret = virtio_find_vqs(vdev, vq_cnt, vqs, scmi_vio_complete_callbacks,
+ scmi_vio_vqueue_names, NULL);
+ if (ret) {
+ dev_err(dev, "Failed to get %d virtqueue(s)\n", vq_cnt);
And free allocated memory here?
Sorry, which memory: the only allocated memory at this point is channels
and that is done using a devm_ call, so I suppose on failure here we
bail out of the probe and that memeory is automagically freed by devres,
or am I missing something ?
quoted
+ return ret;
+ }
+
+ for (i = 0; i < vq_cnt; i++) {
+ unsigned int sz;
+
+ spin_lock_init(&channels[i].lock);
+ spin_lock_init(&channels[i].ready_lock);
+ INIT_LIST_HEAD(&channels[i].free_list);
+ channels[i].vqueue = vqs[i];
+
+ sz = virtqueue_get_vring_size(channels[i].vqueue);
+ /* Tx messages need multiple descriptors. */
+ if (!channels[i].is_rx)
+ sz /= DESCRIPTORS_PER_TX_MSG;
+
+ if (sz > MSG_TOKEN_MAX) {
+ dev_info_once(dev,
+ "%s virtqueue could hold %d messages. Only %ld allowed to be pending.\n",
+ channels[i].is_rx ? "rx" : "tx",
+ sz, MSG_TOKEN_MAX);
+ sz = MSG_TOKEN_MAX;
+ }
+ channels[i].max_msg = sz;
What happens if sz is too small here? E.g. 0? Should we fail probe?
No, because I think it should be up to the SCMI core that uses this
SCMI virtio transport to decide WHICH size is too small and what to do
when it happens (.max_msg is exposed to the SCMI core via .get_max_msg)
On the other side I'll check for max size because that's builtin limit
of the protocol, since the SCMI msg header contains a sequence number
bit-field limited to MSG_TOKEN_MAX: the SCMI can and will indeed limit
probably the effective maximum number of pending inflight messages to
some lower limit depending on the specific platform, but anyway is not
possible to have more than MSG_TOKEN_MAX inflight messages because there
are not enough bit in the header to represent the seq numbers.
Btw in this moment, in the core, the logic for the check of min size
I mentioned above is still under discussion.
So this is read in parallel, unless there's a lock I think you need an
smp_wmb here and WRITE_ONCE. Or maybe add locking instead so we
do not worry about ordering ...
Right, I'll use a barrier since I'd prefer to avoid introducing locking
on a global which is really written once at start and then only read
until the stack is shutdown and the virtio is removed.
This will stop processing buffers immediately.
You generally need to do something about outstanding
buffers though.
Yes, but in fact the complete_cb processes messages only when
vioch->ready is true: such flag is set to true on chan_setup()
after a successfull probe and reset to false in .chan_free() which is
called by the SCMI core well before this _remove is called; moreover
such vioch->ready flag is spinlocked so an outstanding message
possibly actively processed by complete_cb when chan_free is called
will be orderly and fully processed while any subsequent oustanding
message will hit the !ready flag and ignored, till the vqs are deleted
in the next step.
It certainly will help adding a comment about this.
Well doing this here does not prevent some users being outstanding.
If what I said above holds true, once we get here the channel is marked
NOT ready and any outstanding buffer would have been fully processed or
it will be totally ignored.
I.e. I see device_link_add above but we should do
device_link_del somewhere ...
So my understanding of this was that since the device_link_add() uses
DL_FLAG_AUTOREMOVE_CONSUMER the link deletion happens automatically once
the consumer (SCMI stack dev) unbinds on shutdown. (but I'm not so
familiar with these so I could be wrong, even though I have not seen any
anomaly on repeated load/unload and related re-inits of the whole stack)
If what I said above in general makes sense to You, I would post after
-rc1 some fixes for the issues you spotted (barrier / unneeded_irqsave /
ret_EBUSY / print _onces)
Thanks again for the review !
Cristian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel