All of lore.kernel.org
 help / color / mirror / Atom feed
From: Vinod Koul <vkoul@kernel.org>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: Bard Liao <yung-chuan.liao@linux.intel.com>,
	alsa-devel@alsa-project.org, tiwai@suse.de,
	gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
	ranjani.sridharan@linux.intel.com, hui.wang@canonical.com,
	broonie@kernel.org, srinivas.kandagatla@linaro.org,
	jank@cadence.com, sanyog.r.kale@intel.com,
	rander.wang@linux.intel.com, bard.liao@intel.com
Subject: Re: [PATCH 1/7] soundwire: bus: use sdw_update_no_pm when initializing a device
Date: Mon, 7 Dec 2020 10:13:34 +0530	[thread overview]
Message-ID: <20201207044334.GA8403@vkoul-mobl> (raw)
In-Reply-To: <1db93c2e-3c87-bc5e-ddeb-56424870b897@linux.intel.com>

On 05-12-20, 08:59, Pierre-Louis Bossart wrote:
> Thanks for the review Vinod.
> 
> On 12/5/20 1:45 AM, Vinod Koul wrote:
> > On 03-12-20, 04:46, Bard Liao wrote:
> > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > 
> > > When a Slave device is resumed, it may resume the bus and restart the
> > > enumeration. During that process, we absolutely don't want to call
> > > regular read/write routines which will wait for the resume to
> > > complete, otherwise a deadlock occurs.
> > > 
> > > Fixes: 60ee9be25571 ('soundwire: bus: add PM/no-PM versions of read/write functions')
> > 
> > Change looks okay, but not sure why this is a fix for adding no pm
> > version?
> 
> when we added the no_pm version, we missed the two cases below where
> sdw_update() was used and that creates a deadlock. To me that's a conceptual
> bug, we didn't fully use the no_pm versions, hence the Fixes tag.

Documentation says:
"A Fixes: tag indicates that the patch fixes an issue in a previous commit. It
is used to make it easy to determine where a bug originated, which can help
review a bug fix. This tag also assists the stable kernel team in determining
which stable kernel versions should receive your fix. This is the preferred
method for indicating a bug fixed by the patch. See :ref:`describe_changes`
for more details."

I do not this this helps here as this does not help distros etc
I would say this is incremental development which improved a case not
done properly earlier, unless you would like this to be backported.. In
that case it helps folks...

> It's ok to remove the tag if you don't think it's useful/relevant, what
> matters is that we agree on the content.

-- 
~Vinod

WARNING: multiple messages have this Message-ID (diff)
From: Vinod Koul <vkoul@kernel.org>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: alsa-devel@alsa-project.org, tiwai@suse.de,
	gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
	ranjani.sridharan@linux.intel.com, hui.wang@canonical.com,
	broonie@kernel.org, srinivas.kandagatla@linaro.org,
	jank@cadence.com, sanyog.r.kale@intel.com,
	Bard Liao <yung-chuan.liao@linux.intel.com>,
	rander.wang@linux.intel.com, bard.liao@intel.com
Subject: Re: [PATCH 1/7] soundwire: bus: use sdw_update_no_pm when initializing a device
Date: Mon, 7 Dec 2020 10:13:34 +0530	[thread overview]
Message-ID: <20201207044334.GA8403@vkoul-mobl> (raw)
In-Reply-To: <1db93c2e-3c87-bc5e-ddeb-56424870b897@linux.intel.com>

On 05-12-20, 08:59, Pierre-Louis Bossart wrote:
> Thanks for the review Vinod.
> 
> On 12/5/20 1:45 AM, Vinod Koul wrote:
> > On 03-12-20, 04:46, Bard Liao wrote:
> > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > 
> > > When a Slave device is resumed, it may resume the bus and restart the
> > > enumeration. During that process, we absolutely don't want to call
> > > regular read/write routines which will wait for the resume to
> > > complete, otherwise a deadlock occurs.
> > > 
> > > Fixes: 60ee9be25571 ('soundwire: bus: add PM/no-PM versions of read/write functions')
> > 
> > Change looks okay, but not sure why this is a fix for adding no pm
> > version?
> 
> when we added the no_pm version, we missed the two cases below where
> sdw_update() was used and that creates a deadlock. To me that's a conceptual
> bug, we didn't fully use the no_pm versions, hence the Fixes tag.

Documentation says:
"A Fixes: tag indicates that the patch fixes an issue in a previous commit. It
is used to make it easy to determine where a bug originated, which can help
review a bug fix. This tag also assists the stable kernel team in determining
which stable kernel versions should receive your fix. This is the preferred
method for indicating a bug fixed by the patch. See :ref:`describe_changes`
for more details."

I do not this this helps here as this does not help distros etc
I would say this is incremental development which improved a case not
done properly earlier, unless you would like this to be backported.. In
that case it helps folks...

> It's ok to remove the tag if you don't think it's useful/relevant, what
> matters is that we agree on the content.

-- 
~Vinod

  reply	other threads:[~2020-12-07  4:44 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-02 20:46 [PATCH 0/7] soundwire/regmap: use _no_pm routines Bard Liao
2020-12-02 20:46 ` Bard Liao
2020-12-02 20:46 ` [PATCH 1/7] soundwire: bus: use sdw_update_no_pm when initializing a device Bard Liao
2020-12-02 20:46   ` Bard Liao
2020-12-05  7:45   ` Vinod Koul
2020-12-05  7:45     ` Vinod Koul
2020-12-05 14:59     ` Pierre-Louis Bossart
2020-12-07  4:43       ` Vinod Koul [this message]
2020-12-07  4:43         ` Vinod Koul
2020-12-07 15:31         ` Pierre-Louis Bossart
2020-12-07 15:31           ` Pierre-Louis Bossart
2020-12-08  4:56           ` Vinod Koul
2020-12-08  4:56             ` Vinod Koul
2020-12-02 20:46 ` [PATCH 2/7] soundwire: bus: use sdw_write_no_pm when setting the bus scale registers Bard Liao
2020-12-02 20:46   ` Bard Liao
2020-12-02 20:46 ` [PATCH 3/7] soundwire: bus: use no_pm IO routines for all interrupt handling Bard Liao
2020-12-02 20:46   ` Bard Liao
2020-12-02 20:46 ` [PATCH 4/7] soundwire/regmap: use _no_pm functions in regmap_read/write Bard Liao
2020-12-02 20:46   ` Bard Liao
2020-12-05  7:45   ` Vinod Koul
2020-12-05  7:45     ` Vinod Koul
2020-12-05 14:43     ` Pierre-Louis Bossart
2020-12-02 20:46 ` [PATCH 5/7] regmap: sdw: use no_pm routines for SoundWire 1.2 MBQ Bard Liao
2020-12-02 20:46   ` Bard Liao
2020-12-05  7:46   ` Vinod Koul
2020-12-05  7:46     ` Vinod Koul
2020-12-05 14:52     ` Pierre-Louis Bossart
2020-12-05 16:31       ` Greg KH
2020-12-05 16:31         ` Greg KH
2020-12-07  4:47         ` Vinod Koul
2020-12-07  4:47           ` Vinod Koul
2020-12-02 20:46 ` [PATCH 6/7] soundwire: bus: fix confusion on device used by pm_runtime Bard Liao
2020-12-02 20:46   ` Bard Liao
2020-12-02 20:46 ` [PATCH 7/7] soundwire: bus: clarify dev_err/dbg device references Bard Liao
2020-12-02 20:46   ` Bard Liao

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=20201207044334.GA8403@vkoul-mobl \
    --to=vkoul@kernel.org \
    --cc=alsa-devel@alsa-project.org \
    --cc=bard.liao@intel.com \
    --cc=broonie@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hui.wang@canonical.com \
    --cc=jank@cadence.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pierre-louis.bossart@linux.intel.com \
    --cc=rander.wang@linux.intel.com \
    --cc=ranjani.sridharan@linux.intel.com \
    --cc=sanyog.r.kale@intel.com \
    --cc=srinivas.kandagatla@linaro.org \
    --cc=tiwai@suse.de \
    --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.