From patchwork Thu Jun 29 12:39:19 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Cyril Bur X-Patchwork-Id: 782230 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3wz0120qBBz9s3T for ; Thu, 29 Jun 2017 22:52:06 +1000 (AEST) Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 3wz01173kxzDrDD for ; Thu, 29 Jun 2017 22:52:05 +1000 (AEST) X-Original-To: skiboot@lists.ozlabs.org Delivered-To: skiboot@lists.ozlabs.org Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3wyzlV5N2RzDr5M for ; Thu, 29 Jun 2017 22:40:22 +1000 (AEST) Received: from pps.filterd (m0098414.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v5TCd6kb112197 for ; Thu, 29 Jun 2017 08:40:17 -0400 Received: from e23smtp08.au.ibm.com (e23smtp08.au.ibm.com [202.81.31.141]) by mx0b-001b2d01.pphosted.com with ESMTP id 2bcv37ebvh-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Thu, 29 Jun 2017 08:40:16 -0400 Received: from localhost by e23smtp08.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 29 Jun 2017 22:40:08 +1000 Received: from d23relay09.au.ibm.com (202.81.31.228) by e23smtp08.au.ibm.com (202.81.31.205) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Thu, 29 Jun 2017 22:40:04 +1000 Received: from d23av02.au.ibm.com (d23av02.au.ibm.com [9.190.235.138]) by d23relay09.au.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v5TCe25J66977996 for ; Thu, 29 Jun 2017 22:40:02 +1000 Received: from d23av02.au.ibm.com (localhost [127.0.0.1]) by d23av02.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id v5TCdsWA026518 for ; Thu, 29 Jun 2017 22:39:54 +1000 Received: from ozlabs.au.ibm.com (ozlabs.au.ibm.com [9.192.253.14]) by d23av02.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id v5TCdsqv026512; Thu, 29 Jun 2017 22:39:54 +1000 Received: from camb691.ozlabs.ibm.com (haven.au.ibm.com [9.192.254.114]) (using TLSv1.2 with cipher DHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ozlabs.au.ibm.com (Postfix) with ESMTPSA id EFCBDA0286; Thu, 29 Jun 2017 22:40:01 +1000 (AEST) From: Cyril Bur To: skiboot@lists.ozlabs.org, sjitindarsingh@gmail.com Date: Thu, 29 Jun 2017 22:39:19 +1000 X-Mailer: git-send-email 2.13.2 In-Reply-To: <20170629123925.28243-1-cyril.bur@au1.ibm.com> References: <20170629123925.28243-1-cyril.bur@au1.ibm.com> X-TM-AS-MML: disable x-cbid: 17062912-0048-0000-0000-0000024D2837 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17062912-0049-0000-0000-000047FE27F9 Message-Id: <20170629123925.28243-6-cyril.bur@au1.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2017-06-29_09:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1706290207 Subject: [Skiboot] [PATCH 05/11] hw/lpc-mbox: Simplify message bookkeeping and timeouts X-BeenThere: skiboot@lists.ozlabs.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Mailing list for skiboot development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: alistair@popple.id.au, sam@mendozajonas.com MIME-Version: 1.0 Errors-To: skiboot-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "Skiboot" Currently the hw/lpc-mbox layer keeps a pointer for the currently inflight message for the duration of the mbox call. This creates problems when messages timeout, is that pointer still valid, what can we do with it. The memory is owned by the caller but if the caller has declared a timeout, it may have freed that memory. Another problem is locking. This patch also locks around sending and receiving to avoid races with timeouts and possible resends. There was some locking previously which was likely insufficient - definitely too hard to be sure is correct All this is made much easier with the previous rework which moves sequence number allocation and verification into lpc-mbox rather than the caller. Signed-off-by: Cyril Bur --- hw/lpc-mbox.c | 38 +++++++++++++++----------------------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/hw/lpc-mbox.c b/hw/lpc-mbox.c index f303f4d5..4b1d5e52 100644 --- a/hw/lpc-mbox.c +++ b/hw/lpc-mbox.c @@ -61,8 +61,7 @@ struct mbox { void *drv_data; void (*attn)(uint8_t bits, void *priv); void *attn_data; - struct lock lock; /* Protect in_flight */ - struct bmc_mbox_msg *in_flight; + struct lock lock; uint8_t sequence; unsigned long timeout; }; @@ -124,7 +123,7 @@ int bmc_mbox_enqueue(struct bmc_mbox_msg *msg, unsigned int timeout_sec) } lock(&mbox.lock); - if (mbox.in_flight) { + if (mbox.timeout) { prlog(PR_DEBUG, "MBOX message already in flight\n"); if (mftb() > mbox.timeout) { prlog(PR_ERR, "In flight message dropped on the floor\n"); @@ -135,11 +134,10 @@ int bmc_mbox_enqueue(struct bmc_mbox_msg *msg, unsigned int timeout_sec) } mbox.timeout = mftb() + secs_to_tb(timeout_sec); - mbox.in_flight = msg; - unlock(&mbox.lock); msg->seq = ++mbox.sequence; bmc_mbox_send_message(msg); + unlock(&mbox.lock); schedule_timer(&mbox.poller, mbox.irq_ok ? TIMER_POLL : msecs_to_tb(MBOX_DEFAULT_POLL_MS)); @@ -150,42 +148,34 @@ int bmc_mbox_enqueue(struct bmc_mbox_msg *msg, unsigned int timeout_sec) static void mbox_poll(struct timer *t __unused, void *data __unused, uint64_t now __unused) { - struct bmc_mbox_msg *msg; + struct bmc_mbox_msg msg; + + if (!lpc_ok()) + return; /* * This status bit being high means that someone touched the * response byte (byte 13). * There is probably a response for the previously sent commant */ + lock(&mbox.lock); if (bmc_mbox_inb(MBOX_STATUS_1) & MBOX_STATUS_1_RESP) { /* W1C on that reg */ bmc_mbox_outb(MBOX_STATUS_1_RESP, MBOX_STATUS_1); prlog(PR_INSANE, "Got a regular interrupt\n"); - /* - * This should be safe lockless - */ - msg = mbox.in_flight; - if (msg == NULL) { - prlog(PR_CRIT, "Couldn't find the message!!\n"); - goto out_response; - } - bmc_mbox_recv_message(msg); - if (mbox.sequence != msg->seq) { + bmc_mbox_recv_message(&msg); + if (mbox.sequence != msg.seq) { prlog(PR_ERR, "Got a response to a message we no longer care about\n"); goto out_response; } + mbox.timeout = 0; if (mbox.callback) - mbox.callback(msg, mbox.drv_data); + mbox.callback(&msg, mbox.drv_data); else prlog(PR_ERR, "Detected NULL callback for mbox message\n"); - - /* Yeah we'll need locks here */ - lock(&mbox.lock); - mbox.in_flight = NULL; - unlock(&mbox.lock); } out_response: @@ -230,6 +220,8 @@ out_response: mbox.attn(all, mbox.attn_data); } + unlock(&mbox.lock); + schedule_timer(&mbox.poller, mbox.irq_ok ? TIMER_POLL : msecs_to_tb(MBOX_DEFAULT_POLL_MS)); } @@ -341,9 +333,9 @@ void mbox_init(void) bmc_mbox_outb(MBOX_STATUS_1_RESP | MBOX_STATUS_1_ATTN, MBOX_STATUS_1); mbox.queue_len = 0; - mbox.in_flight = NULL; mbox.callback = NULL; mbox.drv_data = NULL; + mbox.timeout = 0; mbox.sequence = 0; init_lock(&mbox.lock);