Re: [Acx100-devel] Radio upload failure should be fatal

[Carlos N. <ca...@cm...>]

On Fri, 2007-01-05 at 19:48 +0100, Andreas Mohr wrote:
> Hi,
>=20
> On Fri, Jan 05, 2007 at 06:59:43PM +0100, Carlos Mart=C3=ADn Nieto wrote:
> > On Fri, 2007-01-05 at 16:27 +0100, Andreas Mohr wrote:
> > > Hi,
> > >=20
> > > On Fri, Jan 05, 2007 at 01:04:56PM +0100, Carlos Mart=C3=ADn Nieto wr=
ote:
> > > > Hi,
> > > >=20
> > > >  I've finally managed to find out what broke my card. I've spent tw=
o
> > > > days hunting this down, until I finally realised that I do indeed n=
eed
> > > > the radio module. I didn't see that line in dmesg in between all th=
ose
> > > > cfgopt output lines.
> > > >=20
> > > >  The question now is, why isn't radio upload failure fatal? The car=
d is
> > > > absolutely worthless without radio, as you can probably guess from =
its
> > > > main purpose being to send and receive radio signals.
> > >=20
> > > Well, because... uh... why don't you know that??
> > > It's such a central characteristic of the TI cards that I'd have expe=
cted
> > > more internal people to know about it:
> >=20
> >  I never had to fiddle with the firmware loading. It was all done befor=
e
> > my time.
>=20
> But as a user you have to, and you have to understand it correctly to get=
 it right ;)
> That's why I expected more involved people to know it.

 Well, I usually just dump all the acx100* files into the firmware
directory.

>=20
> > > (in the words of the recent change I introduced:)
> > >=20
> > >         printk( "acx: need to load firmware for acx1%02d chipset with=
 radio ID %02x, ple
> > > ase provide via firmware hotplug:\n"
> > >                 "acx: either one file only (<c>ombined firmware image=
 file, radio-specif
> > > ic) or two files (radio-less base image file *plus* separate <r>adio-=
specific extension
> > > file)\n",
> > >                 IS_ACX111(adev)*11, adev->radio_type);
> > >=20
> > > IOW, we *cannot* make radio image upload failure fatal since there's =
*NO WAY*
> > > to find out whether we're dealing with a combined (plus radio part) o=
r non-combined
> >=20
> >  Are you saying there is a combined firmware which needs an extra radio
> > part? Or did the (plus radio part) just slip a bit to the left?
>=20
> Uh, ok, I should have said "including radio part", since "plus" is too si=
milar
> to the "plus" used in the printk, which was NOT the same intention.

 Yes, I believe the correct pronoun would be 'with' here.

>=20
> > > base firmware image. Or at least such a way is sufficiently difficult=
 for me to
> > > not have figured it out so far (possibly one might be able to do a bi=
nary scan
> > > for some radio power level tables or so, but this is about as "easy" =
as it might
> > > get).
> >=20
> >  Current logic in the upload is this if I understand correctly:
> >  Try to load the combined firmware. If this fails, set to true the
> > adev->need_radio_fw flag and try to load the standalone firmware. Later
> > in _init_mac(), we upload the radio. If it's not needed
> > (adev->need_radio_fw =3D=3D 0), we return OK, if not, we upload and rep=
ort
> > success or failure.
> >=20
> >  It seems to me that we could make _init_mac() fail if radio upload
> > fails, as the only time it will return an error is when we need the
> > radio and we couldn't find it/upload it.
>=20
> Ah, I see. That would be possible, yes.
> That would be when the filename given by the user indicates a non-combine=
d firmware
> but the radio image is missing.

 The filename is chosen by the driver following very strict rules. We
provide firmware which adheres to these rules. If the user provides the
wrong image, it's their own fault.

>=20
> However if we implement a radio I/O register verification mechanism then =
this
> wouldn't be required in this form anyway since we'd make radio image uplo=
ad
> dependent on the radio still not being initialized after having loaded th=
e
> base (combined or non-combined) image.
> In that case we'd finally have hard data on whether radio initialization =
code
> is existing or not, so failure handling could be made much more rigid/rob=
ust.
> We could even detect and warn about the case where a user indicates
> (via ill-informed filename renaming) a non-combined base image which actu=
ally
> is a combined one however, since then the radio would already be initiali=
zed
> after having loaded the base image.

 Verification shouldn't be needed (we make it easy to pick the right
one), but still nice.

>=20
> > > IOW, this *will* blow up in the faces of people who use a non-combine=
d base firmware
> > > image and then fail to provide the *REQUIRED* (in this case only!) ra=
dio image. ;)
> >=20
> >  It doesn't blow up (this is what prompted this e-mail). The driver wil=
l
> > happily go about its business as though nothing had happened and we hav=
e
> > a confused user who doesn't understand why the card fails to find the
> > wireless network that sits about two meters from him. Besides, with
> > default logging that message disappears fast and you get some others
> > telling you it's scanning.
>=20
> "blowing up" is catastrophical failure, and what could be more catastroph=
ical
> than wasting days on a problem? ;)

 When I see "blowing up" I think something very visual and/or loud.
Spending two days to find out that I was missing a file is neither. And
neither is having a card without a working radio (that happens to be
very silent).

>=20
> >  I've spent two days trying to figure out whether some initialisation
> > had broken ACX100, and then it turns out it was because of the missing
> > radio (for some reason I had the idea my image was a combined one even
> > though the file name didn't match). It was my fault, but a clearer hint
> > would be nice.
>=20
> Due to TI's messup a clearer hint wasn't possible (minus the radio upload
> return code screwup above), so thanks for you starting the discussion sin=
ce it
> finally made me realize the radio I/O register verification possibility.

 Glad to help.

>=20
> >  I'm sure it would be something worth seeing. However, I think my
> > proposed solution above is much simpler, and a one-liner (or two-liner
> > if we print out that why we're failing.
>=20
> The next version will contain either the full solution (I/O checks)
> or the partial fix (init_mac() failure).
> Hmm, radio I/O register verification might be a problem in the USB case,
> though.

Poor USB, people just don't understand it ;)

   cmn
--=20
Carlos Mart=C3=ADn Nieto http://www.cmartin.tk