| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 13 Feb 2009 10:50:38 +0100
From: Jiri Slaby <jirislaby@...il.com>
To: jie.yang@...eros.com
CC: davem@...emloft.net, jcliburn@...il.com, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] atl1c:Atheros L1C Gigabit Ethernet driver
On 02/12/2009 08:55 AM, jie.yang@...eros.com wrote:
> From: Jie Yang <jie.yang@...eros.com>
>
> Supporting AR8131, and AR8132.
>
> Signed-off-by: Jie Yang <jie.yang@...eros.com>
> ---
> Updated on David Miller's comments.
>
> new file mode 100644
> index 0000000..979e802
> --- /dev/null
> +++ b/drivers/net/atl1c/atl1c.h
[...]
> +#define AT_TAG_TO_VLAN(_tag, _vlan) \
> + _vlan = ((((_tag) >> 8) & 0xF) |\
> + (((_tag) & 0xF0) << 8) |\
> + (((_tag) & 0xF) >> 8))
The last line is always 0, right?
> diff --git a/drivers/net/atl1c/atl1c_hw.c b/drivers/net/atl1c/atl1c_hw.c
> new file mode 100644
> index 0000000..bd47350
> --- /dev/null
> +++ b/drivers/net/atl1c/atl1c_hw.c
[...]
> +int atl1c_write_phy_reg(struct atl1c_hw *hw, u32 reg_addr, u16 phy_data)
> +{
> + int i;
> + u32 val;
> +
> + val = ((u32)(phy_data & MDIO_DATA_MASK)) << MDIO_DATA_SHIFT |
> + (reg_addr & MDIO_REG_ADDR_MASK) << MDIO_REG_ADDR_SHIFT |
> + MDIO_SUP_PREAMBLE | MDIO_START |
> + MDIO_CLK_25_4 << MDIO_CLK_SEL_SHIFT;
> +
> + AT_WRITE_REG(hw, REG_MDIO_CTRL, val);
> + wmb();
Did you mean mmiowb()? Why it is needed? This doesn't cope with pci posting. The
read below does.
> +
> + for (i = 0; i < MDIO_WAIT_TIMES; i++) {
> + udelay(2);
> + AT_READ_REG(hw, REG_MDIO_CTRL, &val);
> + if (!(val & (MDIO_START | MDIO_BUSY)))
> + break;
> + wmb();
> + }
> +
> + if (!(val & (MDIO_START | MDIO_BUSY)))
> + return 0;
> +
> + return -1;
> +}
[...]
> +int atl1c_phy_reset(struct atl1c_hw *hw)
> +{
> + struct atl1c_adapter *adapter = hw->adapter;
> + struct pci_dev *pdev = adapter->pdev;
> + u32 phy_ctrl_data = GPHY_CTRL_DEFAULT;
> + u32 mii_ier_data = IER_LINK_UP | IER_LINK_DOWN;
> + int err;
> +
> + if (hw->ctrl_flags & ATL1C_HIB_DISABLE)
> + phy_ctrl_data &= ~GPHY_CTRL_HIB_EN;
> +
> + AT_WRITE_REG(hw, REG_GPHY_CTRL, phy_ctrl_data);
> + msleep(40);
Do you need the write to reach the device and wait 40ms? Then you need to
AT_WRITE_FLUSH, I suppose.
> + phy_ctrl_data |= GPHY_CTRL_EXT_RESET;
> + AT_WRITE_REG(hw, REG_GPHY_CTRL, phy_ctrl_data);
> + msleep(10);
ditto
> + /*Enable PHY LinkChange Interrupt */
> + err = atl1c_write_phy_reg(hw, MII_IER, mii_ier_data);
> + if (err) {
> + dev_err(&pdev->dev, "Error enable PHY linkChange Interrupt\n");
> + return err;
> + }
> + if (!(hw->ctrl_flags & ATL1C_FPGA_VERSION))
> + atl1c_phy_magic_data(hw);
> + return 0;
> +}
[...]
> diff --git a/drivers/net/atl1c/atl1c_main.c b/drivers/net/atl1c/atl1c_main.c
> new file mode 100644
> index 0000000..61bef3d
> --- /dev/null
> +++ b/drivers/net/atl1c/atl1c_main.c
[...]
> +static void atl1c_reset_pcie(struct atl1c_hw *hw, u32 flag)
> +{
> + u32 data;
> + u32 pci_cmd;
> + struct pci_dev *pdev = hw->adapter->pdev;
> +
> + AT_READ_REG(hw, PCI_COMMAND, &pci_cmd);
> + pci_cmd &= ~PCI_COMMAND_INTX_DISABLE;
> + pci_cmd |= (PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER |
> + PCI_COMMAND_IO);
> + AT_WRITE_REG(hw, PCI_COMMAND, pci_cmd);
> +
> + /*
> + * Clear any PowerSaveing Settings
> + */
> + pci_enable_wake(pdev, PCI_D3hot, 0);
> + pci_enable_wake(pdev, PCI_D3cold, 0);
> +
> + /*
> + * Mask some pcie error bits
> + */
> + AT_READ_REG(hw, REG_PCIE_UC_SEVERITY, &data);
> + data &= ~PCIE_UC_SERVRITY_DLP;
> + data &= ~PCIE_UC_SERVRITY_FCP;
> + AT_WRITE_REG(hw, REG_PCIE_UC_SEVERITY, data);
> +
> + if (flag & ATL1C_PCIE_L0S_L1_DISABLE)
> + atl1c_disable_l0s_l1(hw);
> + if (flag & ATL1C_PCIE_PHY_RESET)
> + AT_WRITE_REG(hw, REG_GPHY_CTRL, GPHY_CTRL_DEFAULT);
> + else
> + AT_WRITE_REG(hw, REG_GPHY_CTRL,
> + GPHY_CTRL_DEFAULT | GPHY_CTRL_EXT_RESET);
> +
> + msleep(1);
again.
> +static void atl1c_configure_rss(struct atl1c_adapter *adapter)
> +{
> + struct atl1c_hw *hw = (struct atl1c_hw *)&adapter->hw;
Why the cast?
> +
> + AT_WRITE_REG(hw, REG_IDT_TABLE, hw->indirect_tab);
> + AT_WRITE_REG(hw, REG_BASE_CPU_NUMBER, hw->base_cpu);
> +
> + return;
Unneeded return.
> +static int atl1c_reset_mac(struct atl1c_hw *hw)
> +{
> + struct atl1c_adapter *adapter = (struct atl1c_adapter *)hw->adapter;
> + struct pci_dev *pdev = adapter->pdev;
> + u32 idle_status_data = 0;
> + int timeout = 0;
> + int ret;
> +
> + AT_WRITE_REG(hw, REG_IMR, 0);
> + AT_WRITE_REG(hw, REG_ISR, ISR_DIS_INT);
> +
> + ret = atl1c_stop_mac(hw);
> + if (ret)
> + return ret;
> + /*
> + * Issue Soft Reset to the MAC. This will reset the chip's
> + * transmit, receive, DMA. It will not effect
> + * the current PCI configuration. The global reset bit is self-
> + * clearing, and should clear within a microsecond.
> + */
> + AT_WRITE_REGW(hw, REG_MASTER_CTRL, MASTER_CTRL_SOFT_RST);
> + msleep(10);
> + wmb();
again
> +static u16 atl1c_cal_tpd_req(const struct sk_buff *skb)
> +{
> + int i = 0;
> + u16 tpd_req = 1;
> + u16 proto_hdr_len = 0;
> +
> + for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
> + tpd_req++;
Is this
tpd_req = skb_shinfo(skb)->nr_frags + 1;
?
> +static void atl1c_tx_map(struct atl1c_adapter *adapter,
> + struct sk_buff *skb, struct atl1c_tpd_desc *tpd,
> + enum atl1c_trans_queue type)
> +{
> + struct atl1c_tpd_desc *use_tpd = NULL;
> + struct atl1c_buffer *buffer_info = NULL;
> + u16 buf_len = skb->len - skb->data_len;
skb_headlen()
> + u16 map_len = 0;
> + u16 mapped_len = 0;
> + u16 hdr_len = 0;
> + u16 nr_frags;
> + u16 f;
> + int tso;
> +
[...]
> + for (f = 0; f < nr_frags; f++) {
> + struct skb_frag_struct *frag;
> +
> + frag = &skb_shinfo(skb)->frags[f];
> +
> + use_tpd = atl1c_get_tpd(adapter, type);
> + memcpy(use_tpd, tpd, sizeof(struct atl1c_tpd_desc));
> +
> + buffer_info = atl1c_get_tx_buffer(adapter, use_tpd);
> + buffer_info->length = frag->size;
> + buffer_info->dma =
> + pci_map_page(adapter->pdev, frag->page,
> + frag->page_offset,
> + buffer_info->length,
> + PCI_DMA_TODEVICE);
> + buffer_info->state = ATL1_BUFFER_BUSY;
> + if (buffer_info->dma == 0) {
> + printk(KERN_EMERG "buffer_info dma is 0\n");
> + dump_stack();
So you don't rollback here, do you allow the device to overwrite memory at phys
addr 0?
> + }
> +
> + use_tpd->buffer_addr = cpu_to_le64(buffer_info->dma);
> + use_tpd->buffer_len = cpu_to_le16(buffer_info->length);
> + }
> +
> + /* The last tpd */
> + use_tpd->word1 |= 1 << TPD_EOP_SHIFT;
> + /* The last buffer info contain the skb address,
> + so it will be free after unmap */
> + buffer_info->skb = skb;
> +}
> +static int atl1c_request_irq(struct atl1c_adapter *adapter)
> +{
> + struct pci_dev *pdev = adapter->pdev;
> + struct net_device *netdev = adapter->netdev;
> + int flags = 0;
> + int err = 0;
> +
> + adapter->have_msi = true;
> + err = pci_enable_msi(adapter->pdev);
> + if (err) {
> + dev_dbg(&pdev->dev,
> + "Unable to allocate MSI interrupt Error: %d\n", err);
Why not be verbose? Nobody will see this message unless DEBUG is defined (or set
on dynamic printks).
> + adapter->have_msi = false;
> + } else
> + netdev->irq = pdev->irq;
> +
> + if (!adapter->have_msi)
> + flags |= IRQF_SHARED;
> + err = request_irq(adapter->pdev->irq, &atl1c_intr, flags,
> + netdev->name, netdev);
> + if (err) {
> + dev_dbg(&pdev->dev,
> + "Unable to allocate interrupt Error: %d\n", err);
ditto
> + if (adapter->have_msi)
> + pci_disable_msi(adapter->pdev);
> + return err;
> + }
> + dev_dbg(&pdev->dev, "atl1c_request_irq OK\n");
> + return err;
> +}
[...]
> +#ifdef CONFIG_PM
> +static int atl1c_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> + struct net_device *netdev = pci_get_drvdata(pdev);
> + struct atl1c_adapter *adapter = netdev_priv(netdev);
> + struct atl1c_hw *hw = &adapter->hw;
> + u32 ctrl = 0;
> + u32 mac_ctrl_data = 0;
> + u32 master_ctrl_data = 0;
> + u32 wol_ctrl_data = 0;
> + u16 mii_bmsr_data = 0;
> + u16 save_autoneg_advertised;
> + u16 mii_intr_status_data = 0;
> + u32 wufc = adapter->wol;
> + u32 i;
> +#ifdef CONFIG_PM
> + int retval = 0;
> +#endif
> +
> + printk(KERN_EMERG "in atl1c_suspend!! wufc is %d\n", wufc);
Why is it EMERG? It looks like a sort of debug info.
> + if (netif_running(netdev)) {
> + WARN_ON(test_bit(__AT_RESETTING, &adapter->flags));
> + atl1c_down(adapter);
> + }
> + netif_device_detach(netdev);
> + atl1c_disable_l0s_l1(hw);
> +#ifdef CONFIG_PM
> + retval = pci_save_state(pdev);
> + if (retval)
> + return retval;
> +#endif
> + if (wufc) {
> + AT_READ_REG(hw, REG_MASTER_CTRL, &master_ctrl_data);
> + master_ctrl_data &= ~MASTER_CTRL_CLK_SEL_DIS;
> +
> + /* get link status */
> + atl1c_read_phy_reg(hw, MII_BMSR, (u16 *)&mii_bmsr_data);
> + atl1c_read_phy_reg(hw, MII_BMSR, (u16 *)&mii_bmsr_data);
> + save_autoneg_advertised = hw->autoneg_advertised;
> + hw->autoneg_advertised = ADVERTISED_10baseT_Half;
> + if (atl1c_restart_autoneg(hw) != 0)
> + dev_dbg(&pdev->dev, "phy autoneg failed\n");
> + hw->phy_configured = false; /* re-init PHY when resume */
> + hw->autoneg_advertised = save_autoneg_advertised;
> + /* turn on magic packet wol */
> + if (wufc & AT_WUFC_MAG)
> + wol_ctrl_data |= WOL_MAGIC_EN | WOL_MAGIC_PME_EN;
> +
> + if (wufc & AT_WUFC_LNKC) {
> + for (i = 0; i < AT_SUSPEND_LINK_TIMEOUT; i++) {
> + msleep(100);
> + atl1c_read_phy_reg(hw, MII_BMSR,
> + (u16 *)&mii_bmsr_data);
> + if (mii_bmsr_data & BMSR_LSTATUS)
> + break;
> + }
> + if ((mii_bmsr_data & BMSR_LSTATUS) == 0)
> + dev_dbg(&pdev->dev,
> + "%s: Link may change"
> + "when suspend\n",
> + atl1c_driver_name);
too low verbosity
> + wol_ctrl_data |= WOL_LINK_CHG_EN | WOL_LINK_CHG_PME_EN;
> + /* only link up can wake up */
> + if (atl1c_write_phy_reg(hw, MII_IER, IER_LINK_UP) != 0) {
> + dev_dbg(&pdev->dev, "%s: read write phy "
> + "register failed.\n",
> + atl1c_driver_name);
again
> +static int __devinit atl1c_probe(struct pci_dev *pdev,
> + const struct pci_device_id *ent)
> +{
[...]
> + adapter->hw.hw_addr = pci_iomap(pdev, 0, 0);
Ah, and you use readl, writel on it. Use ioremap instead. Or ioread/iowrite.
Hmm, what happened to Arjan's pci_ioremap? <Going to investigate...>
> + init_timer(&adapter->phy_config_timer);
> + adapter->phy_config_timer.function = &atl1c_phy_config;
> + adapter->phy_config_timer.data = (unsigned long) adapter;
setup_timer() is more appropriate here.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists