| 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
| ||
|
Message-ID: <20100709024058.GB29669@verge.net.au>
Date: Fri, 9 Jul 2010 11:40:59 +0900
From: Simon Horman <horms@...ge.net.au>
To: "Choi, David" <David.Choi@...rel.Com>
Cc: netdev@...r.kernel.org, "Li, Charles" <Charles.Li@...rel.Com>
Subject: Re: [PATCH linux-2.6.35-rc3] ks8842 driver
On Thu, Jul 08, 2010 at 12:01:51PM -0700, Choi, David wrote:
> Hi Simon,
>
> Thank you for your comments.
>
> The original ks8842 driver is designed to work on the customized bus
> interface based on an FPGA. This patch is intended to address the more
> commonly used generic bus interface available on the majority of SoC in
> the market.
>
> It is unlikely that for a system to use both FPGA based and generic bus
> interface for ks8842, I am quite certain that those 2 devices are used
> mutual exclusively.
Hi David,
Understood, in that case I think that using #ifdef is reasonable.
Though to my mind it would be nicer to try and abstract it out,
as you have in the first hunk, to something of the form:
foo()
{
A
#ifdef CONFIG_MICREL_KS884X
B
#else
C
#endif
D
}
And then call foo() unconditionally.
I think this is cleaner than, for example ks8842_rx_frame()
where there are several instances of the same #ifdef condition.
>
> Regards,
> David J. Choi
>
>
> -----Original Message-----
> From: Simon Horman [mailto:horms@...ge.net.au]
> Sent: Thursday, July 08, 2010 12:53 AM
> To: Choi, David
> Cc: netdev@...r.kernel.org; Li, Charles
> Subject: Re: [PATCH linux-2.6.35-rc3] ks8842 driver
>
> On Wed, Jul 07, 2010 at 08:24:33AM -0700, Choi, David wrote:
> > To whom it may have concerned,
> >
> > It seems that there are differences between Micrel ks8842 device and
> Timberdale FPGA based device with generic bus interface. In order to
> check the differences, I have sent several times but have not received
> any response. This patch is to support ks8841/ks8842 device from Micrel.
>
> Is it desirable that the code can never be compiled to work with both
> devices?
> Is this code likely to be included in a generic/distro kernel?
>
> >
> > From: David J. Choi <david.choi@...rel.com>
> >
> > Body of the explanation:
> > -support 16bit and 32bit bus width.
> > -add device reset for ks8842/8841 Micrel device.
> > -set 100Mbps as a default for Micrel device.
> > -set MAC address in both MAC/Switch layer with different sequence
> for Micrel device,
> > as mentioned in data sheet.
> >
> > Signed-off-by: David J. Choi <david.choi@...rel.com>
> >
> > ---
> > --- linux-2.6.35-rc3/drivers/net/ks8842.c.orig 2010-07-01
> 16:26:50.000000000 -0700
> > +++ linux-2.6.35-rc3/drivers/net/ks8842.c 2010-07-07
> 07:41:03.000000000 -0700
> > @@ -18,6 +18,7 @@
> >
> > /* Supports:
> > * The Micrel KS8842 behind the timberdale FPGA
> > + * The genuine Micrel KS8841/42 device with ISA 16/32bit bus
> interface
> > */
> >
> > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > @@ -191,6 +192,12 @@ static inline u32 ks8842_read32(struct k
> >
> > static void ks8842_reset(struct ks8842_adapter *adapter)
> > {
> > +#ifdef CONFIG_MICREL_KS884X
> > + ks8842_write16(adapter, 3, 1, REG_GRR);
> > + msleep(10);
> > + iowrite16(0, adapter->hw_addr + REG_GRR);
> > +
> > +#else
> > /* The KS8842 goes haywire when doing softare reset
> > * a work around in the timberdale IP is implemented to
> > * do a hardware reset instead
> > @@ -201,6 +208,7 @@ static void ks8842_reset(struct ks8842_a
> > iowrite16(32, adapter->hw_addr + REG_SELECT_BANK);
> > iowrite32(0x1, adapter->hw_addr + REG_TIMB_RST);
> > msleep(20);
> > +#endif
> > }
> >
> > static void ks8842_update_link_status(struct net_device *netdev,
> > @@ -269,8 +277,11 @@ static void ks8842_reset_hw(struct ks884
> >
> > /* restart port auto-negotiation */
> > ks8842_enable_bits(adapter, 49, 1 << 13, REG_P1CR4);
> > +
> > +#ifndef CONFIG_MICREL_KS884X
> > /* only advertise 10Mbps */
> > ks8842_clear_bits(adapter, 49, 3 << 2, REG_P1CR4);
> > +#endif
> >
> > /* Enable the transmitter */
> > ks8842_enable_tx(adapter);
> > @@ -296,6 +307,20 @@ static void ks8842_read_mac_addr(struct
> > for (i = 0; i < ETH_ALEN; i++)
> > dest[ETH_ALEN - i - 1] = ks8842_read8(adapter, 2,
> REG_MARL + i);
> >
> > +#ifdef CONFIG_MICREL_KS884X
> > + /*
> > + the sequence of saving mac addr between MAC and Switch
> is
> > + different.
> > + */
> > +
> > + mac = ks8842_read16(adapter, 2, REG_MARL);
> > + ks8842_write16(adapter, 39, mac, REG_MACAR3);
> > + mac = ks8842_read16(adapter, 2, REG_MARM);
> > + ks8842_write16(adapter, 39, mac, REG_MACAR2);
> > + mac = ks8842_read16(adapter, 2, REG_MARH);
> > + ks8842_write16(adapter, 39, mac, REG_MACAR1);
> > +#else
> > +
> > /* make sure the switch port uses the same MAC as the QMU */
> > mac = ks8842_read16(adapter, 2, REG_MARL);
> > ks8842_write16(adapter, 39, mac, REG_MACAR1);
> > @@ -303,6 +328,7 @@ static void ks8842_read_mac_addr(struct
> > ks8842_write16(adapter, 39, mac, REG_MACAR2);
> > mac = ks8842_read16(adapter, 2, REG_MARH);
> > ks8842_write16(adapter, 39, mac, REG_MACAR3);
> > +#endif
> > }
> >
> > static void ks8842_write_mac_addr(struct ks8842_adapter *adapter, u8
> *mac)
> > @@ -313,8 +339,13 @@ static void ks8842_write_mac_addr(struct
> > spin_lock_irqsave(&adapter->lock, flags);
> > for (i = 0; i < ETH_ALEN; i++) {
> > ks8842_write8(adapter, 2, mac[ETH_ALEN - i - 1],
> REG_MARL + i);
> > +#ifdef CONFIG_MICREL_KS884X
> > + ks8842_write8(adapter, 39, mac[ETH_ALEN - i - 1],
> > + REG_MACAR3 + 1 - i);
> > +#else
> > ks8842_write8(adapter, 39, mac[ETH_ALEN - i - 1],
> > REG_MACAR1 + i);
> > +#endif
> > }
> > spin_unlock_irqrestore(&adapter->lock, flags);
> > }
> > @@ -328,8 +359,12 @@ static int ks8842_tx_frame(struct sk_buf
> > {
> > struct ks8842_adapter *adapter = netdev_priv(netdev);
> > int len = skb->len;
> > +#ifdef CONFIG_KS884X_16BIT
> > + u16 *ptr16 = (u16 *)skb->data;
> > +#else
> > u32 *ptr = (u32 *)skb->data;
> > u32 ctrl;
> > +#endif
> >
> > dev_dbg(&adapter->pdev->dev,
> > "%s: len %u head %p data %p tail %p end %p\n",
> > @@ -340,6 +375,18 @@ static int ks8842_tx_frame(struct sk_buf
> > if (ks8842_tx_fifo_space(adapter) < len + 8)
> > return NETDEV_TX_BUSY;
> >
> > +#ifdef CONFIG_KS884X_16BIT
> > + ks8842_write16(adapter, 17, 0x8000 | 0x100, REG_QMU_DATA_LO);
> > + ks8842_write16(adapter, 17, (u16)len, REG_QMU_DATA_HI);
> > + netdev->stats.tx_bytes += len;
> > +
> > + /* copy buffer */
> > + while (len > 0) {
> > + iowrite16(*ptr16++, adapter->hw_addr + REG_QMU_DATA_LO);
> > + iowrite16(*ptr16++, adapter->hw_addr + REG_QMU_DATA_HI);
> > + len -= sizeof(u32);
> > + }
> > +#else
> > /* the control word, enable IRQ, port 1 and the length */
> > ctrl = 0x8000 | 0x100 | (len << 16);
> > ks8842_write32(adapter, 17, ctrl, REG_QMU_DATA_LO);
> > @@ -352,6 +399,7 @@ static int ks8842_tx_frame(struct sk_buf
> > len -= sizeof(u32);
> > ptr++;
> > }
> > +#endif
> >
> > /* enqueue packet */
> > ks8842_write16(adapter, 17, 1, REG_TXQCR);
> > @@ -364,10 +412,15 @@ static int ks8842_tx_frame(struct sk_buf
> > static void ks8842_rx_frame(struct net_device *netdev,
> > struct ks8842_adapter *adapter)
> > {
> > +#ifdef CONFIG_KS884X_16BIT
> > + u16 status = ks8842_read16(adapter, 17, REG_QMU_DATA_LO);
> > + int len = (int)ks8842_read16(adapter, 17, REG_QMU_DATA_HI) &
> 0xffff;
> > +#else
> > u32 status = ks8842_read32(adapter, 17, REG_QMU_DATA_LO);
> > int len = (status >> 16) & 0x7ff;
> >
> > status &= 0xffff;
> > +#endif
> >
> > dev_dbg(&adapter->pdev->dev, "%s - rx_data: status: %x\n",
> > __func__, status);
> > @@ -379,13 +432,28 @@ static void ks8842_rx_frame(struct net_d
> > dev_dbg(&adapter->pdev->dev, "%s, got package, len:
> %d\n",
> > __func__, len);
> > if (skb) {
> > +#ifdef CONFIG_KS884X_16BIT
> > + u16 *data16;
> > +#else
> > u32 *data;
> > +#endif
> >
> > netdev->stats.rx_packets++;
> > netdev->stats.rx_bytes += len;
> > if (status & RXSR_MULTICAST)
> > netdev->stats.multicast++;
> >
> > +#ifdef CONFIG_KS884X_16BIT
> > + data16 = (u16 *)skb_put(skb, len);
> > + ks8842_select_bank(adapter, 17);
> > + while (len > 0) {
> > + *data16++ = ioread16(adapter->hw_addr +
> > + REG_QMU_DATA_LO);
> > + *data16++ = ioread16(adapter->hw_addr +
> > + REG_QMU_DATA_HI);
> > + len -= sizeof(u32);
> > + }
> > +#else
> > data = (u32 *)skb_put(skb, len);
> >
> > ks8842_select_bank(adapter, 17);
> > @@ -397,6 +465,7 @@ static void ks8842_rx_frame(struct net_d
> >
> > skb->protocol = eth_type_trans(skb, netdev);
> > netif_rx(skb);
> > +#endif
> > } else
> > netdev->stats.rx_dropped++;
> > } else {
> > --- linux-2.6.35-rc3/drivers/net/Kconfig.orig 2010-07-02
> 15:52:41.000000000 -0700
> > +++ linux-2.6.35-rc3/drivers/net/Kconfig 2010-07-07
> 07:45:47.000000000 -0700
> > @@ -1748,11 +1748,29 @@ config TLAN
> > Please email feedback to <torben.mathiasen@...paq.com>.
> >
> > config KS8842
> > - tristate "Micrel KSZ8842"
> > + tristate "Micrel KSZ8841/42 with generic bus interface"
> > depends on HAS_IOMEM
> > help
> > - This platform driver is for Micrel KSZ8842 / KS8842
> > - 2-port ethernet switch chip (managed, VLAN, QoS).
> > + This platform driver is for KSZ8841(1-port) / KS8842(2-port)
> > + ethernet switch chip (managed, VLAN, QoS) from Micrel or
> > + Timberdale(FPGA).
> > +
> > +if KS8842
> > +config MICREL_KS884X
> > + boolean "KSZ8841/42 device from Micrel"
> > + default n
> > + help
> > + Say Y to use Micrel device. Otherwise Timberdale(FPGA) device
> is
> > + selected.
> > +
> > +config KS884X_16BIT
> > + boolean "16bit bus width"
> > + default y
> > + help
> > + This option specifies 16bit or 32bit bus interface. Say Y to
> use
> > + 16bit bus. Otherwise 32bit bus is selected.
> > +
> > +endif # KS8842
> >
> > config KS8851
> > tristate "Micrel KS8851 SPI"
> > ---
> > --
> > To unsubscribe from this list: send the line "unsubscribe netdev" in
> > the body of a message to majordomo@...r.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists