On Fri, Jan 18, 2002 at 12:07:56AM -0500, Jeff Garzik wrote:
> Jean Tourrilhes wrote:
>
Well... A bit of background...
The Wavelan hardware has been obsoleted for the last 5
years. I put the driver in minimal maintainance mode, because I
already have a lot on my place (you would not want me to spend less
time on the IrDA stack, the Orinoco driver or the new Wireless
Extensions ?).
The point is that the driver didn't work. I didn't look any
further and I decided for better or worse to use the version that is
in the Pcmcia package that has been working for more than one
year. Now that the two version are in sync, I can work from there.
I tried to make sure to not loose any changes. Of course, I'm
only human.
I hope that explains the situation.
Attached you will find a patch that address some of your
concerns. This patch will apply to both the kernel version AND the
Pcmcia package version.
Note also that it applies to the kernel *after* driver move
(that is pending - because I've put this patch at the end of my patch
queue).
> ug... you are much better to put spin_lock_irqsave directly into the
> code, don't wrap it. wrapping, here, only obscures the code using it to
> the casual reader.
>
> This change also makes the diff long.
That's only personal preference. And the wrapper in theory
would allow to simply change the locking strategy if needed (like
disabling irq on the hardware instead of in software).
> > +#if 0
> > + /* Some people don't need this, some other may need it */
> > + nwid=nwid^ntohs(beacon->domain_id);
> > +#endif
>
> maybe better as #ifdef I_NEED_THIS_FEATURE then?
Done.
> > + /* Busy wait while the LAN controller executes the command. */
> > + spin = 1000;
> > + do
> > {
> > - outb(OP0_NOP, LCCR(base));
> > + /* Time calibration of the loop */
> > + udelay(10);
>
> I hate busy-waits :) Long term I wonder if some of the longer ones can
> be pushed into tasklets or even kernel threads. (if a kernel thread,
> then the intr handler would really turn into an async notification to
> the kernel thread of new work)
For such an obsolete driver, it's not worth replacing code
that is working fine and has been debugged and tested for
years. Moreover, on most case the wait is really small.
> > +static inline void
> > +wv_82593_reconfig(device * dev)
> > {
> > - net_local *lp = (net_local *) dev->priv;
> > - dev_link_t *link = ((net_local *) dev->priv)->link;
> > + net_local * lp = (net_local *)dev->priv;
> > + dev_link_t * link = ((net_local *) dev->priv)->link;
>
> Is it possible to use standard kernel types, ie. "struct net_device"
> instead of "device"?
If you want to spend time on this, please send me a patch and
I'll apply it to both the kernel version and the Pcmcia package.
> > #ifdef DEBUG_IOCTL_INFO
> > - printk (KERN_DEBUG "%s: wv_82593_reconfig(): delayed (link =
>
> you could use __FUNCTION__ here if you wanted to. (make sure to pass it
> via %s if you do)
I would not bother.
By the way, I looked at the __FUNCTION__ business, and IrDA
will need a massive cleanup. *sight*...
> > @@ -1404,7 +1417,7 @@ wv_init_info(device * dev)
> > printk("2430.5");
> > break;
> > default:
> > - printk("unknown");
> > + printk("???");
> > }
> > }
> >
>
> you are reverting a change - "???" causes a trigraph-related warning in
> newer gcc
I didn't knew about that, and my gcc doesn't give any
warning. I can't believe that gcc is too stupid to realise that you
can't have trigraph within a string or a comment.
Done. And guess what, it will also go in the Pcmcia package ;-)
> > @@ -1968,7 +1981,7 @@ wavelan_ioctl(struct net_device * dev, /
> >
> > case SIOCGIWFREQ:
> > /* Attempt to recognise 2.00 cards (2.4 GHz frequency selectable)
> > - * (does it work for everybody XXX - especially old cards...) */
> > + * (does it work for everybody ??? - especially old cards...) */
> > if(!(mmc_in(base, mmroff(0, mmr_fee_status)) &
> > (MMR_FEE_STATUS_DWLD | MMR_FEE_STATUS_BUSY)))
> > {
>
> likewise
I'm sorry, but "XXX" doesn't mean "???" in any variation of
the english language that I've ever encoutered.
> > + /* Allocate the generic data structure */
> > + dev = kmalloc(sizeof(struct net_device), GFP_KERNEL);
> > + if (!dev) {
> > + kfree(link);
> > + return NULL;
> > + }
> > + memset(dev, 0x00, sizeof(struct net_device));
> > link->priv = link->irq.Instance = dev;
> >
> > + /* Allocate the wavelan-specific data structure. */
> > + dev->priv = lp = (net_local *) kmalloc(sizeof(net_local), GFP_KERNEL);
> > + if (!lp) {
> > + kfree(link);
> > + kfree(dev);
> > + return NULL;
> > + }
> > + memset(lp, 0x00, sizeof(net_local));
> > +
>
> use alloc_etherdev instead of all this...
No need to change working code.
> Jeff Garzik
Thanks for the quick review...
Jean
This archive was generated by hypermail 2b29 : Wed Jan 23 2002 - 21:00:28 EST