| Message ID | 20210713073946.102501-1-martin.kepplinger@puri.sm (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: tipd: Don't block probing of consumer of "connector" nodes | expand |
+Rafael, Saravana On Tue, Jul 13, 2021 at 09:39:46AM +0200, Martin Kepplinger wrote: > Similar as with tcpm this patch lets fw_devlink know not to wait on the > fwnode to be populated as a struct device. > > Without this patch, USB functionality can be broken on some previously > supported boards. > > Fixes: 28ec344bb891 ("usb: typec: tcpm: Don't block probing of consumers of "connector" nodes") That patch has gone under my radar. > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm> > --- > hi, > > at least the boards based on imx8mq-librem5.dtsi suffer from this, possibly > more. > > thanks, > martin > > > drivers/usb/typec/tipd/core.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c > index 938219bc1b4b..b31aaf57eb3b 100644 > --- a/drivers/usb/typec/tipd/core.c > +++ b/drivers/usb/typec/tipd/core.c > @@ -629,6 +629,8 @@ static int tps6598x_probe(struct i2c_client *client) > if (!fwnode) > return -ENODEV; > > + fw_devlink_purge_absent_suppliers(fwnode); Why do we have to care about this kind of stuff in the drivers? It sounds like something that only affects DT platforms, so why isn't this being fixed in the DT core code? I didn't have time to study this fw_deflink thing yet, it is completely new to me and unfortunately I'm going on vacation now, but it looks like something that has been added to the driver core in a haste without enough thinking, and which is now causing problems. Not cool. thanks,
On Tue, Jul 13, 2021 at 1:58 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > +Rafael, Saravana > > On Tue, Jul 13, 2021 at 09:39:46AM +0200, Martin Kepplinger wrote: > > Similar as with tcpm this patch lets fw_devlink know not to wait on the > > fwnode to be populated as a struct device. > > > > Without this patch, USB functionality can be broken on some previously > > supported boards. > > > > Fixes: 28ec344bb891 ("usb: typec: tcpm: Don't block probing of consumers of "connector" nodes") > > That patch has gone under my radar. > > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm> > > --- > > hi, > > > > at least the boards based on imx8mq-librem5.dtsi suffer from this, possibly > > more. > > > > thanks, > > martin > > > > > > drivers/usb/typec/tipd/core.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c > > index 938219bc1b4b..b31aaf57eb3b 100644 > > --- a/drivers/usb/typec/tipd/core.c > > +++ b/drivers/usb/typec/tipd/core.c > > @@ -629,6 +629,8 @@ static int tps6598x_probe(struct i2c_client *client) > > if (!fwnode) > > return -ENODEV; > > > > + fw_devlink_purge_absent_suppliers(fwnode); Martin, Please include the comment from 28ec344bb891 that explains why the function is called. > > Why do we have to care about this kind of stuff in the drivers? It > sounds like something that only affects DT platforms, so why isn't > this being fixed in the DT core code? > > I didn't have time to study this fw_deflink thing yet, To give some context, fw_devlink (it's NOT limited to DT, but currently only implemented for DT) parses the firmware and infers the inter-device dependencies. And it creates device links between supplier and consumer devices and that has a lot of benefits: No initcall chicken between drivers, no/lot less deferred probing, allowing more drivers to be modules, support for sync_state() ops in drivers, allowing massively asynchronous probing, etc. There's a lot of nuance and corner cases on how the dependencies are inferred (without any help from drivers), but simplifying the explanation to talk about the stuff relevant to this patch: fw_devlink expects all nodes with "compatible" property to be probed by a driver. That's what the "compatible" property is used by Linux for. In this specific case, instead of probing the "connector" with a driver, the USB framework/this driver directly parses the node based on the name (why even have the "compatible" property then?) and uses the info from the node and never probes this DT node. Since this driver/DT node doesn't follow the norms, this specific driver needs to inform fw_devlink that this is the case (that it's not going to probe this DT node). That's what this one line call is doing. > it is > completely new to me and unfortunately I'm going on vacation now, but > it looks like something that has been added to the driver core in a > haste without enough thinking, and which is now causing problems. Not > cool. I'm sorry that you didn't get a chance to look into fw_devlink before, but it certainly wasn't done in haste :) It took close to 2 years to get this in and I talked about this in at least 2 of the LPCs. It also had a trial run to catch issues from 5.12-rc1 (or something early like that) till 5.12-rc7 and then reverted before 5.12 release. And then finally enabled again in 5.13-rc1 and merged in 5.13. -Saravana
diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c index 938219bc1b4b..b31aaf57eb3b 100644 --- a/drivers/usb/typec/tipd/core.c +++ b/drivers/usb/typec/tipd/core.c @@ -629,6 +629,8 @@ static int tps6598x_probe(struct i2c_client *client) if (!fwnode) return -ENODEV; + fw_devlink_purge_absent_suppliers(fwnode); + tps->role_sw = fwnode_usb_role_switch_get(fwnode); if (IS_ERR(tps->role_sw)) { ret = PTR_ERR(tps->role_sw);
Similar as with tcpm this patch lets fw_devlink know not to wait on the fwnode to be populated as a struct device. Without this patch, USB functionality can be broken on some previously supported boards. Fixes: 28ec344bb891 ("usb: typec: tcpm: Don't block probing of consumers of "connector" nodes") Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm> --- hi, at least the boards based on imx8mq-librem5.dtsi suffer from this, possibly more. thanks, martin drivers/usb/typec/tipd/core.c | 2 ++ 1 file changed, 2 insertions(+)