question about i2c_transfer() function (regarding mdio-i2c on RollBall SFPs)

question about i2c_transfer() function (regarding mdio-i2c on RollBall SFPs)

[Marek Behun]

Hi Wolfram and Russell,

I have a question regarding whether the struct i2c_msg array passed to
the i2c_transfer() function can have multiple messages refer to the
same buffers.

Previously Russell raised a point on my patch series adding support
for RollBall SFPs regarding the I2C MDIO access, which is done on I2C
address 0x51 on the upper 128 bytes when SFP page is set to 3.

https://lore.kernel.org/netdev/20201030152033.GC1551@shell.armlinux.org.uk/

Russell wrote
  "Also, shouldn't we ensure that we are on page 1 before attempting
   any access?"
  "I think this needs to be done in the MDIO driver - if we have
   userspace or otherwise expand what we're doing, relying on page 3
   remaining selected will be very fragile."

I have been thinking about this, and I think it is possible to switch
SFP_PAGE to a needed value, do some reads and writes on that page, and
restore the page to original value, all in one call to i2c_transfer.
This would have the advantage to be atomic (unless something breaks int
he I2C driver).

My question is whether this is allowed, whether the msgs array passed
to the i2c_transfer() function can have multiple msgs pointing to the
same buffer (the one into which the original page is first stored
with first i2c_msg and then restored from it in the last i2c_msg).

I looked into I2C drivers i2c-mv64xxx and i2c-pxa, and it looks that at
least for these two drivers, it should work.

What do you think?

It could look like this:

  struct i2c_msg msgs[10], *ptr;
  u8 saved_page[2], new_page[2];

  saved_page[0] = SFP_PAGE;
  new_page[0] = SFP_PAGE;
  new_page[1] = 3; /* RollBall MDIO access page */

  ptr = msgs;
  ptr = fill_read_msg(ptr, 0x51, &saved_page[0], 1, &saved_page[1], 1);
  ptr = fill_write_msg(ptr, 0x51, new_page, 2);

  /* here some more commands can be added */
  ...

  /* and this should restore the original page */
  ptr = fill_write_msg(ptr, 0x51, saved_page, 2);

  return i2c_transfer(i2c, msgs, ptr - msgs);

--
With fill_read_msg and fill_write_msg defined as such

  static inline struct i2c_msg *
  fill_read_msg(struct i2c_msg *msg, int addr,
		void *wbuf, size_t wlen,
		void *rbuf, size_t rlen)
  {
	msg->addr = addr;
	msg->flags = 0;
	msg->len = wlen;
	msg->buf = wbuf;
	++msg;
	msg->addr = addr;
	msg->flags = I2C_M_RD;
	msg->len = rlen;
	msg->buf = rbuf;
	++msg;

	return msg;
  }

  static inline struct i2c_msg *
  fill_write_msg(struct i2c_msg *msg, int addr, void *buf, size_t len)
  {
	msg->addr = addr;
	msg->flags = 0;
	msg->len = len;
	msg->buf = buf;	
	++msg;

	return msg;
  }

Re: question about i2c_transfer() function (regarding mdio-i2c on RollBall SFPs)

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 473 bytes --]


> My question is whether this is allowed, whether the msgs array passed
> to the i2c_transfer() function can have multiple msgs pointing to the
> same buffer (the one into which the original page is first stored
> with first i2c_msg and then restored from it in the last i2c_msg).

Sending the messages is serialized, so the buffers won't interfere. At
first glance, I think it would work this way. But it's late evening
here, so I will have another look again tomorrow.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: question about i2c_transfer() function (regarding mdio-i2c on RollBall SFPs)

[Marek Behun]

On Thu, 7 Jan 2021 22:02:48 +0100
Wolfram Sang <wsa@kernel.org> wrote:

> > My question is whether this is allowed, whether the msgs array passed
> > to the i2c_transfer() function can have multiple msgs pointing to the
> > same buffer (the one into which the original page is first stored
> > with first i2c_msg and then restored from it in the last i2c_msg).  
> 
> Sending the messages is serialized, so the buffers won't interfere. At
> first glance, I think it would work this way. But it's late evening
> here, so I will have another look again tomorrow.
> 

I thought as much, but maybe there is some driver which can offload
whole i2c_transfer to HW, and has to pass the addresses of the buffers
to the HW, and the HW can have problems if the buffers overlap
somewhere...

Marek

Re: question about i2c_transfer() function (regarding mdio-i2c on RollBall SFPs)

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 840 bytes --]


> I thought as much, but maybe there is some driver which can offload
> whole i2c_transfer to HW, and has to pass the addresses of the buffers
> to the HW, and the HW can have problems if the buffers overlap
> somewhere...

Well, sure, you can never know what crazy HW is out there :) But that
shouldn't prevent us from doing legit I2C transfers. The likeliness of
such HW is low enough; They must process the whole transfer in one go
(rare) AND have the limitiation with the buffer pointers (at least I
don't know one) AND have no possibility of a fallback to a simpler mode
where they can handle the transfer per message. If such a controller
exists, it would need a new quirk flag, I'd say, and reject the
transfer. But that shouldn't stop you from fixing your issue.

Thanks for thinking thoroughly about drawbacks! Much appreciated.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]