From: Cristian Marussi <cristian.marussi@arm.com> To: Sudeep Holla <sudeep.holla@arm.com>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 03/15] firmware: arm_scmi: Skip protocol initialisation for additional devices Date: Tue, 10 Dec 2019 18:14:47 +0000 [thread overview] Message-ID: <944a2d76-a3d3-9238-1960-63c3f29bea05@arm.com> (raw) In-Reply-To: <20191210145345.11616-4-sudeep.holla@arm.com> On 10/12/2019 14:53, Sudeep Holla wrote: > The scmi bus now supports adding multiple devices per protocol, > and since scmi_protocol_init is called for each scmi device created, > we must avoid allocating protocol private data and initialising the > protocol itself if it is already initialised. > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- Wouldn't be better to add some kind of per-protocol 'initialized' flag somewhere in the bus abstraction so that the protocol_id itself could be marked as initialized once bus::scmi_protocol_init() completes successfully so that we could just skip the invocation itself of bus::scmi_protocol_init() for all the protocols already detected as initialized ? Or, if not a flag, maybe deactivating the registered protocol init function itself once it has been successfully called once .....something along the lines of: diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c index 7a30952b463d..a551a00586c6 100644 --- a/drivers/firmware/arm_scmi/bus.c +++ b/drivers/firmware/arm_scmi/bus.c @@ -73,6 +73,8 @@ static int scmi_dev_probe(struct device *dev) ret = scmi_protocol_init(scmi_dev->protocol_id, scmi_dev->handle); if (ret) return ret; + idr_replace(&scmi_protocols, dummy_return_0_callback, + scmi_dev->protocol_id); return scmi_drv->probe(scmi_dev); [not really tested eh ... :D] This way we can drop this patch as a whole and avoid any future needs to remember to add this same sort of logic in the next XYZ protocol implementation. Cheers Cristian > drivers/firmware/arm_scmi/clock.c | 3 +++ > drivers/firmware/arm_scmi/perf.c | 3 +++ > drivers/firmware/arm_scmi/power.c | 3 +++ > drivers/firmware/arm_scmi/reset.c | 3 +++ > drivers/firmware/arm_scmi/sensors.c | 3 +++ > 5 files changed, 15 insertions(+) > > diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c > index 32526a793f3a..922b22aaaf84 100644 > --- a/drivers/firmware/arm_scmi/clock.c > +++ b/drivers/firmware/arm_scmi/clock.c > @@ -316,6 +316,9 @@ static int scmi_clock_protocol_init(struct scmi_handle *handle) > int clkid, ret; > struct clock_info *cinfo; > > + if (handle->clk_ops && handle->clk_priv) > + return 0; /* initialised already for the first device */ > + > scmi_version_get(handle, SCMI_PROTOCOL_CLOCK, &version); > > dev_dbg(handle->dev, "Clock Version %d.%d\n", > diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c > index 601af4edad5e..55c2a4c21ccb 100644 > --- a/drivers/firmware/arm_scmi/perf.c > +++ b/drivers/firmware/arm_scmi/perf.c > @@ -710,6 +710,9 @@ static int scmi_perf_protocol_init(struct scmi_handle *handle) > u32 version; > struct scmi_perf_info *pinfo; > > + if (handle->perf_ops && handle->perf_priv) > + return 0; /* initialised already for the first device */ > + > scmi_version_get(handle, SCMI_PROTOCOL_PERF, &version); > > dev_dbg(handle->dev, "Performance Version %d.%d\n", > diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c > index 5abef7079c0a..9a7593238b8f 100644 > --- a/drivers/firmware/arm_scmi/power.c > +++ b/drivers/firmware/arm_scmi/power.c > @@ -185,6 +185,9 @@ static int scmi_power_protocol_init(struct scmi_handle *handle) > u32 version; > struct scmi_power_info *pinfo; > > + if (handle->power_ops && handle->power_priv) > + return 0; /* initialised already for the first device */ > + > scmi_version_get(handle, SCMI_PROTOCOL_POWER, &version); > > dev_dbg(handle->dev, "Power Version %d.%d\n", > diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c > index ab42c21c5517..809dc8faee1e 100644 > --- a/drivers/firmware/arm_scmi/reset.c > +++ b/drivers/firmware/arm_scmi/reset.c > @@ -195,6 +195,9 @@ static int scmi_reset_protocol_init(struct scmi_handle *handle) > u32 version; > struct scmi_reset_info *pinfo; > > + if (handle->reset_ops && handle->reset_priv) > + return 0; /* initialised already for the first device */ > + > scmi_version_get(handle, SCMI_PROTOCOL_RESET, &version); > > dev_dbg(handle->dev, "Reset Version %d.%d\n", > diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c > index a400ea805fc2..b7f92c37c8a4 100644 > --- a/drivers/firmware/arm_scmi/sensors.c > +++ b/drivers/firmware/arm_scmi/sensors.c > @@ -276,6 +276,9 @@ static int scmi_sensors_protocol_init(struct scmi_handle *handle) > u32 version; > struct sensors_info *sinfo; > > + if (handle->sensor_ops && handle->sensor_priv) > + return 0; /* initialised already for the first device */ > + > scmi_version_get(handle, SCMI_PROTOCOL_SENSOR, &version); > > dev_dbg(handle->dev, "Sensor Version %d.%d\n", > -- > 2.17.1 >
WARNING: multiple messages have this Message-ID (diff)
From: Cristian Marussi <cristian.marussi@arm.com> To: Sudeep Holla <sudeep.holla@arm.com>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 03/15] firmware: arm_scmi: Skip protocol initialisation for additional devices Date: Tue, 10 Dec 2019 18:14:47 +0000 [thread overview] Message-ID: <944a2d76-a3d3-9238-1960-63c3f29bea05@arm.com> (raw) In-Reply-To: <20191210145345.11616-4-sudeep.holla@arm.com> On 10/12/2019 14:53, Sudeep Holla wrote: > The scmi bus now supports adding multiple devices per protocol, > and since scmi_protocol_init is called for each scmi device created, > we must avoid allocating protocol private data and initialising the > protocol itself if it is already initialised. > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- Wouldn't be better to add some kind of per-protocol 'initialized' flag somewhere in the bus abstraction so that the protocol_id itself could be marked as initialized once bus::scmi_protocol_init() completes successfully so that we could just skip the invocation itself of bus::scmi_protocol_init() for all the protocols already detected as initialized ? Or, if not a flag, maybe deactivating the registered protocol init function itself once it has been successfully called once .....something along the lines of: diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c index 7a30952b463d..a551a00586c6 100644 --- a/drivers/firmware/arm_scmi/bus.c +++ b/drivers/firmware/arm_scmi/bus.c @@ -73,6 +73,8 @@ static int scmi_dev_probe(struct device *dev) ret = scmi_protocol_init(scmi_dev->protocol_id, scmi_dev->handle); if (ret) return ret; + idr_replace(&scmi_protocols, dummy_return_0_callback, + scmi_dev->protocol_id); return scmi_drv->probe(scmi_dev); [not really tested eh ... :D] This way we can drop this patch as a whole and avoid any future needs to remember to add this same sort of logic in the next XYZ protocol implementation. Cheers Cristian > drivers/firmware/arm_scmi/clock.c | 3 +++ > drivers/firmware/arm_scmi/perf.c | 3 +++ > drivers/firmware/arm_scmi/power.c | 3 +++ > drivers/firmware/arm_scmi/reset.c | 3 +++ > drivers/firmware/arm_scmi/sensors.c | 3 +++ > 5 files changed, 15 insertions(+) > > diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c > index 32526a793f3a..922b22aaaf84 100644 > --- a/drivers/firmware/arm_scmi/clock.c > +++ b/drivers/firmware/arm_scmi/clock.c > @@ -316,6 +316,9 @@ static int scmi_clock_protocol_init(struct scmi_handle *handle) > int clkid, ret; > struct clock_info *cinfo; > > + if (handle->clk_ops && handle->clk_priv) > + return 0; /* initialised already for the first device */ > + > scmi_version_get(handle, SCMI_PROTOCOL_CLOCK, &version); > > dev_dbg(handle->dev, "Clock Version %d.%d\n", > diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c > index 601af4edad5e..55c2a4c21ccb 100644 > --- a/drivers/firmware/arm_scmi/perf.c > +++ b/drivers/firmware/arm_scmi/perf.c > @@ -710,6 +710,9 @@ static int scmi_perf_protocol_init(struct scmi_handle *handle) > u32 version; > struct scmi_perf_info *pinfo; > > + if (handle->perf_ops && handle->perf_priv) > + return 0; /* initialised already for the first device */ > + > scmi_version_get(handle, SCMI_PROTOCOL_PERF, &version); > > dev_dbg(handle->dev, "Performance Version %d.%d\n", > diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c > index 5abef7079c0a..9a7593238b8f 100644 > --- a/drivers/firmware/arm_scmi/power.c > +++ b/drivers/firmware/arm_scmi/power.c > @@ -185,6 +185,9 @@ static int scmi_power_protocol_init(struct scmi_handle *handle) > u32 version; > struct scmi_power_info *pinfo; > > + if (handle->power_ops && handle->power_priv) > + return 0; /* initialised already for the first device */ > + > scmi_version_get(handle, SCMI_PROTOCOL_POWER, &version); > > dev_dbg(handle->dev, "Power Version %d.%d\n", > diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c > index ab42c21c5517..809dc8faee1e 100644 > --- a/drivers/firmware/arm_scmi/reset.c > +++ b/drivers/firmware/arm_scmi/reset.c > @@ -195,6 +195,9 @@ static int scmi_reset_protocol_init(struct scmi_handle *handle) > u32 version; > struct scmi_reset_info *pinfo; > > + if (handle->reset_ops && handle->reset_priv) > + return 0; /* initialised already for the first device */ > + > scmi_version_get(handle, SCMI_PROTOCOL_RESET, &version); > > dev_dbg(handle->dev, "Reset Version %d.%d\n", > diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c > index a400ea805fc2..b7f92c37c8a4 100644 > --- a/drivers/firmware/arm_scmi/sensors.c > +++ b/drivers/firmware/arm_scmi/sensors.c > @@ -276,6 +276,9 @@ static int scmi_sensors_protocol_init(struct scmi_handle *handle) > u32 version; > struct sensors_info *sinfo; > > + if (handle->sensor_ops && handle->sensor_priv) > + return 0; /* initialised already for the first device */ > + > scmi_version_get(handle, SCMI_PROTOCOL_SENSOR, &version); > > dev_dbg(handle->dev, "Sensor Version %d.%d\n", > -- > 2.17.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-12-10 18:14 UTC|newest] Thread overview: 74+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-12-10 14:53 [PATCH 00/15] firmware: arm_scmi: Add support for multiple device per protocol Sudeep Holla 2019-12-10 14:53 ` Sudeep Holla 2019-12-10 14:53 ` [PATCH 01/15] " Sudeep Holla 2019-12-10 14:53 ` Sudeep Holla 2019-12-10 17:02 ` Cristian Marussi 2019-12-10 17:02 ` Cristian Marussi 2019-12-10 14:53 ` [PATCH 02/15] firmware: arm_scmi: Skip scmi mbox channel setup for addtional devices Sudeep Holla 2019-12-10 14:53 ` Sudeep Holla 2019-12-10 17:33 ` Cristian Marussi 2019-12-10 17:33 ` Cristian Marussi 2019-12-10 14:53 ` [PATCH 03/15] firmware: arm_scmi: Skip protocol initialisation for additional devices Sudeep Holla 2019-12-10 14:53 ` Sudeep Holla 2019-12-10 18:14 ` Cristian Marussi [this message] 2019-12-10 18:14 ` Cristian Marussi 2019-12-10 14:53 ` [PATCH 04/15] firmware: arm_scmi: Add names to scmi devices created Sudeep Holla 2019-12-10 14:53 ` Sudeep Holla 2019-12-10 18:26 ` Cristian Marussi 2019-12-10 18:26 ` Cristian Marussi 2019-12-10 14:53 ` [PATCH 05/15] firmware: arm_scmi: Add versions and identifier attributes using dev_groups Sudeep Holla 2019-12-10 14:53 ` Sudeep Holla 2019-12-11 13:21 ` Cristian Marussi 2019-12-11 13:21 ` Cristian Marussi 2019-12-10 14:53 ` [PATCH 06/15] firmware: arm_scmi: Update scmi_prot_init_fn_t to use device instead of handle Sudeep Holla 2019-12-10 14:53 ` Sudeep Holla 2019-12-11 17:34 ` Cristian Marussi 2019-12-11 17:34 ` Cristian Marussi 2019-12-10 14:53 ` [PATCH 07/15] firmware: arm_scmi: Stash version in protocol init functions Sudeep Holla 2019-12-10 14:53 ` Sudeep Holla 2019-12-11 17:35 ` Cristian Marussi 2019-12-11 17:35 ` Cristian Marussi 2019-12-10 14:53 ` [PATCH 08/15] firmware: arm_scmi: Add and initialise protocol version to scmi_device structure Sudeep Holla 2019-12-10 14:53 ` Sudeep Holla 2019-12-11 18:06 ` Cristian Marussi 2019-12-11 18:06 ` Cristian Marussi 2019-12-12 12:15 ` Sudeep Holla 2019-12-12 12:15 ` Sudeep Holla 2019-12-10 14:53 ` [PATCH 09/15] firmware: arm_scmi: Add scmi protocol version and id device attributes Sudeep Holla 2019-12-10 14:53 ` Sudeep Holla 2019-12-11 18:08 ` Cristian Marussi 2019-12-11 18:08 ` Cristian Marussi 2019-12-10 14:53 ` [PATCH 10/15] firmware: arm_scmi: Drop logging individual scmi protocol version Sudeep Holla 2019-12-10 14:53 ` Sudeep Holla 2019-12-11 18:09 ` Cristian Marussi 2019-12-11 18:09 ` Cristian Marussi 2019-12-10 14:53 ` [PATCH 11/15] firmware: arm_scmi: Match scmi device by both name and protocol id Sudeep Holla 2019-12-10 14:53 ` Sudeep Holla 2019-12-11 18:09 ` Cristian Marussi 2019-12-11 18:09 ` Cristian Marussi 2019-12-11 18:10 ` Cristian Marussi 2019-12-11 18:10 ` Cristian Marussi 2019-12-10 14:53 ` [PATCH 12/15] clk: scmi: " Sudeep Holla 2019-12-10 14:53 ` Sudeep Holla 2019-12-24 7:49 ` Stephen Boyd 2019-12-24 7:49 ` Stephen Boyd 2019-12-10 14:53 ` [PATCH 13/15] cpufreq: " Sudeep Holla 2019-12-10 14:53 ` Sudeep Holla 2019-12-11 2:39 ` Viresh Kumar 2019-12-11 2:39 ` Viresh Kumar 2019-12-11 10:13 ` Sudeep Holla 2019-12-11 10:13 ` Sudeep Holla 2019-12-11 10:30 ` Viresh Kumar 2019-12-11 10:30 ` Viresh Kumar 2019-12-10 14:53 ` [PATCH 14/15] hwmon: (scmi-hwmon) " Sudeep Holla 2019-12-10 14:53 ` Sudeep Holla 2019-12-10 18:06 ` Guenter Roeck 2019-12-10 18:06 ` Guenter Roeck 2019-12-10 18:20 ` Sudeep Holla 2019-12-10 18:20 ` Sudeep Holla 2019-12-10 14:53 ` [PATCH 15/15] reset: reset-scmi: " Sudeep Holla 2019-12-10 14:53 ` Sudeep Holla 2019-12-11 9:51 ` Philipp Zabel 2019-12-11 9:51 ` Philipp Zabel 2019-12-11 15:28 ` Sudeep Holla 2019-12-11 15:28 ` Sudeep Holla
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=944a2d76-a3d3-9238-1960-63c3f29bea05@arm.com \ --to=cristian.marussi@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=sudeep.holla@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.