| Message ID | 20250410093146.3776801-2-aitsygunka@yandex.ru |
|---|---|
| State | New |
| Headers | show |
| Series | usb: ueagle-atm: wait for a firmware upload to complete | expand |
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@vger.kernel.org > Signed-off-by: Andrey Tsygunka <aitsygunka@yandex.ru> 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@wp.pl> > 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 >
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);
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@vger.kernel.org Signed-off-by: Andrey Tsygunka <aitsygunka@yandex.ru> --- drivers/usb/atm/ueagle-atm.c | 40 +++++++++++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 5 deletions(-)