Message-Id: <1213699220.26255.977.camel@pmac.infradead.org>
Date:	Tue, 17 Jun 2008 11:40:20 +0100
From:	David Woodhouse <dwmw2@...radead.org>
To:	Jeff Garzik <jeff@...zik.org>
Cc:	Jes Sorensen <jes@...ined-monkey.org>, netdev@...r.kernel.org,
	jaswinder@...radead.org
Subject: Re: [PATCH] firmware: convert acenic driver to request_firmware()

On Mon, 2008-06-16 at 18:11 -0400, Jeff Garzik wrote:
> An intermediate request_firmware() step is something that can go into an 
> upstream kernel _immediately_, without even needing firmware/ install 
> infrastructure.

Not before 2.6.26, it can't -- it's much too late for that. And I'm
planning to ask Linus to take the 'make firmware_install' infrastructure
as soon as the merge window opens for 2.6.27. So there's certainly no
need to separate them on _those_ grounds.

The parts which live in the Fedora kernel specfile to spit out the
kernel-firmware subpackage are also ready to get merged as soon as the
infrastructure is upstream.

> That, in turn, enables the ability to test a driver with externally 
> loaded firmwares, while still being able to fall back to a known 
> working, guaranteed present built-in firmware.
> 
> IOW, the first step for each driver should be:
> 
> +	request_firmware()
> +	if (success)
> +		use externally-loaded firmware
> +	else
> 		use built-in firmware
> 
> and that's it.  Nothing else [in the first patch].

If it were that simple, I would agree with you. In practice, it hasn't
turned out to be that simple. I really do think it would be
counter-productive for both review and testing purposes. 

For review because, as I said, we'd end up providing new copies of
certain functions in the first patch (or conditional noise in them), and
then ripping out the old code path in the second -- the patch which it
would make sense to _read_ and review would be the combined one.

For testing because there's quite a large chance that if something goes
wrong with the request_firmware() path, it'd fall back to the other path
and people would say "OK, that works" when it doesn't. And we both know
that kind of thing will still happen even if we have a bloody great
warning printk in the fallback case.

I suppose what we _could_ do is make your 'use built-in firmware' path
use that firmware in the _same_ form as we have it provided by
request_firmware(). That way, we really are testing the same code path.
Would that meet your approval?

If you look closely, you'll see that that is exactly what I've done
already. But without the complexity in the driver -- the driver just
calls request_firmware(), and the firmware is either provided from
within the kernel image, or from userspace.

> While I'm on the subject, I think the most user-friendly, 
> developer-friendly, and time-friendly ordering is
> 
> 0) prepare kernel-firmware package with extracted firmwares from kernel 
> tree, and have it available in rawhide, cooker, opensuse-devel, etc.
> 
> 1) what I said above: request_firmware() for every driver
> 
> At this point, users have udev-loadable firmware in hand and a driver 
> capable of loading said firmware, and can immediately start operating in 
> the desired environment.
> 
> 2) start peeling built-in firmwares out of the C source

Why would you insist that we support request_firmware() in _every_
driver, before we _depend_ on it in any? That doesn't seem to make a lot
of sense. Especially since we already _have_ a whole bunch of drivers
which _depend_ on request_firmware().

Would you suggest that we shouldn't have drivers like myri10ge depending
on request_firmware(), just because the tg3 driver still doesn't have
that option?

Each driver stands alone, and can just be converted in one step.

> What I think you don't get is that moving the firmware out of the driver 
> is the _easy_ part that comes _last_.
> 
> The core problem with request_firmware() loading external firmwares is 
> usability:  the user damn well needs to have their firmware available, 
> otherwise their hardware doesn't work.
> 
> That means the pressure is on you to have all the elements in place with 
> all the major distros _before_ you do the easy part.

Firstly, distributions already _have_ all the elements in place for
loading firmware, and even for including it in the initrd as necessary.
It's not as if we're making them cope with something new and exciting.
So there should be no particular concern on that front. All they need to
do is put the firmware into /lib/firmware.

Secondly, we are still allowing people to build firmware into the kernel
-- we're not _forcing_ them to use udev or initrds at all. So it really
isn't hard for them to cope with the transition. at all.

> Otherwise the user experience is going to suck, the last thing we all 
> want...

I've given a lot of thought to how the transition affects userspace,
have implemented the Fedora packaging side of it and spoken to some of
the people who would take the brunt of the users' displeasure if it goes
wrong. And I didn't reach the same conclusions you did.

> The truth of the matter is, the user really does want to set that 
> Kconfig option, during the initial time period after your patches are 
> applied.

They shouldn't need to do that. "make firmware_install" works right now,
and the rest of the infrastructure is _already_ in place and actively
used by a lot of other drivers.

> Once things settled down and infrastructure is in place, the 'default y' 
> could go away.

For distributions it doesn't matter; it'll get dealt with properly.
Fedora is already ready; other distributions shouldn't find it hard to
do the same when they update their kernel to 2.6.27.

For individuals, they either need to run 'make firmware_install' or
build the firmware in. Preferably the former, which is why we've put
"Say 'N'" in the help texts for these options.

Perhaps we could put a big reminder in the final output for the
modules_install or vmlinux targets, saying:
	You need to run 'make firmware_install'

I don't think it's a particularly valid concern, though.

> >> 3) there are enough changes to warrant a requirement of a "driver still 
> >> works" test before going in, IMO
> > 
> > Most definitely. As I said, I've been very careful to ensure that the
> > converted firmware is correct, in conjunction with the driver changes
> > (like the endianness considerations in this case). But that is no
> > substitute for testing.
> 
> Cool :)

I'm sorry, perhaps I misspoke. I don't actually plan to hold off on
merging patches until I've got positive testing results for each and
every one, although obviously I'd like to. It's just not practical,
though. For some things, like Ambassador ATM cards, I'm not even sure
anyone still _has_ the hardware. And the changes are, for the most part,
Obviously Correct™ janitorial-style stuff.

But I am actively trying to get people to test these patches on real
hardware, and would be very _happy_ if they were tested before they go
in. That's one of the reasons they're in linux-next already.

And because they _don't_ automatically fall back to the old code paths,
I know that a success report in linux-next is a success report for the
changes we've made.

-- 
dwmw2

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists