Date:   Fri, 17 Mar 2023 14:08:07 +0000
From:   Charles Keepax <ckeepax@opensource.cirrus.com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
CC:     <vkoul@kernel.org>, <yung-chuan.liao@linux.intel.com>,
        <sanyog.r.kale@intel.com>, <alsa-devel@alsa-project.org>,
        <linux-kernel@vger.kernel.org>, <patches@opensource.cirrus.com>
Subject: Re: [PATCH 2/2] soundwire: bus: Update sdw_nread/nwrite_no_pm to
 handle page boundaries
Message-ID: <20230317140807.GI68926@ediswmail.ad.cirrus.com>
References: <20230316155734.3191577-1-ckeepax@opensource.cirrus.com>
 <20230316155734.3191577-2-ckeepax@opensource.cirrus.com>
 <447cac77-4cc7-b2a3-23e7-978e1641a401@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <447cac77-4cc7-b2a3-23e7-978e1641a401@linux.intel.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

On Thu, Mar 16, 2023 at 01:46:57PM -0500, Pierre-Louis Bossart wrote:
> 
> 
> On 3/16/23 10:57, Charles Keepax wrote:
> > Currently issuing a sdw_nread/nwrite_no_pm across a page boundary
> > will silently fail to write correctly as nothing updates the page
> > registers, meaning the same page of the chip will get rewritten
> > with each successive page of data.
> > 
> > As the sdw_msg structure contains page information it seems
> > reasonable that a single sdw_msg should always be within one
> > page. It is also mostly simpler to handle the paging at the
> > bus level rather than each master having to handle it in their
> > xfer_msg callback.
> > 
> > As such add handling to the bus code to split up a transfer into
> > multiple sdw_msg's when they go across page boundaries.
> 
> This sounds good but we need to clarify that the multiple sdw_msg's will
> not necessarily be sent one after the other, the msg_lock is held in the
> sdw_transfer() function, so there should be no expectation that e.g. one
> big chunk of firmware code can be sent without interruption.
> 

I will update the kdoc for nread/nwrite to specify that
transactions that cross a page boundry are not expected to be
atomic.

> I also wonder if we should have a lower bar than the page to avoid
> hogging the bus with large read/write transactions. If there are
> multiple devices on the same link and one of them signals an alert
> status while a large transfer is on-going, the alert handling will
> mechanically be delayed by up to a page - that's 32k reads/writes, isn't it?
> 

I think its 16k, but I would be inclined to say this is a
separate fix. The current code will tie up the bus for longer
than my fix does, since it only calls sdw_transfer once, and it
will write the wrong registers whilst doing it. Also to be clear
this wasn't found with super large transfers just medium sized
ones that happened to cross a page boundry.

If we want to add some transaction size capping that is really
a new feature, this patch a bug fix. I would also be inclined
to say if we are going to add transaction size capping, we
probably want some property to specify what we are capping to.

Thanks,
Charles

> > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > ---
> >  drivers/soundwire/bus.c | 47 +++++++++++++++++++++++------------------
> >  1 file changed, 26 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> > index 3c67266f94834..bdd251e871694 100644
> > --- a/drivers/soundwire/bus.c
> > +++ b/drivers/soundwire/bus.c
> > @@ -386,37 +386,42 @@ int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
> >   * Read/Write IO functions.
> >   */
> >  
> > -int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
> > +static int sdw_ntransfer_no_pm(struct sdw_slave *slave, u32 addr, u8 flags,
> > +			       size_t count, u8 *val)
> >  {
> >  	struct sdw_msg msg;
> > +	size_t size;
> >  	int ret;
> >  
> > -	ret = sdw_fill_msg(&msg, slave, addr, count,
> > -			   slave->dev_num, SDW_MSG_FLAG_READ, val);
> > -	if (ret < 0)
> > -		return ret;
> > +	while (count) {
> > +		// Only handle bytes up to next page boundary
> > +		size = min(count, (SDW_REGADDR + 1) - (addr & SDW_REGADDR));
> >  
> > -	ret = sdw_transfer(slave->bus, &msg);
> > -	if (slave->is_mockup_device)
> > -		ret = 0;
> > -	return ret;
> > +		ret = sdw_fill_msg(&msg, slave, addr, size, slave->dev_num, flags, val);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		ret = sdw_transfer(slave->bus, &msg);
> > +		if (ret < 0 && !slave->is_mockup_device)
> > +			return ret;
> > +
> > +		addr += size;
> > +		val += size;
> > +		count -= size;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
> > +{
> > +	return sdw_ntransfer_no_pm(slave, addr, SDW_MSG_FLAG_READ, count, val);
> >  }
> >  EXPORT_SYMBOL(sdw_nread_no_pm);
> >  
> >  int sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val)
> >  {
> > -	struct sdw_msg msg;
> > -	int ret;
> > -
> > -	ret = sdw_fill_msg(&msg, slave, addr, count,
> > -			   slave->dev_num, SDW_MSG_FLAG_WRITE, (u8 *)val);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > -	ret = sdw_transfer(slave->bus, &msg);
> > -	if (slave->is_mockup_device)
> > -		ret = 0;
> > -	return ret;
> > +	return sdw_ntransfer_no_pm(slave, addr, SDW_MSG_FLAG_WRITE, count, (u8 *)val);
> >  }
> >  EXPORT_SYMBOL(sdw_nwrite_no_pm);
> >