>>> @@ -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.
I rechecked all this and it turns out I was mistaken. This function is part of a pm_runtime sequence indeed, but at the parent bus/manager device level. I confused levels and adding a deplock_assert showed very quickly that the peripheral device was never locked.
Thanks for pushing back on this!
In all other cases, I think it's valid and safe to take the lock and test dev->driver. It can happen that there is no driver enabled in the build, or that the driver is 'blacklisted', and in theory the user could muck with sysfs to trigger a peripheral driver binding sequence that would happen smack while the bus is suspended or resume.
I'll do more validation and send an update next week.
-Pierre
