[RFC PATCH] soundwire: use driver callbacks directly with proper locking

[Pierre-Louis Bossart]

Thanks Vinod for the review.

>> I could use feedback on whether using device_lock() is appropriate and
> 
> Looking at other uses of device_lock() it seems apt to me
> 
>> test results on non-Intel platforms. Thanks!
>> Pierre
>>
>>  drivers/soundwire/bus.c      | 78 ++++++++++++++++++++++++++++--------
>>  drivers/soundwire/bus_type.c |  6 +--
>>  drivers/soundwire/stream.c   | 57 +++++++++++++++++---------
>>  3 files changed, 102 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
>> index 8b7a680f388e..545b379a119e 100644
>> --- a/drivers/soundwire/bus.c
>> +++ b/drivers/soundwire/bus.c
>> @@ -7,6 +7,7 @@
>>  #include <linux/pm_runtime.h>
>>  #include <linux/soundwire/sdw_registers.h>
>>  #include <linux/soundwire/sdw.h>
>> +#include <linux/soundwire/sdw_type.h>
>>  #include "bus.h"
>>  #include "sysfs_local.h"
>>  
>> @@ -846,12 +847,18 @@ static int sdw_slave_clk_stop_callback(struct sdw_slave *slave,
>>  				       enum sdw_clk_stop_mode mode,
>>  				       enum sdw_clk_stop_type type)
>>  {
>> -	int ret;
>> +	struct device *dev = &slave->dev;
>> +	struct sdw_driver *drv;
>>  
>> -	if (slave->ops && slave->ops->clk_stop) {
>> -		ret = slave->ops->clk_stop(slave, mode, type);
>> -		if (ret < 0)
>> -			return ret;
>> +	/*
>> +	 * this function can only be called from a pm_runtime
>> +	 * sequence where the device is already locked
>> +	 */
> 
> If this is guaranteed..
> 
>> +
>> +	if (dev->driver) {
> 
> do we need to check this? Did you find a case where this was not valid
> while device is locked, maybe do this while holding the lock (kind of
> moot to process the calls if driver is gone)

Humm, good feedback. I will re-check for cases where the driver is 'blacklisted' and also cases there there's no power management supported.

> 
>> +		drv = drv_to_sdw_driver(dev->driver);
>> +		if (drv && drv->ops && drv->ops->clk_stop)
>> +			return drv->ops->clk_stop(slave, mode, type);

In the last version I did remove the check for if (drv) above since it can't be NULL

>>  	}
>>  
>>  	return 0;
>> @@ -1616,14 +1623,25 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave)
>>  		}
>>  
>>  		/* Update the Slave driver */
>> -		if (slave_notify && slave->ops &&
>> -		    slave->ops->interrupt_callback) {
>> -			slave_intr.sdca_cascade = sdca_cascade;
>> -			slave_intr.control_port = clear;
>> -			memcpy(slave_intr.port, &port_status,
>> -			       sizeof(slave_intr.port));
>> -
>> -			slave->ops->interrupt_callback(slave, &slave_intr);
>> +		if (slave_notify) {
>> +			struct device *dev = &slave->dev;
>> +			struct sdw_driver *drv;
>> +
>> +			device_lock(dev);
> 
> device is locked
> 
>> +
>> +			if (dev->driver) {
> 
> Same here as well

This is a different case. You can have a headset codec that detects a headset and changes it status to alert, even when there is no driver probed.
So that case is very real, and it's simpler to think of the sequence backwards.

We have to verify if there is indeed a driver bound before invoking the 'interrupt_callback', otherwise we might de-reference a NULL or dangling pointer.
And to prevent a race condition with the .probe/.remove/.shutdown which will modify dev->driver, we have to take the lock before testing dev->driver.
 
> 
>> +				drv = drv_to_sdw_driver(dev->driver);
>> +				if (drv && drv->ops && drv->ops->interrupt_callback) {
>> +					slave_intr.sdca_cascade = sdca_cascade;
>> +					slave_intr.control_port = clear;
>> +					memcpy(slave_intr.port, &port_status,
>> +					       sizeof(slave_intr.port));
>> +
>> +					drv->ops->interrupt_callback(slave, &slave_intr);
>> +				}
>> +			}
>> +
>> +			device_unlock(dev);

>>  	/* Check all non-zero devices */
>> @@ -1878,7 +1908,23 @@ void sdw_clear_slave_status(struct sdw_bus *bus, u32 request)
>>  		if (slave->status != SDW_SLAVE_UNATTACHED) {
>>  			sdw_modify_slave_status(slave, SDW_SLAVE_UNATTACHED);
>>  			slave->first_interrupt_done = false;
>> +
>> +			lock = device_trylock(&slave->dev);
> 
> should we proceed if we dont get a lock? also why the trylock variant.
> We can do the lock, this is wq context

I tried to explain this with the comment below. if we take the lock unconditionally here when it's already taken by the resume sequence, then we created a dead-lock. 

This 'sdw_update_slave_status' is invoked from two completely different contexts.

a) the first case is when a manager resumes. In that case, we want to take a lock to prevent .probe or .remove from updating the dev->driver pointer while we're using it.
b) the remove is already on-going and the lock was taken by the remove routine. In that case we don't need to take a lock.

So the idea of trylock was to detect which of the two cases we have to deal with.

> 
>> +
>> +			/*
>> +			 * this bus/manager-level function can only be called from
>> +			 * a resume sequence. If the peripheral device (child of the
>> +			 *  manager device) is locked, this indicates a resume operation
>> +			 * initiated by the device core to deal with .remove() or .shutdown()
>> +			 * at the peripheral level. With the parent-child order enforced
>> +			 * by PM frameworks on resume, the peripheral resume has not started
>> +			 * yet, so it's safe to assume the lock will not be released while
>> +			 * the update_status callback is invoked.
>> +			 */
>>  			sdw_update_slave_status(slave, SDW_SLAVE_UNATTACHED);
>> +
>> +			if (lock)
>> +				device_unlock(&slave->dev);
>>  		}