| Message ID | 20250113-topic-spmi_node_breakage-v2-1-dd35a3a6daa6@oss.qualcomm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] spmi: Fix controller->node != parent->node breakage | expand |
Il 13/01/25 14:02, Konrad Dybcio ha scritto: > From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > > On some platforms, like recent Qualcomm SoCs with multi-bus SPMI > arbiters, controller->node must be assigned to the individual buses' > subnodes, as the slave devices are children of these, like so: > > arbiter@c400000 > spmi@c42d000 > pmic@0 > > spmi@c432000 > pmic@0 > > The commit referenced in Fixes changed that assignment, such that > spmi_controller_alloc() always assumes the PMICs come directly under > the arbiter node (which is true when there's only a single bus per > controller). > > Make controller->node specifiable to both benefit from Joe's refcount > improvements and un-break the aforementioned platforms. > > Fixes: 821b07853e32 ("spmi: hisi-spmi-controller: manage the OF node reference in device initialization and cleanup") > Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> for spmi.c/.h, spmi-devres and for MediaTek: Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Cheers!
Quoting Konrad Dybcio (2025-01-13 05:02:58) > From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > > On some platforms, like recent Qualcomm SoCs with multi-bus SPMI > arbiters, controller->node must be assigned to the individual buses' > subnodes, as the slave devices are children of these, like so: > > arbiter@c400000 > spmi@c42d000 > pmic@0 > > spmi@c432000 > pmic@0 > > The commit referenced in Fixes changed that assignment, such that > spmi_controller_alloc() always assumes the PMICs come directly under > the arbiter node (which is true when there's only a single bus per > controller). > > Make controller->node specifiable to both benefit from Joe's refcount > improvements and un-break the aforementioned platforms. How is it broken? I see spmi_pmic_arb_bus_init() calls devm_spmi_controller_alloc() which sets the of_node to the parent device and then spmi_pmic_arb_bus_init() overwrites that with 'ctrl->dev.of_node = node' later on in the same function. That will cause one more of_node_put() than is expected. I don't see that removed in this patch though, so the leak is still there? > > Fixes: 821b07853e32 ("spmi: hisi-spmi-controller: manage the OF node reference in device initialization and cleanup") I've dropped this patch from my queue. I don't know if we're really doing anything better by managing the of_node lifetime in that function vs. letting the callers assign the node they want and manage the lifetime themselves. Maybe we don't need to do anything? Presumably the parent device driver will unregister the controller anyway, so the lifetime of the of_node will be ensured regardless. For subnodes like qcom SPMI, the subnodes are child nodes of the parent device so they won't be removed. If they are dynamic nodes, then the caller can manage the lifetime.
On 1/13/25 22:52, Stephen Boyd wrote: > Quoting Konrad Dybcio (2025-01-13 05:02:58) >> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> >> >> On some platforms, like recent Qualcomm SoCs with multi-bus SPMI >> arbiters, controller->node must be assigned to the individual buses' >> subnodes, as the slave devices are children of these, like so: >> >> arbiter@c400000 >> spmi@c42d000 >> pmic@0 >> >> spmi@c432000 >> pmic@0 >> >> The commit referenced in Fixes changed that assignment, such that >> spmi_controller_alloc() always assumes the PMICs come directly under >> the arbiter node (which is true when there's only a single bus per >> controller). >> >> Make controller->node specifiable to both benefit from Joe's refcount >> improvements and un-break the aforementioned platforms. > > How is it broken? I see spmi_pmic_arb_bus_init() calls > devm_spmi_controller_alloc() which sets the of_node to the parent device > and then spmi_pmic_arb_bus_init() overwrites that with > 'ctrl->dev.of_node = node' later on in the same function. That will > cause one more of_node_put() than is expected. I don't see that removed > in this patch though, so the leak is still there? > >> >> Fixes: 821b07853e32 ("spmi: hisi-spmi-controller: manage the OF node reference in device initialization and cleanup") > > I've dropped this patch from my queue. I don't know if we're really > doing anything better by managing the of_node lifetime in that function > vs. letting the callers assign the node they want and manage the > lifetime themselves. Maybe we don't need to do anything? Presumably the > parent device driver will unregister the controller anyway, so the > lifetime of the of_node will be ensured regardless. For subnodes like > qcom SPMI, the subnodes are child nodes of the parent device so they > won't be removed. If they are dynamic nodes, then the caller can manage > the lifetime. Stephen, the wrong node gets assigned in the qcom driver with multi-master controllers, resulting in probe failures. Since the introduction of the commit referenced in fixes, of_spmi_register_devices() sees the controller's subnodes (which describe each of the two masters) as slave devices - meaning no "real" devices ever get to probe Konrad
Quoting Konrad Dybcio (2025-01-13 14:12:28) > > On 1/13/25 22:52, Stephen Boyd wrote: > > Quoting Konrad Dybcio (2025-01-13 05:02:58) > >> > >> Make controller->node specifiable to both benefit from Joe's refcount > >> improvements and un-break the aforementioned platforms. > > > > How is it broken? I see spmi_pmic_arb_bus_init() calls > > devm_spmi_controller_alloc() which sets the of_node to the parent device > > and then spmi_pmic_arb_bus_init() overwrites that with > > 'ctrl->dev.of_node = node' later on in the same function. That will > > cause one more of_node_put() than is expected. I don't see that removed > > in this patch though, so the leak is still there? > > > >> > >> Fixes: 821b07853e32 ("spmi: hisi-spmi-controller: manage the OF node reference in device initialization and cleanup") > > > > I've dropped this patch from my queue. I don't know if we're really > > doing anything better by managing the of_node lifetime in that function > > vs. letting the callers assign the node they want and manage the > > lifetime themselves. Maybe we don't need to do anything? Presumably the > > parent device driver will unregister the controller anyway, so the > > lifetime of the of_node will be ensured regardless. For subnodes like > > qcom SPMI, the subnodes are child nodes of the parent device so they > > won't be removed. If they are dynamic nodes, then the caller can manage > > the lifetime. > > Stephen, the wrong node gets assigned in the qcom driver with > multi-master controllers, resulting in probe failures. > > Since the introduction of the commit referenced in fixes, > of_spmi_register_devices() sees the controller's subnodes > (which describe each of the two masters) as slave devices > - meaning no "real" devices ever get to probe > Ok, I see that I was reading the already reverted state of the tree where the 'ctrl->dev.of_node = node' assignment came back. So there's nothing to do besides drop the patch in fixes, which I already did. We can then apply this patch to drop the duplicate assignment as a cleanup and to avoid a refcount bump on the of_node that isn't needed. ----8<--- diff --git a/drivers/spmi/hisi-spmi-controller.c b/drivers/spmi/hisi-spmi-controller.c index 3cafdf22c909..122140b97579 100644 --- a/drivers/spmi/hisi-spmi-controller.c +++ b/drivers/spmi/hisi-spmi-controller.c @@ -300,9 +300,6 @@ static int spmi_controller_probe(struct platform_device *pdev) spin_lock_init(&spmi_controller->lock); - ctrl->dev.parent = pdev->dev.parent; - ctrl->dev.of_node = of_node_get(pdev->dev.of_node); - /* Callbacks */ ctrl->read_cmd = spmi_read_cmd; ctrl->write_cmd = spmi_write_cmd;
On 1/14/25 19:30, Stephen Boyd wrote: > Quoting Konrad Dybcio (2025-01-13 14:12:28) >> >> On 1/13/25 22:52, Stephen Boyd wrote: >>> Quoting Konrad Dybcio (2025-01-13 05:02:58) >>>> >>>> Make controller->node specifiable to both benefit from Joe's refcount >>>> improvements and un-break the aforementioned platforms. >>> >>> How is it broken? I see spmi_pmic_arb_bus_init() calls >>> devm_spmi_controller_alloc() which sets the of_node to the parent device >>> and then spmi_pmic_arb_bus_init() overwrites that with >>> 'ctrl->dev.of_node = node' later on in the same function. That will >>> cause one more of_node_put() than is expected. I don't see that removed >>> in this patch though, so the leak is still there? >>> >>>> >>>> Fixes: 821b07853e32 ("spmi: hisi-spmi-controller: manage the OF node reference in device initialization and cleanup") >>> >>> I've dropped this patch from my queue. I don't know if we're really >>> doing anything better by managing the of_node lifetime in that function >>> vs. letting the callers assign the node they want and manage the >>> lifetime themselves. Maybe we don't need to do anything? Presumably the >>> parent device driver will unregister the controller anyway, so the >>> lifetime of the of_node will be ensured regardless. For subnodes like >>> qcom SPMI, the subnodes are child nodes of the parent device so they >>> won't be removed. If they are dynamic nodes, then the caller can manage >>> the lifetime. >> >> Stephen, the wrong node gets assigned in the qcom driver with >> multi-master controllers, resulting in probe failures. >> >> Since the introduction of the commit referenced in fixes, >> of_spmi_register_devices() sees the controller's subnodes >> (which describe each of the two masters) as slave devices >> - meaning no "real" devices ever get to probe >> > > Ok, I see that I was reading the already reverted state of the tree > where the 'ctrl->dev.of_node = node' assignment came back. So there's > nothing to do besides drop the patch in fixes, which I already did. We > can then apply this patch to drop the duplicate assignment as a cleanup > and to avoid a refcount bump on the of_node that isn't needed. > > ----8<--- > diff --git a/drivers/spmi/hisi-spmi-controller.c b/drivers/spmi/hisi-spmi-controller.c > index 3cafdf22c909..122140b97579 100644 > --- a/drivers/spmi/hisi-spmi-controller.c > +++ b/drivers/spmi/hisi-spmi-controller.c > @@ -300,9 +300,6 @@ static int spmi_controller_probe(struct platform_device *pdev) > > spin_lock_init(&spmi_controller->lock); > > - ctrl->dev.parent = pdev->dev.parent; > - ctrl->dev.of_node = of_node_get(pdev->dev.of_node); > - > /* Callbacks */ > ctrl->read_cmd = spmi_read_cmd; > ctrl->write_cmd = spmi_write_cmd; That works for me, thank you Konrad
diff --git a/drivers/spmi/hisi-spmi-controller.c b/drivers/spmi/hisi-spmi-controller.c index dd21c5d1ca8301d508b85dfaf61ddfabed17aca9..030b4f86af6329d1c5694ec6440aceaa60f563ad 100644 --- a/drivers/spmi/hisi-spmi-controller.c +++ b/drivers/spmi/hisi-spmi-controller.c @@ -267,7 +267,9 @@ static int spmi_controller_probe(struct platform_device *pdev) struct resource *iores; int ret; - ctrl = devm_spmi_controller_alloc(&pdev->dev, sizeof(*spmi_controller)); + ctrl = devm_spmi_controller_alloc(&pdev->dev, + pdev->dev.of_node, + sizeof(*spmi_controller)); if (IS_ERR(ctrl)) { dev_err(&pdev->dev, "can not allocate spmi_controller data\n"); return PTR_ERR(ctrl); diff --git a/drivers/spmi/spmi-devres.c b/drivers/spmi/spmi-devres.c index 62c4b3f24d0656eea9b6da489b7716b9965bedbe..e84af711714d1892a5781111dc538747f5a5e835 100644 --- a/drivers/spmi/spmi-devres.c +++ b/drivers/spmi/spmi-devres.c @@ -11,7 +11,9 @@ static void devm_spmi_controller_release(struct device *parent, void *res) spmi_controller_put(*(struct spmi_controller **)res); } -struct spmi_controller *devm_spmi_controller_alloc(struct device *parent, size_t size) +struct spmi_controller *devm_spmi_controller_alloc(struct device *parent, + struct device_node *node, + size_t size) { struct spmi_controller **ptr, *ctrl; @@ -19,7 +21,7 @@ struct spmi_controller *devm_spmi_controller_alloc(struct device *parent, size_t if (!ptr) return ERR_PTR(-ENOMEM); - ctrl = spmi_controller_alloc(parent, size); + ctrl = spmi_controller_alloc(parent, node, size); if (IS_ERR(ctrl)) { devres_free(ptr); return ctrl; diff --git a/drivers/spmi/spmi-mtk-pmif.c b/drivers/spmi/spmi-mtk-pmif.c index 160d36f7d238e7eb4122091a53b779c18e9d91a4..8cff60f68f98a9d59d8f4423d746018135514e41 100644 --- a/drivers/spmi/spmi-mtk-pmif.c +++ b/drivers/spmi/spmi-mtk-pmif.c @@ -453,7 +453,9 @@ static int mtk_spmi_probe(struct platform_device *pdev) int err, i; u32 chan_offset; - ctrl = devm_spmi_controller_alloc(&pdev->dev, sizeof(*arb)); + ctrl = devm_spmi_controller_alloc(&pdev->dev, + pdev->dev.of_node, + sizeof(*arb)); if (IS_ERR(ctrl)) return PTR_ERR(ctrl); diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c index 73f2f19737f8cec266a051a956ce2123661a714e..226f51d94a70328c6322664d2d05a8b64645674a 100644 --- a/drivers/spmi/spmi-pmic-arb.c +++ b/drivers/spmi/spmi-pmic-arb.c @@ -1674,7 +1674,7 @@ static int spmi_pmic_arb_bus_init(struct platform_device *pdev, int index, ret; int irq; - ctrl = devm_spmi_controller_alloc(dev, sizeof(*bus)); + ctrl = devm_spmi_controller_alloc(dev, node, sizeof(*bus)); if (IS_ERR(ctrl)) return PTR_ERR(ctrl); diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c index 166beb2083a3f801435d8ffd843310582911e3ab..c1a03da55a265c9294f6a16bc285310cebd00726 100644 --- a/drivers/spmi/spmi.c +++ b/drivers/spmi/spmi.c @@ -435,6 +435,7 @@ EXPORT_SYMBOL_GPL(spmi_device_alloc); /** * spmi_controller_alloc() - Allocate a new SPMI controller * @parent: parent device + * @node: device node to associate with the controller (usually parent->of_node) * @size: size of private data * * Caller is responsible for either calling spmi_controller_add() to add the @@ -443,6 +444,7 @@ EXPORT_SYMBOL_GPL(spmi_device_alloc); * spmi_controller_get_drvdata() */ struct spmi_controller *spmi_controller_alloc(struct device *parent, + struct device_node *node, size_t size) { struct spmi_controller *ctrl; @@ -459,7 +461,7 @@ struct spmi_controller *spmi_controller_alloc(struct device *parent, ctrl->dev.type = &spmi_ctrl_type; ctrl->dev.bus = &spmi_bus_type; ctrl->dev.parent = parent; - device_set_node(&ctrl->dev, of_fwnode_handle(of_node_get(parent->of_node))); + device_set_node(&ctrl->dev, of_fwnode_handle(of_node_get(node))); spmi_controller_set_drvdata(ctrl, &ctrl[1]); id = ida_alloc(&ctrl_ida, GFP_KERNEL); diff --git a/include/linux/spmi.h b/include/linux/spmi.h index 28e8c8bd39441fa6451be3364006fb3b47a47dc9..9de74000911456800237a3244d5e8158fadf8317 100644 --- a/include/linux/spmi.h +++ b/include/linux/spmi.h @@ -105,6 +105,7 @@ static inline void spmi_controller_set_drvdata(struct spmi_controller *ctrl, } struct spmi_controller *spmi_controller_alloc(struct device *parent, + struct device_node *node, size_t size); /** @@ -120,7 +121,9 @@ static inline void spmi_controller_put(struct spmi_controller *ctrl) int spmi_controller_add(struct spmi_controller *ctrl); void spmi_controller_remove(struct spmi_controller *ctrl); -struct spmi_controller *devm_spmi_controller_alloc(struct device *parent, size_t size); +struct spmi_controller *devm_spmi_controller_alloc(struct device *parent, + struct device_node *node, + size_t size); int devm_spmi_controller_add(struct device *parent, struct spmi_controller *ctrl); /**