Re: [RFC PATCH] i2c: Support Smbus 3.0 block sizes up to 255 bytes.

[Daniel Stodden <daniel.stodden@gmail.com>]

Hi.

> On Jul 28, 2020, at 10:04 AM, Jean Delvare <jdelvare@suse.de> wrote:
> 
> Hi Wolfram, Daniel,
> 
> On Tue, 28 Jul 2020 11:40:37 +0200, Wolfram Sang wrote:
>>> -	__u8 block[I2C_SMBUS_BLOCK_MAX + 2]; /* block[0] is used for length */
>>> +	__u8 block[I2C_SMBUS3_BLOCK_MAX + 2]; /* block[0] is used for length */
>>> 			       /* and one more for user-space compatibility */  
>> 
>> I thought about this, too, and wondered if this isn't a size regression
>> in userspace with every i2c_smbus_data getting 8 times the size? But
>> maybe it is worth if backwards compatibility is maintained in an
>> otherwise not so intrusive manner? Jean, what do you think?
> 
> In i2c-tools these are always allocated on the stack and one at a time.
> Size isn't an issue in my opinion.

Cool. Meaning we may then leave it at a single i2c_smbus_data.block[257]
declaration in the headers, and i2c_smbus_data can be 255 bytes.

That doesn’t mean we shut the door on 32-byte buffers. After all, backward
compat requires we maintain support for those. Just that we need not
bother declaring or promoting dedicated small vs large transfer types
while noone wants one.

Code in need can still make its own. If we learn that there’s legitimate 
demand, we could add a i2c_smbus1_data later to sanction that.

> The only thing I'm truly concerned about here is compatibility. You
> need to ensure that:
> 
> * User-space binaries that are around today (obviously compiled with
>  the old versions of the kernel uapi headers) will work the same with
>  kernels including your changes.
> 
> * User-space binaries compiled with the new kernel uapi headers will
>  work with both old and new kernels.
> 
> For all transfer types. You will have to keep in mind that SMBus-style
> and I2C-style read block transfers differ in who decides the length.
> For I2C, it is the caller (I want to read N bytes from this location),
> for SMBus it is the slave (I want to read a block from this location,
> tell me how long it is). In both cases you need to ensure you do not
> write beyond the buffer, no matter what, and return a proper error code
> if it wouldn't fit.

Reverse compatibility would be a more interesting concern.

You’re saying you would need an actual universal binary?
This is indeed beyond only binary backward compatibility.

The headers can’t automate that for you, obviously. The macros would have
to be too use-case specific. 

It has to go into the library. But we can make sure we’re at least
not overcomplicating it.

But this can affect matters on how to name new vs old size macros.

For cases which are less interested in the reverse case, and not the
stack space increment either, making I2C_SMBUS_BLOCK_DATA the new smbus3
number was clearly the better choice.

It equates to source compatibility.

In contrast, if we need an I2C_SMBUS_BLOCK3_DATA instead, everything needs
a patch to move on.

Should we make that case slightly harder, to promote smbus3 defaults elsewhere?
For example, okay if an i2c library build has to compile conditionally, as in

#ifdef I2C_SMBUS_BLOCK3_DATA
# .. know that I2C_SMBUS_BLOCK_DATA means smbus3
#else
# .. know that I2C_SMBUS_BLOCK_DATA means smbus1
endif

to pull off the universal binary?

This would also mean we’re likely to add an I2C_FUNC_ to indicated smbus3
support at runtime. It’s probably what you already have in mind.

We’d probably still prefer to move all kernel/driver buffers to 255 bytes
unconditionally. However, I2C_FUNC_ presence normally indicates lower level
driver + hardware support, right? Not just a kernel upgrade. Which makes 
sense here, too.

I’m just pointing it out because if we want an I2C_FUNC_SMBUS3, and at the driver’s
discretion, not just indicating kernel + compatibility support presence,
and if i2c_utils uses that to pick safe size values, that again
means we’re yet again less likely to actually deprecate the old transfer numbers.
Some drivers will never be SMBUS3.

> The other compatibility type you need to care about is inside the
> kernel itself: SMBus 2 and SMBus 3 controllers and devices may be mixed
> up. Some (I expect most) SMBus 3 devices may be able to work with SMBus
> 2 controllers in "degraded" mode, in which case it will be the driver's
> responsibility to check the capabilities of the controller and only use
> transfer types that are supported.

Yes. This is what’s already happening. It’s the only workaround I’m aware
of to get PMBus devices working with what current kernels provide.

It’s basically why I’m here :)

Let’s say your have a device assuming smbus3 limits. If you have a legacy
smbus_xfer()-based controller you’re pretty much out of luck.

But if you know what you’ve wired together, you’ll have a master_xfer()
implementation with no such limits for fixed length reads 
(I2C_M_RD, but not I2C_M_RECV_LEN). Here is an example to execute a
PMBus command producing unknown length L > 32:

 1. Do a read_byte() on on that command. It will stop the transfer
    after the length field, but at least you know L now.

 2. Re-invoke the command, this time through I2C_RDWR,
    with a fixed message length (1 + L [ + pec]) to get the full transfer
    through. 32B is only for I2C_RECV_LEN.

It’s not pretty, but saves the day. Assume there is plenty of code around
which does such things.

> If such "degraded" mode is not
> possible then the driver would simply refuse to bind.

I’d prefer if we keep things as permissive as possible.

For above reasons: not to fence off workarounds which used to be possible.

We’re not anticipating the invention of smbus3 here. 
We’re not paving the way to get there. We’re not setting a standard.
We’re only trying to top getting in the of what already happened.

It seems perfecty fine to pass an smbus3 transfer from an smbus3 device
found under an smbus2 adapter, just by truncating the data.

The people who open(‘/dev/i2c-%d’) are often not the type which
roots for autoconfiguration or so-helpful warnings spamming their console.

Intuitively, I’d expect the above quiet truncation could be what several 
32B-controller designers may have chosen to do. E.g. when 
anticipating bit errors in the length prefix observed by their hardware 
block-read accellerator?

At least as far as kernel space is concerned, I’d leave it there.

I2c libraries may differ, to some degree. Users unhappy with a strict-mode
library then at least keep their chance to bypass the library and
grow their own ioctls().

> For such checks
> to be possible, I would expect either one global I2C_FUNC_SMBUS* flag
> to be added to denote SMBus 3 compliance, or possibly several flags for
> finer grained control (seems overkill to me but you may have a
> different opinion - also depends on what else SMBus 3 is introducing,
> I admit I did not check). However I did not see that in your prototype.
> Do you believe we can do without it? If so, please explain how.

I think in any case, we can make kernel-side buffers smbus3-sized
unconditionally.

Even if we had an I2C_FUNC_SMBUS3 to signal actual support. So far,
everyone confirms not doing so offers only minor savings.
But for a quite realistic bug scenario where drivers and clients
getting semantics wrong will honor that with occasional stack corruption.

If it sounds important: I recognize one could make the krealloc()
in i2cdev_ioctl_rdwr in the original patch dependent on an
I2C_FUNC_SMBUS3 from the driver, if we want to avoid it. But it would
be an oddball variant.

User buffers can be 32B or 255B, we just require the command to match,
because I2C_SMBUS has currently no other way to disambiguate message
length limits.

I2C_FUNC_SMBUS3 is nice to have. I’m not against it, I’m for it.
For i2c-tools and all clients, it can be valuable to confirm conflicts.

Still, one can keep it indicative only, and not synthesize faults 
because of a perceived client/adapter/device feature mismatch. 

We only don’t want to corrupt data. So far we don’t seem to need an
I2C_FUNC_SMBUS3 to accommodate that.

I’m not even sure if my new -EMSGSIZE condition in i2c-dev.c is such
a good idea.

If noone else thinks it’s critical, maybe we should drop that too.
(But the current code makes sure at least all (truncated) transfer
 gets copied_to before before returning it. So libraries may choose
 to ignore this particular ret val and pass the partial result on
 instead.)

Legitimately naive clients trying to avoid smbus1-vs-3 conditionals,
will look at block[0] anyway, vs their buffer length, to figure out that
something may not work as they though it would. At least their stacks
shall be fine. :)

Daniel

> -- 
> Jean Delvare
> SUSE L3 Support