[Cbe-oss-dev] [patch 15/16] powerpc and x86_64: Initial port of PCIe endpoint device driver for Axon

[Murali Iyer]

On Mon, 2008-03-17 at 05:45 +0100, Arnd Bergmann wrote:

> I think it would be good to also post this driver to the 
> netdev at vger.kernel.org mailing list next time, because we don't
> have many network experts around here.
OK. We do have experts like Ben H, isn't it?

> 
> > +
> > +/* number of Axon devices found in the system for network interfaces */
> > +static	int	num_apnet_if;
> > +
> > +/* Store apnet's private data per interface */
> > +static struct apnet	*apnet_interface[NUM_APNET];
> > +
> 
> Shouldn't be needed. The only place where you access these
> is the deregistration of the driver, and that automatically
> walks the device list and calls the driver->remove function.
For now it is needed, apnet_probe is initializing and apnet_remove is
using because apnet_init is not registering properly and will go
away when base driver supports to register virtual bus for apnet.


> Do I see this right that you call axon_enable_notify_callback() to switch
> on/off the local delivery of callback interrupts, but not the actual calling
> of the interrupt handler?
Correct. 

> 
> > +static void apnet_tx_timeout(struct net_device *ndev)
> > +{
> > +       struct apnet *adev = netdev_priv(ndev);
> > +
> > +       if (apnet_tx_ring_status(adev))
> > +               return;
> > +
> > +       if (adev->astats.tx_dma_err)    /* return if we got DMA error */
> > +               return;
> > +
> > +       if (apnet_get_tx_bd(adev))
> > +               netif_wake_queue(ndev); /* found free slot to xmit */
> > +       else
> > +               axon_send_remote_notification(APNET_MSG_TYPE,
> > adev->if_num);
> > +       return; 
> 
> If the DMA failed, do you really want to send a remote notification?
> Doesn't that cause the broken packet to get received by the remote
> side?
This is not for DMA failure alone.. if remote side is descriptors are
full for various reasons like slow cpu speed, busy etc. this will kick
start the rx. not come across the situation but as a insurance won't
hurt.

> If I read this correctly, you generate a local interrupt for every frame
> you transmit, and from this handler, generate another remote interrupt
> to notify of the packet being transmitted if that is enabled?
> 
> IIRC, you get a lot of tx interrupts this way, and it would be good
> to reduce this number. Can you send the dma-complete event to the
> remote end instead? That would first of all reduced the latency
> of the initial packet rx, and also mean that you can consolidate
> multiple tx-complete events in a single interrupt sent back from
> the receiving end.
This driver uses DMA driver to xmit data. DMA driver could be explored
optimized to reduce the DMA complete interrupts. from apnet perspective
it doesn't generate too many interrupts because of NAPI for RX side.
Even though it appears apnet generates every interrupt for remote,
axon_send_remote_notification() is checked by the base driver and sends
the remote notification interrupt only if
axon_enable_remote_notification() was called otherwise just returns.
This is done because interrupt to be shared between multiple clients.



> remember to make these messages pr_debug at some point. When debugging,
> it shouldn't make a difference, and later they get annoying.
This will be logged only when user changes mac address using something
like "ifconfig apnet? hw ether <MAC-ADDRESS> and expect often.
My intention is to keep it as pr_info.

> 
> > +static inline __be64 read64be(__be64 *addr)
> > +{
> > +	__be64 tmp = be64_to_cpu(__raw_readq(addr));
> > +	rmb();	/* make sure read ordering */
> > +	return tmp;
> > +}
> > +
> > +static inline void write64be(__be64 data, __be64 *addr)
> > +{
> > +	__raw_writeq(cpu_to_be64(data), addr);
> > +	wmb(); /* make sure write ordering */
> > +}
> 
> The use of __raw_writeq in device drivers is a bit suspicious,
> as the __iomem token is not necessarily really a pointer, depending
> on some compile-time options. Also, your read functions have
> somewhat less strict ordering rules than the in_be64 function used
> by the regular accessors.
> 
> Checking this code with sparse should give you a lot of warnings
> about address space mismatches. I think it would be much better
> to have separate functions for accessing local and remote memory,
> even if that means some amount of duplication in code using it.
OK. it is on my todo for long time.. didn't get to it yet.

> 
> > +#define pr_err(fmt, arg...) \
> > +	printk(KERN_ERR fmt, ##arg)
> 
> This is already defined in kernel.h
Thanks for later kernel.h, it isn't in 2.6.23.x


-- 
Bye,

Murali