From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 15FC5C433EF for ; Tue, 1 Feb 2022 22:07:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=x1Lj826+XwWPDGNf9Oza3mgcJqEceMlzPrkm77p9BBE=; b=h2ucXkvWV1GafT MRatxTFNl/Nw9uicExTEyO2kjygWYtSPPsyRO8gD7asEed4qKZICM54N6xllYz1Vwf/88JB8dxY7Z x09J7MrFED28NAbEmoo5VsNCt3sJM25+bP6cP3Yonj7qRuus0o8C07kOjQ7JVSv5eL85cIprhQzhE Hj4aZQhJJkV+88XszI3bpBTQgecbfIjjyoyyNv0hTqanMKSQTLAFybsNDU5Tjrkp2KB3Wg/A9sd1N vYBe8nIo486Z4+fNwsSHvOe2VNfzp71T+kYl8yRCwYBwRwwg2l/MkHPlDMPiojYfSXQ1k14unDHYH +cu/oLnK9zxfKfyQEMsg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nF1HQ-00DkOq-TB; Tue, 01 Feb 2022 22:05:41 +0000 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nF1HM-00DkNO-Sq for linux-arm-kernel@lists.infradead.org; Tue, 01 Feb 2022 22:05:39 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1643753135; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=R+PF+Jmgwz02XcLrLQyKWVtKMoZYNCPV2hRCDrmkX+U=; b=dKsxQk7mPUqVYcVG8U+2aneQq/oWxnUmH6Gz3Npg77oGDm2Bm4nNsUxUNs2mWKxWVwVecS sVfLZtcoMJv+Cmt6NGrhb0imDvCz0TCo1E1DZBj2oaOPfsejmbFN7aI3KqdtEvNZlmekPJ OfRtqqYbGJGhllcANJ1qarmbJq+bTWs= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-596-ii_abJ68OG-bgGFQN2UjPA-1; Tue, 01 Feb 2022 17:05:33 -0500 X-MC-Unique: ii_abJ68OG-bgGFQN2UjPA-1 Received: by mail-wm1-f70.google.com with SMTP id q125-20020a1c4383000000b003503e10c027so4104315wma.2 for ; Tue, 01 Feb 2022 14:05:33 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=R+PF+Jmgwz02XcLrLQyKWVtKMoZYNCPV2hRCDrmkX+U=; b=aNYPcKadrEE7+Pvor6MfGFvxlpPrNAQ9wBAVHKVSStsLuY87AWzpkxF8WxhPS1sSWS vw0jEoETVP8b1iqGjAbZdZnN9I8xBZWtVmZvScvMM+CO/+Eh0LwpvI2WnDBOpjqlplLj Ey0zVKxohS2kLEtvCN9e4xEJI6Si3Pts+fjo2Jzx7AfImvvS0g0NhU0p3cliaAPXqHDQ WcUtGhY3Vw4n5wMZAyg+SJ1vpgYXiGT8evh1LuJxahmTNSxlaf/nTRPYSoF0uolSio0F Iivay2cJucUVamvB+y7JFfI1zome0L1Iv6KieylCMZ4juRkSqvCevUjBhVFD/jNWUYY1 jmFQ== X-Gm-Message-State: AOAM531Syvf7nwlBywcSiHArAe0rt4j/O+hquqsJpTjOnMr/glC8y5nF v67oGZarih3A9gfVyazZwNsQUAJbpz4wqi9yImh0a8vACe8GEw7pnGNL6T4zIVg7O40VDmpRuZ7 JwMQV701j0GOep+KnxU+44duc1bXaUtoGqCU= X-Received: by 2002:a7b:c08b:: with SMTP id r11mr3541842wmh.111.1643753131906; Tue, 01 Feb 2022 14:05:31 -0800 (PST) X-Google-Smtp-Source: ABdhPJwMfMCE7dPkT2LWjrlDLo+rPCcb6cQo5NqhccAHmiLNCvv4S+IfI7Xeb2PxwV5fism+ziv1fA== X-Received: by 2002:a7b:c08b:: with SMTP id r11mr3541823wmh.111.1643753131629; Tue, 01 Feb 2022 14:05:31 -0800 (PST) Received: from redhat.com ([2.52.5.34]) by smtp.gmail.com with ESMTPSA id e13sm18813272wrq.35.2022.02.01.14.05.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 01 Feb 2022 14:05:31 -0800 (PST) Date: Tue, 1 Feb 2022 17:05:27 -0500 From: "Michael S. Tsirkin" To: Cristian Marussi Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.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, peter.hilber@opensynergy.com, igor.skalkin@opensynergy.com Subject: Re: [PATCH v2 1/9] firmware: arm_scmi: Add a virtio channel refcount Message-ID: <20220201165947-mutt-send-email-mst@kernel.org> References: <20220201171601.53316-1-cristian.marussi@arm.com> <20220201171601.53316-2-cristian.marussi@arm.com> MIME-Version: 1.0 In-Reply-To: <20220201171601.53316-2-cristian.marussi@arm.com> Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=mst@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220201_140537_068004_1C389FD0 X-CRM114-Status: GOOD ( 43.24 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Feb 01, 2022 at 05:15:53PM +0000, Cristian Marussi wrote: > Currently SCMI VirtIO channels are marked with a ready flag and related > lock to track channel lifetime and support proper synchronization at > shutdown when virtqueues have to be stopped. > > This leads to some extended spinlocked sections with IRQs off on the RX > path to keep hold of the ready flag and does not scale well especially when > SCMI VirtIO polling mode will be introduced. > > Add an SCMI VirtIO channel dedicated refcount to track active users on both > the TX and the RX path and properly enforce synchronization and cleanup at > shutdown, inhibiting further usage of the channel once freed. > > Cc: Igor Skalkin > Cc: Peter Hilber > Signed-off-by: Cristian Marussi > --- > drivers/firmware/arm_scmi/virtio.c | 148 +++++++++++++++++++---------- > 1 file changed, 100 insertions(+), 48 deletions(-) > > diff --git a/drivers/firmware/arm_scmi/virtio.c b/drivers/firmware/arm_scmi/virtio.c > index fd0f6f91fc0b..536e46eab462 100644 > --- a/drivers/firmware/arm_scmi/virtio.c > +++ b/drivers/firmware/arm_scmi/virtio.c > @@ -17,7 +17,9 @@ > * virtqueue. Access to each virtqueue is protected by spinlocks. > */ > > +#include > #include > +#include > #include > #include > #include > @@ -27,6 +29,7 @@ > > #include "common.h" > > +#define VIRTIO_MAX_RX_TIMEOUT_MS 60000 > #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) > @@ -39,23 +42,21 @@ > * @cinfo: SCMI Tx or Rx channel > * @free_list: List of unused scmi_vio_msg, maintained for Tx channels only > * @is_rx: Whether channel is an Rx channel > - * @ready: Whether transport user is ready to hear about channel > * @max_msg: Maximum number of pending messages for this channel. > - * @lock: Protects access to all members except ready. > - * @ready_lock: Protects access to ready. If required, it must be taken before > - * lock. > + * @lock: Protects access to all members except users. > + * @shutdown_done: A reference to a completion used when freeing this channel. > + * @users: A reference count to currently active users of this channel. > */ > struct scmi_vio_channel { > struct virtqueue *vqueue; > struct scmi_chan_info *cinfo; > struct list_head free_list; > bool is_rx; > - bool ready; > unsigned int max_msg; > - /* lock to protect access to all members except ready. */ > + /* lock to protect access to all members except users. */ > spinlock_t lock; > - /* lock to rotects access to ready flag. */ > - spinlock_t ready_lock; > + struct completion *shutdown_done; > + refcount_t users; > }; > > /** > @@ -76,6 +77,71 @@ struct scmi_vio_msg { > /* Only one SCMI VirtIO device can possibly exist */ > static struct virtio_device *scmi_vdev; > > +static void scmi_vio_channel_ready(struct scmi_vio_channel *vioch, > + struct scmi_chan_info *cinfo) > +{ > + unsigned long flags; > + > + 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); > + > + refcount_set(&vioch->users, 1); > +} > + > +static inline bool scmi_vio_channel_acquire(struct scmi_vio_channel *vioch) > +{ > + return refcount_inc_not_zero(&vioch->users); > +} > + > +static inline void scmi_vio_channel_release(struct scmi_vio_channel *vioch) > +{ > + if (refcount_dec_and_test(&vioch->users)) { > + unsigned long flags; > + > + spin_lock_irqsave(&vioch->lock, flags); > + if (vioch->shutdown_done) { > + vioch->cinfo = NULL; > + complete(vioch->shutdown_done); > + } > + spin_unlock_irqrestore(&vioch->lock, flags); > + } > +} > + > +static void scmi_vio_channel_cleanup_sync(struct scmi_vio_channel *vioch) > +{ > + int timeout; > + char *vq_name; > + unsigned long flags; > + struct device *dev; > + DECLARE_COMPLETION_ONSTACK(vioch_shutdown_done); > + > + /* > + * Prepare to wait for the last release if not already released > + * or in progress. > + */ > + spin_lock_irqsave(&vioch->lock, flags); > + if (!vioch->cinfo || vioch->shutdown_done) { > + spin_unlock_irqrestore(&vioch->lock, flags); > + return; > + } > + vioch->shutdown_done = &vioch_shutdown_done; > + vq_name = vioch->is_rx ? "RX" : "TX"; > + /* vioch->cinfo could be NULLified after the release */ > + dev = vioch->cinfo->dev; > + spin_unlock_irqrestore(&vioch->lock, flags); > + > + scmi_vio_channel_release(vioch); > + > + timeout = msecs_to_jiffies(VIRTIO_MAX_RX_TIMEOUT_MS + 10); > + /* Let any possibly concurrent RX path release the channel */ > + if (!wait_for_completion_timeout(vioch->shutdown_done, timeout)) > + dev_warn(dev, > + "Timeout shutting down %s VQ.\n", vq_name); > +} > + Hmm. So if it times out then what? It's ok to corrupt memory then? Why? I suspect if you want to recover from this you need to mark device as broken, synchronize with all callbacks (we don't have an API for that but we really should). Only then you will know it's not doing anything. > static bool scmi_vio_have_vq_rx(struct virtio_device *vdev) > { > return virtio_has_feature(vdev, VIRTIO_SCMI_F_P2A_CHANNELS); > @@ -119,7 +185,7 @@ static void scmi_finalize_message(struct scmi_vio_channel *vioch, > > static void scmi_vio_complete_cb(struct virtqueue *vqueue) > { > - unsigned long ready_flags; > + unsigned long flags; > unsigned int length; > struct scmi_vio_channel *vioch; > struct scmi_vio_msg *msg; > @@ -130,27 +196,27 @@ static void scmi_vio_complete_cb(struct virtqueue *vqueue) > vioch = &((struct scmi_vio_channel *)vqueue->vdev->priv)[vqueue->index]; > > for (;;) { > - spin_lock_irqsave(&vioch->ready_lock, ready_flags); > - > - if (!vioch->ready) { > + if (!scmi_vio_channel_acquire(vioch)) { > if (!cb_enabled) > (void)virtqueue_enable_cb(vqueue); > - goto unlock_ready_out; > + return; > } > > - /* IRQs already disabled here no need to irqsave */ > - spin_lock(&vioch->lock); > + 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)) > - goto unlock_out; > + if (virtqueue_enable_cb(vqueue)) { > + spin_unlock_irqrestore(&vioch->lock, flags); > + scmi_vio_channel_release(vioch); > + return; > + } > cb_enabled = true; > } > - spin_unlock(&vioch->lock); > + spin_unlock_irqrestore(&vioch->lock, flags); > > if (msg) { > msg->rx_len = length; > @@ -161,19 +227,14 @@ static void scmi_vio_complete_cb(struct virtqueue *vqueue) > } > > /* > - * Release ready_lock and re-enable IRQs between loop iterations > - * to allow virtio_chan_free() to possibly kick in and set the > - * flag vioch->ready to false even in between processing of > - * messages, so as to force outstanding messages to be ignored > - * when system is shutting down. > + * Release vio channel between loop iterations to allow > + * virtio_chan_free() to eventually fully release it when > + * shutting down; in such a case, any outstanding message will > + * be ignored since this loop will bail out at the next > + * iteration. > */ > - spin_unlock_irqrestore(&vioch->ready_lock, ready_flags); > + scmi_vio_channel_release(vioch); > } > - > -unlock_out: > - spin_unlock(&vioch->lock); > -unlock_ready_out: > - spin_unlock_irqrestore(&vioch->ready_lock, ready_flags); > } > > static const char *const scmi_vio_vqueue_names[] = { "tx", "rx" }; > @@ -273,35 +334,20 @@ static int virtio_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, > } > } > > - 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); > + scmi_vio_channel_ready(vioch, cinfo); > > return 0; > } > > static int virtio_chan_free(int id, void *p, void *data) > { > - unsigned long flags; > struct scmi_chan_info *cinfo = p; > struct scmi_vio_channel *vioch = cinfo->transport_info; > > - spin_lock_irqsave(&vioch->ready_lock, flags); > - vioch->ready = false; > - spin_unlock_irqrestore(&vioch->ready_lock, flags); > + scmi_vio_channel_cleanup_sync(vioch); > > scmi_free_channel(cinfo, data, id); > > - spin_lock_irqsave(&vioch->lock, flags); > - vioch->cinfo = NULL; > - spin_unlock_irqrestore(&vioch->lock, flags); > - > return 0; > } > > @@ -316,10 +362,14 @@ static int virtio_send_message(struct scmi_chan_info *cinfo, > int rc; > struct scmi_vio_msg *msg; > > + if (!scmi_vio_channel_acquire(vioch)) > + return -EINVAL; > + > spin_lock_irqsave(&vioch->lock, flags); > > if (list_empty(&vioch->free_list)) { > spin_unlock_irqrestore(&vioch->lock, flags); > + scmi_vio_channel_release(vioch); > return -EBUSY; > } > > @@ -342,6 +392,8 @@ static int virtio_send_message(struct scmi_chan_info *cinfo, > > spin_unlock_irqrestore(&vioch->lock, flags); > > + scmi_vio_channel_release(vioch); > + > return rc; > } > > @@ -416,7 +468,6 @@ static int scmi_vio_probe(struct virtio_device *vdev) > 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]; > > @@ -503,7 +554,8 @@ const struct scmi_desc scmi_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 */ > + /* for non-realtime virtio devices */ > + .max_rx_timeout_ms = VIRTIO_MAX_RX_TIMEOUT_MS, > .max_msg = 0, /* overridden by virtio_get_max_msg() */ > .max_msg_size = VIRTIO_SCMI_MAX_MSG_SIZE, > }; > -- > 2.17.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel