Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 21 Aug 2020 10:45:52 +0530
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, linux-kernel@vger.kernel.org,
        tiwai@suse.de, broonie@kernel.org, gregkh@linuxfoundation.org,
        jank@cadence.com, srinivas.kandagatla@linaro.org,
        rander.wang@linux.intel.com, ranjani.sridharan@linux.intel.com,
        hui.wang@canonical.com, sanyog.r.kale@intel.com,
        mengdong.lin@intel.com, bard.liao@intel.com
Subject: Re: [PATCH 2/2] soundwire: fix port_ready[] dynamic allocation in
 mipi_disco and ASoC codecs
Message-ID: <20200821051552.GK2639@vkoul-mobl>
References: <20200817174727.15139-1-yung-chuan.liao@linux.intel.com>
 <20200817174727.15139-3-yung-chuan.liao@linux.intel.com>
 <20200818063659.GW2639@vkoul-mobl>
 <e9f12e13-49e0-5306-a975-b1b854baef02@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e9f12e13-49e0-5306-a975-b1b854baef02@linux.intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 18-08-20, 07:09, Pierre-Louis Bossart wrote:
> 
> 
> On 8/18/20 1:36 AM, Vinod Koul wrote:
> > On 18-08-20, 01:47, Bard Liao wrote:
> > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > 
> > > The existing code allocates memory for the total number of ports.
> > > This only works if the ports are contiguous, but will break if e.g. a
> > > Devices uses port0, 1, and 14. The port_ready[] array would contain 3
> > > elements, which would lead to an out-of-bounds access. Conversely in
> > > other cases, the wrong port index would be used leading to timeouts on
> > > prepare.
> > > 
> > > This can be fixed by allocating for the worst-case of 15
> > > ports (DP0..DP14). In addition since the number is now fixed, we can
> > > use an array instead of a dynamic allocation.
> > > 
> > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
> > > Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> > > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> > > ---
> > >   drivers/soundwire/mipi_disco.c  | 18 +-----------------
> > >   drivers/soundwire/slave.c       |  4 ++++
> > >   include/linux/soundwire/sdw.h   |  2 +-
> > >   sound/soc/codecs/max98373-sdw.c | 15 +--------------
> > >   sound/soc/codecs/rt1308-sdw.c   | 14 +-------------
> > >   sound/soc/codecs/rt5682-sdw.c   | 15 +--------------
> > >   sound/soc/codecs/rt700-sdw.c    | 15 +--------------
> > >   sound/soc/codecs/rt711-sdw.c    | 15 +--------------
> > >   sound/soc/codecs/rt715-sdw.c    | 33 +--------------------------------
> > 
> > This looks fine, but the asoc changes are not dependent, so maybe we
> > should split them up and then can go thru Mark. Or Mark acks, either way
> > would work for me
> 
> There are 3 dependencies that we tracked between SoundWire and ASoC
> subsystems:
> 
> a) addition of SDCA control macro (needed before SDCA codec drivers can be
> shared)
> b) this series - we could indeed submit the codec changes to Mark's tree
> separately, but then the SoundWire tree would be broken: the codec drivers
> would still try to allocate dynamically what is now a fixed-size array.
> c) configuration of the interrupt masks in codec drivers instead of
> hard-coded in bus driver + spurious parity error workaround (not posted yet
> but ready).
> 
> The changes in ASoC codecs are really only on the initialization part
> (either removing a dynamic allocation or setting masks), there's no
> functional change otherwise.
> 
> I think the simplest to avoid multiple back-and-forth is to have these small
> interface/initialization changes merged through the SoundWire subsystem,
> then merged by Mark from a single immutable tag. Would this work for
> everyone?

That would work for me, but you need to split the asoc, regmap, sdw
patches. I am sure looking at patch tag, other maintainers would have
skipped these patches..

> In addition, there's a WIP change to regmap to add support for SoundWire 1.2
> MBQ-based register access, but this only affects regmap and ASoC trees, all
> handled by Mark.
> 
> I don't think we have any other cross-tree changes planned for now, the SDCA
> infrastructure plumbing is still rather open.

-- 
~Vinod