Re: [PATCH] platform/surface: aggregator_registry: Give devices time to set up when connecting

[Hans de Goede]

Hi,

On 4/6/21 1:12 AM, Maximilian Luz wrote:
> Sometimes, the "base connected" event that we rely on to (re-)attach the
> device connected to the base is sent a bit too early. When this happens,
> some devices may not be completely ready yet.
> 
> Specifically, the battery has been observed to report zero-values for
> things like full charge capacity, which, however, is only loaded once
> when the driver for that device probes. This can thus result in battery
> readings being unavailable.
> 
> As we cannot easily and reliably discern between devices that are not
> ready yet and devices that are not connected (i.e. will never be ready),
> delay adding these devices. This should give them enough time to set up.
> 
> The delay is set to 2.5 seconds, which should give us a good safety
> margin based on testing and still be fairly responsive for users.
> 
> To achieve that delay switch to updating via a delayed work struct,
> which means that we can also get rid of some locking.
> 
> Signed-off-by: Maximilian Luz <luzmaximilian@xxxxxxxxx>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans


> ---
>  .../surface/surface_aggregator_registry.c     | 98 ++++++++-----------
>  1 file changed, 40 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c
> index eccb9d1007cd..685d37a7add1 100644
> --- a/drivers/platform/surface/surface_aggregator_registry.c
> +++ b/drivers/platform/surface/surface_aggregator_registry.c
> @@ -13,10 +13,10 @@
>  #include <linux/kernel.h>
>  #include <linux/limits.h>
>  #include <linux/module.h>
> -#include <linux/mutex.h>
>  #include <linux/platform_device.h>
>  #include <linux/property.h>
>  #include <linux/types.h>
> +#include <linux/workqueue.h>
>  
>  #include <linux/surface_aggregator/controller.h>
>  #include <linux/surface_aggregator/device.h>
> @@ -287,6 +287,13 @@ static int ssam_hub_add_devices(struct device *parent, struct ssam_controller *c
>  
>  /* -- SSAM base-hub driver. ------------------------------------------------- */
>  
> +/*
> + * Some devices (especially battery) may need a bit of time to be fully usable
> + * after being (re-)connected. This delay has been determined via
> + * experimentation.
> + */
> +#define SSAM_BASE_UPDATE_CONNECT_DELAY		msecs_to_jiffies(2500)
> +
>  enum ssam_base_hub_state {
>  	SSAM_BASE_HUB_UNINITIALIZED,
>  	SSAM_BASE_HUB_CONNECTED,
> @@ -296,8 +303,8 @@ enum ssam_base_hub_state {
>  struct ssam_base_hub {
>  	struct ssam_device *sdev;
>  
> -	struct mutex lock;  /* Guards state update checks and transitions. */
>  	enum ssam_base_hub_state state;
> +	struct delayed_work update_work;
>  
>  	struct ssam_event_notifier notif;
>  };
> @@ -335,11 +342,7 @@ static ssize_t ssam_base_hub_state_show(struct device *dev, struct device_attrib
>  					char *buf)
>  {
>  	struct ssam_base_hub *hub = dev_get_drvdata(dev);
> -	bool connected;
> -
> -	mutex_lock(&hub->lock);
> -	connected = hub->state == SSAM_BASE_HUB_CONNECTED;
> -	mutex_unlock(&hub->lock);
> +	bool connected = hub->state == SSAM_BASE_HUB_CONNECTED;
>  
>  	return sysfs_emit(buf, "%d\n", connected);
>  }
> @@ -356,16 +359,20 @@ static const struct attribute_group ssam_base_hub_group = {
>  	.attrs = ssam_base_hub_attrs,
>  };
>  
> -static int __ssam_base_hub_update(struct ssam_base_hub *hub, enum ssam_base_hub_state new)
> +static void ssam_base_hub_update_workfn(struct work_struct *work)
>  {
> +	struct ssam_base_hub *hub = container_of(work, struct ssam_base_hub, update_work.work);
>  	struct fwnode_handle *node = dev_fwnode(&hub->sdev->dev);
> +	enum ssam_base_hub_state state;
>  	int status = 0;
>  
> -	lockdep_assert_held(&hub->lock);
> +	status = ssam_base_hub_query_state(hub, &state);
> +	if (status)
> +		return;
>  
> -	if (hub->state == new)
> -		return 0;
> -	hub->state = new;
> +	if (hub->state == state)
> +		return;
> +	hub->state = state;
>  
>  	if (hub->state == SSAM_BASE_HUB_CONNECTED)
>  		status = ssam_hub_add_devices(&hub->sdev->dev, hub->sdev->ctrl, node);
> @@ -374,51 +381,28 @@ static int __ssam_base_hub_update(struct ssam_base_hub *hub, enum ssam_base_hub_
>  
>  	if (status)
>  		dev_err(&hub->sdev->dev, "failed to update base-hub devices: %d\n", status);
> -
> -	return status;
> -}
> -
> -static int ssam_base_hub_update(struct ssam_base_hub *hub)
> -{
> -	enum ssam_base_hub_state state;
> -	int status;
> -
> -	mutex_lock(&hub->lock);
> -
> -	status = ssam_base_hub_query_state(hub, &state);
> -	if (!status)
> -		status = __ssam_base_hub_update(hub, state);
> -
> -	mutex_unlock(&hub->lock);
> -	return status;
>  }
>  
>  static u32 ssam_base_hub_notif(struct ssam_event_notifier *nf, const struct ssam_event *event)
>  {
> -	struct ssam_base_hub *hub;
> -	struct ssam_device *sdev;
> -	enum ssam_base_hub_state new;
> -
> -	hub = container_of(nf, struct ssam_base_hub, notif);
> -	sdev = hub->sdev;
> +	struct ssam_base_hub *hub = container_of(nf, struct ssam_base_hub, notif);
> +	unsigned long delay;
>  
>  	if (event->command_id != SSAM_EVENT_BAS_CID_CONNECTION)
>  		return 0;
>  
>  	if (event->length < 1) {
> -		dev_err(&sdev->dev, "unexpected payload size: %u\n",
> -			event->length);
> +		dev_err(&hub->sdev->dev, "unexpected payload size: %u\n", event->length);
>  		return 0;
>  	}
>  
> -	if (event->data[0])
> -		new = SSAM_BASE_HUB_CONNECTED;
> -	else
> -		new = SSAM_BASE_HUB_DISCONNECTED;
> +	/*
> +	 * Delay update when the base is being connected to give devices/EC
> +	 * some time to set up.
> +	 */
> +	delay = event->data[0] ? SSAM_BASE_UPDATE_CONNECT_DELAY : 0;
>  
> -	mutex_lock(&hub->lock);
> -	__ssam_base_hub_update(hub, new);
> -	mutex_unlock(&hub->lock);
> +	schedule_delayed_work(&hub->update_work, delay);
>  
>  	/*
>  	 * Do not return SSAM_NOTIF_HANDLED: The event should be picked up and
> @@ -430,7 +414,10 @@ static u32 ssam_base_hub_notif(struct ssam_event_notifier *nf, const struct ssam
>  
>  static int __maybe_unused ssam_base_hub_resume(struct device *dev)
>  {
> -	return ssam_base_hub_update(dev_get_drvdata(dev));
> +	struct ssam_base_hub *hub = dev_get_drvdata(dev);
> +
> +	schedule_delayed_work(&hub->update_work, 0);
> +	return 0;
>  }
>  static SIMPLE_DEV_PM_OPS(ssam_base_hub_pm_ops, NULL, ssam_base_hub_resume);
>  
> @@ -443,8 +430,6 @@ static int ssam_base_hub_probe(struct ssam_device *sdev)
>  	if (!hub)
>  		return -ENOMEM;
>  
> -	mutex_init(&hub->lock);
> -
>  	hub->sdev = sdev;
>  	hub->state = SSAM_BASE_HUB_UNINITIALIZED;
>  
> @@ -456,27 +441,25 @@ static int ssam_base_hub_probe(struct ssam_device *sdev)
>  	hub->notif.event.mask = SSAM_EVENT_MASK_NONE;
>  	hub->notif.event.flags = SSAM_EVENT_SEQUENCED;
>  
> +	INIT_DELAYED_WORK(&hub->update_work, ssam_base_hub_update_workfn);
> +
>  	ssam_device_set_drvdata(sdev, hub);
>  
>  	status = ssam_notifier_register(sdev->ctrl, &hub->notif);
>  	if (status)
> -		goto err_register;
> -
> -	status = ssam_base_hub_update(hub);
> -	if (status)
> -		goto err_update;
> +		return status;
>  
>  	status = sysfs_create_group(&sdev->dev.kobj, &ssam_base_hub_group);
>  	if (status)
> -		goto err_update;
> +		goto err;
>  
> +	schedule_delayed_work(&hub->update_work, 0);
>  	return 0;
>  
> -err_update:
> +err:
>  	ssam_notifier_unregister(sdev->ctrl, &hub->notif);
> +	cancel_delayed_work_sync(&hub->update_work);
>  	ssam_hub_remove_devices(&sdev->dev);
> -err_register:
> -	mutex_destroy(&hub->lock);
>  	return status;
>  }
>  
> @@ -487,9 +470,8 @@ static void ssam_base_hub_remove(struct ssam_device *sdev)
>  	sysfs_remove_group(&sdev->dev.kobj, &ssam_base_hub_group);
>  
>  	ssam_notifier_unregister(sdev->ctrl, &hub->notif);
> +	cancel_delayed_work_sync(&hub->update_work);
>  	ssam_hub_remove_devices(&sdev->dev);
> -
> -	mutex_destroy(&hub->lock);
>  }
>  
>  static const struct ssam_device_id ssam_base_hub_match[] = {
>