Re: [PATCH] platform/chrome: Add ChromeOS EC USB driver

[Dawid Niedźwiecki]

> They doesn't look like shortcomings to me. The corresponding destructor
> callbacks have to be called when a device is removed anyway.
>

Yes, anything related to USB communication itself has to be freed, but
freeing the cros_ec_device structure and calling the cros_ec_unregister
function can crash the system if any of a userland application has the
original file cros_ec open and tries to send a command. The chardev driver
is not aware that the device has been removed and will try to access the
removed structures.

> Instead, re-using the same inode for the userland interface however
> *silently* swapping the underlying devices makes less sense to me.
>

We are sure it is the same EC device, so why do you think accessing the
same inode makes less sense? It is a case for specific interface, that reboot
causes reprobing, which is not a case for other interfaces. I believe
it should be
transparent for the cros_ec device user, what interface type is used
e.g. if it is
SPI, you can reboot the EC device and wait until it responds to a next command,
but if it is USB, you would need to reopen the cros_ec file after reboot.

> My point here is: to let userland programs be aware of the underlying
> device has been removed.
>

I got your point, but they are informed with a proper return code. The
host command
communication doesn't have context. Every command is independent, so userland
programs can not assume any state of the EC device e.g. reboot can
happen. I think
behaviour of the cros_ec file is not a good way to inform the userland
programs that
there were reboot, sysjump etc. There are special commands/events for that.

> What are the reasons for disconnect and reprobe? From your previous message
> and code comment, are they "reboot, sysjump, crash"? Would it happen
> frequently after AP boot?
>

Yes, every boot "from the beginning" , including sysjumps. It depends
on a EC device
type, but it can happen e.g. after AP reboot (RWSIG jumps to RW, it
depends on boot time),
during firmware update procedure, rollback region update etc.

> Note: we are talking about a EC-like device here. For a real EC either reboot
> or crash, the AP will be reset.

That's true. The problem doesn't exist for the real EC device.

> No, I don't think so. I think all EC-like devices share the same concern
> regardless of the transport medium (e.g. SCP over RPMSG, ISH over ISHTP).

Yes, you can run a userland program that opens a cros_ec file and constantly
sends commands e.g. ectool stress, and then manually unbind the ec
device, but I believe it can cause some crashes/memory leakage. I don't think
all drivers are adjusted to hot-plugging.