Messages in this thread | | | Date | Tue, 1 Mar 2022 15:03:33 +0000 | Subject | Re: [PATCH] coresight: Defer probe when the child dev is not probed | From | Suzuki K Poulose <> |
| |
Hi
On 01/03/2022 13:30, Jinlong Mao wrote: > Hi Mike, > > On 3/1/2022 9:15 PM, Mike Leach wrote: >> Hi, >> >> On Tue, 1 Mar 2022 at 11:42, Jinlong Mao <quic_jinlmao@quicinc.com> >> wrote: >>> On 2/28/2022 10:51 PM, Suzuki K Poulose wrote: >>>
...
>>> >>> Hi Suzuki, >>> >>> This issue happens when race condition happens. >>> The condition is that the device and its child_device's probe happens >>> at the same time. >>> >>> For example: device0 and its child device device1. >>> Both of them are calling coresight_register function. device0 is >>> calling coresight_fixup_device_conns. >>> device1 is waiting for device0 to release the coresight_mutex. >>> Because device1's csdev node is allocated, >>> coresight_make_links will be called for device0. Then in >>> coresight_add_sysfs_link, has_conns_grp is true >>> for device0, but has_conns_grp is false for device1 as has_conns_grp >>> is set to true in coresight_create_conns_sysfs_group . >>> The probe of device0 will fail for at this condition. >>> >>> >>> struct coresight_device *coresight_register(struct coresight_desc *desc) >>> { >>> ......... >>> mutex_lock(&coresight_mutex); >>> >>> ret = coresight_create_conns_sysfs_group(csdev); >>> if (!ret) >>> ret = coresight_fixup_device_conns(csdev); >>> if (!ret) >>> ret = coresight_fixup_orphan_conns(csdev); >>> if (!ret && cti_assoc_ops && cti_assoc_ops->add) >>> cti_assoc_ops->add(csdev); >>> >>> mutex_unlock(&coresight_mutex); >>> >>> ......... >>> >>> } >>> >>> static int coresight_fixup_device_conns(struct coresight_device *csdev) >>> { >>> .......... >>> conn->child_dev = >>> coresight_find_csdev_by_fwnode(conn->child_fwnode); >> The issue appears to be a constraint hidden in the lower layers of the >> code. >> Would a better solution not be to alter the code here: >> >> if (conn->child_dev && conn->child_dev->has_conns_grp) { >> ... >> } else { >> csdev->orphan = true; >> } >> >> which would mean that the connection attempt would drop through to >> label the connection as an orphan, to be cleaned up by the child >> itself when it runs coresight_fixup_orphan_conns() >>
Tnanks Mike, I think that is a good solution. Alternatively, we could make sure that device_register() and the fixup following that are atomic.
i.e.
mutex_lock()
device_register() fixup_connections() create_sysfs()
mutex_unlock();
The fix may be a bit invasive than Mike's proposal, but it makes sure we don't end up with half baked device on the coresight-bus.
Suzuki
| |