On Fri, May 26, 2023 at 11:34:44AM -0700, Roy Luo wrote: > On Thu, May 25, 2023 at 5:50 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > > > > On Thu, May 25, 2023 at 01:31:17PM -0700, Roy Luo wrote: > > > Currently the usb core assumes the usb_device that port_dev->child points > > > to is enumerated and port_dev->child->dev is registered when > > > port_dev->child is present. Setting port_dev->child early would break this > > > fundamental assumption, hence I'm a bit reluctant to go this way. > > > > Well, you could remove that assumption by adding a "child_is_registered" > > flag and explicitly checking it. > > > > Alan Stern > > Agree that's doable, with the following overheads: > 1. We can no longer safely access port_dev->child without checking > "child_is_registered", and there are plenty of places in the usb core that > touch port_dev->child. The implicit assumption could also hurt code > maintainability. That doesn't sound like a big deal. Currently you can't safely access port_dev->child without checking whether it is non-NULL. You would just have to replace one check with another. The fact that plenty of places touch port_dev->child indicates someone must have given some thought to protection against racing accesses. Your modifications should be able to benefit from that thought. > 2. In the worst case where enumeration keeps failing, the retry loop in > hub_port_connect() would frequently hold device_state_lock in order > to link/unlink the usb_device to port_dev->child. > This would definitely make sense if more places need port_dev->child early. > However, we only need port_dev->child->state at this point, so it does not > seem like a good deal to me. Another alternative -- possibly a much simpler one -- is to replicate port_dev->child->state in port_dev->state, purely for use by the new sysfs routine. In other words, keep the actual value there instead of a pointer to some other location that might get deallocated at any time. Since presumably you don't care about precise synchronization (that is, it doesn't matter if the value shown in the sysfs file is a few tens of milliseconds out of date) you could do this without extra locking. Just use WRITE_ONCE() for updates and READ_ONCE() to see what the current value is. Alan Stern
