@kib What if we reclaimed some of the spare fields in ifi2creq to a uint16_t magic field. User sets i2creq.magic=PAGE_BANK_REQ and driver sets magic=PAGE_BANK_ACK ? That prevents the confusion you're concerned about.

Why not simply not allocate the new number for the new ioctl. Rename current ioctl to something like SIOCGI2C_NOBANK. Then, at the ioctl level, do two things:

• for new SIOCGI2C check that reserved bytes in the structure are zero
• for SIOCGI2C_NOBANK, zero bank/page/padding and call into SIOCGI2C

In D55912#1279758, @kib wrote:

Why not simply not allocate the new number for the new ioctl. Rename current ioctl to something like SIOCGI2C_NOBANK. Then, at the ioctl level, do two things:

• for new SIOCGI2C check that reserved bytes in the structure are zero
• for SIOCGI2C_NOBANK, zero bank/page/padding and call into SIOCGI2C

I think I'd rather go the other direction, so as to avoid touching legacy ioctl users from ports. It turns out there are many from the ports sweep.

Eg, we add SIOCGI2CPB (where PB means page/bank) for new code. Drivers like mlx5 which can support page and/or bank access add a fallthru to SIOCGI2CPB to their SIOCGI2C case and zero page & bank for SIOCGI2C. Eg:

	case SIOCGI2C:
	case SIOCGI2CPB:
		ifr = (struct ifreq *)data;

		/*
		 * Copy from the user-space address ifr_data to the
		 * kernel-space address i2c
		 */
		error = copyin(ifr_data_get_ptr(ifr), &i2c, sizeof(i2c));
		if (error)
			break;

		/* Esnure page & bank are zero for legacy SIOCGI2C ioctl */
		if (command == SIOCGI2C) {
			i2c.page = i2c.bank = 0;
		}

In D55912#1280104, @gallatin wrote:

In D55912#1279758, @kib wrote:

Why not simply not allocate the new number for the new ioctl. Rename current ioctl to something like SIOCGI2C_NOBANK. Then, at the ioctl level, do two things:

• for new SIOCGI2C check that reserved bytes in the structure are zero
• for SIOCGI2C_NOBANK, zero bank/page/padding and call into SIOCGI2C

I think I'd rather go the other direction, so as to avoid touching legacy ioctl users from ports. It turns out there are many from the ports sweep.

Eg, we add SIOCGI2CPB (where PB means page/bank) for new code. Drivers like mlx5 which can support page and/or bank access add a fallthru to SIOCGI2CPB to their SIOCGI2C case and zero page & bank for SIOCGI2C. Eg:

	case SIOCGI2C:
	case SIOCGI2CPB:
		ifr = (struct ifreq *)data;

		/*
		 * Copy from the user-space address ifr_data to the
		 * kernel-space address i2c
		 */
		error = copyin(ifr_data_get_ptr(ifr), &i2c, sizeof(i2c));
		if (error)
			break;

		/* Esnure page & bank are zero for legacy SIOCGI2C ioctl */
		if (command == SIOCGI2C) {
			i2c.page = i2c.bank = 0;
		}

I think your proposal is also fine, but why? What I propose

1. provide perfect binary compat for old consumers
2. allows to avoid modifying drivers at all, except in case when the bank/page support is added.

I think that I may not understand your proposal. When you say "at the ioctl level..", Do you mean intercept the ioctls in ifhwioctl()? I think that seems awkward because ifi2req lives in userspace, and individual drivers copy it in / out, rather than it being done in a single spot for all drivers. So to zero fields, ifhwioctl would need copy-in ifi2creq, fix it up, & copy it out again..

My proposal doesn't have this requirement, and also only requires changes to drivers supporting page/bank operation...

I mean something like this (ignore naming), in the generic ioctl handler, not in the driver code;

case SIOCGI2C_OLD:
     i2c.page = i2c.bank = 0;
     ioctl(SIOCG2C, &i2c);
     break;

I.e. drivers only need to implement the new ioctl that knows about bank/page.

Or put it differently, your original patch for mlx5en then would be correct, with this addition to the net/, instead of requiring each driver to have code to handle both ioctls.

In D55912#1282432, @kib wrote:

I mean something like this (ignore naming), in the generic ioctl handler, not in the driver code;

case SIOCGI2C_OLD:
     i2c.page = i2c.bank = 0;
     ioctl(SIOCG2C, &i2c);
     break;

I.e. drivers only need to implement the new ioctl that knows about bank/page.

Or put it differently, your original patch for mlx5en then would be correct, with this addition to the net/, instead of requiring each driver to have code to handle both ioctls.

Where, exactly, would you put that? Since drivers copy-in the i2c structure themselves, I just can't see where you'd put this that didn't involve and awkward copyin/copyout fixup.

Sorry for the late reply.

I think this should work as is. See below.

In D55912#1283564, @gallatin wrote:

In D55912#1282432, @kib wrote:

I mean something like this (ignore naming), in the generic ioctl handler, not in the driver code;

case SIOCGI2C_OLD:
     i2c.page = i2c.bank = 0;
     ioctl(SIOCG2C, &i2c);
     break;

I.e. drivers only need to implement the new ioctl that knows about bank/page.

Or put it differently, your original patch for mlx5en then would be correct, with this addition to the net/, instead of requiring each driver to have code to handle both ioctls.

Where, exactly, would you put that? Since drivers copy-in the i2c structure themselves, I just can't see where you'd put this that didn't involve and awkward copyin/copyout fixup.

Then we can add an if_t method for accessing i2c, and handle ioctl+copyin/out centralized. Drivers would only need to fill the buffer.
IMO moving the copying out of drivers is cleaner by itself, but there it also simplifies the functionality.

In D55912#1287051, @kib wrote:

Sorry for the late reply.

I think this should work as is. See below.

To be clear, do you approve of the current patch as-is? If so, I rebase my ifconfig patches and ensure it all works properly and put those up for review after this..

In D55912#1283564, @gallatin wrote:

In D55912#1282432, @kib wrote:

I mean something like this (ignore naming), in the generic ioctl handler, not in the driver code;

case SIOCGI2C_OLD:
     i2c.page = i2c.bank = 0;
     ioctl(SIOCG2C, &i2c);
     break;

I.e. drivers only need to implement the new ioctl that knows about bank/page.

Or put it differently, your original patch for mlx5en then would be correct, with this addition to the net/, instead of requiring each driver to have code to handle both ioctls.

Where, exactly, would you put that? Since drivers copy-in the i2c structure themselves, I just can't see where you'd put this that didn't involve and awkward copyin/copyout fixup.

Then we can add an if_t method for accessing i2c, and handle ioctl+copyin/out centralized. Drivers would only need to fill the buffer.
IMO moving the copying out of drivers is cleaner by itself, but there it also simplifies the functionality.

You mean as a later, separate project, correct?