[1/1] regmap: spi-avmm: Fix regmap_bus max_raw_write

  The max_raw_write member of the regmap_spi_avmm_bus structure is defined
as:
	.max_raw_write = SPI_AVMM_VAL_SIZE * MAX_WRITE_CNT

SPI_AVMM_VAL_SIZE == 4 and MAX_WRITE_CNT == 1 so this results in a
maximum write transfer size of 4 bytes which provides only enough space to
transfer the address of the target register. It provides no space for the
value to be transferred. This bug became an issue (divide-by-zero in
_regmap_raw_write()) after the following was accepted into mainline:

commit 3981514180c9 ("regmap: Account for register length when chunking")

Change max_raw_write to include space (4 additional bytes) for both the
register address and value:

	.max_raw_write = SPI_AVMM_REG_SIZE + SPI_AVMM_VAL_SIZE * MAX_WRITE_CNT

Fixes: 7f9fb67358a2 ("regmap: add Intel SPI Slave to AVMM Bus Bridge support")
Reviewed-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
 drivers/base/regmap/regmap-spi-avmm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

On 6/20/23 13:28, Russ Weight wrote:
> The max_raw_write member of the regmap_spi_avmm_bus structure is defined
> as:
> 	.max_raw_write = SPI_AVMM_VAL_SIZE * MAX_WRITE_CNT
>
> SPI_AVMM_VAL_SIZE == 4 and MAX_WRITE_CNT == 1 so this results in a
> maximum write transfer size of 4 bytes which provides only enough space to
> transfer the address of the target register. It provides no space for the
> value to be transferred. This bug became an issue (divide-by-zero in
> _regmap_raw_write()) after the following was accepted into mainline:
>
> commit 3981514180c9 ("regmap: Account for register length when chunking")
>
> Change max_raw_write to include space (4 additional bytes) for both the
> register address and value:
>
> 	.max_raw_write = SPI_AVMM_REG_SIZE + SPI_AVMM_VAL_SIZE * MAX_WRITE_CNT
>
> Fixes: 7f9fb67358a2 ("regmap: add Intel SPI Slave to AVMM Bus Bridge support")
> Reviewed-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>

Cc: stable@vger.kernel.org
> ---
>  drivers/base/regmap/regmap-spi-avmm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/regmap/regmap-spi-avmm.c b/drivers/base/regmap/regmap-spi-avmm.c
> index 4c2b94b3e30b..6af692844c19 100644
> --- a/drivers/base/regmap/regmap-spi-avmm.c
> +++ b/drivers/base/regmap/regmap-spi-avmm.c
> @@ -660,7 +660,7 @@ static const struct regmap_bus regmap_spi_avmm_bus = {
>  	.reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
>  	.val_format_endian_default = REGMAP_ENDIAN_NATIVE,
>  	.max_raw_read = SPI_AVMM_VAL_SIZE * MAX_READ_CNT,
> -	.max_raw_write = SPI_AVMM_VAL_SIZE * MAX_WRITE_CNT,
> +	.max_raw_write = SPI_AVMM_REG_SIZE + SPI_AVMM_VAL_SIZE * MAX_WRITE_CNT,
>  	.free_context = spi_avmm_bridge_ctx_free,
>  };
>
  

On Tue, 20 Jun 2023 13:28:24 -0700, Russ Weight wrote:
> The max_raw_write member of the regmap_spi_avmm_bus structure is defined
> as:
> 	.max_raw_write = SPI_AVMM_VAL_SIZE * MAX_WRITE_CNT
> 
> SPI_AVMM_VAL_SIZE == 4 and MAX_WRITE_CNT == 1 so this results in a
> maximum write transfer size of 4 bytes which provides only enough space to
> transfer the address of the target register. It provides no space for the
> value to be transferred. This bug became an issue (divide-by-zero in
> _regmap_raw_write()) after the following was accepted into mainline:
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

Thanks!

[1/1] regmap: spi-avmm: Fix regmap_bus max_raw_write
      commit: c8e796895e2310b6130e7577248da1d771431a77

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark
  

On 2023-06-20 at 13:28:24 -0700, Russ Weight wrote:
> The max_raw_write member of the regmap_spi_avmm_bus structure is defined
> as:
> 	.max_raw_write = SPI_AVMM_VAL_SIZE * MAX_WRITE_CNT
> 
> SPI_AVMM_VAL_SIZE == 4 and MAX_WRITE_CNT == 1 so this results in a
> maximum write transfer size of 4 bytes which provides only enough space to
> transfer the address of the target register. It provides no space for the
> value to be transferred. This bug became an issue (divide-by-zero in
> _regmap_raw_write()) after the following was accepted into mainline:
> 
> commit 3981514180c9 ("regmap: Account for register length when chunking")

Sorry for late reply.

IIUC, max_raw_write/read is the max batch *DATA* size that could be
handled by the receiver. reg addr bytes are not counted in. I'm not 100%
sure this is obeyed by all drivers. But see several examples:

static const struct regmap_config ar9331_mdio_regmap_config = {
	.reg_bits = 32,
	.val_bits = 32,
	.reg_stride = 4,
	[...]
};

static struct regmap_bus ar9331_sw_bus = {
	[...]
	.max_raw_read = 4,
	.max_raw_write = 4,
};

Another one:

static struct regmap_config qca8k_regmap_config = {
	.reg_bits = 16,
	.val_bits = 32,
	.reg_stride = 4,
	[...]
	.max_raw_read = 32, /* mgmt eth can read/write up to 8 registers at time */
	.max_raw_write = 32,
}

And regmap-spi.c:

static const struct regmap_bus *regmap_get_spi_bus(struct spi_device *spi,
						   const struct regmap_config *config)
{
	size_t max_size = spi_max_transfer_size(spi);
	size_t max_msg_size, reg_reserve_size;
	struct regmap_bus *bus;

	if (max_size != SIZE_MAX) {
		bus = kmemdup(&regmap_spi, sizeof(*bus), GFP_KERNEL);
		if (!bus)
			return ERR_PTR(-ENOMEM);

		max_msg_size = spi_max_message_size(spi);
		reg_reserve_size = config->reg_bits / BITS_PER_BYTE
				 + config->pad_bits / BITS_PER_BYTE;
		if (max_size + reg_reserve_size > max_msg_size)
			max_size -= reg_reserve_size;

		bus->free_on_exit = true;
		bus->max_raw_read = max_size;
		bus->max_raw_write = max_size;

		return bus;
	}

	return &regmap_spi;
}

So I'm not sure if commit 3981514180c9 is actually necessary.

Thanks,
Yilun

> 
> Change max_raw_write to include space (4 additional bytes) for both the
> register address and value:
> 
> 	.max_raw_write = SPI_AVMM_REG_SIZE + SPI_AVMM_VAL_SIZE * MAX_WRITE_CNT


> 
> Fixes: 7f9fb67358a2 ("regmap: add Intel SPI Slave to AVMM Bus Bridge support")
> Reviewed-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
>  drivers/base/regmap/regmap-spi-avmm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/base/regmap/regmap-spi-avmm.c b/drivers/base/regmap/regmap-spi-avmm.c
> index 4c2b94b3e30b..6af692844c19 100644
> --- a/drivers/base/regmap/regmap-spi-avmm.c
> +++ b/drivers/base/regmap/regmap-spi-avmm.c
> @@ -660,7 +660,7 @@ static const struct regmap_bus regmap_spi_avmm_bus = {
>  	.reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
>  	.val_format_endian_default = REGMAP_ENDIAN_NATIVE,
>  	.max_raw_read = SPI_AVMM_VAL_SIZE * MAX_READ_CNT,
> -	.max_raw_write = SPI_AVMM_VAL_SIZE * MAX_WRITE_CNT,
> +	.max_raw_write = SPI_AVMM_REG_SIZE + SPI_AVMM_VAL_SIZE * MAX_WRITE_CNT,
>  	.free_context = spi_avmm_bridge_ctx_free,
>  };
>  
> -- 
> 2.25.1
>
  

On Sun, Jun 25, 2023 at 12:26:31PM +0800, Xu Yilun wrote:

> IIUC, max_raw_write/read is the max batch *DATA* size that could be
> handled by the receiver. reg addr bytes are not counted in. I'm not 100%
> sure this is obeyed by all drivers. But see several examples:

There's clearly been some confusion in a bunch of drivers, those you've
identified below need fixing too for the new code from the looks of it.  
I'm frankly unclear why some of the drivers you're pointing at are even
implementing raw buses.

> So I'm not sure if commit 3981514180c9 is actually necessary.

That's "regmap: Account for register length when chunking".  It's
certainly a bit unclear now I go do another survey, though it's also
clear that things like the handling of padding are intermittent at best.
We probably would be safe reverting that.

Jim, where were you seeing the issue here?

Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.
  

On Mon, Jun 26, 2023 at 2:47 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Sun, Jun 25, 2023 at 12:26:31PM +0800, Xu Yilun wrote:
>
> > IIUC, max_raw_write/read is the max batch *DATA* size that could be
> > handled by the receiver. reg addr bytes are not counted in. I'm not 100%
> > sure this is obeyed by all drivers. But see several examples:
>
> There's clearly been some confusion in a bunch of drivers, those you've
> identified below need fixing too for the new code from the looks of it.
> I'm frankly unclear why some of the drivers you're pointing at are even
> implementing raw buses.
>
> > So I'm not sure if commit 3981514180c9 is actually necessary.
>
> That's "regmap: Account for register length when chunking".  It's
> certainly a bit unclear now I go do another survey, though it's also
> clear that things like the handling of padding are intermittent at best.
> We probably would be safe reverting that.
>
> Jim, where were you seeing the issue here?

Hope I am answering your question.

The issue I experienced is that if a bus (in my case a limited i2c controller)
defines a quirk with max_raw_write, then the chunking algorithm would
divide the data into max_raw_write chunks.  The i2c bus would then
prepend the address values to the chunk which would
always get rejected because it was at least one byte too large.

My original fix, that I posted was to add a special flag (reg_in_write)
that a bus could set to choose the to have the register accounted
for in the chunking algorithm.  This was admittedly inelegant.

After reviews, we thought using the reg_bytes would be a better
solution and that padding should be accounted for.

I had not seen an issue with padding for this algorithm.  Only
the case specified above with i2c with prepending the address.

Would it be possible to reconsider adding a flag or argument to
regmap_bus to guard this chunking behavior?

>
> Please include human readable descriptions of things like commits and
> issues being discussed in e-mail in your mails, this makes them much
> easier for humans to read especially when they have no internet access.
> I do frequently catch up on my mail on flights or while otherwise
> travelling so this is even more pressing for me than just being about
> making things a bit easier to read.
  

On 2023-06-26 at 15:23:45 -0500, Jim Wylder wrote:
> On Mon, Jun 26, 2023 at 2:47 PM Mark Brown <broonie@kernel.org> wrote:
> >
> > On Sun, Jun 25, 2023 at 12:26:31PM +0800, Xu Yilun wrote:
> >
> > > IIUC, max_raw_write/read is the max batch *DATA* size that could be
> > > handled by the receiver. reg addr bytes are not counted in. I'm not 100%
> > > sure this is obeyed by all drivers. But see several examples:
> >
> > There's clearly been some confusion in a bunch of drivers, those you've
> > identified below need fixing too for the new code from the looks of it.
> > I'm frankly unclear why some of the drivers you're pointing at are even
> > implementing raw buses.
> >
> > > So I'm not sure if commit 3981514180c9 is actually necessary.
> >
> > That's "regmap: Account for register length when chunking".  It's

Yes, will try to be as readable next time.

> > certainly a bit unclear now I go do another survey, though it's also
> > clear that things like the handling of padding are intermittent at best.

Handling of padding is good.

> > We probably would be safe reverting that.
> >
> > Jim, where were you seeing the issue here?
> 
> Hope I am answering your question.
> 
> The issue I experienced is that if a bus (in my case a limited i2c controller)
> defines a quirk with max_raw_write, then the chunking algorithm would
> divide the data into max_raw_write chunks.  The i2c bus would then
> prepend the address values to the chunk which would
> always get rejected because it was at least one byte too large.
> 
> My original fix, that I posted was to add a special flag (reg_in_write)
> that a bus could set to choose the to have the register accounted
> for in the chunking algorithm.  This was admittedly inelegant.
> 
> After reviews, we thought using the reg_bytes would be a better
> solution and that padding should be accounted for.
> 
> I had not seen an issue with padding for this algorithm.  Only
> the case specified above with i2c with prepending the address.
> 
> Would it be possible to reconsider adding a flag or argument to
> regmap_bus to guard this chunking behavior?

I think this is just software definition difference whether to
include reg addr in max_raw_read/write or not, so no need to branch
the regmap implementation by a flag.

I'm a bit prefer to exclude the reg addr, as it is in stable tree now
and doesn't see strong reason to change it. And suggest regmap-i2c does
the same as spi do, that is to reserve space for reg addr/padding by
reducing the max_raw_read/write value, see:

https://lore.kernel.org/all/20220818104851.429479-1-cristian.ciocaltea@collabora.com/

Thanks,
Yilun

> 
> >
> > Please include human readable descriptions of things like commits and
> > issues being discussed in e-mail in your mails, this makes them much
> > easier for humans to read especially when they have no internet access.
> > I do frequently catch up on my mail on flights or while otherwise
> > travelling so this is even more pressing for me than just being about
> > making things a bit easier to read.
  

On Tue, Jun 27, 2023 at 11:36:06AM +0800, Xu Yilun wrote:

> I'm a bit prefer to exclude the reg addr, as it is in stable tree now
> and doesn't see strong reason to change it. And suggest regmap-i2c does
> the same as spi do, that is to reserve space for reg addr/padding by
> reducing the max_raw_read/write value, see:

It seems better to keep things like this in the core since every time a
bus has to open code something that's something that the bus can get
wrong, as we've seen here.  This would mean some churn now but going
forwards it'd be less error prone.
  

On Tue, Jun 27, 2023 at 6:58 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jun 27, 2023 at 11:36:06AM +0800, Xu Yilun wrote:
>
> > I'm a bit prefer to exclude the reg addr, as it is in stable tree now
> > and doesn't see strong reason to change it. And suggest regmap-i2c does
> > the same as spi do, that is to reserve space for reg addr/padding by
> > reducing the max_raw_read/write value, see:
>
> It seems better to keep things like this in the core since every time a
> bus has to open code something that's something that the bus can get
> wrong, as we've seen here.  This would mean some churn now but going
> forwards it'd be less error prone.

Agreed.