netdev - Re: [PATCH net-next] dmfe: enforce consistent timing delay.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANEJEGsb6_6btnu4OdriK=V2MbHcmxmvGy8UgsjNG1VPS_B0oQ@mail.gmail.com>
Date:	Tue, 17 Apr 2012 14:45:38 -0700
From:	Grant Grundler <grantgrundler@...il.com>
To:	Francois Romieu <romieu@...zoreil.com>
Cc:	netdev@...r.kernel.org, David Miller <davem@...emloft.net>
Subject: Re: [PATCH net-next] dmfe: enforce consistent timing delay.

On Tue, Apr 17, 2012 at 2:11 PM, Francois Romieu <romieu@...zoreil.com> wrote:
> The driver does not always use the same timing for what looks like
> the same operations.
>
> - DCR0
>  Use the same udelay everywhere for reset. Upper bound is 100 us.
> - DCR9
>  Use 5us delay for srom clock. 1us delay for phy_write_1bit (writes
>  PHY_DATA_[01]) are not changed as they stay withing a 2,5MHz MDIO
>  clock range.
>
> Signed-off-by: Francois Romieu <romieu@...zoreil.com>


Reviewed-by: Grant Grundler <grundler@...isc-linux.org>

> ---
>
> I noticed those while staring at the driver.
>
> Grant, regarding your previous remarks, I do not see where the problem
> could be with posted writes MMIO accesses:
> - the driver has always used the first (#0) bar register. It's hardcoded.
>  The driver still uses this same bar register.
> - the driver has never checked whether bar #0 was related to I/O or
>  memory space. It assumed an I/O decoder and issued out* instructions.
> - the DM9102 datasheet states BAR #0 is an IO only BAR. No idea for the
>  DM9132, 9100 and 9009 though.

I didn't realize BAR0 is hard coded in the driver. BAR0 unlikely to
become MMIO for other devices. I think you are right - there is no
problem.

> My changes have replaced in/out* accesses with ioread/write* - mostly
> to help detecting places where type checking would have uncloaked a
> forgotten net_device.base_addr.
>
> Thus, if someone has a dmfe device including a MMIO space decoder behind
> bar #0, iowrite* will now emit memory write instructions where the driver
> previously used I/O operations. It may not work very well due to posted
> writes issues, sure.

unlikely. I'll worry about it when someone does in fact find this configuration.

> However I wonder on which platform - if any - the
> former MMIO space + I/O accesses (!) combo would have behaved correctly.
> ?

MMIO and I/O can interoperate to the same device since the device
registers are just having transactions sent to them via different
address decoders. Drivers typically only use one or the other though
if both are available.

thanks,
grant

>
>  drivers/net/ethernet/dec/tulip/dmfe.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/ethernet/dec/tulip/dmfe.c b/drivers/net/ethernet/dec/tulip/dmfe.c
> index 0ef5b68..4d6fe60 100644
> --- a/drivers/net/ethernet/dec/tulip/dmfe.c
> +++ b/drivers/net/ethernet/dec/tulip/dmfe.c
> @@ -767,7 +767,7 @@ static int dmfe_stop(struct DEVICE *dev)
>
>        /* Reset & stop DM910X board */
>        dw32(DCR0, DM910X_RESET);
> -       udelay(5);
> +       udelay(100);
>        phy_write(ioaddr, db->phy_addr, 0, 0x8000, db->chip_id);
>
>        /* free interrupt */
> @@ -1601,7 +1601,9 @@ static u16 read_srom_word(void __iomem *ioaddr, int offset)
>        int i;
>
>        dw32(DCR9, CR9_SROM_READ);
> +       udelay(5);
>        dw32(DCR9, CR9_SROM_READ | CR9_SRCS);
> +       udelay(5);
>
>        /* Send the Read Command 110b */
>        srom_clk_write(ioaddr, SROM_DATA_1);
> @@ -1615,6 +1617,7 @@ static u16 read_srom_word(void __iomem *ioaddr, int offset)
>        }
>
>        dw32(DCR9, CR9_SROM_READ | CR9_SRCS);
> +       udelay(5);
>
>        for (i = 16; i > 0; i--) {
>                dw32(DCR9, CR9_SROM_READ | CR9_SRCS | CR9_SRCLK);
> @@ -1626,6 +1629,7 @@ static u16 read_srom_word(void __iomem *ioaddr, int offset)
>        }
>
>        dw32(DCR9, CR9_SROM_READ);
> +       udelay(5);
>        return srom_data;
>  }
>
> --
> 1.7.7.6
>
--
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