| Message ID | 20220623124742.2492164-3-cristian.marussi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce SCMI System Power Control driver | expand |
On Thu, Jun 23, 2022 at 01:47:39PM +0100, Cristian Marussi wrote: > In order to minimize SCMI platform fw-side complexity, only one single SCMI > platform should be in charge of SCMI SystemPower protocol communications > with the OSPM: enforce the existence of one single unique device associated > with SystemPower protocol across any possible number of SCMI platforms, and > warn if a system tries to register different SystemPower devices from > multiple platforms. > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > --- > drivers/firmware/arm_scmi/bus.c | 31 ++++++++++++++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c > index a7cbf4d09081..476855d3dccb 100644 > --- a/drivers/firmware/arm_scmi/bus.c > +++ b/drivers/firmware/arm_scmi/bus.c > @@ -19,6 +19,11 @@ static DEFINE_IDA(scmi_bus_id); > static DEFINE_IDR(scmi_protocols); > static DEFINE_SPINLOCK(protocol_lock); > > +/* Track globally the creation of SCMI SystemPower related devices */ > +static bool scmi_syspower_registered; > +/* Protect access to scmi_syspower_registered */ > +static DEFINE_MUTEX(scmi_syspower_mtx); > + Since we create device from the driver, can't we do this from there and keep the bus code free from handling all these special conditions which are checked for each device creation. Yes scmi_device_create can be called outside the exiting code but since it is not exported(yet), we can assume all users are in kernel and we can catch that if anyone attempts to add. And probably we don't need the lock as well if it is taken care in the single loop creating the device.
On Fri, Jul 01, 2022 at 02:45:09PM +0100, Sudeep Holla wrote: > On Thu, Jun 23, 2022 at 01:47:39PM +0100, Cristian Marussi wrote: > > In order to minimize SCMI platform fw-side complexity, only one single SCMI > > platform should be in charge of SCMI SystemPower protocol communications > > with the OSPM: enforce the existence of one single unique device associated > > with SystemPower protocol across any possible number of SCMI platforms, and > > warn if a system tries to register different SystemPower devices from > > multiple platforms. > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > > --- > > drivers/firmware/arm_scmi/bus.c | 31 ++++++++++++++++++++++++++++++- > > 1 file changed, 30 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c > > index a7cbf4d09081..476855d3dccb 100644 > > --- a/drivers/firmware/arm_scmi/bus.c > > +++ b/drivers/firmware/arm_scmi/bus.c > > @@ -19,6 +19,11 @@ static DEFINE_IDA(scmi_bus_id); > > static DEFINE_IDR(scmi_protocols); > > static DEFINE_SPINLOCK(protocol_lock); > > > > +/* Track globally the creation of SCMI SystemPower related devices */ > > +static bool scmi_syspower_registered; > > +/* Protect access to scmi_syspower_registered */ > > +static DEFINE_MUTEX(scmi_syspower_mtx); > > + > Hi Sudeep, thanks for the review first of all. > Since we create device from the driver, can't we do this from there > and keep the bus code free from handling all these special conditions > which are checked for each device creation. > > Yes scmi_device_create can be called outside the exiting code but since it > is not exported(yet), we can assume all users are in kernel and we can > catch that if anyone attempts to add. And probably we don't need the lock > as well if it is taken care in the single loop creating the device. > Do you mean to move the check inside driver.c common routines like in scmi_get_protocol_device() right before calling scmi_device_create() ? If this is what you meant, yes I can do that to avoid polluting the bus code...indeed it would be easier than dealing with all the internals in scmi_device_create() like it is now, BUT regarding the mutex I'm not so sure I can avoid it since the device creation is triggered at the end of main platform probe (driver:scmi_probe()) BUT potentially also whenever a new SCMI driver is (lately) loaded and asks for the device creation after (or worst concurrently to) the main probe loop. Beside that, there is the case of definitions of multiple SCMI platforms, which is not officially supported I know but that is, in my understanding, one of the most possible cause of having multiple instances of an SCMI SystemPower driver trying to register. (i.e. multiple scmi DT nodes ALL defining a SystemPower protocol with potentially multiple underlying FWs advertising SystemPower support which was the thing we wanted to avoid promoting ... AFAIU...but I could be missing something..) Thanks, Cristian
On Fri, Jul 01, 2022 at 03:31:18PM +0100, Cristian Marussi wrote: > On Fri, Jul 01, 2022 at 02:45:09PM +0100, Sudeep Holla wrote: > > On Thu, Jun 23, 2022 at 01:47:39PM +0100, Cristian Marussi wrote: > > > In order to minimize SCMI platform fw-side complexity, only one single SCMI > > > platform should be in charge of SCMI SystemPower protocol communications > > > with the OSPM: enforce the existence of one single unique device associated > > > with SystemPower protocol across any possible number of SCMI platforms, and > > > warn if a system tries to register different SystemPower devices from > > > multiple platforms. > > > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > > > --- > > > drivers/firmware/arm_scmi/bus.c | 31 ++++++++++++++++++++++++++++++- > > > 1 file changed, 30 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c > > > index a7cbf4d09081..476855d3dccb 100644 > > > --- a/drivers/firmware/arm_scmi/bus.c > > > +++ b/drivers/firmware/arm_scmi/bus.c > > > @@ -19,6 +19,11 @@ static DEFINE_IDA(scmi_bus_id); > > > static DEFINE_IDR(scmi_protocols); > > > static DEFINE_SPINLOCK(protocol_lock); > > > > > > +/* Track globally the creation of SCMI SystemPower related devices */ > > > +static bool scmi_syspower_registered; > > > +/* Protect access to scmi_syspower_registered */ > > > +static DEFINE_MUTEX(scmi_syspower_mtx); > > > + > > > > Hi Sudeep, > > thanks for the review first of all. > > > Since we create device from the driver, can't we do this from there > > and keep the bus code free from handling all these special conditions > > which are checked for each device creation. > > > > Yes scmi_device_create can be called outside the exiting code but since it > > is not exported(yet), we can assume all users are in kernel and we can > > catch that if anyone attempts to add. And probably we don't need the lock > > as well if it is taken care in the single loop creating the device. > > > > Do you mean to move the check inside driver.c common routines like in > scmi_get_protocol_device() right before calling scmi_device_create() ? > > If this is what you meant, yes I can do that to avoid polluting the > bus code...indeed it would be easier than dealing with all the internals > in scmi_device_create() like it is now, BUT regarding the mutex I'm not so > sure I can avoid it since the device creation is triggered at the end of > main platform probe (driver:scmi_probe()) BUT potentially also whenever a > new SCMI driver is (lately) loaded and asks for the device creation after > (or worst concurrently to) the main probe loop. > You got it right, I meant exactly that. Agreed and that's why I mentioned we don't export that and hence it is not possible. I don't want to make bus code complex checking for this. We may have to do that if that is the only way in the future, but let us defer until we have to. > Beside that, there is the case of definitions of multiple SCMI platforms, > which is not officially supported I know but that is, in my understanding, > one of the most possible cause of having multiple instances of an SCMI > SystemPower driver trying to register. (i.e. multiple scmi DT nodes ALL > defining a SystemPower protocol with potentially multiple underlying FWs > advertising SystemPower support which was the thing we wanted to avoid > promoting ... AFAIU...but I could be missing something..) > Agreed, I am not questioning the addition of this change, just how and where :).
diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c index a7cbf4d09081..476855d3dccb 100644 --- a/drivers/firmware/arm_scmi/bus.c +++ b/drivers/firmware/arm_scmi/bus.c @@ -19,6 +19,11 @@ static DEFINE_IDA(scmi_bus_id); static DEFINE_IDR(scmi_protocols); static DEFINE_SPINLOCK(protocol_lock); +/* Track globally the creation of SCMI SystemPower related devices */ +static bool scmi_syspower_registered; +/* Protect access to scmi_syspower_registered */ +static DEFINE_MUTEX(scmi_syspower_mtx); + static const struct scmi_device_id * scmi_dev_match_id(struct scmi_device *scmi_dev, struct scmi_driver *scmi_drv) { @@ -207,11 +212,31 @@ scmi_device_create(struct device_node *np, struct device *parent, int protocol, scmi_dev->dev.release = scmi_device_release; dev_set_name(&scmi_dev->dev, "scmi_dev.%d", id); + mutex_lock(&scmi_syspower_mtx); + if (protocol == SCMI_PROTOCOL_SYSTEM && scmi_syspower_registered) { + dev_warn(parent, + "SCMI SystemPower protocol device must be unique !\n"); + + mutex_unlock(&scmi_syspower_mtx); + ida_free(&scmi_bus_id, id); + kfree_const(scmi_dev->name); + kfree(scmi_dev); + return NULL; + } + retval = device_register(&scmi_dev->dev); - if (retval) + if (retval) { + mutex_unlock(&scmi_syspower_mtx); goto put_dev; + } + + if (protocol == SCMI_PROTOCOL_SYSTEM) + scmi_syspower_registered = true; + + mutex_unlock(&scmi_syspower_mtx); return scmi_dev; + put_dev: kfree_const(scmi_dev->name); put_device(&scmi_dev->dev); @@ -221,6 +246,10 @@ scmi_device_create(struct device_node *np, struct device *parent, int protocol, void scmi_device_destroy(struct scmi_device *scmi_dev) { + mutex_lock(&scmi_syspower_mtx); + if (scmi_dev->protocol_id == SCMI_PROTOCOL_SYSTEM) + scmi_syspower_registered = false; + mutex_unlock(&scmi_syspower_mtx); kfree_const(scmi_dev->name); scmi_handle_put(scmi_dev->handle); ida_free(&scmi_bus_id, scmi_dev->id);
In order to minimize SCMI platform fw-side complexity, only one single SCMI platform should be in charge of SCMI SystemPower protocol communications with the OSPM: enforce the existence of one single unique device associated with SystemPower protocol across any possible number of SCMI platforms, and warn if a system tries to register different SystemPower devices from multiple platforms. Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> --- drivers/firmware/arm_scmi/bus.c | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)