| 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: Thu, 6 Jan 2011 08:10:47 +0100
From: Uwe Kleine-König
<u.kleine-koenig@...gutronix.de>
To: Shawn Guo <shawn.guo@...escale.com>
Cc: davem@...emloft.net, gerg@...pgear.com, baruch@...s.co.il,
eric@...rea.com, bryan.wu@...onical.com, r64343@...escale.com,
B32542@...escale.com, lw@...o-electronics.de,
w.sang@...gutronix.de, s.hauer@...gutronix.de,
netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 05/10] net/fec: add dual fec support for mx28
Hello Shawn,
On Thu, Jan 06, 2011 at 12:14:59PM +0800, Shawn Guo wrote:
> On Wed, Jan 05, 2011 at 05:34:49PM +0100, Uwe Kleine-König wrote:
> > On Wed, Jan 05, 2011 at 10:07:32PM +0800, Shawn Guo wrote:
> > > +#define DRIVER_NAME "fec"
> > > +#define ENET_MAC_NAME "enet-mac"
> > > +
> > > +static struct platform_device_id fec_devtype[] = {
> > > + {
> > > + .name = DRIVER_NAME,
> > > + }, {
> > > + .name = ENET_MAC_NAME,
> > > + }
> > I'd done it differently:
> >
> > {
> > .name = "fec",
> > .driver_data = 0,
> > }, {
> > .name = "imx28-fec",
> > .driver_data = HAS_ENET_MAC | ...,
> > }
> >
> > and then test the bits in driver_data (which you get using
> > platform_get_device_id() when you need to distinguish.
> > Comparing names doesn't scale, assume there are three further features
> > to distinguish, then you need to use strtok or index and get device
> > names like enet-mac-with-feature1-but-without-feature2-and-feature3.
> >
> Makes sense. The frame endian issue will be fixed in future revision,
> so I would define bit SWAP_FRAME for testing.
sounds sane
> > > + /*
> > > + * enet-mac design made an improper assumption that it's running
> > > + * on a big endian system. As the result, driver has to swap
> > if he was really aware that he limits the performant use of the fec to
> > big endian systems, can you please make him stop designing hardware!?
> >
> You over looked my power :) BTW, he had left Freescale.
so everything seems OK with your power :-)
> > > + * every frame going to and coming from the controller.
> > > + */
> > > + if (fec_is_enetmac)
> > > + swap_buffer(bufaddr, skb->len);
> > > +
> > > /* Save skb pointer */
> > > fep->tx_skbuff[fep->skb_cur] = skb;
> > >
> > > @@ -487,6 +522,9 @@ fec_enet_rx(struct net_device *dev)
> > > dma_unmap_single(NULL, bdp->cbd_bufaddr, bdp->cbd_datlen,
> > > DMA_FROM_DEVICE);
> > >
> > > + if (fec_is_enetmac)
> > > + swap_buffer(data, pkt_len);
> > > +
> > > /* This does 16 byte alignment, exactly what we need.
> > > * The packet length includes FCS, but we don't want to
> > > * include that when passing upstream as it messes up
> > > @@ -689,6 +727,7 @@ static int fec_enet_mii_probe(struct net_device *dev)
> > > char mdio_bus_id[MII_BUS_ID_SIZE];
> > > char phy_name[MII_BUS_ID_SIZE + 3];
> > > int phy_id;
> > > + int dev_id = fep->pdev->id;
> > >
> > > fep->phy_dev = NULL;
> > >
> > > @@ -700,6 +739,8 @@ static int fec_enet_mii_probe(struct net_device *dev)
> > > continue;
> > > if (fep->mii_bus->phy_map[phy_id]->phy_id == 0)
> > > continue;
> > > + if (fec_is_enetmac && dev_id--)
> > > + continue;
> > > strncpy(mdio_bus_id, fep->mii_bus->id, MII_BUS_ID_SIZE);
> > > break;
> > > }
> > > @@ -741,6 +782,28 @@ static int fec_enet_mii_init(struct platform_device *pdev)
> > > struct fec_enet_private *fep = netdev_priv(dev);
> > > int err = -ENXIO, i;
> > >
> > > + /*
> > > + * The dual fec interfaces are not equivalent with enet-mac.
> > > + * Here are the differences:
> > > + *
> > > + * - fec0 supports MII & RMII modes while fec1 only supports RMII
> > > + * - fec0 acts as the 1588 time master while fec1 is slave
> > > + * - external phys can only be configured by fec0
> > > + *
> > > + * That is to say fec1 can not work independently. It only works
> > > + * when fec0 is working. The reason behind this design is that the
> > > + * second interface is added primarily for Switch mode.
> > > + *
> > > + * Because of the last point above, both phys are attached on fec0
> > > + * mdio interface in board design, and need to be configured by
> > > + * fec0 mii_bus.
> > > + */
> > > + if (fec_is_enetmac && pdev->id) {
> > > + /* fec1 uses fec0 mii_bus */
> > > + fep->mii_bus = fec_mii_bus;
> > > + return 0;
> > > + }
> > > +
> > > fep->mii_timeout = 0;
> > >
> > > /*
> > > @@ -777,6 +840,10 @@ static int fec_enet_mii_init(struct platform_device *pdev)
> > > if (mdiobus_register(fep->mii_bus))
> > > goto err_out_free_mdio_irq;
> > >
> > > + /* save fec0 mii_bus */
> > > + if (fec_is_enetmac)
> > > + fec_mii_bus = fep->mii_bus;
> > > +
> > > return 0;
> > >
> > > err_out_free_mdio_irq:
> > > @@ -1149,11 +1216,22 @@ fec_restart(struct net_device *dev, int duplex)
> > > {
> > > struct fec_enet_private *fep = netdev_priv(dev);
> > > int i;
> > > + u32 val, temp_mac[2];
> > >
> > > /* Whack a reset. We should wait for this. */
> > > writel(1, fep->hwp + FEC_ECNTRL);
> > > udelay(10);
> > >
> > > + /*
> > > + * enet-mac reset will reset mac address registers too,
> > > + * so need to reconfigure it.
> > > + */
> > > + if (fec_is_enetmac) {
> > > + memcpy(&temp_mac, dev->dev_addr, ETH_ALEN);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > + writel(cpu_to_be32(temp_mac[0]), fep->hwp + FEC_ADDR_LOW);
> > > + writel(cpu_to_be32(temp_mac[1]), fep->hwp + FEC_ADDR_HIGH);
> > where is the value saved to temp_mac[]? For me it looks you write
> > uninitialized data into the mac registers.
>
> memcpy above.
oops. right. I looked for something like
temp_mac[0] = dev->dev_addr[0] << $shiftfor0 | ...
which AFAIK is what you want here. memcpy is sensible to (at least)
endian issues. If you ask me factor out the setting of the mac address
in probe to a function and use that here, too.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
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