On Wed, 17 Aug 2005 14:30:31 -0400
Jeff Garzik <[EMAIL PROTECTED]> wrote:
> 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.
No read and udelay can swap.
>
> > | > +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.
Reworking that stuff anyway to avoid wasting slots. Saving ring slots
is worth it.
> > | > +
> > | > + 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.
Did you know that pci_unmap_addr_set() is a macro that hides the fact
that some arch need iommu but for many it is a no-op. This is the expected
usage.
#define pci_unmap_addr_set(PTR, ADDR_NAME, VAL) \
(((PTR)->ADDR_NAME) = (VAL))
> > | > + 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().
Calling sky2_reset first is enough, it makes sure that all the hardware
and data structures are ready.
-
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
