Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
On 05/21/2015 08:13 PM, Marcel Holtmann wrote:
Hi Laura,
Then avoiding the failed firmware is no solution, indeed.
If it's a new probe, it should be never executed during resume.
Can you expand this comment? What's wrong with probing during resume?
The USB stack does carry out probes during resume under certain
circumstances. A driver lacking a reset_resume callback is one of
those circumstances.
in case the platform kills the power to the USB lines, we can never
do anything about this. I do not want to hack around this in the
driver.
What are the cases where we should implement reset_resume and would
it really help here. Since the btusb.ko driver implements
suspend/resume support, would reset_resume ever be called?
One of those cases is exactly what you have been talking about: when
the platform kills power to the USB lines during suspend. The driver's
reset_resume routine will be called during resume, as opposed to the
probe routine being called. Therefore the driver will be able to tell
that this is not a new device instance.
The other cases are less likely to occur: a device is unable to resume
normally and requires a reset before it will start working again, or
something else goes wrong along those lines.
However I get the feeling someone needs to go back and see if the
device is the same one and just gets probed again or if it is a new
one from the USB host stack perspective.
That can be done easily enough by enabling usbcore debugging before
carrying out the system suspend:
echo 'module usbcore =p' /debug/dynamic_debug/control
The debugging information in the kernel log will tell just what
happened.
Playing around in my test setup as a baseline
[ 41.991035] usb usb1-port11: not reset yet, waiting 50ms
[ 42.092902] usb 1-11: reset full-speed USB device number 4 using xhci_hcd
[ 42.143575] usb usb1-port11: not reset yet, waiting 50ms
[ 42.257822] btusb 1-11:1.0: no reset_resume for driver btusb?
[ 42.257823] btusb 1-11:1.1: no reset_resume for driver btusb?
[ 42.257825] btusb 1-11:1.0: forced unbind
[ 42.258305] kworker/dying (826) used greatest stack depth: 10680 bytes left
[ 42.331342] usb 1-9.2: reset full-speed USB device number 7 using xhci_hcd
[ 42.416631] usb 1-9.2: ep0 maxpacket = 8
[ 42.681288] usb 1-9.1: reset low-speed USB device number 5 using xhci_hcd
[ 42.968138] usb 1-9.1: ep 0x81 - rounding interval to 64 microframes, ep
desc says 80 microframes
[ 42.968157] usb 1-9.1: ep 0x82 - rounding interval to 64 microframes, ep
desc says 80 microframes
[ 43.036290] usb 1-9.4: reset high-speed USB device number 8 using xhci_hcd
[ 43.123126] hub 1-9.4:1.0: hub_reset_resume
[ 43.123581] hub 1-9.4:1.0: enabling power on all ports
[ 43.224853] PM: resume of devices complete after 2456.587 msecs
[ 43.225038] btusb 1-11:1.0: usb_probe_interface
[ 43.225040] btusb 1-11:1.0: usb_probe_interface - got id
[ 43.225802] [ cut here ]
[ 43.225807] WARNING: CPU: 7 PID: 2844 at drivers/base/firmware_class.c:1118
_request_firmware+0x5ee/0x890()
so it is trying to call the reset resume. If I try a 'dummy reset resume'
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index a7bdac0..cda8137 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3401,6 +3401,7 @@ static struct usb_driver btusb_driver = {
#ifdef CONFIG_PM
.suspend= btusb_suspend,
.resume = btusb_resume,
+ .reset_resume = btusb_resume,
#endif
.id_table = btusb_table,
.supports_autosuspend = 1,
I no longer see the warning which means that probe is no longer being called.
Marcel, does implementing a proper reset_resume callback seem like the right
approach or do you need more information?
I wonder what is the right work that needs to be done in the reset_resume
callback. I am curious if devices are forgetting their firmware or not. There
is a patch to make the Realtek devices forcefully reset_resume since they
forget their firmware. So there is at least one kind of devices where the
firmware does not survive normal suspend/resume behaviour.
If the devices forget the firmware, then this means that we actually need to tell
the Bluetooth core that this device has been reset and it has to run through
hdev-setup() again. If it does that, then we have the same problem that the
firmware will not be found since userspace is not yet ready. However we could note
the fact that we tried to lookup the firmware and just know that it was not found.
So that might help already.
Devices that always require the firmware, we can assume that the firmware will
have been cached since we successfully loaded it in the first place. Which will
most likely make the Realtek devices function just fine. It has the advantage
that we do not have to go through the disconnect() and probe() cycle which will
in turn unregister and re-register the HCI device.
The
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
On 05/22/15 09:37, Arend van Spriel wrote:
On 05/22/15 02:21, Laura Abbott wrote:
On 05/21/2015 08:26 AM, Alan Stern wrote:
On Thu, 21 May 2015, Marcel Holtmann wrote:
Hi Alan,
Then avoiding the failed firmware is no solution, indeed.
If it's a new probe, it should be never executed during resume.
Can you expand this comment? What's wrong with probing during resume?
The USB stack does carry out probes during resume under certain
circumstances. A driver lacking a reset_resume callback is one of
those circumstances.
in case the platform kills the power to the USB lines, we can never
do anything about this. I do not want to hack around this in the
driver.
What are the cases where we should implement reset_resume and would
it really help here. Since the btusb.ko driver implements
suspend/resume support, would reset_resume ever be called?
One of those cases is exactly what you have been talking about: when
the platform kills power to the USB lines during suspend. The driver's
reset_resume routine will be called during resume, as opposed to the
probe routine being called. Therefore the driver will be able to tell
that this is not a new device instance.
The other cases are less likely to occur: a device is unable to resume
normally and requires a reset before it will start working again, or
something else goes wrong along those lines.
However I get the feeling someone needs to go back and see if the
device is the same one and just gets probed again or if it is a new
one from the USB host stack perspective.
That can be done easily enough by enabling usbcore debugging before
carrying out the system suspend:
echo 'module usbcore =p' /debug/dynamic_debug/control
The debugging information in the kernel log will tell just what
happened.
Playing around in my test setup as a baseline
[ 41.991035] usb usb1-port11: not reset yet, waiting 50ms
[ 42.092902] usb 1-11: reset full-speed USB device number 4 using
xhci_hcd
[ 42.143575] usb usb1-port11: not reset yet, waiting 50ms
[ 42.257822] btusb 1-11:1.0: no reset_resume for driver btusb?
[ 42.257823] btusb 1-11:1.1: no reset_resume for driver btusb?
[ 42.257825] btusb 1-11:1.0: forced unbind
[ 42.258305] kworker/dying (826) used greatest stack depth: 10680 bytes
left
[ 42.331342] usb 1-9.2: reset full-speed USB device number 7 using
xhci_hcd
[ 42.416631] usb 1-9.2: ep0 maxpacket = 8
[ 42.681288] usb 1-9.1: reset low-speed USB device number 5 using
xhci_hcd
[ 42.968138] usb 1-9.1: ep 0x81 - rounding interval to 64 microframes,
ep desc says 80 microframes
[ 42.968157] usb 1-9.1: ep 0x82 - rounding interval to 64 microframes,
ep desc says 80 microframes
[ 43.036290] usb 1-9.4: reset high-speed USB device number 8 using
xhci_hcd
[ 43.123126] hub 1-9.4:1.0: hub_reset_resume
[ 43.123581] hub 1-9.4:1.0: enabling power on all ports
[ 43.224853] PM: resume of devices complete after 2456.587 msecs
[ 43.225038] btusb 1-11:1.0: usb_probe_interface
[ 43.225040] btusb 1-11:1.0: usb_probe_interface - got id
[ 43.225802] [ cut here ]
[ 43.225807] WARNING: CPU: 7 PID: 2844 at
drivers/base/firmware_class.c:1118 _request_firmware+0x5ee/0x890()
so it is trying to call the reset resume. If I try a 'dummy reset resume'
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index a7bdac0..cda8137 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3401,6 +3401,7 @@ static struct usb_driver btusb_driver = {
#ifdef CONFIG_PM
.suspend = btusb_suspend,
.resume = btusb_resume,
+ .reset_resume = btusb_resume,
#endif
.id_table = btusb_table,
.supports_autosuspend = 1,
I no longer see the warning which means that probe is no longer being
called.
Marcel, does implementing a proper reset_resume callback seem like the
right
approach or do you need more information?
Hi, Laura
I believe that some devices supported by btusb would need to do a
request_firmware() in the reset_resume() callback and thus end up with
the same issue. btusb could store the firmware obtained during the probe
in it driver private structure and use that in reset_resume() callback,
but it means the memory for the firmware blobs will not be released
until the driver is unloaded.
Same is true if caching is done in firmware_loader so it may not be a
big deal.
Regards,
Arend
Regards,
Arend
Thanks,
Laura
--
To unsubscribe from this list: send the line unsubscribe
linux-bluetooth in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe
linux-bluetooth in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
On 05/22/15 02:21, Laura Abbott wrote:
On 05/21/2015 08:26 AM, Alan Stern wrote:
On Thu, 21 May 2015, Marcel Holtmann wrote:
Hi Alan,
Then avoiding the failed firmware is no solution, indeed.
If it's a new probe, it should be never executed during resume.
Can you expand this comment? What's wrong with probing during resume?
The USB stack does carry out probes during resume under certain
circumstances. A driver lacking a reset_resume callback is one of
those circumstances.
in case the platform kills the power to the USB lines, we can never
do anything about this. I do not want to hack around this in the
driver.
What are the cases where we should implement reset_resume and would
it really help here. Since the btusb.ko driver implements
suspend/resume support, would reset_resume ever be called?
One of those cases is exactly what you have been talking about: when
the platform kills power to the USB lines during suspend. The driver's
reset_resume routine will be called during resume, as opposed to the
probe routine being called. Therefore the driver will be able to tell
that this is not a new device instance.
The other cases are less likely to occur: a device is unable to resume
normally and requires a reset before it will start working again, or
something else goes wrong along those lines.
However I get the feeling someone needs to go back and see if the
device is the same one and just gets probed again or if it is a new
one from the USB host stack perspective.
That can be done easily enough by enabling usbcore debugging before
carrying out the system suspend:
echo 'module usbcore =p' /debug/dynamic_debug/control
The debugging information in the kernel log will tell just what
happened.
Playing around in my test setup as a baseline
[ 41.991035] usb usb1-port11: not reset yet, waiting 50ms
[ 42.092902] usb 1-11: reset full-speed USB device number 4 using xhci_hcd
[ 42.143575] usb usb1-port11: not reset yet, waiting 50ms
[ 42.257822] btusb 1-11:1.0: no reset_resume for driver btusb?
[ 42.257823] btusb 1-11:1.1: no reset_resume for driver btusb?
[ 42.257825] btusb 1-11:1.0: forced unbind
[ 42.258305] kworker/dying (826) used greatest stack depth: 10680 bytes
left
[ 42.331342] usb 1-9.2: reset full-speed USB device number 7 using xhci_hcd
[ 42.416631] usb 1-9.2: ep0 maxpacket = 8
[ 42.681288] usb 1-9.1: reset low-speed USB device number 5 using xhci_hcd
[ 42.968138] usb 1-9.1: ep 0x81 - rounding interval to 64 microframes,
ep desc says 80 microframes
[ 42.968157] usb 1-9.1: ep 0x82 - rounding interval to 64 microframes,
ep desc says 80 microframes
[ 43.036290] usb 1-9.4: reset high-speed USB device number 8 using xhci_hcd
[ 43.123126] hub 1-9.4:1.0: hub_reset_resume
[ 43.123581] hub 1-9.4:1.0: enabling power on all ports
[ 43.224853] PM: resume of devices complete after 2456.587 msecs
[ 43.225038] btusb 1-11:1.0: usb_probe_interface
[ 43.225040] btusb 1-11:1.0: usb_probe_interface - got id
[ 43.225802] [ cut here ]
[ 43.225807] WARNING: CPU: 7 PID: 2844 at
drivers/base/firmware_class.c:1118 _request_firmware+0x5ee/0x890()
so it is trying to call the reset resume. If I try a 'dummy reset resume'
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index a7bdac0..cda8137 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3401,6 +3401,7 @@ static struct usb_driver btusb_driver = {
#ifdef CONFIG_PM
.suspend = btusb_suspend,
.resume = btusb_resume,
+ .reset_resume = btusb_resume,
#endif
.id_table = btusb_table,
.supports_autosuspend = 1,
I no longer see the warning which means that probe is no longer being
called.
Marcel, does implementing a proper reset_resume callback seem like the
right
approach or do you need more information?
Hi, Laura
I believe that some devices supported by btusb would need to do a
request_firmware() in the reset_resume() callback and thus end up with
the same issue. btusb could store the firmware obtained during the probe
in it driver private structure and use that in reset_resume() callback,
but it means the memory for the firmware blobs will not be released
until the driver is unloaded.
Regards,
Arend
Thanks,
Laura
--
To unsubscribe from this list: send the line unsubscribe
linux-bluetooth in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
On Thu, 2015-05-21 at 22:46 +0200, Arend van Spriel wrote: On 05/21/15 19:32, Takashi Iwai wrote: Well, if the probe requires the access to a user-space file, it can't be done during resume. That's the very problem we're seeing now. The firmware loader can't help much alone if it's a new device object. So you are saying each device driver should come up with some retry mechanism. Would make more sense to come up with something like that behind the scenes in the firmware loader so all device drivers can rely on one and the same solution. There is already a notifier for this. I don't see why the firmware layer couldn't retrigger a match for all unbound devices, just like we do when a new driver is loaded. Regards Oliver -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
At Thu, 21 May 2015 13:37:56 -0400 (EDT), Alan Stern wrote: On Thu, 21 May 2015, Takashi Iwai wrote: At Thu, 21 May 2015 11:26:17 -0400 (EDT), Alan Stern wrote: On Thu, 21 May 2015, Takashi Iwai wrote: At Thu, 21 May 2015 10:18:08 -0400 (EDT), Alan Stern wrote: On Thu, 21 May 2015, Takashi Iwai wrote: Then avoiding the failed firmware is no solution, indeed. If it's a new probe, it should be never executed during resume. Can you expand this comment? What's wrong with probing during resume? Well, if the probe requires the access to a user-space file, it can't be done during resume. That's the very problem we're seeing now. The firmware loader can't help much alone if it's a new device object. But the same thing happens during early boot, if the driver is built into the kernel. When the probe occurs, userspace isn't up and running yet, so the firmware loader can't do anything. Why should probe during resume be any worse than probe during early boot? The early boot has initrd, so the files can be there. But the resume has no way to fetch the file except for cached data. I suppose USB could delay re-probing until userspace is running again, if we knew when that was. But it would be awkward and prone to races. It also would leave a user-visible window of time during which the device does not exist, which we want to avoid. (This may not matter for bluetooth, but it does matter for other kinds of devices.) Right. I would prefer to solve this problem in a different way, if possible. Well, we're back in square again :) But, before going further the discussion in loop again, I'd like to know which firmware file actually hits. Is it a non-existing firmware? Or is it a firmware that should have been cached? In the latter case, why it isn't used? Takashi -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
On 05/21/2015 11:11 AM, Takashi Iwai wrote: At Thu, 21 May 2015 13:37:56 -0400 (EDT), Alan Stern wrote: On Thu, 21 May 2015, Takashi Iwai wrote: At Thu, 21 May 2015 11:26:17 -0400 (EDT), Alan Stern wrote: On Thu, 21 May 2015, Takashi Iwai wrote: At Thu, 21 May 2015 10:18:08 -0400 (EDT), Alan Stern wrote: On Thu, 21 May 2015, Takashi Iwai wrote: Then avoiding the failed firmware is no solution, indeed. If it's a new probe, it should be never executed during resume. Can you expand this comment? What's wrong with probing during resume? Well, if the probe requires the access to a user-space file, it can't be done during resume. That's the very problem we're seeing now. The firmware loader can't help much alone if it's a new device object. But the same thing happens during early boot, if the driver is built into the kernel. When the probe occurs, userspace isn't up and running yet, so the firmware loader can't do anything. Why should probe during resume be any worse than probe during early boot? The early boot has initrd, so the files can be there. But the resume has no way to fetch the file except for cached data. I suppose USB could delay re-probing until userspace is running again, if we knew when that was. But it would be awkward and prone to races. It also would leave a user-visible window of time during which the device does not exist, which we want to avoid. (This may not matter for bluetooth, but it does matter for other kinds of devices.) Right. I would prefer to solve this problem in a different way, if possible. Well, we're back in square again :) But, before going further the discussion in loop again, I'd like to know which firmware file actually hits. Is it a non-existing firmware? Or is it a firmware that should have been cached? In the latter case, why it isn't used? Non-existent firmware. The firmware was never present in the system and was never loaded at all. Takashi Thanks, Laura -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
At Thu, 21 May 2015 14:07:11 +0200,
Marcel Holtmann wrote:
Hi Takashi,
The data is cached in RAM. More specifically, the former loaded
firmware files are reloaded and saved at suspend for each device
object. See fw_pm_notify() in firmware_class.c.
OK, this may be a stupid idea, but do we know the firmware
was successfully loaded in the first place?
Also btusb is in the habit of falling back to a generic
firmware in some places. It seems to me that caching
firmware is conceptually not enough, but we'd also need
to record the absence of firmware images.
in a lot of cases the firmware is optional. The device will operate fine
without the firmware. There are a few devices where the firmware is
required, but for many it just contains patches.
It would be nice if we could tell request_firmware() if it is optional
or mandatory firmware. Or if it should just cache the status of a
missing firmware as well.
OK, below is a quick hack to record the failed f/w files, too.
Not sure whether this helps, though. Proper tests are appreciated.
This doesn't quite work. We end up with the name on fw_names but
the firmware isn't actually on the firmware cache list.
If request_firmware fails to get the firmware from the filesystem,
release firmware will be called which is going to free the
firmware_buf which has been marked as failed anyway. The only
way to make this work would be to always piggy back and increase
the ref so it always stays around. But this also marks the firmware
as a permanent failure. There would need to be a hook somewhere
to force a cache drop, else there would be no way to add new
firmware to a running system without a reboot.
Perhaps we split the difference: keep a list of firmware images
that failed to load in the past and if one is requested during
a time when usermodehelper isn't available, silently return an
error? This way, if correct firmware is loaded at a regular time
the item can be removed from the list.
Well, IMO, it's way too much expectation for the generic f/w loader.
The driver itself must know already which should be really loaded.
The fact is that it's the driver who calls the function that might not
work in the resume path. So the driver can deal with such exceptions
at best.
I keep repeating myself here. From the driver point of view it goes
via probe() callback of the USB driver. So the driver does not
know. For the driver it looks like a brand new device. There are
platforms that might decide to just kill the power to the USB bus
where the Bluetooth controller sits on. It gets the power back on
resume. However this means it is a brand new device at that
point. So the driver should not have to remember everything.
Then avoiding the failed firmware is no solution, indeed.
If it's a new probe, it should be never executed during resume.
That is, either freeze the work like Laura's patch or explicitly allow
the UMH lock wait like my patch. Laura's patch has a merit that it's
much simpler. OTOH, if you want to keep the changes only in
request_firmware() call, you can think of changes like my patch; a
revised version is attached below.
Takashi
---
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 171841ad1008..87157f557263 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -97,21 +97,6 @@ static inline long firmware_loading_timeout(void)
return loading_timeout 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
}
-/* firmware behavior options */
-#define FW_OPT_UEVENT (1U 0)
-#define FW_OPT_NOWAIT (1U 1)
-#ifdef CONFIG_FW_LOADER_USER_HELPER
-#define FW_OPT_USERHELPER (1U 2)
-#else
-#define FW_OPT_USERHELPER 0
-#endif
-#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
-#define FW_OPT_FALLBACKFW_OPT_USERHELPER
-#else
-#define FW_OPT_FALLBACK0
-#endif
-#define FW_OPT_NO_WARN (1U 3)
-
struct firmware_cache {
/* firmware_buf instance will be added into the below list */
spinlock_t lock;
@@ -1085,8 +1070,7 @@ static int assign_firmware_buf(struct firmware *fw,
struct device *device,
}
/* called from request_firmware() and request_firmware_work_func() */
-static int
-_request_firmware(const struct firmware **firmware_p, const char *name,
+int _request_firmware(const struct firmware **firmware_p, const char *name,
struct device *device, unsigned int opt_flags)
{
struct firmware *fw;
@@ -1099,13 +1083,15 @@ _request_firmware(const struct firmware **firmware_p,
const char *name,
if (!name || name[0] == '\0')
return -EINVAL;
+ /* Need to pin this module until return */
+ __module_get(THIS_MODULE);
ret = _request_firmware_prepare(fw, name, device);
if (ret = 0) /* error or already assigned */
goto out;
ret = 0;
timeout =
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
Hi Takashi, The data is cached in RAM. More specifically, the former loaded firmware files are reloaded and saved at suspend for each device object. See fw_pm_notify() in firmware_class.c. OK, this may be a stupid idea, but do we know the firmware was successfully loaded in the first place? Also btusb is in the habit of falling back to a generic firmware in some places. It seems to me that caching firmware is conceptually not enough, but we'd also need to record the absence of firmware images. in a lot of cases the firmware is optional. The device will operate fine without the firmware. There are a few devices where the firmware is required, but for many it just contains patches. It would be nice if we could tell request_firmware() if it is optional or mandatory firmware. Or if it should just cache the status of a missing firmware as well. OK, below is a quick hack to record the failed f/w files, too. Not sure whether this helps, though. Proper tests are appreciated. This doesn't quite work. We end up with the name on fw_names but the firmware isn't actually on the firmware cache list. If request_firmware fails to get the firmware from the filesystem, release firmware will be called which is going to free the firmware_buf which has been marked as failed anyway. The only way to make this work would be to always piggy back and increase the ref so it always stays around. But this also marks the firmware as a permanent failure. There would need to be a hook somewhere to force a cache drop, else there would be no way to add new firmware to a running system without a reboot. Perhaps we split the difference: keep a list of firmware images that failed to load in the past and if one is requested during a time when usermodehelper isn't available, silently return an error? This way, if correct firmware is loaded at a regular time the item can be removed from the list. Well, IMO, it's way too much expectation for the generic f/w loader. The driver itself must know already which should be really loaded. The fact is that it's the driver who calls the function that might not work in the resume path. So the driver can deal with such exceptions at best. I keep repeating myself here. From the driver point of view it goes via probe() callback of the USB driver. So the driver does not know. For the driver it looks like a brand new device. There are platforms that might decide to just kill the power to the USB bus where the Bluetooth controller sits on. It gets the power back on resume. However this means it is a brand new device at that point. So the driver should not have to remember everything. Regards Marcel -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
On 05/21/15 19:32, Takashi Iwai wrote: At Thu, 21 May 2015 19:27:41 +0200, Arend van Spriel wrote: On 05/21/15 17:35, Takashi Iwai wrote: At Thu, 21 May 2015 11:26:17 -0400 (EDT), Alan Stern wrote: On Thu, 21 May 2015, Takashi Iwai wrote: At Thu, 21 May 2015 10:18:08 -0400 (EDT), Alan Stern wrote: On Thu, 21 May 2015, Takashi Iwai wrote: Then avoiding the failed firmware is no solution, indeed. If it's a new probe, it should be never executed during resume. Can you expand this comment? What's wrong with probing during resume? Well, if the probe requires the access to a user-space file, it can't be done during resume. That's the very problem we're seeing now. The firmware loader can't help much alone if it's a new device object. So you are saying each device driver should come up with some retry mechanism. Would make more sense to come up with something like that behind the scenes in the firmware loader so all device drivers can rely on one and the same solution. Regards, Arend But the same thing happens during early boot, if the driver is built into the kernel. When the probe occurs, userspace isn't up and running yet, so the firmware loader can't do anything. Why should probe during resume be any worse than probe during early boot? The early boot has initrd, so the files can be there. But the resume has no way to fetch the file except for cached data. but initrd is optional so without initrd it is pretty much the same. User can build the firmware into the kernel. Takashi -- To unsubscribe from this list: send the line unsubscribe linux-bluetooth in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
Hi Alan, Then avoiding the failed firmware is no solution, indeed. If it's a new probe, it should be never executed during resume. Can you expand this comment? What's wrong with probing during resume? The USB stack does carry out probes during resume under certain circumstances. A driver lacking a reset_resume callback is one of those circumstances. in case the platform kills the power to the USB lines, we can never do anything about this. I do not want to hack around this in the driver. What are the cases where we should implement reset_resume and would it really help here. Since the btusb.ko driver implements suspend/resume support, would reset_resume ever be called? However I get the feeling someone needs to go back and see if the device is the same one and just gets probed again or if it is a new one from the USB host stack perspective. Regards Marcel -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
At Thu, 21 May 2015 10:18:08 -0400 (EDT), Alan Stern wrote: On Thu, 21 May 2015, Takashi Iwai wrote: Then avoiding the failed firmware is no solution, indeed. If it's a new probe, it should be never executed during resume. Can you expand this comment? What's wrong with probing during resume? Well, if the probe requires the access to a user-space file, it can't be done during resume. That's the very problem we're seeing now. The firmware loader can't help much alone if it's a new device object. The USB stack does carry out probes during resume under certain circumstances. A driver lacking a reset_resume callback is one of those circumstances. So, having a proper reset_resume in btusb would help in the end? Takashi -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
On Thu, 21 May 2015, Marcel Holtmann wrote: Hi Alan, Then avoiding the failed firmware is no solution, indeed. If it's a new probe, it should be never executed during resume. Can you expand this comment? What's wrong with probing during resume? The USB stack does carry out probes during resume under certain circumstances. A driver lacking a reset_resume callback is one of those circumstances. in case the platform kills the power to the USB lines, we can never do anything about this. I do not want to hack around this in the driver. What are the cases where we should implement reset_resume and would it really help here. Since the btusb.ko driver implements suspend/resume support, would reset_resume ever be called? One of those cases is exactly what you have been talking about: when the platform kills power to the USB lines during suspend. The driver's reset_resume routine will be called during resume, as opposed to the probe routine being called. Therefore the driver will be able to tell that this is not a new device instance. The other cases are less likely to occur: a device is unable to resume normally and requires a reset before it will start working again, or something else goes wrong along those lines. However I get the feeling someone needs to go back and see if the device is the same one and just gets probed again or if it is a new one from the USB host stack perspective. That can be done easily enough by enabling usbcore debugging before carrying out the system suspend: echo 'module usbcore =p' /debug/dynamic_debug/control The debugging information in the kernel log will tell just what happened. On Thu, 21 May 2015, Takashi Iwai wrote: At Thu, 21 May 2015 10:18:08 -0400 (EDT), Alan Stern wrote: On Thu, 21 May 2015, Takashi Iwai wrote: Then avoiding the failed firmware is no solution, indeed. If it's a new probe, it should be never executed during resume. Can you expand this comment? What's wrong with probing during resume? Well, if the probe requires the access to a user-space file, it can't be done during resume. That's the very problem we're seeing now. The firmware loader can't help much alone if it's a new device object. But the same thing happens during early boot, if the driver is built into the kernel. When the probe occurs, userspace isn't up and running yet, so the firmware loader can't do anything. Why should probe during resume be any worse than probe during early boot? The USB stack does carry out probes during resume under certain circumstances. A driver lacking a reset_resume callback is one of those circumstances. So, having a proper reset_resume in btusb would help in the end? It might, depending on how the driver is written. I don't know enough about the details of btusb to say. Alan Stern -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
On Thu, 21 May 2015, Takashi Iwai wrote: Then avoiding the failed firmware is no solution, indeed. If it's a new probe, it should be never executed during resume. Can you expand this comment? What's wrong with probing during resume? The USB stack does carry out probes during resume under certain circumstances. A driver lacking a reset_resume callback is one of those circumstances. Alan Stern -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
On 05/21/2015 08:26 AM, Alan Stern wrote:
On Thu, 21 May 2015, Marcel Holtmann wrote:
Hi Alan,
Then avoiding the failed firmware is no solution, indeed.
If it's a new probe, it should be never executed during resume.
Can you expand this comment? What's wrong with probing during resume?
The USB stack does carry out probes during resume under certain
circumstances. A driver lacking a reset_resume callback is one of
those circumstances.
in case the platform kills the power to the USB lines, we can never
do anything about this. I do not want to hack around this in the
driver.
What are the cases where we should implement reset_resume and would
it really help here. Since the btusb.ko driver implements
suspend/resume support, would reset_resume ever be called?
One of those cases is exactly what you have been talking about: when
the platform kills power to the USB lines during suspend. The driver's
reset_resume routine will be called during resume, as opposed to the
probe routine being called. Therefore the driver will be able to tell
that this is not a new device instance.
The other cases are less likely to occur: a device is unable to resume
normally and requires a reset before it will start working again, or
something else goes wrong along those lines.
However I get the feeling someone needs to go back and see if the
device is the same one and just gets probed again or if it is a new
one from the USB host stack perspective.
That can be done easily enough by enabling usbcore debugging before
carrying out the system suspend:
echo 'module usbcore =p' /debug/dynamic_debug/control
The debugging information in the kernel log will tell just what
happened.
Playing around in my test setup as a baseline
[ 41.991035] usb usb1-port11: not reset yet, waiting 50ms
[ 42.092902] usb 1-11: reset full-speed USB device number 4 using xhci_hcd
[ 42.143575] usb usb1-port11: not reset yet, waiting 50ms
[ 42.257822] btusb 1-11:1.0: no reset_resume for driver btusb?
[ 42.257823] btusb 1-11:1.1: no reset_resume for driver btusb?
[ 42.257825] btusb 1-11:1.0: forced unbind
[ 42.258305] kworker/dying (826) used greatest stack depth: 10680 bytes left
[ 42.331342] usb 1-9.2: reset full-speed USB device number 7 using xhci_hcd
[ 42.416631] usb 1-9.2: ep0 maxpacket = 8
[ 42.681288] usb 1-9.1: reset low-speed USB device number 5 using xhci_hcd
[ 42.968138] usb 1-9.1: ep 0x81 - rounding interval to 64 microframes, ep
desc says 80 microframes
[ 42.968157] usb 1-9.1: ep 0x82 - rounding interval to 64 microframes, ep
desc says 80 microframes
[ 43.036290] usb 1-9.4: reset high-speed USB device number 8 using xhci_hcd
[ 43.123126] hub 1-9.4:1.0: hub_reset_resume
[ 43.123581] hub 1-9.4:1.0: enabling power on all ports
[ 43.224853] PM: resume of devices complete after 2456.587 msecs
[ 43.225038] btusb 1-11:1.0: usb_probe_interface
[ 43.225040] btusb 1-11:1.0: usb_probe_interface - got id
[ 43.225802] [ cut here ]
[ 43.225807] WARNING: CPU: 7 PID: 2844 at drivers/base/firmware_class.c:1118
_request_firmware+0x5ee/0x890()
so it is trying to call the reset resume. If I try a 'dummy reset resume'
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index a7bdac0..cda8137 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3401,6 +3401,7 @@ static struct usb_driver btusb_driver = {
#ifdef CONFIG_PM
.suspend= btusb_suspend,
.resume = btusb_resume,
+ .reset_resume = btusb_resume,
#endif
.id_table = btusb_table,
.supports_autosuspend = 1,
I no longer see the warning which means that probe is no longer being called.
Marcel, does implementing a proper reset_resume callback seem like the right
approach or do you need more information?
Thanks,
Laura
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
Hi Laura,
Then avoiding the failed firmware is no solution, indeed.
If it's a new probe, it should be never executed during resume.
Can you expand this comment? What's wrong with probing during resume?
The USB stack does carry out probes during resume under certain
circumstances. A driver lacking a reset_resume callback is one of
those circumstances.
in case the platform kills the power to the USB lines, we can never
do anything about this. I do not want to hack around this in the
driver.
What are the cases where we should implement reset_resume and would
it really help here. Since the btusb.ko driver implements
suspend/resume support, would reset_resume ever be called?
One of those cases is exactly what you have been talking about: when
the platform kills power to the USB lines during suspend. The driver's
reset_resume routine will be called during resume, as opposed to the
probe routine being called. Therefore the driver will be able to tell
that this is not a new device instance.
The other cases are less likely to occur: a device is unable to resume
normally and requires a reset before it will start working again, or
something else goes wrong along those lines.
However I get the feeling someone needs to go back and see if the
device is the same one and just gets probed again or if it is a new
one from the USB host stack perspective.
That can be done easily enough by enabling usbcore debugging before
carrying out the system suspend:
echo 'module usbcore =p' /debug/dynamic_debug/control
The debugging information in the kernel log will tell just what
happened.
Playing around in my test setup as a baseline
[ 41.991035] usb usb1-port11: not reset yet, waiting 50ms
[ 42.092902] usb 1-11: reset full-speed USB device number 4 using xhci_hcd
[ 42.143575] usb usb1-port11: not reset yet, waiting 50ms
[ 42.257822] btusb 1-11:1.0: no reset_resume for driver btusb?
[ 42.257823] btusb 1-11:1.1: no reset_resume for driver btusb?
[ 42.257825] btusb 1-11:1.0: forced unbind
[ 42.258305] kworker/dying (826) used greatest stack depth: 10680 bytes left
[ 42.331342] usb 1-9.2: reset full-speed USB device number 7 using xhci_hcd
[ 42.416631] usb 1-9.2: ep0 maxpacket = 8
[ 42.681288] usb 1-9.1: reset low-speed USB device number 5 using xhci_hcd
[ 42.968138] usb 1-9.1: ep 0x81 - rounding interval to 64 microframes, ep
desc says 80 microframes
[ 42.968157] usb 1-9.1: ep 0x82 - rounding interval to 64 microframes, ep
desc says 80 microframes
[ 43.036290] usb 1-9.4: reset high-speed USB device number 8 using xhci_hcd
[ 43.123126] hub 1-9.4:1.0: hub_reset_resume
[ 43.123581] hub 1-9.4:1.0: enabling power on all ports
[ 43.224853] PM: resume of devices complete after 2456.587 msecs
[ 43.225038] btusb 1-11:1.0: usb_probe_interface
[ 43.225040] btusb 1-11:1.0: usb_probe_interface - got id
[ 43.225802] [ cut here ]
[ 43.225807] WARNING: CPU: 7 PID: 2844 at
drivers/base/firmware_class.c:1118 _request_firmware+0x5ee/0x890()
so it is trying to call the reset resume. If I try a 'dummy reset resume'
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index a7bdac0..cda8137 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3401,6 +3401,7 @@ static struct usb_driver btusb_driver = {
#ifdef CONFIG_PM
.suspend= btusb_suspend,
.resume = btusb_resume,
+ .reset_resume = btusb_resume,
#endif
.id_table = btusb_table,
.supports_autosuspend = 1,
I no longer see the warning which means that probe is no longer being called.
Marcel, does implementing a proper reset_resume callback seem like the right
approach or do you need more information?
I wonder what is the right work that needs to be done in the reset_resume
callback. I am curious if devices are forgetting their firmware or not. There
is a patch to make the Realtek devices forcefully reset_resume since they
forget their firmware. So there is at least one kind of devices where the
firmware does not survive normal suspend/resume behaviour.
If the devices forget the firmware, then this means that we actually need to
tell the Bluetooth core that this device has been reset and it has to run
through hdev-setup() again. If it does that, then we have the same problem
that the firmware will not be found since userspace is not yet ready. However
we could note the fact that we tried to lookup the firmware and just know that
it was not found. So that might help already.
Devices that always require the firmware, we can assume that the firmware will
have been cached since we successfully loaded it in the first place. Which will
most likely make the Realtek devices function just fine. It has the advantage
that we do not have to go through the disconnect() and probe() cycle which will
in turn unregister and re-register
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
At Thu, 21 May 2015 11:26:17 -0400 (EDT), Alan Stern wrote: On Thu, 21 May 2015, Takashi Iwai wrote: At Thu, 21 May 2015 10:18:08 -0400 (EDT), Alan Stern wrote: On Thu, 21 May 2015, Takashi Iwai wrote: Then avoiding the failed firmware is no solution, indeed. If it's a new probe, it should be never executed during resume. Can you expand this comment? What's wrong with probing during resume? Well, if the probe requires the access to a user-space file, it can't be done during resume. That's the very problem we're seeing now. The firmware loader can't help much alone if it's a new device object. But the same thing happens during early boot, if the driver is built into the kernel. When the probe occurs, userspace isn't up and running yet, so the firmware loader can't do anything. Why should probe during resume be any worse than probe during early boot? The early boot has initrd, so the files can be there. But the resume has no way to fetch the file except for cached data. Takashi -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
On 05/21/15 17:35, Takashi Iwai wrote: At Thu, 21 May 2015 11:26:17 -0400 (EDT), Alan Stern wrote: On Thu, 21 May 2015, Takashi Iwai wrote: At Thu, 21 May 2015 10:18:08 -0400 (EDT), Alan Stern wrote: On Thu, 21 May 2015, Takashi Iwai wrote: Then avoiding the failed firmware is no solution, indeed. If it's a new probe, it should be never executed during resume. Can you expand this comment? What's wrong with probing during resume? Well, if the probe requires the access to a user-space file, it can't be done during resume. That's the very problem we're seeing now. The firmware loader can't help much alone if it's a new device object. But the same thing happens during early boot, if the driver is built into the kernel. When the probe occurs, userspace isn't up and running yet, so the firmware loader can't do anything. Why should probe during resume be any worse than probe during early boot? The early boot has initrd, so the files can be there. But the resume has no way to fetch the file except for cached data. but initrd is optional so without initrd it is pretty much the same. Regards, Arend Takashi -- To unsubscribe from this list: send the line unsubscribe linux-bluetooth in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
At Thu, 21 May 2015 19:27:41 +0200, Arend van Spriel wrote: On 05/21/15 17:35, Takashi Iwai wrote: At Thu, 21 May 2015 11:26:17 -0400 (EDT), Alan Stern wrote: On Thu, 21 May 2015, Takashi Iwai wrote: At Thu, 21 May 2015 10:18:08 -0400 (EDT), Alan Stern wrote: On Thu, 21 May 2015, Takashi Iwai wrote: Then avoiding the failed firmware is no solution, indeed. If it's a new probe, it should be never executed during resume. Can you expand this comment? What's wrong with probing during resume? Well, if the probe requires the access to a user-space file, it can't be done during resume. That's the very problem we're seeing now. The firmware loader can't help much alone if it's a new device object. But the same thing happens during early boot, if the driver is built into the kernel. When the probe occurs, userspace isn't up and running yet, so the firmware loader can't do anything. Why should probe during resume be any worse than probe during early boot? The early boot has initrd, so the files can be there. But the resume has no way to fetch the file except for cached data. but initrd is optional so without initrd it is pretty much the same. User can build the firmware into the kernel. Takashi -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
On Thu, 21 May 2015, Takashi Iwai wrote: At Thu, 21 May 2015 11:26:17 -0400 (EDT), Alan Stern wrote: On Thu, 21 May 2015, Takashi Iwai wrote: At Thu, 21 May 2015 10:18:08 -0400 (EDT), Alan Stern wrote: On Thu, 21 May 2015, Takashi Iwai wrote: Then avoiding the failed firmware is no solution, indeed. If it's a new probe, it should be never executed during resume. Can you expand this comment? What's wrong with probing during resume? Well, if the probe requires the access to a user-space file, it can't be done during resume. That's the very problem we're seeing now. The firmware loader can't help much alone if it's a new device object. But the same thing happens during early boot, if the driver is built into the kernel. When the probe occurs, userspace isn't up and running yet, so the firmware loader can't do anything. Why should probe during resume be any worse than probe during early boot? The early boot has initrd, so the files can be there. But the resume has no way to fetch the file except for cached data. I suppose USB could delay re-probing until userspace is running again, if we knew when that was. But it would be awkward and prone to races. It also would leave a user-visible window of time during which the device does not exist, which we want to avoid. (This may not matter for bluetooth, but it does matter for other kinds of devices.) I would prefer to solve this problem in a different way, if possible. Alan Stern -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
On Wed, May 20, 2015 at 4:40 PM, Oliver Neukum oneu...@suse.com wrote: On Wed, 2015-05-20 at 08:29 +0200, Takashi Iwai wrote: The data is cached in RAM. More specifically, the former loaded firmware files are reloaded and saved at suspend for each device object. See fw_pm_notify() in firmware_class.c. OK, this may be a stupid idea, but do we know the firmware was successfully loaded in the first place? Yes, the firmware loader records that as device resource. In reality, there won't be lots of devices requiring firmware in one running system, so the idea of caching for every successful loading is workable. Also btusb is in the habit of falling back to a generic firmware in some places. It seems to me that caching firmware is conceptually not enough, but we'd also need to record the absence of firmware images. The caching can't cover the case which starts to load fw during resume in the 1st time. Regards Oliver -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
At Wed, 20 May 2015 11:46:31 +0200,
Marcel Holtmann wrote:
Hi Oliver,
The data is cached in RAM. More specifically, the former loaded
firmware files are reloaded and saved at suspend for each device
object. See fw_pm_notify() in firmware_class.c.
OK, this may be a stupid idea, but do we know the firmware
was successfully loaded in the first place?
Also btusb is in the habit of falling back to a generic
firmware in some places. It seems to me that caching
firmware is conceptually not enough, but we'd also need
to record the absence of firmware images.
in a lot of cases the firmware is optional. The device will operate fine
without the firmware. There are a few devices where the firmware is required,
but for many it just contains patches.
It would be nice if we could tell request_firmware() if it is optional or
mandatory firmware. Or if it should just cache the status of a missing
firmware as well.
OK, below is a quick hack to record the failed f/w files, too.
Not sure whether this helps, though. Proper tests are appreciated.
Takashi
---
From: Takashi Iwai ti...@suse.de
Subject: [PATCH] firmware: cache failed firmwares, too
Signed-off-by: Takashi Iwai ti...@suse.de
---
drivers/base/firmware_class.c | 33 -
1 file changed, 12 insertions(+), 21 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 171841ad1008..a15af7289c94 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1035,6 +1035,8 @@ _request_firmware_prepare(struct firmware **firmware_p,
const char *name,
firmware-priv = buf;
if (ret 0) {
+ if (buf-size == -1UL)
+ return -ENOENT; /* already recorded as failure */
ret = sync_cached_firmware_buf(buf);
if (!ret) {
fw_set_page_data(buf, firmware);
@@ -1047,17 +1049,12 @@ _request_firmware_prepare(struct firmware **firmware_p,
const char *name,
return 1; /* need to load */
}
-static int assign_firmware_buf(struct firmware *fw, struct device *device,
+static void assign_firmware_buf(struct firmware *fw, struct device *device,
unsigned int opt_flags)
{
struct firmware_buf *buf = fw-priv;
mutex_lock(fw_lock);
- if (!buf-size || is_fw_load_aborted(buf)) {
- mutex_unlock(fw_lock);
- return -ENOENT;
- }
-
/*
* add firmware name into devres list so that we can auto cache
* and uncache firmware for device.
@@ -1079,9 +1076,9 @@ static int assign_firmware_buf(struct firmware *fw,
struct device *device,
}
/* pass the pages buffer to driver at the last minute */
- fw_set_page_data(buf, fw);
+ if (buf-size != -1UL)
+ fw_set_page_data(buf, fw);
mutex_unlock(fw_lock);
- return 0;
}
/* called from request_firmware() and request_firmware_work_func() */
@@ -1124,6 +1121,9 @@ _request_firmware(const struct firmware **firmware_p,
const char *name,
ret = fw_get_filesystem_firmware(device, fw-priv);
if (ret) {
+ struct firmware_buf *buf = fw-priv;
+
+ buf-size = -1UL; /* failed */
if (!(opt_flags FW_OPT_NO_WARN))
dev_warn(device,
Direct firmware load for %s failed with error
%d\n,
@@ -1132,12 +1132,12 @@ _request_firmware(const struct firmware **firmware_p,
const char *name,
dev_warn(device, Falling back to user helper\n);
ret = fw_load_from_user_helper(fw, name, device,
opt_flags, timeout);
+ if (ret)
+ buf-size = -1UL; /* failed */
}
}
- if (!ret)
- ret = assign_firmware_buf(fw, device, opt_flags);
-
+ assign_firmware_buf(fw, device, opt_flags);
usermodehelper_read_unlock();
out:
@@ -1435,17 +1435,8 @@ static void __async_dev_cache_fw_image(void *fw_entry,
async_cookie_t cookie)
{
struct fw_cache_entry *fce = fw_entry;
- struct firmware_cache *fwc = fw_cache;
- int ret;
-
- ret = cache_firmware(fce-name);
- if (ret) {
- spin_lock(fwc-name_lock);
- list_del(fce-list);
- spin_unlock(fwc-name_lock);
- free_fw_cache_entry(fce);
- }
+ cache_firmware(fce-name);
}
/* called with dev-devres_lock held */
--
2.4.1
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
On 05/20/2015 05:44 AM, Takashi Iwai wrote: At Wed, 20 May 2015 11:46:31 +0200, Marcel Holtmann wrote: Hi Oliver, The data is cached in RAM. More specifically, the former loaded firmware files are reloaded and saved at suspend for each device object. See fw_pm_notify() in firmware_class.c. OK, this may be a stupid idea, but do we know the firmware was successfully loaded in the first place? Also btusb is in the habit of falling back to a generic firmware in some places. It seems to me that caching firmware is conceptually not enough, but we'd also need to record the absence of firmware images. in a lot of cases the firmware is optional. The device will operate fine without the firmware. There are a few devices where the firmware is required, but for many it just contains patches. It would be nice if we could tell request_firmware() if it is optional or mandatory firmware. Or if it should just cache the status of a missing firmware as well. OK, below is a quick hack to record the failed f/w files, too. Not sure whether this helps, though. Proper tests are appreciated. This doesn't quite work. We end up with the name on fw_names but the firmware isn't actually on the firmware cache list. If request_firmware fails to get the firmware from the filesystem, release firmware will be called which is going to free the firmware_buf which has been marked as failed anyway. The only way to make this work would be to always piggy back and increase the ref so it always stays around. But this also marks the firmware as a permanent failure. There would need to be a hook somewhere to force a cache drop, else there would be no way to add new firmware to a running system without a reboot. Perhaps we split the difference: keep a list of firmware images that failed to load in the past and if one is requested during a time when usermodehelper isn't available, silently return an error? This way, if correct firmware is loaded at a regular time the item can be removed from the list. Thanks, Laura -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable
At Wed, 20 May 2015 16:42:44 -0700, Laura Abbott wrote: On 05/20/2015 05:44 AM, Takashi Iwai wrote: At Wed, 20 May 2015 11:46:31 +0200, Marcel Holtmann wrote: Hi Oliver, The data is cached in RAM. More specifically, the former loaded firmware files are reloaded and saved at suspend for each device object. See fw_pm_notify() in firmware_class.c. OK, this may be a stupid idea, but do we know the firmware was successfully loaded in the first place? Also btusb is in the habit of falling back to a generic firmware in some places. It seems to me that caching firmware is conceptually not enough, but we'd also need to record the absence of firmware images. in a lot of cases the firmware is optional. The device will operate fine without the firmware. There are a few devices where the firmware is required, but for many it just contains patches. It would be nice if we could tell request_firmware() if it is optional or mandatory firmware. Or if it should just cache the status of a missing firmware as well. OK, below is a quick hack to record the failed f/w files, too. Not sure whether this helps, though. Proper tests are appreciated. This doesn't quite work. We end up with the name on fw_names but the firmware isn't actually on the firmware cache list. If request_firmware fails to get the firmware from the filesystem, release firmware will be called which is going to free the firmware_buf which has been marked as failed anyway. The only way to make this work would be to always piggy back and increase the ref so it always stays around. But this also marks the firmware as a permanent failure. There would need to be a hook somewhere to force a cache drop, else there would be no way to add new firmware to a running system without a reboot. Perhaps we split the difference: keep a list of firmware images that failed to load in the past and if one is requested during a time when usermodehelper isn't available, silently return an error? This way, if correct firmware is loaded at a regular time the item can be removed from the list. Well, IMO, it's way too much expectation for the generic f/w loader. The driver itself must know already which should be really loaded. The fact is that it's the driver who calls the function that might not work in the resume path. So the driver can deal with such exceptions at best. This can be either delaying the f/w loading via proper UMH lock (like my former patch or your patch) or avoiding the f/w request of non-existing files that the driver already knows of. Takashi -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
