Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/805585/?format=api
{ "id": 805585, "url": "http://patchwork.ozlabs.org/api/patches/805585/?format=api", "web_url": "http://patchwork.ozlabs.org/project/skiboot/patch/20170824174736.909-1-oohall@gmail.com/", "project": { "id": 44, "url": "http://patchwork.ozlabs.org/api/projects/44/?format=api", "name": "skiboot firmware development", "link_name": "skiboot", "list_id": "skiboot.lists.ozlabs.org", "list_email": "skiboot@lists.ozlabs.org", "web_url": "http://github.com/open-power/skiboot", "scm_url": "http://github.com/open-power/skiboot", "webscm_url": "", "list_archive_url": "", "list_archive_url_format": "", "commit_url_format": "" }, "msgid": "<20170824174736.909-1-oohall@gmail.com>", "list_archive_url": null, "date": "2017-08-24T17:47:36", "name": "[v2] hw/p8-i2c: Fix OCC locking", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": false, "hash": "95be9a4a5950bb4c2623e3532692bd570f5ce14f", "submitter": { "id": 68108, "url": "http://patchwork.ozlabs.org/api/people/68108/?format=api", "name": "Oliver O'Halloran", "email": "oohall@gmail.com" }, "delegate": null, "mbox": "http://patchwork.ozlabs.org/project/skiboot/patch/20170824174736.909-1-oohall@gmail.com/mbox/", "series": [], "comments": "http://patchwork.ozlabs.org/api/patches/805585/comments/", "check": "pending", "checks": "http://patchwork.ozlabs.org/api/patches/805585/checks/", "tags": {}, "related": [], "headers": { "Return-Path": "<skiboot-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org>", "X-Original-To": [ "incoming@patchwork.ozlabs.org", "skiboot@lists.ozlabs.org" ], "Delivered-To": [ "patchwork-incoming@bilbo.ozlabs.org", "skiboot@lists.ozlabs.org" ], "Received": [ "from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xdWwj2wdNz9sDB\n\tfor <incoming@patchwork.ozlabs.org>;\n\tFri, 25 Aug 2017 03:48:05 +1000 (AEST)", "from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xdWwj0y8GzDrJr\n\tfor <incoming@patchwork.ozlabs.org>;\n\tFri, 25 Aug 2017 03:48:05 +1000 (AEST)", "from mail-pg0-x241.google.com (mail-pg0-x241.google.com\n\t[IPv6:2607:f8b0:400e:c05::241])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xdWwS4vN6zDrJN\n\tfor <skiboot@lists.ozlabs.org>; Fri, 25 Aug 2017 03:47:52 +1000 (AEST)", "by mail-pg0-x241.google.com with SMTP id t3so212336pgt.5\n\tfor <skiboot@lists.ozlabs.org>; Thu, 24 Aug 2017 10:47:52 -0700 (PDT)", "from flat-canetoad.au.ibm.com ([103.57.0.128])\n\tby smtp.gmail.com with ESMTPSA id\n\t196sm3891657pfy.93.2017.08.24.10.47.48\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tThu, 24 Aug 2017 10:47:49 -0700 (PDT)" ], "Authentication-Results": [ "ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"WHHdrn5m\"; dkim-atps=neutral", "lists.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"WHHdrn5m\"; dkim-atps=neutral", "lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"WHHdrn5m\"; dkim-atps=neutral" ], "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=from:to:cc:subject:date:message-id;\n\tbh=quR+xtGLt3ewD718cj7bWr+PjecRl7dzU9xONJt6PGk=;\n\tb=WHHdrn5mROt9yF25H4L0J+d2fQMX95SSXJslLu9+FFKRlpCtzE1/8QHr2/eB0HG2os\n\tkJVjQ/wv5qJ7ebl5Rl5j01GcLezEzZAmLxqnKUg5vC5AlOe07ynolMQ1wgXBGognt2gJ\n\tdTe0FDpxVSDYt1DT3SJfFyml6M519L817a32CiQAfUGDobGE+UTluWxYL0GjVMDaHx8c\n\tyVWKZaUOB25oPrI2KRmBrUFc1puLeVqgWu5sb5aWnSmv45Edzwnegd2W9JdAlswXsDnL\n\t6bdsoBk9ufgalqtvMAPOuKL2aVN4Nt0baw5bbdVJdecpoQkkUnZK6YOlOlruDaKVm+L7\n\tR95Q==", "X-Google-DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:from:to:cc:subject:date:message-id;\n\tbh=quR+xtGLt3ewD718cj7bWr+PjecRl7dzU9xONJt6PGk=;\n\tb=IGAVmfuNvTOiArFNTQoJR+Psz+rx7NtF70HNnpSAx5qNBUjXmt4oHRZ/SUminFMgOa\n\tEDRJ9fYAmI7Ebver0ke6pOucaowQ1a8fl69vyVuMg8GbwhLzH9y+CSW73jHnJvJORlEW\n\t0e7px3zmf7YWHvhrnRl4DcUxmvP7jJk5c1WRdaZYb+4sQvcqmMba1OL7C6oT/CiwjN0k\n\tGkHpBum0IEburkwI5Y4GiIounBtHb7kfrARrEnM8ssP3UQ+QJmlKqXvdSNhIMghf6/JF\n\tlNzc4LlUXD6/ca/JQfZmYR1BaEWbzFnZ6GfX5+bevyHW3fregGPE4SmisbBrBnwzayY4\n\tm/Jw==", "X-Gm-Message-State": "AHYfb5iE3DyciNajp6+5S+xT6ir4WmFpcBgXHoGpEhy6xzw4y8IktPlm\n\t5fmTALpvB3qUhLME", "X-Received": "by 10.99.107.3 with SMTP id g3mr7052074pgc.172.1503596870312;\n\tThu, 24 Aug 2017 10:47:50 -0700 (PDT)", "From": "Oliver O'Halloran <oohall@gmail.com>", "To": "skiboot@lists.ozlabs.org", "Date": "Fri, 25 Aug 2017 03:47:36 +1000", "Message-Id": "<20170824174736.909-1-oohall@gmail.com>", "X-Mailer": "git-send-email 2.9.5", "Subject": "[Skiboot] [PATCH v2] hw/p8-i2c: Fix OCC locking", "X-BeenThere": "skiboot@lists.ozlabs.org", "X-Mailman-Version": "2.1.23", "Precedence": "list", "List-Id": "Mailing list for skiboot development <skiboot.lists.ozlabs.org>", "List-Unsubscribe": "<https://lists.ozlabs.org/options/skiboot>,\n\t<mailto:skiboot-request@lists.ozlabs.org?subject=unsubscribe>", "List-Archive": "<http://lists.ozlabs.org/pipermail/skiboot/>", "List-Post": "<mailto:skiboot@lists.ozlabs.org>", "List-Help": "<mailto:skiboot-request@lists.ozlabs.org?subject=help>", "List-Subscribe": "<https://lists.ozlabs.org/listinfo/skiboot>,\n\t<mailto:skiboot-request@lists.ozlabs.org?subject=subscribe>", "MIME-Version": "1.0", "Content-Type": "text/plain; charset=\"utf-8\"", "Content-Transfer-Encoding": "base64", "Errors-To": "skiboot-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org", "Sender": "\"Skiboot\"\n\t<skiboot-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org>" }, "content": "There's a few issues with the Host<->OCC I2C bus handshaking. First up,\nskiboot is currently examining the wrong bit when checking if the OCC\nis currently using the bus. Secondly, when we need to wait for the OCC\nto release the bus we are scheduling a recovery timer to run zero\ntimebase ticks after the current moment so the recovery timeout handler\nwill run immediately after the bus was requested, which will in turn\nre-schedule itself, etc, etc. There's also a race between the OCC\ninterrupt and the recovery handler which can result in an assertion\nfailure in the recovery thread. All of this is bad.\n\nThis patch addresses all these issues and sets the recovery timeout to\n10ms.\n\nSigned-off-by: Oliver O'Halloran <oohall@gmail.com>\n---\n hw/p8-i2c.c | 58 +++++++++++++++++++++++++++++++++++++---------------------\n 1 file changed, 37 insertions(+), 21 deletions(-)\n---\nI've been using this script to test the changes. With this patch it's fairly\nstable, but there's still intermittent data corruption issues when\nwhen reading from the bus which I'm still investigating.\n---\n#!/bin/bash -e\n\nwhile true;\ndo\n\tfor f in $(find /sys/devices/platform/ -name '*-????'|grep a3000 )\n\tdo\n\t\tbus=\"$(basename $f | awk -F - -- '{ print $1 }' )\"\n\t\tdev=\"$(basename $f | awk -F - -- '{ print $2 }' )\"\n\n\t\ti2cdump -y $bus 0x$dev i > $bus-$dev\n\t\tmd5sum $bus-$dev\n\t\tsleep 0.1\n\tdone\ndone\n---", "diff": "diff --git a/hw/p8-i2c.c b/hw/p8-i2c.c\nindex f4666e24953e..a6274171debb 100644\n--- a/hw/p8-i2c.c\n+++ b/hw/p8-i2c.c\n@@ -1063,8 +1063,11 @@ static int occ_i2c_lock(struct p8_i2c_master *master)\n \n \tbusflag = PPC_BIT(16 + (master->engine_id - 1) * 2);\n \n-\tDBG(\"occflags = %llx (locks = %.6llx)\\n\", (u64)occflags,\n-\t GETFIELD(PPC_BITMASK(16, 22), occflags));\n+\tDBG(\"I2C: c%de%d: occflags = %llx (locks = %x:%x:%x)\\n\",\n+\t\tmaster->chip_id, master->engine_id, (u64) occflags,\n+\t\t(u32) GETFIELD(PPC_BITMASK(16, 17), occflags),\n+\t\t(u32) GETFIELD(PPC_BITMASK(18, 19), occflags),\n+\t\t(u32) GETFIELD(PPC_BITMASK(20, 21), occflags));\n \n \trc = xscom_write(master->chip_id, OCCFLG_SET, busflag);\n \tif (rc) {\n@@ -1073,8 +1076,11 @@ static int occ_i2c_lock(struct p8_i2c_master *master)\n \t}\n \n \t/* If the OCC also has this bus locked then wait for IRQ */\n-\tif (occflags & (busflag << 1))\n+\tif (occflags & (busflag >> 1)) {\n+\t\tDBG(\"I2C: c%de%d: Master in use by OCC\\n\",\n+\t\t\tmaster->chip_id, master->engine_id);\n \t\treturn 1;\n+\t}\n \n \tmaster->occ_lock_acquired = true;\n \n@@ -1098,8 +1104,8 @@ static int occ_i2c_unlock(struct p8_i2c_master *master)\n \tbusflag = PPC_BIT(16 + (master->engine_id - 1) * 2);\n \n \tif (!(occflags & busflag)) {\n-\t\tprerror(\"I2C: busflag for %d already cleared (flags = %.16llx)\",\n-\t\t\tmaster->engine_id, occflags);\n+\t\tDBG(\"I2C: spurious unlock for c%de%d already cleared (flags = %.16llx)\",\n+\t\t\tmaster->chip_id, master->engine_id, occflags);\n \t}\n \n \trc = xscom_write(master->chip_id, OCCFLG_CLEAR, busflag);\n@@ -1161,7 +1167,7 @@ static int p8_i2c_start_request(struct p8_i2c_master *master,\n \tif (rc > 0) {\n \t\t/* Wait for OCC IRQ */\n \t\tmaster->state = state_occache_dis;\n-\t\tschedule_timer(&master->recovery, rc);\n+\t\tschedule_timer(&master->recovery, msecs_to_tb(10));\n \t\treturn 0;\n \t}\n \n@@ -1281,29 +1287,29 @@ void p9_i2c_bus_owner_change(u32 chip_id)\n {\n \tstruct proc_chip *chip = get_chip(chip_id);\n \tstruct p8_i2c_master *master = NULL;\n-\tint rc;\n \n \tassert(chip);\n \tlist_for_each(&chip->i2cms, master, link) {\n-\t\tif (master->state == state_idle ||\n-\t\t master->state != state_occache_dis)\n-\t\t\tcontinue;\n-\n \t\tlock(&master->lock);\n \n+\t\t/* spurious */\n+\t\tif (master->state != state_occache_dis)\n+\t\t\tgoto done;\n+\n \t\t/* Can we now lock this master? */\n-\t\trc = occ_i2c_lock(master);\n-\t\tif (rc) {\n-\t\t\tunlock(&master->lock);\n-\t\t\tcontinue;\n-\t\t}\n+\t\tif (occ_i2c_lock(master))\n+\t\t\tgoto done;\n \n-\t\t/* Run the state machine */\n-\t\tp8_i2c_check_status(master);\n+\t\t/* clear the existing wait timer */\n+\t\tcancel_timer(&master->recovery);\n+\n+\t\t/* re-start the request now that we own the master */\n+\t\tmaster->state = state_idle;\n \n-\t\t/* Check for new work */\n \t\tp8_i2c_check_work(master);\n+\t\tp8_i2c_check_status(master);\n \n+done:\n \t\tunlock(&master->lock);\n \t}\n }\n@@ -1453,8 +1459,18 @@ static void p8_i2c_recover(struct timer *t __unused, void *data,\n \tstruct p8_i2c_master *master = data;\n \n \tlock(&master->lock);\n-\tassert(master->state == state_recovery ||\n-\t master->state == state_occache_dis);\n+\n+\t/*\n+\t * The recovery timer can race with the OCC interrupt. If the interrupt\n+\t * comes in just before this is called, then we'll get a spurious\n+\t * timeout which we need to ignore.\n+\t */\n+\tif (master->state != state_recovery &&\n+\t\tmaster->state != state_occache_dis) {\n+\t\tunlock(&master->lock);\n+\t\treturn;\n+\t}\n+\n \tmaster->state = state_idle;\n \n \t/* We may or may not still have work pending, re-enable the sensor cache\n", "prefixes": [ "v2" ] }