Re: [PATCH] bnx2 - use request_firmware()

Re: [PATCH] bnx2 - use request_firmware()

[David Woodhouse]

On Sat, 2008-07-05 at 00:54 +0200, Bastian Blank wrote:
> Hi David
> 
> This patch is used by Debian since 2.6.25 to use request_firmware in the
> bnx2 driver. It lacks a small piece of inline patching for now.
> 
> The firmware files includes 7 firmwares with up to 3 sections plus some
> additional initialisation data. The corresponding firmware file
> generator is located at
> svn://svn.debian.org/kernel/dists/trunk/firmware-nonfree/bnx2/fwcutter.
> 
> Signed-off-by: Bastian Blank <waldi@debian.org>

Thanks for that. I had said I was going to stop working on drivers/net
for now though, so I'm not sure whether to apply it right now. Do you
have any more of these patches, for _non_ network drivers?

The firmware tree is intended to be obviously correct and uncontentious,
just updating drivers to what is common practice these days _anyway_,
with a few other things to make that easier for users.

Given that the majority of normal people are happy with it, we can cope
with a small amount of mostly-unfounded whinging, which we've got from
_both_ of the extreme ends of the spectrum as we try to find a sensible
path somewhere in the middle. But I don't want to provoke it further
right now, so I'm quite tempted to leave this patch until later, to
ensure that the tantrums from the network maintainers don't get in the
way of the rest of the real work and cleanups we're doing.

But if _Michael_ signs off on it, and if you can provide it in the form
used in the firmware-2.6.git tree, with the firmware files added into
the firmware/ subdirectory so they're still being shipped with the
kernel and the user doesn't have to go find them elsewhere, then I
suppose I'll probably take it.

I believe you no longer need to 'select FW_LOADER', btw.

-- 
dwmw2

Re: [PATCH] bnx2 - use request_firmware()

[Michael Chan]

David Woodhouse wrote:

> But if _Michael_ signs off on it, and if you can provide it
> in the form
> used in the firmware-2.6.git tree, with the firmware files added into
> the firmware/ subdirectory so they're still being shipped with the
> kernel and the user doesn't have to go find them elsewhere, then I
> suppose I'll probably take it.
>

I cannot sign off on this until I understand more about the impact
of this change.  Unlike the tg3 firmware which hasn't changed for at
least 4 or 5 years, the bnx2 firmware is constantly changing and it
has to match the driver.  For example, we'll be adding multi-tx ring
to the driver soon and it will require the feature in the firmware.

Re: [PATCH] bnx2 - use request_firmware()

[Bastian Blank]

On Sun, Jul 06, 2008 at 09:21:21PM -0700, Michael Chan wrote:
> I cannot sign off on this until I understand more about the impact
> of this change.

The change is targeted for the firmware tree. The firmware tree
seperates the firmwares from the driver _within_ the kernel tree. Both
parts are still shipped in the same tree. The driver themself is
modified to use request_firmware. If the driver is builtin the kernel,
the firmware is appended to vmlinux where request_firmware is able to
find them. If it is built as a module the firmware is copied into
/lib/firmware during installation where the famous hotplug handler can
find it.

So the impact is that you need a hotplug handler in the module case.
Most of the modern wireless cards drivers (e.g. b43, iwl*) needs them
anyway.

My patch does not yet include the firmware move within the tree because
it would make the patch really large. Will do that.

>                  Unlike the tg3 firmware which hasn't changed for at
> least 4 or 5 years, the bnx2 firmware is constantly changing and it
> has to match the driver.  For example, we'll be adding multi-tx ring
> to the driver soon and it will require the feature in the firmware.

Thats why the firmware files got a "version" string included. You can
change it for incompatible changes in the firmware. Your workflow will
not change drastically. You still can modify the firmware and the source
in one tree.

Also it is not that uncommon that you need to update firmwares for new
kernel versions in other devices.

Bastian

-- 
Fascinating, a totally parochial attitude.
		-- Spock, "Metamorphosis", stardate 3219.8

Re: [PATCH] bnx2 - use request_firmware()

[David Woodhouse]

On Sun, 2008-07-06 at 21:21 -0700, Michael Chan wrote:
> David Woodhouse wrote:
> 
> > But if _Michael_ signs off on it, and if you can provide it
> > in the form
> > used in the firmware-2.6.git tree, with the firmware files added into
> > the firmware/ subdirectory so they're still being shipped with the
> > kernel and the user doesn't have to go find them elsewhere, then I
> > suppose I'll probably take it.
> >
> 
> I cannot sign off on this until I understand more about the impact
> of this change.  Unlike the tg3 firmware which hasn't changed for at
> least 4 or 5 years, the bnx2 firmware is constantly changing and it
> has to match the driver.  For example, we'll be adding multi-tx ring
> to the driver soon and it will require the feature in the firmware.

The firmware will continue to be shipped with the kernel source tree
after this patch. Before applying it, will will update the patch so that
it doesn't just make the driver use request_firmware(), but also adds
the firmware back into the firmware/ directory of the source tree.

We retain the option to build the firmware into the kernel image -- so
if the driver is built in, nothing at all needs to change.

When the driver is built at a module, the 'make modules_install' command
will install the firmware into /lib/firmware where userspace can load
it.

The real concern I see would be if you make an incompatible change in
the firmware when you add the new feature -- so that the older drivers
would no longer work with the new firmware. But mostly, people don't do
that -- I'm guessing that your planned new firmware should work just
fine with older drivers, and the older drivers just won't use the new
feature?

If it _is_ backward-incompatible, that's not really a problem. The
solution is relatively simple; you just change the name of the firmware
file that gets requested by the new driver -- so the old driver
continues to request the old name, and get the firmware that works with
it. It's a bit like an soname in userspace libraries, in that respect.

Please support this patch.

At some point in the future, we'll have a discussion about moving the
firmware/ directory out to a separate, non-GPL'd, repository, where more
firmware can be incorporated. But that's a way off yet.

-- 
dwmw2

Re: [PATCH] bnx2 - use request_firmware()

[Michael Chan]

On Mon, 2008-07-07 at 02:03 -0700, David Woodhouse wrote:
> The real concern I see would be if you make an incompatible change in
> the firmware when you add the new feature -- so that the older drivers
> would no longer work with the new firmware. But mostly, people don't
> do
> that -- I'm guessing that your planned new firmware should work just
> fine with older drivers, and the older drivers just won't use the new
> feature?
> 
> If it _is_ backward-incompatible, that's not really a problem. The
> solution is relatively simple; you just change the name of the
> firmware
> file that gets requested by the new driver -- so the old driver
> continues to request the old name, and get the firmware that works
> with
> it. It's a bit like an soname in userspace libraries, in that respect.

The driver is not guaranteed to be backward or forward compatible with
the firmware.  It may be forward compatible in most cases (new firmware
may work with older driver) but there is no guarantee because it is
simply not necessary in the current model.

We also only test 1 driver + 1 firmware and no other combinations.
Separating the 2 makes things more complicated and prone to random
failures.

Re: [PATCH] bnx2 - use request_firmware()

[David Miller]

From: "Michael Chan" <mchan@broadcom.com>
Date: Mon, 07 Jul 2008 11:56:21 -0700

> The driver is not guaranteed to be backward or forward compatible with
> the firmware.  It may be forward compatible in most cases (new firmware
> may work with older driver) but there is no guarantee because it is
> simply not necessary in the current model.
> 
> We also only test 1 driver + 1 firmware and no other combinations.
> Separating the 2 makes things more complicated and prone to random
> failures.

Right.

I want to know what the actual "use case" is of this new stuff.

Who in the world is going to actually want request_firmware() to find
a firmware image other than the one which has been properly tested
together with the driver by the driver maintainer?

What "use case" is there other than the desire to seperate out the
firmware in order to skirt the legal issues?

These drivers which include their own firmware and do not use
request_firmware() are functioning perfectly fine, have done so for
many many years, and gain zero by having request_firmware() support.

I think it is, in fact, the driver maintainer's perogative of whether
they want request_firmware() to be supported by their driver or not.
It is they who have to deal with any possible fallout.

Re: [PATCH] bnx2 - use request_firmware()

[Alan Cox]

> Who in the world is going to actually want request_firmware() to find
> a firmware image other than the one which has been properly tested
> together with the driver by the driver maintainer?

That misses the point, intentionally I am sure. In the majority of cases
the firmware doesn't change between releases so shipping a billion copies
of is a pain in the butt.

> What "use case" is there other than the desire to seperate out the
> firmware in order to skirt the legal issues?

Not shipping lots of copies
Not leaving crap locked in kernel memory when it isn't needed
Letting vendors issue firmware updates (which especially in enterprise
space is a big issue and right now gets messy with compiled in firmware)

> I think it is, in fact, the driver maintainer's perogative of whether
> they want request_firmware() to be supported by their driver or not.
> It is they who have to deal with any possible fallout.

And their users and the distributors for whom it can cause enormous pain.

If the two are closely tied then it makes a lot of sense to keep them
tied, but that doesn't mean wasting a ton of kernel memory and bandwidth
and disk space in the process. Loading the firmware and insisting on a
specific version is quite civilised for a driver with such a tie.

(of course we had this argument over ten years ago about modules when
various authors couldn't be bothered to modularise their driver which
caused endless pain to the distributions and end users. Remember the
sound driver situation in early Red Hat. Mind you it got me a job there
fixing it ;))

Driver authors aren't God. There are other important considerations, but
for tg3 if that means 'wrong MD5sum, no load' then fine.


Alan

Re: [PATCH] bnx2 - use request_firmware()

[David Miller]

From: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date: Mon, 7 Jul 2008 22:19:50 +0100

> Not leaving crap locked in kernel memory when it isn't needed
> Letting vendors issue firmware updates (which especially in enterprise
> space is a big issue and right now gets messy with compiled in firmware)

The firmware needs to be reloaded every time the chip resets.
You're not saving anything.

Or do you want a request_firmware() call failure to hose your
ethernet device when it gets a TX timeout?

<sarcasm>
Sounds like a real error resiliant system to me...
</sarcasm>

Distribution vendors can just as easily ship the driver itself
seperately to get the firmware update.  And by getting it together the
user knows they are receiving something the driver maintainer tested
as a unit.

> And their users and the distributors for whom it can cause enormous
> pain.

Distribution vendors just want the separation so that they don't have
to keep patching the fimrware out of the tg3.c driver source every
single release, as some do :-)

> If the two are closely tied then it makes a lot of sense to keep
> them tied, but that doesn't mean wasting a ton of kernel memory and
> bandwidth and disk space in the process. Loading the firmware and
> insisting on a specific version is quite civilised for a driver with
> such a tie.

See above, you aren't saving anything.  The firmware needs to stay
around so it can be reloaded into the card during exceptions.

That is, unless you want a more failure prone system.

> Driver authors aren't God.

They (actually, more specifically the maintainers) to a certain extent
are, because they are the ones who eat doo-doo when something explodes.

 There are other important considerations, but
> for tg3 if that means 'wrong MD5sum, no load' then fine.

So in your "firmware updated seperately" argument above how in the
world does this work?  How can you update the firmware seperately if
the MD5sum check is in the driver itself?

Re: [PATCH] bnx2 - use request_firmware()

[Alan Cox]

> The firmware needs to be reloaded every time the chip resets.
> You're not saving anything/

> See above, you aren't saving anything.  The firmware needs to stay
> around so it can be reloaded into the card during exceptions.
> 
> That is, unless you want a more failure prone system.

Ok so if tg3 always needs the same firmware and always needs it in memory
then maybe it isn't a significant candidate for request_firmware beyond
the neatness of distribution. I note the firmware hasn't changed in years
so it can easily be shipped separately and the one package would have
done for all this time.

> > Driver authors aren't God.
> 
> They (actually, more specifically the maintainers) to a certain extent
> are, because they are the ones who eat doo-doo when something explodes.

So do the distributions and the users.

Alan

Re: [PATCH] bnx2 - use request_firmware()

[David Miller]

From: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date: Tue, 8 Jul 2008 07:39:22 +0100

> > The firmware needs to be reloaded every time the chip resets.
> > You're not saving anything/
> 
> > See above, you aren't saving anything.  The firmware needs to stay
> > around so it can be reloaded into the card during exceptions.
> > 
> > That is, unless you want a more failure prone system.
> 
> Ok so if tg3 always needs the same firmware and always needs it in memory
> then maybe it isn't a significant candidate for request_firmware beyond
> the neatness of distribution. I note the firmware hasn't changed in years
> so it can easily be shipped separately and the one package would have
> done for all this time.

It isn't just tg3.  All the broadcom gigabit chips need this
kind of handling.

Basically all of the drivers we are pushing back on.

I bet there are other similar examples.

> > > Driver authors aren't God.
> > 
> > They (actually, more specifically the maintainers) to a certain extent
> > are, because they are the ones who eat doo-doo when something explodes.
> 
> So do the distributions and the users.

Not really. The dist folks and users hit a problem, and it rolls
downhill quickly, and more often than not it plops right on the head
of the driver maintainer.

request_firmware vs. resume (was Re: [PATCH] bnx2 - use request_firmware())

[Pavel Machek]

Hi!

> > > The firmware needs to be reloaded every time the chip resets.
> > > You're not saving anything/
> > 
> > > See above, you aren't saving anything.  The firmware needs to stay
> > > around so it can be reloaded into the card during exceptions.
> > > 
> > > That is, unless you want a more failure prone system.
> > 
> > Ok so if tg3 always needs the same firmware and always needs it in memory
> > then maybe it isn't a significant candidate for request_firmware beyond
> > the neatness of distribution. I note the firmware hasn't changed in years
> > so it can easily be shipped separately and the one package would have
> > done for all this time.
> 
> It isn't just tg3.  All the broadcom gigabit chips need this
> kind of handling.
> 
> Basically all of the drivers we are pushing back on.
> 
> I bet there are other similar examples.

Be careful about request_firmware. Doing it right w.r.t.
suspend/resume is quite tricky: you have to load it from userspace
before kernel starts, so that you can use it during resume...

							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: request_firmware vs. resume (was Re: [PATCH] bnx2 - use request_firmware())

[Rafael J. Wysocki]

On Wednesday, 9 of July 2008, Pavel Machek wrote:
> Hi!
> 
> > > > The firmware needs to be reloaded every time the chip resets.
> > > > You're not saving anything/
> > > 
> > > > See above, you aren't saving anything.  The firmware needs to stay
> > > > around so it can be reloaded into the card during exceptions.
> > > > 
> > > > That is, unless you want a more failure prone system.
> > > 
> > > Ok so if tg3 always needs the same firmware and always needs it in memory
> > > then maybe it isn't a significant candidate for request_firmware beyond
> > > the neatness of distribution. I note the firmware hasn't changed in years
> > > so it can easily be shipped separately and the one package would have
> > > done for all this time.
> > 
> > It isn't just tg3.  All the broadcom gigabit chips need this
> > kind of handling.
> > 
> > Basically all of the drivers we are pushing back on.
> > 
> > I bet there are other similar examples.
> 
> Be careful about request_firmware. Doing it right w.r.t.
> suspend/resume is quite tricky: you have to load it from userspace
> before kernel starts, so that you can use it during resume...

Rather, you have to cache it in memory before your ->suspend() is invoked.

Thanks,
Rafael

Re: request_firmware vs. resume (was Re: [PATCH] bnx2 - use request_firmware())

[Theodore Tso]

On Wed, Jul 09, 2008 at 11:13:54PM +0200, Rafael J. Wysocki wrote:
> > Be careful about request_firmware. Doing it right w.r.t.
> > suspend/resume is quite tricky: you have to load it from userspace
> > before kernel starts, so that you can use it during resume...
> 
> Rather, you have to cache it in memory before your ->suspend() is invoked.

Translation: so much for saving "non-swappable kernel memory".
Unless, of course we add a pre-suspend hook.  (Which doesn't exist
yet, AFAICT from a quick perusal of Documentation/power/devices.txt.)


     	    	   	 	    - Ted

Re: request_firmware vs. resume (was Re: [PATCH] bnx2 - use request_firmware())

[Rafael J. Wysocki]

On Wednesday, 9 of July 2008, Theodore Tso wrote:
> On Wed, Jul 09, 2008 at 11:13:54PM +0200, Rafael J. Wysocki wrote:
> > > Be careful about request_firmware. Doing it right w.r.t.
> > > suspend/resume is quite tricky: you have to load it from userspace
> > > before kernel starts, so that you can use it during resume...
> > 
> > Rather, you have to cache it in memory before your ->suspend() is invoked.
> 
> Translation: so much for saving "non-swappable kernel memory".
> Unless, of course we add a pre-suspend hook.  (Which doesn't exist
> yet, AFAICT from a quick perusal of Documentation/power/devices.txt.)

No, it doesn't.

You will be able to register a suspend notifier instead, but this requires
one fix which is in the works.

Thanks,
Rafael

Re: request_firmware vs. resume (was Re: [PATCH] bnx2 - use request_firmware())

[Theodore Tso]

On Wed, Jul 09, 2008 at 11:58:31PM +0200, Rafael J. Wysocki wrote:
> > Translation: so much for saving "non-swappable kernel memory".
> > Unless, of course we add a pre-suspend hook.  (Which doesn't exist
> > yet, AFAICT from a quick perusal of Documentation/power/devices.txt.)
> 
> No, it doesn't.
> 
> You will be able to register a suspend notifier instead, but this requires
> one fix which is in the works.

Will a suspend notifier be able to block the suspend until userspace
gets back to the device driver calling request_firmware()?

							- Ted

Re: request_firmware vs. resume (was Re: [PATCH] bnx2 - use request_firmware())

[Rafael J. Wysocki]

On Thursday, 10 of July 2008, Theodore Tso wrote:
> On Wed, Jul 09, 2008 at 11:58:31PM +0200, Rafael J. Wysocki wrote:
> > > Translation: so much for saving "non-swappable kernel memory".
> > > Unless, of course we add a pre-suspend hook.  (Which doesn't exist
> > > yet, AFAICT from a quick perusal of Documentation/power/devices.txt.)
> > 
> > No, it doesn't.
> > 
> > You will be able to register a suspend notifier instead, but this requires
> > one fix which is in the works.
> 
> Will a suspend notifier be able to block the suspend until userspace
> gets back to the device driver calling request_firmware()?

I think so.

To be exact, I'm going to put the disabling of usermode helpers after the
invocation of the suspend notifiers and that will wait for all of the already
running helpers to finish.

Thanks,
Rafael

Re: [PATCH] bnx2 - use request_firmware()

[Jeff Garzik]

Alan Cox wrote:
>> Who in the world is going to actually want request_firmware() to find
>> a firmware image other than the one which has been properly tested
>> together with the driver by the driver maintainer?
> 
> That misses the point, intentionally I am sure. In the majority of cases
> the firmware doesn't change between releases so shipping a billion copies
> of is a pain in the butt.
> 
>> What "use case" is there other than the desire to seperate out the
>> firmware in order to skirt the legal issues?
> 
> Not shipping lots of copies
> Not leaving crap locked in kernel memory when it isn't needed
> Letting vendors issue firmware updates (which especially in enterprise
> space is a big issue and right now gets messy with compiled in firmware)

Do these benefits justify the removal of an actively used feature, one 
more reliable than its replacement?


>> I think it is, in fact, the driver maintainer's perogative of whether
>> they want request_firmware() to be supported by their driver or not.
>> It is they who have to deal with any possible fallout.
> 
> And their users and the distributors for whom it can cause enormous pain.

Where is this enormous pain associated with tg3's compiled-in firmware? 
  It's been quite convenient.

	Jeff

Re: [PATCH] bnx2 - use request_firmware()

[David Miller]

From: Jeff Garzik <jeff@garzik.org>
Date: Mon, 07 Jul 2008 18:08:56 -0400

> Alan Cox wrote:
> > And their users and the distributors for whom it can cause enormous pain.
> 
> Where is this enormous pain associated with tg3's compiled-in firmware? 

Indeed.

Re: [PATCH] bnx2 - use request_firmware()

[Alan Cox]

On Mon, 07 Jul 2008 18:08:56 -0400
> > Not shipping lots of copies
> > Not leaving crap locked in kernel memory when it isn't needed
> > Letting vendors issue firmware updates (which especially in enterprise
> > space is a big issue and right now gets messy with compiled in firmware)
> 
> Do these benefits justify the removal of an actively used feature, one 
> more reliable than its replacement?

I think they do for a lot of drivers. And I dispute the actively used
feature claim except for a tiny number of people.
> 
> >> I think it is, in fact, the driver maintainer's perogative of whether
> >> they want request_firmware() to be supported by their driver or not.
> >> It is they who have to deal with any possible fallout.
> > 
> > And their users and the distributors for whom it can cause enormous pain.
> 
> Where is this enormous pain associated with tg3's compiled-in firmware? 
>   It's been quite convenient.

How many kernel updates do you think all the worlds users and
distributors have transferred. How much bandwidth and time do you think
that cost ?

Alan

Re: [PATCH] bnx2 - use request_firmware()

[David Miller]

From: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date: Tue, 8 Jul 2008 07:41:32 +0100

> On Mon, 07 Jul 2008 18:08:56 -0400
> > Where is this enormous pain associated with tg3's compiled-in firmware? 
> >   It's been quite convenient.
> 
> How many kernel updates do you think all the worlds users and
> distributors have transferred. How much bandwidth and time do you think
> that cost ?

Sometimes we pay a price for being able to sleep at night.

How many cpus in the world have burned useless cycles computing the
SHA1 checksum of some file's contents for GIT :-)

Re: [PATCH] bnx2 - use request_firmware()

[david]

On Mon, 7 Jul 2008, Alan Cox wrote:

>> Who in the world is going to actually want request_firmware() to find
>> a firmware image other than the one which has been properly tested
>> together with the driver by the driver maintainer?
>
> That misses the point, intentionally I am sure. In the majority of cases
> the firmware doesn't change between releases so shipping a billion copies
> of is a pain in the butt.
>
>> What "use case" is there other than the desire to seperate out the
>> firmware in order to skirt the legal issues?
>
> Not shipping lots of copies
> Not leaving crap locked in kernel memory when it isn't needed
> Letting vendors issue firmware updates (which especially in enterprise
> space is a big issue and right now gets messy with compiled in firmware)
>
>> I think it is, in fact, the driver maintainer's perogative of whether
>> they want request_firmware() to be supported by their driver or not.
>> It is they who have to deal with any possible fallout.
>
> And their users and the distributors for whom it can cause enormous pain.
>
> If the two are closely tied then it makes a lot of sense to keep them
> tied, but that doesn't mean wasting a ton of kernel memory and bandwidth
> and disk space in the process. Loading the firmware and insisting on a
> specific version is quite civilised for a driver with such a tie.

so make the firmware part of the module if the driver is compiled as a 
module. this way if the driver (and firmware) end up not being used they 
don't take up any space. this seems a lot simpler (as well as more 
reliable) then adding a mandatory dependancy on a different userspace 
tool.

David Lang

> (of course we had this argument over ten years ago about modules when
> various authors couldn't be bothered to modularise their driver which
> caused endless pain to the distributions and end users. Remember the
> sound driver situation in early Red Hat. Mind you it got me a job there
> fixing it ;))
>
> Driver authors aren't God. There are other important considerations, but
> for tg3 if that means 'wrong MD5sum, no load' then fine.
>
>
> Alan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH] bnx2 - use request_firmware()

[Alan Cox]

> so make the firmware part of the module if the driver is compiled as a 
> module. this way if the driver (and firmware) end up not being used they 
> don't take up any space. this seems a lot simpler (as well as more 
> reliable) then adding a mandatory dependancy on a different userspace 
> tool.

For the majority of drivers that would be a serious regression as they
load the firmware when needed, which is not when loaded. In some cases
there are many sets of firmware and only one is needed according to the
device, and only needed once per device detect.

It might well make sense for tg3 although even there it seems most tg3
work fine without loading new firmware, and examples like aic7xxx where
the firmware and code must match.

Alan