| Message ID | 1403649583-12707-3-git-send-email-mcgrof@do-not-panic.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
[[ Hopefully this makes it through to the kernel.org lists -- I’m using the Mac OS/X Mailer and it’s not clear how to force it not to use HTML format. -- Casey ]] So does request_firmware_direct() only fail if the requested file is not present on the file system or does it fail in other cases as well? If it’s the former, then the change to cxgb4 is fine. But if it’s the latter, then it’s definitely not okay. While the driver _can_ continue running without the local on-disk Firmware Configuration File, that file can be used to significantly change the behavior and capabilities of the adapter and is user-customizable. If a user makes changes to the local on-disk Firmware Configuration File and these are randomly silently ignored this will lead to highly annoying support issues. Casey On Jun 24, 2014, at 3:39 PM, Luis R. Rodriguez <mcgrof@do-not-panic.com> wrote: > From: "Luis R. Rodriguez" <mcgrof@suse.com> > > cxgb4 uses request_firmware() 3 times, one for firmware, one for > optional configuration files and another for ethtools flash. Since the > configuration update is optional on devices that don't have a > configuration file update it means we'd wait unnecessarily for the > extra udev timeout, which by default is 60 seconds. Avoid this > extra delay. > > This was found with the following SmPL patch. > > @ firmware_not_critical @ > expression cf; > expression config_file; > expression dev; > int ret; > identifier l; > statement S; > @@ > > - ret = request_firmware(&cf, config_file, dev); > + ret = request_firmware_direct(&cf, config_file, dev); > if (ret < 0) { > ... when != goto l; > when != return ret; > when any > } else { > ... > release_firmware(cf); > ... > } > > Cc: Philip Oswald <poswald@suse.com> > Cc: Santosh Rastapur <santosh@chelsio.com> > Cc: Jeffrey Cheung <jcheung@suse.com> > Cc: David Chang <dchang@suse.com> > Cc: Casey Leedom <leedom@chelsio.com> > Cc: Hariprasad Shenai <hariprasad@chelsio.com> > Cc: Takashi Iwai <tiwai@suse.de> > Cc: cocci@systeme.lip6.fr > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com> > --- > drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c > index 02a0ebf..bd57177 100644 > --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c > +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c > @@ -4999,7 +4999,7 @@ static int adap_init0_config(struct adapter *adapter, int reset) > goto bye; > } > > - ret = request_firmware(&cf, fw_config_file, adapter->pdev_dev); > + ret = request_firmware_direct(&cf, fw_config_file, adapter->pdev_dev); > if (ret < 0) { > config_name = "On FLASH"; > mtype = FW_MEMTYPE_CF_FLASH; > -- > 2.0.0 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 24, 2014 at 03:54:44PM -0700, Casey Leedom wrote: > [[ Hopefully this makes it through to the kernel.org lists -- I’m using the > Mac OS/X Mailer and it’s not clear how to force it not to use HTML format. > -- Casey ]] > > So does request_firmware_direct() only fail if the requested file is not > present on the file system or does it fail in other cases as well? Same as before they are the same exact call with the only difference being udev is not used as an extra helper, so it saves the extra delay caused by udev. That's all. > If it’s the former, then the change to cxgb4 is fine. > > But if it’s the latter, then it’s definitely not okay. While the driver > _can_ continue running without the local on-disk Firmware Configuration > File, that file can be used to significantly change the behavior and > capabilities of the adapter and is user-customizable. If a user makes > changes to the local on-disk Firmware Configuration File and these are > randomly silently ignored this will lead to highly annoying support issues. This just avoids udev, the request goes directly to the filesystem. The failure will happen when the file is not present just as before, the only difference here is skipping udev, it doesn't suffer from the extra 60 second timeout. There's another possible failure, when usermodehelper_read_trylock() fails but that is just as the code was before so this change doesn't introduce that as a new false check. When that triggers yout get a nasty WARN_ON() just as before. One thing to note though is that the kernel firmware loader does not log any failure to the kernel ring buffer if the firmware is not on the filesystem, while the udev loader is a bit more chatty: [ 2463.666120] platform fake-dev.0: Direct firmware load failed with error -2 [ 2463.666129] platform fake-dev.0: Falling back to user helper Stuffing a print into the non-udev approach upstream seems a bit excessive though (unless folks disagree), so if you want the driver to be a bit more informative I think its best to place this feedback on the driver for now. I see the driver does provide this information already though so is any additional prints really desirable ? dev_info(adapter->pdev_dev, "Successfully configured using Firmware "\ "Configuration File \"%s\", version %#x, computed checksum %#x\n", config_name, finiver, cfcsum); Luis -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/24/14 18:50, Luis R. Rodriguez wrote: > On Tue, Jun 24, 2014 at 03:54:44PM -0700, Casey Leedom wrote: >> [[ Hopefully this makes it through to the kernel.org lists -- I’m using the >> Mac OS/X Mailer and it’s not clear how to force it not to use HTML format. >> -- Casey ]] >> >> So does request_firmware_direct() only fail if the requested file is not >> present on the file system or does it fail in other cases as well? > Same as before they are the same exact call with the only difference > being udev is not used as an extra helper, so it saves the extra > delay caused by udev. That's all. > >> If it’s the former, then the change to cxgb4 is fine. >> >> But if it’s the latter, then it’s definitely not okay. While the driver >> _can_ continue running without the local on-disk Firmware Configuration >> File, that file can be used to significantly change the behavior and >> capabilities of the adapter and is user-customizable. If a user makes >> changes to the local on-disk Firmware Configuration File and these are >> randomly silently ignored this will lead to highly annoying support issues. > This just avoids udev, the request goes directly to the filesystem. The > failure will happen when the file is not present just as before, the > only difference here is skipping udev, it doesn't suffer from the extra > 60 second timeout. There's another possible failure, when > usermodehelper_read_trylock() fails but that is just as the code was before > so this change doesn't introduce that as a new false check. When that > triggers yout get a nasty WARN_ON() just as before. Huh, okay. I guess I'm confused about the value of request_firmware() and the User Device helper. If request_firmware_direct() just goes to the file system to grab the file and returns with ENOENT if it's not there, then you could replace every usage of request_firmware() in the cxgb4 driver as far as I can see ... Either the files are there and we'll use them or they won't be and we'll have to cope with that. Am I missing something? And again, this definitely isn't going to solve the problem that started this whole line of research: we're still going to time out the load of cxgb4 if there are multiple 10Gb/s BT adapters in a system and we need to load each one with both base firmware and PHY firmware. Casey > One thing to note though is that the kernel firmware loader does not > log any failure to the kernel ring buffer if the firmware is not on the > filesystem, while the udev loader is a bit more chatty: > > [ 2463.666120] platform fake-dev.0: Direct firmware load failed with error -2 > [ 2463.666129] platform fake-dev.0: Falling back to user helper > > Stuffing a print into the non-udev approach upstream seems a bit excessive > though (unless folks disagree), so if you want the driver to be a bit more > informative I think its best to place this feedback on the driver for now. > > I see the driver does provide this information already though so is any > additional prints really desirable ? > > dev_info(adapter->pdev_dev, "Successfully configured using Firmware "\ > "Configuration File \"%s\", version %#x, computed checksum %#x\n", > config_name, finiver, cfcsum); > > Luis -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 25, 2014 at 10:12:20AM -0700, Casey Leedom wrote: > > On 06/24/14 18:50, Luis R. Rodriguez wrote: >> On Tue, Jun 24, 2014 at 03:54:44PM -0700, Casey Leedom wrote: >>> [[ Hopefully this makes it through to the kernel.org lists -- I’m using the >>> Mac OS/X Mailer and it’s not clear how to force it not to use HTML format. >>> -- Casey ]] >>> >>> So does request_firmware_direct() only fail if the requested file is not >>> present on the file system or does it fail in other cases as well? >> Same as before they are the same exact call with the only difference >> being udev is not used as an extra helper, so it saves the extra >> delay caused by udev. That's all. >> >>> If it’s the former, then the change to cxgb4 is fine. >>> >>> But if it’s the latter, then it’s definitely not okay. While the driver >>> _can_ continue running without the local on-disk Firmware Configuration >>> File, that file can be used to significantly change the behavior and >>> capabilities of the adapter and is user-customizable. If a user makes >>> changes to the local on-disk Firmware Configuration File and these are >>> randomly silently ignored this will lead to highly annoying support issues. >> This just avoids udev, the request goes directly to the filesystem. The >> failure will happen when the file is not present just as before, the >> only difference here is skipping udev, it doesn't suffer from the extra >> 60 second timeout. There's another possible failure, when >> usermodehelper_read_trylock() fails but that is just as the code was before >> so this change doesn't introduce that as a new false check. When that >> triggers yout get a nasty WARN_ON() just as before. > > Huh, okay. I guess I'm confused about the value of request_firmware() > and the User Device helper. If request_firmware_direct() just goes to the > file system to grab the file and returns with ENOENT if it's not there, > then you could replace every usage of request_firmware() in the cxgb4 > driver as far as I can see ... Either the files are there and we'll use > them or they won't be and we'll have to cope with that. Am I missing > something? You're actually right specially given that udev firmware uploading will hopefully eventually be removed eventually (it seems it was just one driver that caused to consider waiting on the removal, some driver that required looking for firmware on some custom path I think or used a custom loader), for now however its best to keep things consistent otherwise we'd replace everything already. The _direct() call then is best used for optional firmware for now. Perhaps in the end will be that the non _direct() call will have an explicit print to the ring buffer if the file was not found. > And again, this definitely isn't going to solve the problem that started > this whole line of research: I consider this research part of understanding and optimizing firmware loading on cxgb4, in this case this would save 60 seconds for each optional configuration file not present when loading, its not clear to me how often devices don't have optional configs so its unclear to me the exact savings in general, but if there's at least one user that should speed things up. > we're still going to time out the load of > cxgb4 if there are multiple 10Gb/s BT adapters in a system and we need to > load each one with both base firmware and PHY firmware. Again, the timeout is *within* firmware_request(), firmware_release() does not tell the firmware loader the timeout is over. The timeout is for the kernel doing the hunt for the file. As I see it the next steps on the evaluation on firmware loading on cxgb4 would be to evaluate a clean strategy to split things up, and also would appreciate feedback on the bus master thing. Perhaps best we continue that discussoin on that thread? Luis -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Okay, I'll leave the whole request_firmware{,_direct,_nowait}() thing
alone. The request_firmware_direct() will "solve" a non-problem (since
all of our "firmware" files are _supposed to be_ always present. (And
the 60 second timeout for udev to confirm that a file doesn't exist
seems like udev is just basically broken.)
That aside, we still need to solve the real problem that we're
experiencing in that the boot-time load of cxgb4 is timing out on SLE12
because a maximum load timeout has been instituted in that distribution
for driver modules and if there are two 10Gb/s-BT adapters present in a
system which need both base firmware and BT PHY firmware, we exceed that
timeout. The timeout really should be per device (since there ~could~
be an arbitrary number of devices in a system) and there probably should
be a way for the driver to notify the kernel timeout mechanism that
forward progress is being made ...
Casey
On 06/25/14 10:31, Luis R. Rodriguez wrote:
> On Wed, Jun 25, 2014 at 10:12:20AM -0700, Casey Leedom wrote:
>> On 06/24/14 18:50, Luis R. Rodriguez wrote:
>>> On Tue, Jun 24, 2014 at 03:54:44PM -0700, Casey Leedom wrote:
>>>> [[ Hopefully this makes it through to the kernel.org lists -- I’m using the
>>>> Mac OS/X Mailer and it’s not clear how to force it not to use HTML format.
>>>> -- Casey ]]
>>>>
>>>> So does request_firmware_direct() only fail if the requested file is not
>>>> present on the file system or does it fail in other cases as well?
>>> Same as before they are the same exact call with the only difference
>>> being udev is not used as an extra helper, so it saves the extra
>>> delay caused by udev. That's all.
>>>
>>>> If it’s the former, then the change to cxgb4 is fine.
>>>>
>>>> But if it’s the latter, then it’s definitely not okay. While the driver
>>>> _can_ continue running without the local on-disk Firmware Configuration
>>>> File, that file can be used to significantly change the behavior and
>>>> capabilities of the adapter and is user-customizable. If a user makes
>>>> changes to the local on-disk Firmware Configuration File and these are
>>>> randomly silently ignored this will lead to highly annoying support issues.
>>> This just avoids udev, the request goes directly to the filesystem. The
>>> failure will happen when the file is not present just as before, the
>>> only difference here is skipping udev, it doesn't suffer from the extra
>>> 60 second timeout. There's another possible failure, when
>>> usermodehelper_read_trylock() fails but that is just as the code was before
>>> so this change doesn't introduce that as a new false check. When that
>>> triggers yout get a nasty WARN_ON() just as before.
>> Huh, okay. I guess I'm confused about the value of request_firmware()
>> and the User Device helper. If request_firmware_direct() just goes to the
>> file system to grab the file and returns with ENOENT if it's not there,
>> then you could replace every usage of request_firmware() in the cxgb4
>> driver as far as I can see ... Either the files are there and we'll use
>> them or they won't be and we'll have to cope with that. Am I missing
>> something?
> You're actually right specially given that udev firmware uploading will
> hopefully eventually be removed eventually (it seems it was just one driver
> that caused to consider waiting on the removal, some driver that required
> looking for firmware on some custom path I think or used a custom loader), for
> now however its best to keep things consistent otherwise we'd replace
> everything already. The _direct() call then is best used for optional firmware
> for now. Perhaps in the end will be that the non _direct() call will have an
> explicit print to the ring buffer if the file was not found.
>
>> And again, this definitely isn't going to solve the problem that started
>> this whole line of research:
> I consider this research part of understanding and optimizing firmware
> loading on cxgb4, in this case this would save 60 seconds for each
> optional configuration file not present when loading, its not clear to
> me how often devices don't have optional configs so its unclear to me the
> exact savings in general, but if there's at least one user that should
> speed things up.
>
>> we're still going to time out the load of
>> cxgb4 if there are multiple 10Gb/s BT adapters in a system and we need to
>> load each one with both base firmware and PHY firmware.
> Again, the timeout is *within* firmware_request(), firmware_release() does
> not tell the firmware loader the timeout is over. The timeout is for the
> kernel doing the hunt for the file.
>
> As I see it the next steps on the evaluation on firmware loading on cxgb4
> would be to evaluate a clean strategy to split things up, and also would
> appreciate feedback on the bus master thing. Perhaps best we continue that
> discussoin on that thread?
>
> Luis
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 25, 2014 at 11:58:52AM -0700, Casey Leedom wrote: > Okay, I'll leave the whole request_firmware{,_direct,_nowait}() thing > alone. The request_firmware_direct() will "solve" a non-problem (since all > of our "firmware" files are _supposed to be_ always present. The code does not reflect that, it allows for files to not be present for the config stuff. If in practice files are always present that's a bit different. > (And the 60 > second timeout for udev to confirm that a file doesn't exist seems like > udev is just basically broken.) Its one reason it being tossed. > That aside, we still need to solve the real problem that we're > experiencing in that the boot-time load of cxgb4 is timing out on SLE12 > because a maximum load timeout has been instituted in that distribution for > driver modules and if there are two 10Gb/s-BT adapters present in a system > which need both base firmware and BT PHY firmware, we exceed that timeout. As for that it'd be great if you can answer some questions I had about the rest of firmware load processing on the other thread, I had a bit of questions for you there. > The timeout really should be per device (since there ~could~ be an > arbitrary number of devices in a system) and there probably should be a way > for the driver to notify the kernel timeout mechanism that forward progress > is being made ... I'd prefer we dive into this on the other thread. Luis -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c index 02a0ebf..bd57177 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c @@ -4999,7 +4999,7 @@ static int adap_init0_config(struct adapter *adapter, int reset) goto bye; } - ret = request_firmware(&cf, fw_config_file, adapter->pdev_dev); + ret = request_firmware_direct(&cf, fw_config_file, adapter->pdev_dev); if (ret < 0) { config_name = "On FLASH"; mtype = FW_MEMTYPE_CF_FLASH;