Re: [PATCH v2 0/4] i2c: SMBus ARP support

[Jeremy Kerr <jk@codeconstruct.com.au>]

Hi Heikki,

> Uh, no. You should only use interfaces like new_device with the
> devices that really can't be detected in kernel, which isn't the case
> here.

Who is deciding this "you should only" case? If the facility works, it's
suitable. You raise some good points that may mean it is not a suitable
approach for an ARP implementation, but we should still make sure we're
taking the right approach.

[You seem pretty defensive here? I'm not saying no to the kernel
implementation, just doing our homework before agreeing to it]

> So why would you want involve the user space at all since it would
> just add complexity and limitations without any benefits?

Because we have fewer risks implementing this in userspace.

As an example, you currently seem to have a stack information leak in
the proposed Get UDID implementation, which would be much less of an
issue for the equivalent protocol handling implemented in userspace.

> - You still need to deliver the UDID to the kernel because of things
>   like the PEC flag, and the drivers will also need information from
>   it.

That seems like the main reason for requiring a kernel approach, in that
we need more information than just the assigned address. It's not
possible (at present) to specify the PEC flag through existing
interfaces, right?

For me, this would be the deciding factor to go for a kernel approach,
in that we otherwise cannot properly describe ARPed devices to the i2c
subsystem. We *could* push a new_device with a UDID, but I'm not sure
that's a great idea.

> - With the static (not hotplugged) devices you need to assign the
>   correct ACPI node (or what ever fwnode) to the ARP-device.

Is that possible at present? how are you planning to represent ARPed
devices in the DT - or more importantly, correlate DT (or other fwnode)
nodes to discovered devices?

> - When the device is hotplugged, you would need new ABI, like I think
>   you already noticed, but this really does not make any sense. We
>   simply don't need it, because the kernel can process this
>   information on its own very simply.

Even this "very simple" implementation may have bugs.

Assuming we go with a kernel approach: For the MCTP case, for full ARP
support of MCTP endpoints, we would still need to consume a hotplug
event that indicates that the device is available at its new address
- there is no kernel driver bound for the remote MCTP endpoints. This
event would be consumed by the (existing) MCTP infrastructure in order
to start MCTP enumeration. Is this something you have looked at
already? If so, if you can send an example of an actual event, I will
look at the mctpd part of this.

Cheers,


Jeremy