* [PATCH] soundwire: add slave device to linked list after device_register()
@ 2021-03-23 2:25 ` Bard Liao
0 siblings, 0 replies; 7+ messages in thread
From: Bard Liao @ 2021-03-23 2:25 UTC (permalink / raw)
To: alsa-devel, vkoul
Cc: vinod.koul, linux-kernel, gregkh, srinivas.kandagatla,
rander.wang, hui.wang, pierre-louis.bossart, sanyog.r.kale,
bard.liao
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We currently add the slave device to a linked list before
device_register(), and then remove it if device_register() fails.
It's not clear why this sequence was necessary, this patch moves the
linked list management to after the device_register().
Suggested-by: Keyon Jie <yang.jie@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
drivers/soundwire/slave.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index 112b21967c7a..0c92db2e1ddc 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -65,9 +65,6 @@ int sdw_slave_add(struct sdw_bus *bus,
for (i = 0; i < SDW_MAX_PORTS; i++)
init_completion(&slave->port_ready[i]);
- mutex_lock(&bus->bus_lock);
- list_add_tail(&slave->node, &bus->slaves);
- mutex_unlock(&bus->bus_lock);
ret = device_register(&slave->dev);
if (ret) {
@@ -77,13 +74,15 @@ int sdw_slave_add(struct sdw_bus *bus,
* On err, don't free but drop ref as this will be freed
* when release method is invoked.
*/
- mutex_lock(&bus->bus_lock);
- list_del(&slave->node);
- mutex_unlock(&bus->bus_lock);
put_device(&slave->dev);
return ret;
}
+
+ mutex_lock(&bus->bus_lock);
+ list_add_tail(&slave->node, &bus->slaves);
+ mutex_unlock(&bus->bus_lock);
+
sdw_slave_debugfs_init(slave);
return ret;
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] soundwire: add slave device to linked list after device_register()
@ 2021-03-23 2:25 ` Bard Liao
0 siblings, 0 replies; 7+ messages in thread
From: Bard Liao @ 2021-03-23 2:25 UTC (permalink / raw)
To: alsa-devel, vkoul
Cc: vinod.koul, gregkh, linux-kernel, pierre-louis.bossart, hui.wang,
srinivas.kandagatla, sanyog.r.kale, rander.wang, bard.liao
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We currently add the slave device to a linked list before
device_register(), and then remove it if device_register() fails.
It's not clear why this sequence was necessary, this patch moves the
linked list management to after the device_register().
Suggested-by: Keyon Jie <yang.jie@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
drivers/soundwire/slave.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index 112b21967c7a..0c92db2e1ddc 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -65,9 +65,6 @@ int sdw_slave_add(struct sdw_bus *bus,
for (i = 0; i < SDW_MAX_PORTS; i++)
init_completion(&slave->port_ready[i]);
- mutex_lock(&bus->bus_lock);
- list_add_tail(&slave->node, &bus->slaves);
- mutex_unlock(&bus->bus_lock);
ret = device_register(&slave->dev);
if (ret) {
@@ -77,13 +74,15 @@ int sdw_slave_add(struct sdw_bus *bus,
* On err, don't free but drop ref as this will be freed
* when release method is invoked.
*/
- mutex_lock(&bus->bus_lock);
- list_del(&slave->node);
- mutex_unlock(&bus->bus_lock);
put_device(&slave->dev);
return ret;
}
+
+ mutex_lock(&bus->bus_lock);
+ list_add_tail(&slave->node, &bus->slaves);
+ mutex_unlock(&bus->bus_lock);
+
sdw_slave_debugfs_init(slave);
return ret;
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] soundwire: add slave device to linked list after device_register()
2021-03-23 2:25 ` Bard Liao
@ 2021-03-23 6:57 ` Vinod Koul
-1 siblings, 0 replies; 7+ messages in thread
From: Vinod Koul @ 2021-03-23 6:57 UTC (permalink / raw)
To: Bard Liao
Cc: alsa-devel, linux-kernel, gregkh, srinivas.kandagatla,
rander.wang, hui.wang, pierre-louis.bossart, sanyog.r.kale,
bard.liao
On 23-03-21, 10:25, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>
> We currently add the slave device to a linked list before
> device_register(), and then remove it if device_register() fails.
>
> It's not clear why this sequence was necessary, this patch moves the
> linked list management to after the device_register().
Maybe add a comment :-)
The reason here is that before calling device_register() we need to be
ready and completely initialized. device_register is absolutely the last
step in the flow, always.
The probe of the device can happen and before you get a chance to
add to list, many number of things can happen.. So adding after is not a
very good idea :)
HTH
>
> Suggested-by: Keyon Jie <yang.jie@linux.intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
> drivers/soundwire/slave.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
> index 112b21967c7a..0c92db2e1ddc 100644
> --- a/drivers/soundwire/slave.c
> +++ b/drivers/soundwire/slave.c
> @@ -65,9 +65,6 @@ int sdw_slave_add(struct sdw_bus *bus,
> for (i = 0; i < SDW_MAX_PORTS; i++)
> init_completion(&slave->port_ready[i]);
>
> - mutex_lock(&bus->bus_lock);
> - list_add_tail(&slave->node, &bus->slaves);
> - mutex_unlock(&bus->bus_lock);
>
> ret = device_register(&slave->dev);
> if (ret) {
> @@ -77,13 +74,15 @@ int sdw_slave_add(struct sdw_bus *bus,
> * On err, don't free but drop ref as this will be freed
> * when release method is invoked.
> */
> - mutex_lock(&bus->bus_lock);
> - list_del(&slave->node);
> - mutex_unlock(&bus->bus_lock);
> put_device(&slave->dev);
>
> return ret;
> }
> +
> + mutex_lock(&bus->bus_lock);
> + list_add_tail(&slave->node, &bus->slaves);
> + mutex_unlock(&bus->bus_lock);
> +
> sdw_slave_debugfs_init(slave);
>
> return ret;
> --
> 2.17.1
--
~Vinod
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] soundwire: add slave device to linked list after device_register()
@ 2021-03-23 6:57 ` Vinod Koul
0 siblings, 0 replies; 7+ messages in thread
From: Vinod Koul @ 2021-03-23 6:57 UTC (permalink / raw)
To: Bard Liao
Cc: alsa-devel, gregkh, linux-kernel, pierre-louis.bossart, hui.wang,
srinivas.kandagatla, sanyog.r.kale, rander.wang, bard.liao
On 23-03-21, 10:25, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>
> We currently add the slave device to a linked list before
> device_register(), and then remove it if device_register() fails.
>
> It's not clear why this sequence was necessary, this patch moves the
> linked list management to after the device_register().
Maybe add a comment :-)
The reason here is that before calling device_register() we need to be
ready and completely initialized. device_register is absolutely the last
step in the flow, always.
The probe of the device can happen and before you get a chance to
add to list, many number of things can happen.. So adding after is not a
very good idea :)
HTH
>
> Suggested-by: Keyon Jie <yang.jie@linux.intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
> drivers/soundwire/slave.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
> index 112b21967c7a..0c92db2e1ddc 100644
> --- a/drivers/soundwire/slave.c
> +++ b/drivers/soundwire/slave.c
> @@ -65,9 +65,6 @@ int sdw_slave_add(struct sdw_bus *bus,
> for (i = 0; i < SDW_MAX_PORTS; i++)
> init_completion(&slave->port_ready[i]);
>
> - mutex_lock(&bus->bus_lock);
> - list_add_tail(&slave->node, &bus->slaves);
> - mutex_unlock(&bus->bus_lock);
>
> ret = device_register(&slave->dev);
> if (ret) {
> @@ -77,13 +74,15 @@ int sdw_slave_add(struct sdw_bus *bus,
> * On err, don't free but drop ref as this will be freed
> * when release method is invoked.
> */
> - mutex_lock(&bus->bus_lock);
> - list_del(&slave->node);
> - mutex_unlock(&bus->bus_lock);
> put_device(&slave->dev);
>
> return ret;
> }
> +
> + mutex_lock(&bus->bus_lock);
> + list_add_tail(&slave->node, &bus->slaves);
> + mutex_unlock(&bus->bus_lock);
> +
> sdw_slave_debugfs_init(slave);
>
> return ret;
> --
> 2.17.1
--
~Vinod
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] soundwire: add slave device to linked list after device_register()
2021-03-23 6:57 ` Vinod Koul
(?)
@ 2021-03-23 18:30 ` Pierre-Louis Bossart
2021-04-19 1:46 ` Liao, Bard
-1 siblings, 1 reply; 7+ messages in thread
From: Pierre-Louis Bossart @ 2021-03-23 18:30 UTC (permalink / raw)
To: Vinod Koul, Bard Liao
Cc: alsa-devel, gregkh, linux-kernel, hui.wang, srinivas.kandagatla,
sanyog.r.kale, rander.wang, bard.liao
Hi Vinod,
>> We currently add the slave device to a linked list before
>> device_register(), and then remove it if device_register() fails.
>>
>> It's not clear why this sequence was necessary, this patch moves the
>> linked list management to after the device_register().
>
> Maybe add a comment :-)
>
> The reason here is that before calling device_register() we need to be
> ready and completely initialized. device_register is absolutely the last
> step in the flow, always.
>
> The probe of the device can happen and before you get a chance to
> add to list, many number of things can happen.. So adding after is not a
> very good idea :)
Can you describe that 'many number of things' in the SoundWire context?
While you are correct in general on the use of device_register(), in
this specific case the device registration and the possible Slave driver
probe if there's a match doesn't seem to use this linked list.
This sdw_slave_add() routine is called while parsing ACPI/DT tables and
at this point the bus isn't functional. This sequence only deals with
device registration and driver probe, the actual activation and
enumeration happen much later. What does matter is that by the time all
ACPI/DT devices have been scanned all initialization is complete. The
last part of the flow is not the device_register() at the individual
peripheral level.
Even for the Qualcomm case, the steps are different:
ret = sdw_bus_master_add(&ctrl->bus, dev, dev->fwnode);
if (ret) {
dev_err(dev, "Failed to register Soundwire controller (%d)\n",
ret);
goto err_clk;
}
qcom_swrm_init(ctrl); <<< that's where the bus is functional
Note that we are not going to lay on the tracks for this, this sequence
was tagged by static analysis tools who don't understand that
put_device() actually frees memory by way of the .release callback, but
that led us to ask ourselves whether this sequence was actually necessary.
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] soundwire: add slave device to linked list after device_register()
2021-03-23 18:30 ` Pierre-Louis Bossart
@ 2021-04-19 1:46 ` Liao, Bard
0 siblings, 0 replies; 7+ messages in thread
From: Liao, Bard @ 2021-04-19 1:46 UTC (permalink / raw)
To: Pierre-Louis Bossart, Vinod Koul, Bard Liao
Cc: alsa-devel, gregkh, linux-kernel, hui.wang, srinivas.kandagatla,
Kale, Sanyog R, rander.wang
> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Sent: Wednesday, March 24, 2021 2:31 AM
> To: Vinod Koul <vkoul@kernel.org>; Bard Liao <yung-
> chuan.liao@linux.intel.com>
> Cc: alsa-devel@alsa-project.org; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org; hui.wang@canonical.com;
> srinivas.kandagatla@linaro.org; Kale, Sanyog R <sanyog.r.kale@intel.com>;
> rander.wang@linux.intel.com; Liao, Bard <bard.liao@intel.com>
> Subject: Re: [PATCH] soundwire: add slave device to linked list after
> device_register()
>
> Hi Vinod,
>
> >> We currently add the slave device to a linked list before
> >> device_register(), and then remove it if device_register() fails.
> >>
> >> It's not clear why this sequence was necessary, this patch moves the
> >> linked list management to after the device_register().
> >
> > Maybe add a comment :-)
> >
> > The reason here is that before calling device_register() we need to be
> > ready and completely initialized. device_register is absolutely the
> > last step in the flow, always.
> >
> > The probe of the device can happen and before you get a chance to add
> > to list, many number of things can happen.. So adding after is not a
> > very good idea :)
>
> Can you describe that 'many number of things' in the SoundWire context?
>
> While you are correct in general on the use of device_register(), in this specific
> case the device registration and the possible Slave driver probe if there's a
> match doesn't seem to use this linked list.
>
> This sdw_slave_add() routine is called while parsing ACPI/DT tables and at this
> point the bus isn't functional. This sequence only deals with device registration
> and driver probe, the actual activation and enumeration happen much later.
> What does matter is that by the time all ACPI/DT devices have been scanned all
> initialization is complete. The last part of the flow is not the device_register() at
> the individual peripheral level.
>
> Even for the Qualcomm case, the steps are different:
>
> ret = sdw_bus_master_add(&ctrl->bus, dev, dev->fwnode);
> if (ret) {
> dev_err(dev, "Failed to register Soundwire controller (%d)\n",
> ret);
> goto err_clk;
> }
>
> qcom_swrm_init(ctrl); <<< that's where the bus is functional
>
> Note that we are not going to lay on the tracks for this, this sequence was
> tagged by static analysis tools who don't understand that
> put_device() actually frees memory by way of the .release callback, but that led
> us to ask ourselves whether this sequence was actually necessary.
Hi Vinod,
Do you have any comment or objection after Pierre's explanation?
Regards,
Bard
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] soundwire: add slave device to linked list after device_register()
@ 2021-04-19 1:46 ` Liao, Bard
0 siblings, 0 replies; 7+ messages in thread
From: Liao, Bard @ 2021-04-19 1:46 UTC (permalink / raw)
To: Pierre-Louis Bossart, Vinod Koul, Bard Liao
Cc: alsa-devel, gregkh, linux-kernel, hui.wang, Kale, Sanyog R, rander.wang
> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Sent: Wednesday, March 24, 2021 2:31 AM
> To: Vinod Koul <vkoul@kernel.org>; Bard Liao <yung-
> chuan.liao@linux.intel.com>
> Cc: alsa-devel@alsa-project.org; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org; hui.wang@canonical.com;
> srinivas.kandagatla@linaro.org; Kale, Sanyog R <sanyog.r.kale@intel.com>;
> rander.wang@linux.intel.com; Liao, Bard <bard.liao@intel.com>
> Subject: Re: [PATCH] soundwire: add slave device to linked list after
> device_register()
>
> Hi Vinod,
>
> >> We currently add the slave device to a linked list before
> >> device_register(), and then remove it if device_register() fails.
> >>
> >> It's not clear why this sequence was necessary, this patch moves the
> >> linked list management to after the device_register().
> >
> > Maybe add a comment :-)
> >
> > The reason here is that before calling device_register() we need to be
> > ready and completely initialized. device_register is absolutely the
> > last step in the flow, always.
> >
> > The probe of the device can happen and before you get a chance to add
> > to list, many number of things can happen.. So adding after is not a
> > very good idea :)
>
> Can you describe that 'many number of things' in the SoundWire context?
>
> While you are correct in general on the use of device_register(), in this specific
> case the device registration and the possible Slave driver probe if there's a
> match doesn't seem to use this linked list.
>
> This sdw_slave_add() routine is called while parsing ACPI/DT tables and at this
> point the bus isn't functional. This sequence only deals with device registration
> and driver probe, the actual activation and enumeration happen much later.
> What does matter is that by the time all ACPI/DT devices have been scanned all
> initialization is complete. The last part of the flow is not the device_register() at
> the individual peripheral level.
>
> Even for the Qualcomm case, the steps are different:
>
> ret = sdw_bus_master_add(&ctrl->bus, dev, dev->fwnode);
> if (ret) {
> dev_err(dev, "Failed to register Soundwire controller (%d)\n",
> ret);
> goto err_clk;
> }
>
> qcom_swrm_init(ctrl); <<< that's where the bus is functional
>
> Note that we are not going to lay on the tracks for this, this sequence was
> tagged by static analysis tools who don't understand that
> put_device() actually frees memory by way of the .release callback, but that led
> us to ask ourselves whether this sequence was actually necessary.
Hi Vinod,
Do you have any comment or objection after Pierre's explanation?
Regards,
Bard
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-04-19 1:48 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-23 2:25 [PATCH] soundwire: add slave device to linked list after device_register() Bard Liao
2021-03-23 2:25 ` Bard Liao
2021-03-23 6:57 ` Vinod Koul
2021-03-23 6:57 ` Vinod Koul
2021-03-23 18:30 ` Pierre-Louis Bossart
2021-04-19 1:46 ` Liao, Bard
2021-04-19 1:46 ` Liao, Bard
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.