* [PATCH v4 0/2] remoteproc: get rproc devices for clusters @ 2024-01-03 22:11 Tanmay Shah 2024-01-03 22:11 ` [PATCH v4 1/2] remoteproc: Make rproc_get_by_phandle() work " Tanmay Shah 2024-01-03 22:11 ` [PATCH v4 2/2] remoteproc: enhance rproc_put() " Tanmay Shah 0 siblings, 2 replies; 7+ messages in thread From: Tanmay Shah @ 2024-01-03 22:11 UTC (permalink / raw) To: andersson, mathieu.poirier; +Cc: linux-remoteproc, linux-kernel, Tanmay Shah This series extends original patch and makes rproc_put() work for clusters along with rprog_get_by_phandle(). Changes in v4: - Retrieve cluster device using rproc->dev.parent->parent relation Changes in v3: - remove module_put call that was introduced in the patch by mistake - remove redundant check in rproc_put - Add inline comments in rproc_put that explains functionality Changes in v2: - Introduce patch to fix rproc_put as per modified rproc_get_by_phandle v1: https://lore.kernel.org/all/20221214221643.1286585-1-mathieu.poirier@linaro.org/ Mathieu Poirier (1): remoteproc: Make rproc_get_by_phandle() work for clusters Tanmay Shah (1): remoteproc: enhance rproc_put() for clusters drivers/remoteproc/remoteproc_core.c | 29 ++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) base-commit: ff9af5732fe761fa8e7aa66cb482f93a37e284ee -- 2.25.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4 1/2] remoteproc: Make rproc_get_by_phandle() work for clusters 2024-01-03 22:11 [PATCH v4 0/2] remoteproc: get rproc devices for clusters Tanmay Shah @ 2024-01-03 22:11 ` Tanmay Shah 2024-01-15 18:14 ` Mathieu Poirier 2024-01-26 17:33 ` Bjorn Andersson 2024-01-03 22:11 ` [PATCH v4 2/2] remoteproc: enhance rproc_put() " Tanmay Shah 1 sibling, 2 replies; 7+ messages in thread From: Tanmay Shah @ 2024-01-03 22:11 UTC (permalink / raw) To: andersson, mathieu.poirier; +Cc: linux-remoteproc, linux-kernel, Ben Levinsky From: Mathieu Poirier <mathieu.poirier@linaro.org> Multi-cluster remoteproc designs typically have the following DT declaration: remoteproc_cluster { compatible = "soc,remoteproc-cluster"; core0: core0 { compatible = "soc,remoteproc-core" memory-region; sram; }; core1: core1 { compatible = "soc,remoteproc-core" memory-region; sram; } }; A driver exists for the cluster rather than the individual cores themselves so that operation mode and HW specific configurations applicable to the cluster can be made. Because the driver exists at the cluster level and not the individual core level, function rproc_get_by_phandle() fails to return the remoteproc associated with the phandled it is called for. This patch enhances rproc_get_by_phandle() by looking for the cluster's driver when the driver for the immediate remoteproc's parent is not found. Reported-by: Ben Levinsky <ben.levinsky@xilinx.com> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> --- drivers/remoteproc/remoteproc_core.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 695cce218e8c..0b3b34085e2f 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -33,6 +33,7 @@ #include <linux/idr.h> #include <linux/elf.h> #include <linux/crc32.h> +#include <linux/of_platform.h> #include <linux/of_reserved_mem.h> #include <linux/virtio_ids.h> #include <linux/virtio_ring.h> @@ -2112,6 +2113,7 @@ EXPORT_SYMBOL(rproc_detach); struct rproc *rproc_get_by_phandle(phandle phandle) { struct rproc *rproc = NULL, *r; + struct device_driver *driver; struct device_node *np; np = of_find_node_by_phandle(phandle); @@ -2122,7 +2124,26 @@ struct rproc *rproc_get_by_phandle(phandle phandle) list_for_each_entry_rcu(r, &rproc_list, node) { if (r->dev.parent && device_match_of_node(r->dev.parent, np)) { /* prevent underlying implementation from being removed */ - if (!try_module_get(r->dev.parent->driver->owner)) { + + /* + * If the remoteproc's parent has a driver, the + * remoteproc is not part of a cluster and we can use + * that driver. + */ + driver = r->dev.parent->driver; + + /* + * If the remoteproc's parent does not have a driver, + * look for the driver associated with the cluster. + */ + if (!driver) { + if (r->dev.parent->parent) + driver = r->dev.parent->parent->driver; + if (!driver) + break; + } + + if (!try_module_get(driver->owner)) { dev_err(&r->dev, "can't get owner\n"); break; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4 1/2] remoteproc: Make rproc_get_by_phandle() work for clusters 2024-01-03 22:11 ` [PATCH v4 1/2] remoteproc: Make rproc_get_by_phandle() work " Tanmay Shah @ 2024-01-15 18:14 ` Mathieu Poirier 2024-01-26 17:33 ` Bjorn Andersson 1 sibling, 0 replies; 7+ messages in thread From: Mathieu Poirier @ 2024-01-15 18:14 UTC (permalink / raw) To: Tanmay Shah; +Cc: andersson, linux-remoteproc, linux-kernel, Ben Levinsky Hi Tanmay, Thanks for the refactoring, this is in line with what Bjorn and I have talked about at Plumbers. Please see my comments below. On Wed, Jan 03, 2024 at 02:11:24PM -0800, Tanmay Shah wrote: > From: Mathieu Poirier <mathieu.poirier@linaro.org> > > Multi-cluster remoteproc designs typically have the following DT > declaration: > > remoteproc_cluster { > compatible = "soc,remoteproc-cluster"; > > core0: core0 { > compatible = "soc,remoteproc-core" > memory-region; > sram; > }; > > core1: core1 { > compatible = "soc,remoteproc-core" > memory-region; > sram; > } > }; > > A driver exists for the cluster rather than the individual cores > themselves so that operation mode and HW specific configurations > applicable to the cluster can be made. > > Because the driver exists at the cluster level and not the individual > core level, function rproc_get_by_phandle() fails to return the > remoteproc associated with the phandled it is called for. > > This patch enhances rproc_get_by_phandle() by looking for the cluster's > driver when the driver for the immediate remoteproc's parent is not > found. > > Reported-by: Ben Levinsky <ben.levinsky@xilinx.com> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> Humm... You wrote the code in this patch so you also deserve some credit. If I end up applying this set I will add myself as a co-developer, i.e Co-developed-by:, and add your SoB. If you end up re-spinning this set then simply do so for the next revision. As far as I am concerned this patchset is ready. I will wait to see if other people would like to see something adjusted. Mathieu > --- > drivers/remoteproc/remoteproc_core.c | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 695cce218e8c..0b3b34085e2f 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -33,6 +33,7 @@ > #include <linux/idr.h> > #include <linux/elf.h> > #include <linux/crc32.h> > +#include <linux/of_platform.h> > #include <linux/of_reserved_mem.h> > #include <linux/virtio_ids.h> > #include <linux/virtio_ring.h> > @@ -2112,6 +2113,7 @@ EXPORT_SYMBOL(rproc_detach); > struct rproc *rproc_get_by_phandle(phandle phandle) > { > struct rproc *rproc = NULL, *r; > + struct device_driver *driver; > struct device_node *np; > > np = of_find_node_by_phandle(phandle); > @@ -2122,7 +2124,26 @@ struct rproc *rproc_get_by_phandle(phandle phandle) > list_for_each_entry_rcu(r, &rproc_list, node) { > if (r->dev.parent && device_match_of_node(r->dev.parent, np)) { > /* prevent underlying implementation from being removed */ > - if (!try_module_get(r->dev.parent->driver->owner)) { > + > + /* > + * If the remoteproc's parent has a driver, the > + * remoteproc is not part of a cluster and we can use > + * that driver. > + */ > + driver = r->dev.parent->driver; > + > + /* > + * If the remoteproc's parent does not have a driver, > + * look for the driver associated with the cluster. > + */ > + if (!driver) { > + if (r->dev.parent->parent) > + driver = r->dev.parent->parent->driver; > + if (!driver) > + break; > + } > + > + if (!try_module_get(driver->owner)) { > dev_err(&r->dev, "can't get owner\n"); > break; > } > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 1/2] remoteproc: Make rproc_get_by_phandle() work for clusters 2024-01-03 22:11 ` [PATCH v4 1/2] remoteproc: Make rproc_get_by_phandle() work " Tanmay Shah 2024-01-15 18:14 ` Mathieu Poirier @ 2024-01-26 17:33 ` Bjorn Andersson 1 sibling, 0 replies; 7+ messages in thread From: Bjorn Andersson @ 2024-01-26 17:33 UTC (permalink / raw) To: Tanmay Shah; +Cc: mathieu.poirier, linux-remoteproc, linux-kernel, Ben Levinsky On Wed, Jan 03, 2024 at 02:11:24PM -0800, Tanmay Shah wrote: > From: Mathieu Poirier <mathieu.poirier@linaro.org> > > Multi-cluster remoteproc designs typically have the following DT > declaration: > > remoteproc_cluster { > compatible = "soc,remoteproc-cluster"; > > core0: core0 { > compatible = "soc,remoteproc-core" > memory-region; > sram; > }; > > core1: core1 { > compatible = "soc,remoteproc-core" > memory-region; > sram; > } > }; The indention of this snippet looks weird in my client, because it contains a mixture of tabs and spaces. Please clean that up, and while at it, '_' is not a valid character in DT node names... > > A driver exists for the cluster rather than the individual cores > themselves so that operation mode and HW specific configurations > applicable to the cluster can be made. > > Because the driver exists at the cluster level and not the individual > core level, function rproc_get_by_phandle() fails to return the > remoteproc associated with the phandled it is called for. > > This patch enhances rproc_get_by_phandle() by looking for the cluster's > driver when the driver for the immediate remoteproc's parent is not > found. > > Reported-by: Ben Levinsky <ben.levinsky@xilinx.com> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> The s-o-b is used to certify the origin of the patch, Mathieu provided his signature here, then as you handle the patch you need to append your s-o-b to provide the same certification. The for appropriate tracking of reality, Mathieu should append his s-o-b when/if he applies the patch. TL;DR please add your S-o-b after Mathieu's. Change itself looks good to me. Regards, Bjorn > --- > drivers/remoteproc/remoteproc_core.c | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 695cce218e8c..0b3b34085e2f 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -33,6 +33,7 @@ > #include <linux/idr.h> > #include <linux/elf.h> > #include <linux/crc32.h> > +#include <linux/of_platform.h> > #include <linux/of_reserved_mem.h> > #include <linux/virtio_ids.h> > #include <linux/virtio_ring.h> > @@ -2112,6 +2113,7 @@ EXPORT_SYMBOL(rproc_detach); > struct rproc *rproc_get_by_phandle(phandle phandle) > { > struct rproc *rproc = NULL, *r; > + struct device_driver *driver; > struct device_node *np; > > np = of_find_node_by_phandle(phandle); > @@ -2122,7 +2124,26 @@ struct rproc *rproc_get_by_phandle(phandle phandle) > list_for_each_entry_rcu(r, &rproc_list, node) { > if (r->dev.parent && device_match_of_node(r->dev.parent, np)) { > /* prevent underlying implementation from being removed */ > - if (!try_module_get(r->dev.parent->driver->owner)) { > + > + /* > + * If the remoteproc's parent has a driver, the > + * remoteproc is not part of a cluster and we can use > + * that driver. > + */ > + driver = r->dev.parent->driver; > + > + /* > + * If the remoteproc's parent does not have a driver, > + * look for the driver associated with the cluster. > + */ > + if (!driver) { > + if (r->dev.parent->parent) > + driver = r->dev.parent->parent->driver; > + if (!driver) > + break; > + } > + > + if (!try_module_get(driver->owner)) { > dev_err(&r->dev, "can't get owner\n"); > break; > } > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4 2/2] remoteproc: enhance rproc_put() for clusters 2024-01-03 22:11 [PATCH v4 0/2] remoteproc: get rproc devices for clusters Tanmay Shah 2024-01-03 22:11 ` [PATCH v4 1/2] remoteproc: Make rproc_get_by_phandle() work " Tanmay Shah @ 2024-01-03 22:11 ` Tanmay Shah 2024-01-26 17:38 ` Bjorn Andersson 1 sibling, 1 reply; 7+ messages in thread From: Tanmay Shah @ 2024-01-03 22:11 UTC (permalink / raw) To: andersson, mathieu.poirier Cc: linux-remoteproc, linux-kernel, Tanmay Shah, Tarak Reddy This patch enhances rproc_put() to support remoteproc clusters with multiple child nodes as in rproc_get_by_phandle(). Signed-off-by: Tarak Reddy <tarak.reddy@amd.com> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com> --- drivers/remoteproc/remoteproc_core.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 0b3b34085e2f..f276956f2c5c 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -2554,7 +2554,11 @@ EXPORT_SYMBOL(rproc_free); */ void rproc_put(struct rproc *rproc) { - module_put(rproc->dev.parent->driver->owner); + if (rproc->dev.parent->driver) + module_put(rproc->dev.parent->driver->owner); + else + module_put(rproc->dev.parent->parent->driver->owner); + put_device(&rproc->dev); } EXPORT_SYMBOL(rproc_put); -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4 2/2] remoteproc: enhance rproc_put() for clusters 2024-01-03 22:11 ` [PATCH v4 2/2] remoteproc: enhance rproc_put() " Tanmay Shah @ 2024-01-26 17:38 ` Bjorn Andersson 2024-01-29 17:43 ` Tanmay Shah 0 siblings, 1 reply; 7+ messages in thread From: Bjorn Andersson @ 2024-01-26 17:38 UTC (permalink / raw) To: Tanmay Shah; +Cc: mathieu.poirier, linux-remoteproc, linux-kernel, Tarak Reddy On Wed, Jan 03, 2024 at 02:11:25PM -0800, Tanmay Shah wrote: > This patch enhances rproc_put() to support remoteproc clusters > with multiple child nodes as in rproc_get_by_phandle(). > > Signed-off-by: Tarak Reddy <tarak.reddy@amd.com> > Signed-off-by: Tanmay Shah <tanmay.shah@amd.com> As described in the first patch, this documents that Tarak first certified the origin of this patch, then you certify the origin as you handle the patch. But according to From: you're the author, so how could Tarak have certified the origin before you authored the patch? Either correct the author, or add Co-developed-by, if that's what happened. > --- > drivers/remoteproc/remoteproc_core.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 0b3b34085e2f..f276956f2c5c 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -2554,7 +2554,11 @@ EXPORT_SYMBOL(rproc_free); > */ > void rproc_put(struct rproc *rproc) > { > - module_put(rproc->dev.parent->driver->owner); > + if (rproc->dev.parent->driver) > + module_put(rproc->dev.parent->driver->owner); > + else > + module_put(rproc->dev.parent->parent->driver->owner); > + This does however highlight a bug that was introduced by patch 1, please avoid this by squashing the two patches together (and use Co-developed-by as needed). Regards, Bjorn > put_device(&rproc->dev); > } > EXPORT_SYMBOL(rproc_put); > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 2/2] remoteproc: enhance rproc_put() for clusters 2024-01-26 17:38 ` Bjorn Andersson @ 2024-01-29 17:43 ` Tanmay Shah 0 siblings, 0 replies; 7+ messages in thread From: Tanmay Shah @ 2024-01-29 17:43 UTC (permalink / raw) To: Bjorn Andersson Cc: mathieu.poirier, linux-remoteproc, linux-kernel, Tarak Reddy On 1/26/24 11:38 AM, Bjorn Andersson wrote: > On Wed, Jan 03, 2024 at 02:11:25PM -0800, Tanmay Shah wrote: > > This patch enhances rproc_put() to support remoteproc clusters > > with multiple child nodes as in rproc_get_by_phandle(). > > > > Signed-off-by: Tarak Reddy <tarak.reddy@amd.com> > > Signed-off-by: Tanmay Shah <tanmay.shah@amd.com> > > As described in the first patch, this documents that Tarak first > certified the origin of this patch, then you certify the origin as you > handle the patch. > > But according to From: you're the author, so how could Tarak have > certified the origin before you authored the patch? > > Either correct the author, or add Co-developed-by, if that's what > happened. > > > --- > > drivers/remoteproc/remoteproc_core.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > index 0b3b34085e2f..f276956f2c5c 100644 > > --- a/drivers/remoteproc/remoteproc_core.c > > +++ b/drivers/remoteproc/remoteproc_core.c > > @@ -2554,7 +2554,11 @@ EXPORT_SYMBOL(rproc_free); > > */ > > void rproc_put(struct rproc *rproc) > > { > > - module_put(rproc->dev.parent->driver->owner); > > + if (rproc->dev.parent->driver) > > + module_put(rproc->dev.parent->driver->owner); > > + else > > + module_put(rproc->dev.parent->parent->driver->owner); > > + > > This does however highlight a bug that was introduced by patch 1, please > avoid this by squashing the two patches together (and use > Co-developed-by as needed). Thanks Bjorn for catching this. This change originally was developed by Tarak, but I sent upstream based on his patch so I missed to update his name as author. I should update author name. However, if we are going to squash this in first patch, then I think, first patch's author will stay as it is. Following Action Item on me for v5: 1) Fix commit text in first patch. 2) Squash second patch in first. 3) Add my s-o-b signature after Mathieu's 4) Add Tarak's s-o-b as well. As he developed second patch. Hope got it all. Thanks, Tanmay > > Regards, > Bjorn > > > put_device(&rproc->dev); > > } > > EXPORT_SYMBOL(rproc_put); > > -- > > 2.25.1 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-01-29 17:43 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-01-03 22:11 [PATCH v4 0/2] remoteproc: get rproc devices for clusters Tanmay Shah 2024-01-03 22:11 ` [PATCH v4 1/2] remoteproc: Make rproc_get_by_phandle() work " Tanmay Shah 2024-01-15 18:14 ` Mathieu Poirier 2024-01-26 17:33 ` Bjorn Andersson 2024-01-03 22:11 ` [PATCH v4 2/2] remoteproc: enhance rproc_put() " Tanmay Shah 2024-01-26 17:38 ` Bjorn Andersson 2024-01-29 17:43 ` Tanmay Shah
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox