Re: [PATCH v2 3/4] Bluetooth: hci_h5: use device_schedule_reprobe()

[Hans de Goede]

Hi,

On 20-Aug-26 02:32, Daniel Golle wrote:
> h5_btrtl_resume() open-codes a deferred re-probe for RTL devices that
> lose their firmware state over suspend: it takes a module reference,
> allocates a work item, and the work function calls device_reprobe()
> and then ends with put_device(); kfree(); module_put(THIS_MODULE); in
> module text. That final module_put() is racy: once the reference
> count is decremented a concurrent rmmod can free the module text
> before the work function's epilogue has finished executing. The work
> also does not synchronize against shutdown or unbind, so a stale
> re-probe could undo an administrative unbind or detach a device whose
> ->shutdown() callback has already run.
> 
> Convert to the new device_schedule_reprobe() helper, whose work
> function is builtin text and which skips the re-probe when the device
> was removed, shutdown reached it, or it is no longer bound to the
> driver that scheduled the re-probe.
> 
> The old worker suppressed its error message for -EPROBE_DEFER; the
> helper needs no equivalent because its attach half is
> device_attach(), which folds probe deferral into the deferred-probe
> machinery silently.
> 
> Behavioral changes:
> 
> - A pending re-probe no longer pins the module: rmmod with a re-probe
>   pending now succeeds immediately and the re-probe becomes a no-op,
>   instead of rmmod failing with EBUSY.
> 
> - A re-probe scheduled before a system shutdown or before an
>   administrative unbind no longer detaches and rebinds the device
>   afterwards.
> 
> - A re-probe racing the next suspend now detaches immediately while
>   the probe is deferred until the following resume by the
>   defer_all_probes machinery, instead of probing mid-suspend.
> 
> Signed-off-by: Daniel Golle <daniel@xxxxxxxxxxxxxx>

Thanks, this looks good to me and works as advertised on
a device where this code path gets hit:

Tested-by: Hans de Goede <johannes.goede@xxxxxxxxxxxxxxxx>
Reviewed-by: Hans de Goede <johannes.goede@xxxxxxxxxxxxxxxx>

Regards,

Hans



> ---
>  drivers/bluetooth/hci_h5.c | 43 ++++++--------------------------------
>  1 file changed, 6 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
> index 93cdde981840..68eaa03a2005 100644
> --- a/drivers/bluetooth/hci_h5.c
> +++ b/drivers/bluetooth/hci_h5.c
> @@ -990,7 +990,7 @@ static int h5_btrtl_setup(struct h5 *h5)
>  static void h5_btrtl_open(struct h5 *h5)
>  {
>  	/*
> -	 * Since h5_btrtl_resume() does a device_reprobe() the suspend handling
> +	 * Since h5_btrtl_resume() schedules a device re-probe the suspend handling
>  	 * done by the hci_suspend_notifier is not necessary; it actually causes
>  	 * delays and a bunch of errors to get logged, so disable it.
>  	 */
> @@ -1047,46 +1047,15 @@ static int h5_btrtl_suspend(struct h5 *h5)
>  	return 0;
>  }
>  
> -struct h5_btrtl_reprobe {
> -	struct device *dev;
> -	struct work_struct work;
> -};
> -
> -static void h5_btrtl_reprobe_worker(struct work_struct *work)
> -{
> -	struct h5_btrtl_reprobe *reprobe =
> -		container_of(work, struct h5_btrtl_reprobe, work);
> -	int ret;
> -
> -	ret = device_reprobe(reprobe->dev);
> -	if (ret && ret != -EPROBE_DEFER)
> -		dev_err(reprobe->dev, "Reprobe error %d\n", ret);
> -
> -	put_device(reprobe->dev);
> -	kfree(reprobe);
> -	module_put(THIS_MODULE);
> -}
> -
>  static int h5_btrtl_resume(struct h5 *h5)
>  {
> -	if (test_bit(H5_WAKEUP_DISABLE, &h5->flags)) {
> -		struct h5_btrtl_reprobe *reprobe;
> -
> -		reprobe = kzalloc_obj(*reprobe);
> -		if (!reprobe)
> -			return -ENOMEM;
> -
> -		__module_get(THIS_MODULE);
> +	if (test_bit(H5_WAKEUP_DISABLE, &h5->flags))
> +		return device_schedule_reprobe(&h5->hu->serdev->dev, 0);
>  
> -		INIT_WORK(&reprobe->work, h5_btrtl_reprobe_worker);
> -		reprobe->dev = get_device(&h5->hu->serdev->dev);
> -		queue_work(system_long_wq, &reprobe->work);
> -	} else {
> -		gpiod_set_value_cansleep(h5->device_wake_gpio, 1);
> +	gpiod_set_value_cansleep(h5->device_wake_gpio, 1);
>  
> -		if (test_bit(H5_HW_FLOW_CONTROL, &h5->flags))
> -			serdev_device_set_flow_control(h5->hu->serdev, true);
> -	}
> +	if (test_bit(H5_HW_FLOW_CONTROL, &h5->flags))
> +		serdev_device_set_flow_control(h5->hu->serdev, true);
>  
>  	return 0;
>  }