| Message ID | 20220330150551.2573938-3-cristian.marussi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SCMIv3.1 Miscellaneous changes | expand |
On Wed, Mar 30, 2022 at 04:05:31PM +0100, Cristian Marussi wrote: > Bail out of protocol initialization routine early when basic information > about protocol version and attributes could not be retrieved: failing to > act this way can lead to a successfully initialized SCMI protocol which > is in fact not fully functional. > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > --- > drivers/firmware/arm_scmi/base.c | 5 ++++- > drivers/firmware/arm_scmi/clock.c | 8 ++++++-- > drivers/firmware/arm_scmi/perf.c | 10 +++++++--- > drivers/firmware/arm_scmi/power.c | 10 +++++++--- > drivers/firmware/arm_scmi/reset.c | 10 +++++++--- > drivers/firmware/arm_scmi/sensors.c | 4 +++- > drivers/firmware/arm_scmi/system.c | 5 ++++- > 7 files changed, 38 insertions(+), 14 deletions(-) > > @@ -370,7 +372,9 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph) > if (!cinfo) > return -ENOMEM; > > - scmi_clock_protocol_attributes_get(ph, cinfo); > + ret = scmi_clock_protocol_attributes_get(ph, cinfo); > + if (ret) > + return ret; Does this result in removal of scmi_dev associated with devm_* calls ? Otherwise we may need to free the allocated buffers ? I am not sure if the dev here is individual scmi_dev or the platform scmi device. I assume latter and it is unlikely to be removed/freed with the error in the above path. Similarly in couple of other instances/protocols. -- Regards, Sudeep
On Tue, Apr 26, 2022 at 04:35:28PM +0100, Sudeep Holla wrote: > On Wed, Mar 30, 2022 at 04:05:31PM +0100, Cristian Marussi wrote: > > Bail out of protocol initialization routine early when basic information > > about protocol version and attributes could not be retrieved: failing to > > act this way can lead to a successfully initialized SCMI protocol which > > is in fact not fully functional. > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > > --- > > drivers/firmware/arm_scmi/base.c | 5 ++++- > > drivers/firmware/arm_scmi/clock.c | 8 ++++++-- > > drivers/firmware/arm_scmi/perf.c | 10 +++++++--- > > drivers/firmware/arm_scmi/power.c | 10 +++++++--- > > drivers/firmware/arm_scmi/reset.c | 10 +++++++--- > > drivers/firmware/arm_scmi/sensors.c | 4 +++- > > drivers/firmware/arm_scmi/system.c | 5 ++++- > > 7 files changed, 38 insertions(+), 14 deletions(-) > > Hi Sudeep, thanks for the review first of all... > > @@ -370,7 +372,9 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph) > > if (!cinfo) > > return -ENOMEM; > > > > - scmi_clock_protocol_attributes_get(ph, cinfo); > > + ret = scmi_clock_protocol_attributes_get(ph, cinfo); > > + if (ret) > > + return ret; > > Does this result in removal of scmi_dev associated with devm_* calls ? > Otherwise we may need to free the allocated buffers ? I am not sure > if the dev here is individual scmi_dev or the platform scmi device. > I assume latter and it is unlikely to be removed/freed with the error in > the above path. > > Similarly in couple of other instances/protocols. So, ph->dev used in the above devm_ is indeed the arm_scmi platform device and I was *almost* gonna tell you 'Good catch', BUT then, rereading my own code (O_o), I saw/remembered that when a protocol instance is initialized on it first usage, there is indeed a devres_group internally managed by the SCMI core, so that: scmi_get_protocol_instance() @first_protocol_usage (refcount pi->users): --> scmi_get_protocol() // just in case was LKM proto --> scmi_alloc_init_protocol_instance() gid = devres_open_group(handle->dev, NULL, GFP_KERNEL); ret = pi->proto->instance_init(&pi->ph); ====>>> i.e. scmi_clock_protocol_init(ph) if (ret) goto clean; ..... clean: devres_release_group(handle->dev, gid); So basically all that happens at initialization time in scmi_clock_protocol_init, BUT also everything that happens implicitly inside scmi_alloc_init_protocol_instance during that protocol initialization (like the events registration) is undone on failure transparently by the SCMI core init/free management functions (via devres_ groups...) All of the above is because each protocol is initialized only once on its first usage, no matter how many SCMI driver users (and scmi_devs) are using it...only in case (unsupported) we have multiple SCMI instances (platforms) there will be one instance of protocol initialized per SCMI server. ... having said that, now I'll go and double check (test) this behaviour since I even had forgot about having implemented this kind of design :P Thanks, Cristian
On Tue, Apr 26, 2022 at 05:25:28PM +0100, Cristian Marussi wrote: > On Tue, Apr 26, 2022 at 04:35:28PM +0100, Sudeep Holla wrote: > > On Wed, Mar 30, 2022 at 04:05:31PM +0100, Cristian Marussi wrote: > > > Bail out of protocol initialization routine early when basic information > > > about protocol version and attributes could not be retrieved: failing to > > > act this way can lead to a successfully initialized SCMI protocol which > > > is in fact not fully functional. > > > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > > > --- > > > drivers/firmware/arm_scmi/base.c | 5 ++++- > > > drivers/firmware/arm_scmi/clock.c | 8 ++++++-- > > > drivers/firmware/arm_scmi/perf.c | 10 +++++++--- > > > drivers/firmware/arm_scmi/power.c | 10 +++++++--- > > > drivers/firmware/arm_scmi/reset.c | 10 +++++++--- > > > drivers/firmware/arm_scmi/sensors.c | 4 +++- > > > drivers/firmware/arm_scmi/system.c | 5 ++++- > > > 7 files changed, 38 insertions(+), 14 deletions(-) > > > > > Hi Sudeep, > > thanks for the review first of all... > > > > @@ -370,7 +372,9 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph) > > > if (!cinfo) > > > return -ENOMEM; > > > > > > - scmi_clock_protocol_attributes_get(ph, cinfo); > > > + ret = scmi_clock_protocol_attributes_get(ph, cinfo); > > > + if (ret) > > > + return ret; > > > > Does this result in removal of scmi_dev associated with devm_* calls ? > > Otherwise we may need to free the allocated buffers ? I am not sure > > if the dev here is individual scmi_dev or the platform scmi device. > > I assume latter and it is unlikely to be removed/freed with the error in > > the above path. > > > > Similarly in couple of other instances/protocols. > > So, ph->dev used in the above devm_ is indeed the arm_scmi platform device > and I was *almost* gonna tell you 'Good catch', BUT then, rereading my own > code (O_o), I saw/remembered that when a protocol instance is initialized on > it first usage, there is indeed a devres_group internally managed by > the SCMI core, so that: > > scmi_get_protocol_instance() > > @first_protocol_usage (refcount pi->users): > > --> scmi_get_protocol() // just in case was LKM proto > --> scmi_alloc_init_protocol_instance() > gid = devres_open_group(handle->dev, NULL, GFP_KERNEL); > > ret = pi->proto->instance_init(&pi->ph); > ====>>> i.e. scmi_clock_protocol_init(ph) > if (ret) > goto clean; > ..... > > clean: > devres_release_group(handle->dev, gid); > > > So basically all that happens at initialization time in scmi_clock_protocol_init, > BUT also everything that happens implicitly inside scmi_alloc_init_protocol_instance > during that protocol initialization (like the events registration) is undone on > failure transparently by the SCMI core init/free management functions > (via devres_ groups...) > > All of the above is because each protocol is initialized only once on > its first usage, no matter how many SCMI driver users (and scmi_devs) are > using it...only in case (unsupported) we have multiple SCMI instances > (platforms) there will be one instance of protocol initialized per SCMI > server. > > ... having said that, now I'll go and double check (test) this behaviour since I > even had forgot about having implemented this kind of design :P > Makes sense, thanks for the detailed explanation. I had totally forgotten how devres_group works
On Thu, Apr 28, 2022 at 11:25:24AM +0100, Sudeep Holla wrote: > On Tue, Apr 26, 2022 at 05:25:28PM +0100, Cristian Marussi wrote: > > On Tue, Apr 26, 2022 at 04:35:28PM +0100, Sudeep Holla wrote: > > > On Wed, Mar 30, 2022 at 04:05:31PM +0100, Cristian Marussi wrote: > > > > Bail out of protocol initialization routine early when basic information > > > > about protocol version and attributes could not be retrieved: failing to > > > > act this way can lead to a successfully initialized SCMI protocol which > > > > is in fact not fully functional. > > > > > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > > > > --- > > > > drivers/firmware/arm_scmi/base.c | 5 ++++- > > > > drivers/firmware/arm_scmi/clock.c | 8 ++++++-- > > > > drivers/firmware/arm_scmi/perf.c | 10 +++++++--- > > > > drivers/firmware/arm_scmi/power.c | 10 +++++++--- > > > > drivers/firmware/arm_scmi/reset.c | 10 +++++++--- > > > > drivers/firmware/arm_scmi/sensors.c | 4 +++- > > > > drivers/firmware/arm_scmi/system.c | 5 ++++- > > > > 7 files changed, 38 insertions(+), 14 deletions(-) > > > > > > > > Hi Sudeep, > > > > thanks for the review first of all... > > > > > > @@ -370,7 +372,9 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph) > > > > if (!cinfo) > > > > return -ENOMEM; > > > > > > > > - scmi_clock_protocol_attributes_get(ph, cinfo); > > > > + ret = scmi_clock_protocol_attributes_get(ph, cinfo); > > > > + if (ret) > > > > + return ret; > > > > > > Does this result in removal of scmi_dev associated with devm_* calls ? > > > Otherwise we may need to free the allocated buffers ? I am not sure > > > if the dev here is individual scmi_dev or the platform scmi device. > > > I assume latter and it is unlikely to be removed/freed with the error in > > > the above path. > > > > > > Similarly in couple of other instances/protocols. > > > > So, ph->dev used in the above devm_ is indeed the arm_scmi platform device > > and I was *almost* gonna tell you 'Good catch', BUT then, rereading my own > > code (O_o), I saw/remembered that when a protocol instance is initialized on > > it first usage, there is indeed a devres_group internally managed by > > the SCMI core, so that: > > > > scmi_get_protocol_instance() > > > > @first_protocol_usage (refcount pi->users): > > > > --> scmi_get_protocol() // just in case was LKM proto > > --> scmi_alloc_init_protocol_instance() > > gid = devres_open_group(handle->dev, NULL, GFP_KERNEL); > > > > ret = pi->proto->instance_init(&pi->ph); > > ====>>> i.e. scmi_clock_protocol_init(ph) > > if (ret) > > goto clean; > > ..... > > > > clean: > > devres_release_group(handle->dev, gid); > > > > > > So basically all that happens at initialization time in scmi_clock_protocol_init, > > BUT also everything that happens implicitly inside scmi_alloc_init_protocol_instance > > during that protocol initialization (like the events registration) is undone on > > failure transparently by the SCMI core init/free management functions > > (via devres_ groups...) > > > > All of the above is because each protocol is initialized only once on > > its first usage, no matter how many SCMI driver users (and scmi_devs) are > > using it...only in case (unsupported) we have multiple SCMI instances > > (platforms) there will be one instance of protocol initialized per SCMI > > server. > > > > ... having said that, now I'll go and double check (test) this behaviour since I > > even had forgot about having implemented this kind of design :P > > > > Makes sense, thanks for the detailed explanation. I had totally forgotten how > devres_group works
diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c index f5219334fd3a..ebaef5d320af 100644 --- a/drivers/firmware/arm_scmi/base.c +++ b/drivers/firmware/arm_scmi/base.c @@ -359,7 +359,10 @@ static int scmi_base_protocol_init(const struct scmi_protocol_handle *ph) rev->minor_ver = PROTOCOL_REV_MINOR(version); ph->set_priv(ph, rev); - scmi_base_attributes_get(ph); + ret = scmi_base_attributes_get(ph); + if (ret) + return ret; + scmi_base_vendor_id_get(ph, false); scmi_base_vendor_id_get(ph, true); scmi_base_implementation_version_get(ph); diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c index ef6431c6eb1c..0ae39ee920e9 100644 --- a/drivers/firmware/arm_scmi/clock.c +++ b/drivers/firmware/arm_scmi/clock.c @@ -361,7 +361,9 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph) int clkid, ret; struct clock_info *cinfo; - ph->xops->version_get(ph, &version); + ret = ph->xops->version_get(ph, &version); + if (ret) + return ret; dev_dbg(ph->dev, "Clock Version %d.%d\n", PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version)); @@ -370,7 +372,9 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph) if (!cinfo) return -ENOMEM; - scmi_clock_protocol_attributes_get(ph, cinfo); + ret = scmi_clock_protocol_attributes_get(ph, cinfo); + if (ret) + return ret; cinfo->clk = devm_kcalloc(ph->dev, cinfo->num_clocks, sizeof(*cinfo->clk), GFP_KERNEL); diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c index f4cd5193b961..e9f68b91580c 100644 --- a/drivers/firmware/arm_scmi/perf.c +++ b/drivers/firmware/arm_scmi/perf.c @@ -873,11 +873,13 @@ static const struct scmi_protocol_events perf_protocol_events = { static int scmi_perf_protocol_init(const struct scmi_protocol_handle *ph) { - int domain; + int domain, ret; u32 version; struct scmi_perf_info *pinfo; - ph->xops->version_get(ph, &version); + ret = ph->xops->version_get(ph, &version); + if (ret) + return ret; dev_dbg(ph->dev, "Performance Version %d.%d\n", PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version)); @@ -886,7 +888,9 @@ static int scmi_perf_protocol_init(const struct scmi_protocol_handle *ph) if (!pinfo) return -ENOMEM; - scmi_perf_attributes_get(ph, pinfo); + ret = scmi_perf_attributes_get(ph, pinfo); + if (ret) + return ret; pinfo->dom_info = devm_kcalloc(ph->dev, pinfo->num_domains, sizeof(*pinfo->dom_info), GFP_KERNEL); diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c index ad2ab080f344..0f0b94f0b624 100644 --- a/drivers/firmware/arm_scmi/power.c +++ b/drivers/firmware/arm_scmi/power.c @@ -280,11 +280,13 @@ static const struct scmi_protocol_events power_protocol_events = { static int scmi_power_protocol_init(const struct scmi_protocol_handle *ph) { - int domain; + int domain, ret; u32 version; struct scmi_power_info *pinfo; - ph->xops->version_get(ph, &version); + ret = ph->xops->version_get(ph, &version); + if (ret) + return ret; dev_dbg(ph->dev, "Power Version %d.%d\n", PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version)); @@ -293,7 +295,9 @@ static int scmi_power_protocol_init(const struct scmi_protocol_handle *ph) if (!pinfo) return -ENOMEM; - scmi_power_attributes_get(ph, pinfo); + ret = scmi_power_attributes_get(ph, pinfo); + if (ret) + return ret; pinfo->dom_info = devm_kcalloc(ph->dev, pinfo->num_domains, sizeof(*pinfo->dom_info), GFP_KERNEL); diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c index 9bf2478ec6d1..9cdbd133db10 100644 --- a/drivers/firmware/arm_scmi/reset.c +++ b/drivers/firmware/arm_scmi/reset.c @@ -293,11 +293,13 @@ static const struct scmi_protocol_events reset_protocol_events = { static int scmi_reset_protocol_init(const struct scmi_protocol_handle *ph) { - int domain; + int domain, ret; u32 version; struct scmi_reset_info *pinfo; - ph->xops->version_get(ph, &version); + ret = ph->xops->version_get(ph, &version); + if (ret) + return ret; dev_dbg(ph->dev, "Reset Version %d.%d\n", PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version)); @@ -306,7 +308,9 @@ static int scmi_reset_protocol_init(const struct scmi_protocol_handle *ph) if (!pinfo) return -ENOMEM; - scmi_reset_attributes_get(ph, pinfo); + ret = scmi_reset_attributes_get(ph, pinfo); + if (ret) + return ret; pinfo->dom_info = devm_kcalloc(ph->dev, pinfo->num_domains, sizeof(*pinfo->dom_info), GFP_KERNEL); diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c index cdbb287bd8bc..f37ac9824a87 100644 --- a/drivers/firmware/arm_scmi/sensors.c +++ b/drivers/firmware/arm_scmi/sensors.c @@ -966,7 +966,9 @@ static int scmi_sensors_protocol_init(const struct scmi_protocol_handle *ph) int ret; struct sensors_info *sinfo; - ph->xops->version_get(ph, &version); + ret = ph->xops->version_get(ph, &version); + if (ret) + return ret; dev_dbg(ph->dev, "Sensor Version %d.%d\n", PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version)); diff --git a/drivers/firmware/arm_scmi/system.c b/drivers/firmware/arm_scmi/system.c index e5175ef73b40..cbfc19f7a463 100644 --- a/drivers/firmware/arm_scmi/system.c +++ b/drivers/firmware/arm_scmi/system.c @@ -113,10 +113,13 @@ static const struct scmi_protocol_events system_protocol_events = { static int scmi_system_protocol_init(const struct scmi_protocol_handle *ph) { + int ret; u32 version; struct scmi_system_info *pinfo; - ph->xops->version_get(ph, &version); + ret = ph->xops->version_get(ph, &version); + if (ret) + return ret; dev_dbg(ph->dev, "System Power Version %d.%d\n", PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
Bail out of protocol initialization routine early when basic information about protocol version and attributes could not be retrieved: failing to act this way can lead to a successfully initialized SCMI protocol which is in fact not fully functional. Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> --- drivers/firmware/arm_scmi/base.c | 5 ++++- drivers/firmware/arm_scmi/clock.c | 8 ++++++-- drivers/firmware/arm_scmi/perf.c | 10 +++++++--- drivers/firmware/arm_scmi/power.c | 10 +++++++--- drivers/firmware/arm_scmi/reset.c | 10 +++++++--- drivers/firmware/arm_scmi/sensors.c | 4 +++- drivers/firmware/arm_scmi/system.c | 5 ++++- 7 files changed, 38 insertions(+), 14 deletions(-)