Stephen Hemminger wrote:
|
| Applied to 'sky2' branch of netdev-2.6.git. Let me know when the RX
| hangs are fixed, and we can push upstream.
Okay. Most all of your comments are straightforward
and fixes will be in the next version, but some need
a little more discussion...
|
| > +static void gm_phy_write(struct sky2_hw *hw, unsigned port, u16 reg, u16 val)
| > +{
| > + int i;
| > +
| > + gma_write16(hw, port, GM_SMI_DATA, val);
| > + gma_write16(hw, port, GM_SMI_CTRL,
| > + GM_SMI_CT_PHY_AD(PHY_ADDR_MARV) |
GM_SMI_CT_REG_AD(reg));
| > +
| > + for (i = 0; i < PHY_RETRIES; i++) {
| > + udelay(1);
|
| PCI posting bug... do we care?
We always end up doing a read of the control register
so how is that a problem?
The first udelay() is superfluous due to PCI posting. If you -need-
such a delay before the first read, then the code is buggy. If there is
no such need, things are OK as-is.
| > +static inline void sky2_rx_add(struct sky2_port *sky2, dma_addr_t map, u16
len)
| > +{
| > + struct sky2_rx_le *le;
| > +
| > + if (sizeof(map) > sizeof(u32)) {
| > + le = sky2_next_rx(sky2);
| > + le->rx.addr = cpu_to_le32((u64) map >> 32);
|
| use (map >> 16) >> 16
Does it matter? because the map will be a 64 bit value
if we ever get to that part of the code anyway. On 32bit
address systems this code disappears.
On systems where the code will get optimized out, 'map >> 32' is an
undefined operation. Sure the optimizer will snip the code, but
sometime in the future the compiler may decide to error out, rather than
ignore the undefined operation.
Gotta get the code correct, before handing it to the optimizer.
| > +
| > + paddr = pci_map_single(sky2->hw->pdev, re->skb->data,
| > + rx_buf_size, PCI_DMA_FROMDEVICE);
| > +
| > + pci_unmap_len_set(re, maplen, rx_buf_size);
| > + pci_unmap_addr_set(re, mapaddr, paddr);
|
| pci_unmap???
This is book keeping for iommu systems. see other drivers.
It's just highly irregular to use pci_unmap_xxx functions immediately
after calling a pci_map_xxx function.
| > + goto err_out_free_hw;
| > + }
| > +
| > + err = request_irq(pdev->irq, sky2_intr, SA_SHIRQ, DRV_NAME, hw);
| > + if (err) {
| > + printk(KERN_ERR PFX "%s: cannot assign irq %d\n",
| > + pci_name(pdev), pdev->irq);
| > + goto err_out_iounmap;
| > + }
|
| don't do this in probe, do it in dev->open(). reading your interrupt
| handling code you are OBVIOUSLY not ready to handle interrupts at this
| point.
Since this driver has to support dual port boards, it
gets wierd. I would rather just change to mask of irq's in HW
then get it at probe.
Your interrupt handler will still get called, for cases such as e.g.
shared interrupts. You have to make sure ALL your data structures are
capable of handling interrupts, at the point you call request_irq().
Jeff
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
