Re: [PATCH 1/1] usb: ueagle-atm: wait for a firmware upload to complete

[Stanislaw Gruszka]

On Thu, Apr 10, 2025 at 12:31:46PM +0300, Andrey Tsygunka wrote:
> Syzkaller reported:
> 
> sysfs group 'power' not found for kobject 'ueagle-atm!adi930.fw'
> WARNING: CPU: 1 PID: 6804 at fs/sysfs/group.c:278 sysfs_remove_group+0x120/0x170 fs/sysfs/group.c:278
> Modules linked in:
> CPU: 1 PID: 6804 Comm: kworker/1:5 Not tainted 6.1.128 #1
> Hardware name: linux,dummy-virt (DT)
> Workqueue: events request_firmware_work_func
> Call trace:
>  sysfs_remove_group+0x120/0x170 fs/sysfs/group.c:278
>  dpm_sysfs_remove+0x9c/0xc0 drivers/base/power/sysfs.c:837
>  device_del+0x1e0/0xb30 drivers/base/core.c:3861
>  fw_load_sysfs_fallback drivers/base/firmware_loader/fallback.c:120 [inline]
>  fw_load_from_user_helper drivers/base/firmware_loader/fallback.c:158 [inline]
>  firmware_fallback_sysfs+0x880/0xa30 drivers/base/firmware_loader/fallback.c:234
>  _request_firmware+0xcc0/0x1030 drivers/base/firmware_loader/main.c:884
>  request_firmware_work_func+0xf0/0x240 drivers/base/firmware_loader/main.c:1135
>  process_one_work+0x878/0x1770 kernel/workqueue.c:2292
>  worker_thread+0x48c/0xe40 kernel/workqueue.c:2439
>  kthread+0x274/0x2e0 kernel/kthread.c:376
>  ret_from_fork+0x10/0x20 arch/arm64/kernel/entry.S:864
> 
> When calling the usb-device probe() method, request_firmware_nowait()
> is called, an async task is created that creates a child device
> to load the firmware and waits (fw_sysfs_wait_timeout()) for the
> firmware to be ready. If an async disconnect event occurs for
> usb-device while waiting, we may get a WARN() when calling
> firmware_fallback_sysfs() about "no sysfs group 'power' found
> for kobject" because it was removed by usb_disconnect().
> 
> To avoid this, add a routine to wait for the firmware loading process
> to complete to prevent premature device disconnection.
> 
> Fixes: b72458a80c75 ("[PATCH] USB: Eagle and ADI 930 usb adsl modem driver")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Andrey Tsygunka <aitsygunka@xxxxxxxxx>

Hi, thanks for the patch, but I do not see benefit of fix ex-WARN and
now pr_debug(). Only downside of adding extra 40 lines of code.

Nacked-by: Stanislaw Gruszka <stf_xl@xxxxx>

>  drivers/usb/atm/ueagle-atm.c | 40 +++++++++++++++++++++++++++++++-----
>  1 file changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/atm/ueagle-atm.c b/drivers/usb/atm/ueagle-atm.c
> index cd0f7b4bd82a..eaa5ad316d89 100644
> --- a/drivers/usb/atm/ueagle-atm.c
> +++ b/drivers/usb/atm/ueagle-atm.c
> @@ -570,6 +570,12 @@ MODULE_PARM_DESC(annex,
>  #define LOAD_INTERNAL     0xA0
>  #define F8051_USBCS       0x7f92
>  
> +struct uea_interface_data {
> +	struct completion fw_upload_complete;
> +	struct usb_device *usb;
> +	struct usb_interface *intf;
> +};
> +
>  /*
>   * uea_send_modem_cmd - Send a command for pre-firmware devices.
>   */
> @@ -599,7 +605,8 @@ static int uea_send_modem_cmd(struct usb_device *usb,
>  static void uea_upload_pre_firmware(const struct firmware *fw_entry,
>  								void *context)
>  {
> -	struct usb_device *usb = context;
> +	struct uea_interface_data *uea_intf_data = context;
> +	struct usb_device *usb = uea_intf_data->usb;
>  	const u8 *pfw;
>  	u8 value;
>  	u32 crc = 0;
> @@ -669,15 +676,17 @@ static void uea_upload_pre_firmware(const struct firmware *fw_entry,
>  	uea_err(usb, "firmware is corrupted\n");
>  err:
>  	release_firmware(fw_entry);
> +	complete(&uea_intf_data->fw_upload_complete);
>  	uea_leaves(usb);
>  }
>  
>  /*
>   * uea_load_firmware - Load usb firmware for pre-firmware devices.
>   */
> -static int uea_load_firmware(struct usb_device *usb, unsigned int ver)
> +static int uea_load_firmware(struct uea_interface_data *uea_intf_data, unsigned int ver)
>  {
>  	int ret;
> +	struct usb_device *usb = uea_intf_data->usb;
>  	char *fw_name = EAGLE_FIRMWARE;
>  
>  	uea_enters(usb);
> @@ -702,7 +711,7 @@ static int uea_load_firmware(struct usb_device *usb, unsigned int ver)
>  	}
>  
>  	ret = request_firmware_nowait(THIS_MODULE, 1, fw_name, &usb->dev,
> -					GFP_KERNEL, usb,
> +					GFP_KERNEL, uea_intf_data,
>  					uea_upload_pre_firmware);
>  	if (ret)
>  		uea_err(usb, "firmware %s is not available\n", fw_name);
> @@ -2586,6 +2595,7 @@ static struct usbatm_driver uea_usbatm_driver = {
>  static int uea_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  {
>  	struct usb_device *usb = interface_to_usbdev(intf);
> +	struct uea_interface_data *uea_intf_data;
>  	int ret;
>  
>  	uea_enters(usb);
> @@ -2597,8 +2607,23 @@ static int uea_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  
>  	usb_reset_device(usb);
>  
> -	if (UEA_IS_PREFIRM(id))
> -		return uea_load_firmware(usb, UEA_CHIP_VERSION(id));
> +	if (UEA_IS_PREFIRM(id)) {
> +		uea_intf_data = devm_kzalloc(&usb->dev, sizeof(*uea_intf_data), GFP_KERNEL);
> +		if (!uea_intf_data)
> +			return -ENOMEM;
> +
> +		init_completion(&uea_intf_data->fw_upload_complete);
> +		uea_intf_data->usb = usb;
> +		uea_intf_data->intf = intf;
> +
> +		usb_set_intfdata(intf, uea_intf_data);
> +
> +		ret = uea_load_firmware(uea_intf_data, UEA_CHIP_VERSION(id));
> +		if (ret)
> +			complete(&uea_intf_data->fw_upload_complete);
> +
> +		return ret;
> +	}
>  
>  	ret = usbatm_usb_probe(intf, id, &uea_usbatm_driver);
>  	if (ret == 0) {
> @@ -2618,6 +2643,7 @@ static int uea_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  static void uea_disconnect(struct usb_interface *intf)
>  {
>  	struct usb_device *usb = interface_to_usbdev(intf);
> +	struct uea_interface_data *uea_intf_data;
>  	int ifnum = intf->altsetting->desc.bInterfaceNumber;
>  	uea_enters(usb);
>  
> @@ -2629,6 +2655,10 @@ static void uea_disconnect(struct usb_interface *intf)
>  		usbatm_usb_disconnect(intf);
>  		mutex_unlock(&uea_mutex);
>  		uea_info(usb, "ADSL device removed\n");
> +	} else {
> +		uea_intf_data = usb_get_intfdata(intf);
> +		uea_info(usb, "wait for completion uploading firmware\n");
> +		wait_for_completion(&uea_intf_data->fw_upload_complete);
>  	}
>  
>  	uea_leaves(usb);
> -- 
> 2.25.1
>