FreeLists / openbeosnetteam / [openbeosnetteam] Re: Realtek 8139 Driver

["Niels Reedijk" <n.reedijk@xxxxxxxxx>]

> Niels, I've received your driver source.
> I'm bit suprised how you handle the rx buffer!
> You know it's suppose to be a ring buffer, don't you?

Jup, I know.

> According to Realtek datasheet, their chip will handle it that way.
> But I can't see where you handle this circular property in your driver
> code!?

To be frank: I have completely forgotten to finish that (but it shoudn't
be a large problem though). It's something that needs to go to the TODO
file. The reason it isn't finished, is because I can't even receive the
first packets normally, so there's little use in perfecting the whole
thing already. 

> You simply always increment the buffer tail address on 32bits
boundary.
> There should be some "modulo 64k"  magic somewhere!!!
> ;-)

Undocumented todo.

> Next, a packet could wraps around the ring buffer, something your
> append_packet() function don't care (yet !?).

Honestly, I don't really like the append_packet function. IHMO it is
double work. In theory, I could simply keep a pointer to the start of
the packet and the length, however, all drivers update the CAPR with
-16, so that sort of is out of the question (though I am wondering why
the -16 is there ... ).

> I don't have any RTL8139 card to stress your code more, but I don't
see
> how the reception could works right
> looking at current code.

I do think you agree that the first 64k reception should do fine.
 
> I notice, too, that both NewOS rtl8139.c driver (which, unfortunatly
> for you, is no more
> present in our CVS and as NewOS source depot seem unavailable these
> days... bad luck)
> and NetBSD rtl81x9.c drivers disable chip interrupts by writing 0 to
> IMR register at start of the interrupt handler,
> and re-enable them at exit.
> Maybe it's a good design to follow?

If I spinlock the whole interrupt handler, it shouldn't be interrupted,
right? I don't think it is necessary to do so.

> Next, it's may be a good idea to clear all interrupt status masks,
even
> if you don't handle them yet.
> Instead of building this isr_write value, simply *acknoledge* the
whole
> interrupt status masks by writing it:
> 
> isr_contents = m_pcimodule->read_io_16( data->reg_base + ISR );
> if (isr_contents)
>   m_pcimodule->write_io_16(data->reg_base + ISR, isr_contents);

I remember I first wrote oxFF to the ISR but that didn't work, it didn't
clear the ISR. However, the main reason is that if such an unimplemented
interrupt occurred, I'd notice and implement them as well. But with the
driver now (perhaps) reaching a wider audience, it may be wise to fix
this behaviour.

> Last, looking at NetBSD and NewOS drivers, they both seems to loop on
> reading the ISR and handling each interrupt case until there's no
more.
> Maybe this chip don't raise all interrupt status flags at same time?

I think I missed this, I'll look at the NetBSD driver.

> I'll create a new rtl8139 directoy to host your driver in the next
> hours or days, depends on son noise level :-p

Thanks a million. I will send any updated files to you too. 

Niels