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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210107192500.54d2d0f0@nic.cz>
Date:   Thu, 7 Jan 2021 19:25:00 +0100
From:   Marek Behun <marek.behun@....cz>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Wolfram Sang <wsa@...nel.org>
Cc:     Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
        Pali Rohár <pali@...nel.org>,
        linux-i2c@...r.kernel.org
Subject: question about i2c_transfer() function (regarding mdio-i2c on
 RollBall SFPs)

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;
  }