All of lore.kernel.org
 help / color / mirror / Atom feed
From: Richard Fitzgerald <rf@opensource.cirrus.com>
To: <vkoul@kernel.org>, <yung-chuan.liao@linux.intel.com>,
	<pierre-louis.bossart@linux.intel.com>, <sanyog.r.kale@intel.com>
Cc: patches@opensource.cirrus.com, alsa-devel@alsa-project.org,
	Richard Fitzgerald <rf@opensource.cirrus.com>,
	linux-kernel@vger.kernel.org
Subject: [PATCH v2 3/5] soundwire: bus: Don't re-enumerate before status is UNATTACHED
Date: Wed, 7 Sep 2022 09:52:57 +0100	[thread overview]
Message-ID: <20220907085259.3602-4-rf@opensource.cirrus.com> (raw)
In-Reply-To: <20220907085259.3602-1-rf@opensource.cirrus.com>

Don't re-enumerate a peripheral on #0 until we have seen and
handled an UNATTACHED notification for that peripheral.

Without this, it is possible for the UNATTACHED status to be missed
and so the slave->status remains at ATTACHED. If slave->status never
changes to UNATTACHED the child driver will never be notified of the
UNATTACH, and the code in sdw_handle_slave_status() will skip the
second part of enumeration because the slave->status has not changed.

This scenario can happen because PINGs are handled in a workqueue
function which is working from a snapshot of an old PING, and there
is no guarantee when this function will run.

A peripheral could report attached in the PING being handled by
sdw_handle_slave_status(), but has since reverted to device #0 and is
then found in the loop in sdw_program_device_num(). Previously the
code would not have updated slave->status to UNATTACHED because it had
not yet handled a PING where that peripheral had UNATTACHED.

This situation happens fairly frequently with multiple peripherals on
a bus that are intentionally reset (for example after downloading
firmware).

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 drivers/soundwire/bus.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 1cc858b4107d..6e569a875a9b 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -773,6 +773,16 @@ static int sdw_program_device_num(struct sdw_bus *bus)
 			if (sdw_compare_devid(slave, id) == 0) {
 				found = true;
 
+				/*
+				 * To prevent skipping state-machine stages don't
+				 * program a device until we've seen it UNATTACH.
+				 * Must return here because no other device on #0
+				 * can be detected until this one has been
+				 * assigned a device ID.
+				 */
+				if (slave->status != SDW_SLAVE_UNATTACHED)
+					return 0;
+
 				/*
 				 * Assign a new dev_num to this Slave and
 				 * not mark it present. It will be marked
-- 
2.30.2


WARNING: multiple messages have this Message-ID (diff)
From: Richard Fitzgerald <rf@opensource.cirrus.com>
To: <vkoul@kernel.org>, <yung-chuan.liao@linux.intel.com>,
	<pierre-louis.bossart@linux.intel.com>, <sanyog.r.kale@intel.com>
Cc: <alsa-devel@alsa-project.org>, <linux-kernel@vger.kernel.org>,
	<patches@opensource.cirrus.com>,
	Richard Fitzgerald <rf@opensource.cirrus.com>
Subject: [PATCH v2 3/5] soundwire: bus: Don't re-enumerate before status is UNATTACHED
Date: Wed, 7 Sep 2022 09:52:57 +0100	[thread overview]
Message-ID: <20220907085259.3602-4-rf@opensource.cirrus.com> (raw)
In-Reply-To: <20220907085259.3602-1-rf@opensource.cirrus.com>

Don't re-enumerate a peripheral on #0 until we have seen and
handled an UNATTACHED notification for that peripheral.

Without this, it is possible for the UNATTACHED status to be missed
and so the slave->status remains at ATTACHED. If slave->status never
changes to UNATTACHED the child driver will never be notified of the
UNATTACH, and the code in sdw_handle_slave_status() will skip the
second part of enumeration because the slave->status has not changed.

This scenario can happen because PINGs are handled in a workqueue
function which is working from a snapshot of an old PING, and there
is no guarantee when this function will run.

A peripheral could report attached in the PING being handled by
sdw_handle_slave_status(), but has since reverted to device #0 and is
then found in the loop in sdw_program_device_num(). Previously the
code would not have updated slave->status to UNATTACHED because it had
not yet handled a PING where that peripheral had UNATTACHED.

This situation happens fairly frequently with multiple peripherals on
a bus that are intentionally reset (for example after downloading
firmware).

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 drivers/soundwire/bus.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 1cc858b4107d..6e569a875a9b 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -773,6 +773,16 @@ static int sdw_program_device_num(struct sdw_bus *bus)
 			if (sdw_compare_devid(slave, id) == 0) {
 				found = true;
 
+				/*
+				 * To prevent skipping state-machine stages don't
+				 * program a device until we've seen it UNATTACH.
+				 * Must return here because no other device on #0
+				 * can be detected until this one has been
+				 * assigned a device ID.
+				 */
+				if (slave->status != SDW_SLAVE_UNATTACHED)
+					return 0;
+
 				/*
 				 * Assign a new dev_num to this Slave and
 				 * not mark it present. It will be marked
-- 
2.30.2


  parent reply	other threads:[~2022-09-07  8:54 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-07  8:52 [PATCH v2 0/5] soundwire: Fixes for spurious and missing UNATTACH Richard Fitzgerald
2022-09-07  8:52 ` Richard Fitzgerald
2022-09-07  8:52 ` [PATCH v2 1/5] soundwire: cadence: fix updating slave status when a bus has multiple peripherals Richard Fitzgerald
2022-09-07  8:52   ` Richard Fitzgerald
2022-09-07  8:52 ` [PATCH v2 2/5] soundwire: bus: Don't lose unattach notifications Richard Fitzgerald
2022-09-07  8:52   ` Richard Fitzgerald
2022-09-07  8:52 ` Richard Fitzgerald [this message]
2022-09-07  8:52   ` [PATCH v2 3/5] soundwire: bus: Don't re-enumerate before status is UNATTACHED Richard Fitzgerald
2022-09-12 11:00   ` Pierre-Louis Bossart
2022-09-12 11:00     ` Pierre-Louis Bossart
2022-09-07  8:52 ` [PATCH v2 4/5] soundwire: cadence: Fix lost ATTACHED interrupts when enumerating Richard Fitzgerald
2022-09-07  8:52   ` Richard Fitzgerald
2022-09-12 11:05   ` Pierre-Louis Bossart
2022-09-12 11:05     ` Pierre-Louis Bossart
2022-09-12 12:36     ` Richard Fitzgerald
2022-09-12 12:36       ` Richard Fitzgerald
2022-09-07  8:52 ` [PATCH v2 5/5] soundwire: bus: Don't exit early if no device IDs were programmed Richard Fitzgerald
2022-09-07  8:52   ` Richard Fitzgerald
2022-09-12 11:43   ` Pierre-Louis Bossart
2022-09-12 12:25     ` Richard Fitzgerald
2022-09-12 12:25       ` Richard Fitzgerald
2022-09-12 17:09       ` Pierre-Louis Bossart
2022-09-13 15:30         ` Richard Fitzgerald
2022-09-13 15:30           ` Richard Fitzgerald
2022-09-13 17:59           ` Pierre-Louis Bossart

Reply instructions:

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

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

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

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

  git send-email \
    --in-reply-to=20220907085259.3602-4-rf@opensource.cirrus.com \
    --to=rf@opensource.cirrus.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=patches@opensource.cirrus.com \
    --cc=pierre-louis.bossart@linux.intel.com \
    --cc=sanyog.r.kale@intel.com \
    --cc=vkoul@kernel.org \
    --cc=yung-chuan.liao@linux.intel.com \
    /path/to/YOUR_REPLY

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

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.