[U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Michal Simek]

Mike Frysinger wrote:
> On Monday, February 28, 2011 13:50:55 Michal Simek wrote:
>> Mike Frysinger wrote:
>>> On Monday, February 28, 2011 04:40:33 Michal Simek wrote:
>>>> +	return 1;
>>> a bunch of these funcs return 1 when i'm pretty sure they should be 0
>> init function is called from eth.c:eth_init(). From the code below you see
>> that return can be >=0.
> 
> funny enough, that func in your patch is returning 0 when it should be 1.  it 
> doesnt explain why your recv/send are returning 1 when it should be 0.

for recv:
Doesn't matter what you return because there is no return value checking at all.
$ grep -rn "eth_rx" net/
net/eth.c:398:int eth_rx(void)
net/eth.c:433:          eth_rx();
net/net.c:480:          eth_rx();

+ I think that doesn't matter what you return.
For recv I am returning packet length as for example uli526x, ep93xx_eth and I believe other do.

The truth is that in README.drivers.eth is suggested to return 0 for recv.


for sending - yes, there should be 0 but why this even work.

net/net.c:252:  (void) eth_send (NetTxPacket, (pkt - NetTxPacket) + ARP_HDR_SIZE);
net/net.c:641:  (void) eth_send(pkt, len);
net/net.c:686:  (void) eth_send(NetTxPacket, (pkt - NetTxPacket) + IP_HDR_SIZE + len);
net/net.c:970:  (void) eth_send(NetTxPacket, (uchar *)s - NetTxPacket);
net/net.c:1452:                 (void) eth_send((uchar *)et, (pkt - (uchar *)et) + ARP_HDR_SIZE);
net/net.c:1484:                         (void) eth_send(NetArpWaitTxPacket, NetArpWaitTxPacketSize);
net/net.c:1618:                         (void) eth_send((uchar *)et, ETHER_HDR_SIZE + len);
+
api/api_net.c:100:      return eth_send(buf, len);

Only api_net checks return values.

If both functions should return 0 then any code should check it and all others drivers should be fixed.

> 
> the init func is a special beast -- it returns the # of devices registered, 
> not "1 is success".

Where is it written?
ep93xx_eth.c returns also 1.
Anyway if is number of registered devices, "1" should means one registered device.
If zero means one registered device then please point me to that documentation.

Does any code even work with return value?

I expect that if I use bad return values that network driver shouldn't work but it works.

> 
>>>> +int xilinx_axiemac_initialize(bd_t *bis, int base_addr, int dma_addr)
>>>> +{
>>>> +	struct eth_device *dev;
>>>> +	struct axidma_priv *dma;
>>>> +
>>>> +	dev = malloc(sizeof(*dev));
>>>> +	if (dev == NULL)
>>>> +		hang();
>>>> +
>>>> +	memset(dev, 0, sizeof(*dev));
>>>> +	sprintf(dev->name, "Xilinx_AxiEmac");
>>>> +
>>>> +	dev->iobase = base_addr;
>>>> +	dma = dev->priv;
>>>> +	dma->dmatx = dma_addr;
>>>> +	dma->dmarx = (u32)dma->dmatx + 0x30; /* rx channel offset */
>>> hmm, this is weird.  you just memset(dev) to 0, and then used dev->priv
>>> without assigning it storage.  so you're scribbling on top of address 0
>>> with your dma struct here arent you ?
>> dev contains:
>> iobase - axi emac baseaddr
>> init, halt, send, recv pointers to functions
>> and pointer priv
>> + others
>>
>> I need to clear it that's why memset.
>>
>> dma controller is special IP that's why I use priv for storing pointer to
>> axidma_priv structure which contains two pointers to dma for master to
>> slave and slave to master directions (rx and tx if you like). As I wrote
>> dma controller is different IP that's why there are different addresses.
>> axi_dma has offset between channels which is 0x30.
>> I used structures for hw access.
> 
> ok, but i think you missed my point.  let me use this example:
> dev->priv = 0;	/* the memset */
> dma = dev->priv;
> dma->dmatx = dma_addr;
> 
> you're doing a NULL pointer deref here.

you are right. I have to fix it.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian