Re: [PATCH v3 2/4] serdev: add rust private data to serdev_device

[Markus Probst <markus.probst@posteo.de>]

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

On Sat, 2026-03-14 at 14:54 +0100, Danilo Krummrich wrote:
> On Sat Mar 14, 2026 at 2:49 PM CET, Markus Probst wrote:
> > On Sat, 2026-03-14 at 14:42 +0100, Danilo Krummrich wrote:
> > > On Sat Mar 14, 2026 at 2:31 PM CET, Greg Kroah-Hartman wrote:
> > > > On Sat, Mar 14, 2026 at 12:08:09PM +0000, Markus Probst wrote:
> > > > > On Sat, 2026-03-14 at 12:52 +0100, Greg Kroah-Hartman wrote:
> > > > > > On Sat, Mar 14, 2026 at 11:42:02AM +0000, Markus Probst wrote:
> > > > > > > On Sat, 2026-03-14 at 09:07 +0100, Greg Kroah-Hartman wrote:
> > > > > > > > On Fri, Mar 13, 2026 at 06:12:31PM +0000, Markus Probst wrote:
> > > > > > > > > Add rust private data to `struct serdev_device`, as it is required by the
> > > > > > > > > rust abstraction added in the following commit
> > > > > > > > > (rust: add basic serial device bus abstractions).
> > > > > > > > 
> > > > > > > > why is rust "special" here?  What's wrong with the existing private
> > > > > > > > pointer in this structure?  Why must we add another one?
> > > > > > > Because in rust, the device drvdata will be set after probe has run. In
> > > > > > > serdev, once the device has been opened, it can receive data. It must
> > > > > > > be opened either inside probe or before probe, because it can only be
> > > > > > > configured (baudrate, flow control etc.) and data written to after it
> > > > > > > has been opened. Because it can receive data before drvdata has been
> > > > > > > set yet, we need to ensure it waits on data receival for the probe to
> > > > > > > be finished. Otherwise this would be a null pointer dereference. To do
> > > > > > > this, we need to store a `Completion` for it to wait and a `bool` in
> > > > > > > case the probe exits with an error. We cannot store this data in the
> > > > > > > device drvdata, because this is where the drivers drvdata goes. We also
> > > > > > > cannot create a wrapper of the drivers drvdata, because
> > > > > > > `Device::drvdata::<T>()` would always fail in that case. That is why we
> > > > > > > need a "rust_private_data" for this abstraction to store the
> > > > > > > `Completion` and `bool`.
> > > > > > 
> > > > > > So why is this any different from any other bus type?  I don't see the
> > > > > > "uniqueness" here that has not required this to happen for PCI or USB or
> > > > > > anything else.
> > > > > > 
> > > > > > What am I missing?
> > > > > In Short:
> > > > > In serdev, we have to handle incoming device data (serdev calls on a
> > > > > function pointer we provide in advance), even in the case that the
> > > > > driver hasn't completed probe yet.
> > > > 
> > > > But how is that any different from a USB or PCI driver doing the same
> > > > thing?  Why is serdev so unique here?  What specific serdev function
> > > > causes this and why isn't it an issue with the C api?  Can we change the
> > > > C code to not require this?
> > > 
> > > I think the idea is to avoid bugs as in the mhz19b driver [1].
> > > 
> > > This driver's probe() looks like this:
> > > 
> > > 
> > > 	serdev_device_set_client_ops(serdev, &mhz19b_ops);
> > > 	ret = devm_serdev_device_open(dev, serdev);
> > > 
> > > 	// Lots of other initialization.
> > > 
> > > 	serdev_device_set_drvdata(serdev, indio_dev);
> > > 
> > > But the receive_buf() callback from mhz19b_ops dereferences the driver's private
> > > data.
> > > 
> > > Now, maybe this is actually prevented to become an actual race, since some
> > > regulator is only enabled subsequently:
> > > 
> > > 	devm_regulator_get_enable(dev, "vin");
> > > 
> > > But in any case in Rust it would be unsound as with this a driver could easily
> > > cause undefined behavior with safe APIs.
> > > 
> > > Maybe it is as simple as letting the abstraction call serdev_device_open() only
> > > after the driver's probe() has completed, but maybe there are reasons why that
> > > is not an option, that's a serdev question.
> > If we call it after probe, calls to `serdev_device_set_baudrate`,
> > `serdev_device_set_flow_control`, `serdev_device_set_parity`,
> > `serdev_device_write_buf`, `serdev_device_write`,
> > `serdev_device_write_flush`, which are exposed via the rust abstraction
> > would result in a null pointer dereference.
> 
> Then maybe ensure that the driver's receive_buf() callback can only ever be
> called after probe() has been completed? E.g. receive_buf() could be optional
> and swapped out later on.
I am not exactly sure what you mean by "could be optional and swapped
out later on".

Also,
the function pointer cannot be changed while the device is open, as
this could introduce a race condition. In addition if it was prior set
to NULL and data was received, this data would be lost.

Thanks
- Markus Probst


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]