| 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, 8 Sep 2017 11:18:56 +0200
From: Pavel Machek <pavel@....cz>
To: Tristram.Ha@...rochip.com
Cc: andrew@...n.ch, muvarov@...il.com, nathan.leigh.conrad@...il.com,
vivien.didelot@...oirfairelinux.com, f.fainelli@...il.com,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
Woojung.Huh@...rochip.com
Subject: Re: [PATCH RFC 3/5] Add KSZ8795 switch driver
On Thu 2017-09-07 21:17:16, Tristram.Ha@...rochip.com wrote:
> From: Tristram Ha <Tristram.Ha@...rochip.com>
>
> Add KSZ8795 switch support with function code.
English? "Add KSZ8795 switch support." ?
> Signed-off-by: Tristram Ha <Tristram.Ha@...rochip.com>
> ---
> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> new file mode 100644
> index 0000000..e4d4e6a
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -0,0 +1,2066 @@
> +/*
> + * Microchip KSZ8795 switch driver
> + *
> + * Copyright (C) 2017 Microchip Technology Inc.
> + * Tristram Ha <Tristram.Ha@...rochip.com>
> + *
> + * Permission to use, copy, modify, and/or distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
This is not exactly GPL, right? But tagging below says it is
GPL. Please fix one.
> +/**
> + * Some counters do not need to be read too often because they are less likely
> + * to increase much.
> + */
Kerneldoc markup but this does not look like kerneldoc. Use plain /* instead?
> +static void ksz_cfg(struct ksz_device *dev, u32 addr, u8 bits, bool set)
> +{
> + u8 data;
> +
> + ksz_read8(dev, addr, &data);
> + if (set)
> + data |= bits;
> + else
> + data &= ~bits;
> + ksz_write8(dev, addr, data);
> +}
> +
> +static void ksz_port_cfg(struct ksz_device *dev, int port, int offset, u8 bits,
> + bool set)
> +{
> + u32 addr;
> + u8 data;
> +
> + addr = PORT_CTRL_ADDR(port, offset);
> + ksz_read8(dev, addr, &data);
> +
> + if (set)
> + data |= bits;
> + else
> + data &= ~bits;
> +
> + ksz_write8(dev, addr, data);
> +}
Other drivers will need this code, right? Does it make sense to put it
into header?
> +static int chk_last_port(struct ksz_device *dev, int p)
> +{
> + if (dev->last_port && p == dev->last_port)
> + p = dev->cpu_port;
> + return p;
> +}
We often prefix even static functions with common prefix. Up to you, I
guess.
> +static int ksz_reset_switch(struct ksz_device *dev)
> +{
> + /* reset switch */
> + ksz_write8(dev, REG_POWER_MANAGEMENT_1,
> + SW_SOFTWARE_POWER_DOWN << SW_POWER_MANAGEMENT_MODE_S);
> + ksz_write8(dev, REG_POWER_MANAGEMENT_1, 0);
> +
> + return 0;
> +}
This is going to be same in other drivers, right?
> +
> + for (timeout = 1; timeout > 0; timeout--) {
> + ksz_read8(dev, REG_IND_MIB_CHECK, &check);
> +
> + if (check & MIB_COUNTER_VALID) {
> + ksz_read32(dev, REG_IND_DATA_LO, &data);
> + if (addr < 2) {
> + u64 total;
> +
> + total = check & MIB_TOTAL_BYTES_H;
> + total <<= 32;
> + *cnt += total;
> + *cnt += data;
> + if (check & MIB_COUNTER_OVERFLOW) {
> + total = MIB_TOTAL_BYTES_H + 1;
> + total <<= 32;
> + *cnt += total;
> + }
> + } else {
> + if (check & MIB_COUNTER_OVERFLOW)
> + *cnt += MIB_PACKET_DROPPED + 1;
> + *cnt += data & MIB_PACKET_DROPPED;
> + }
> + break;
> + }
> + }
Why do you need a loop here? This is quite strange code. (And you have
similar strangeness elsewhere. Please fix.)
> +static int valid_dyn_entry(struct ksz_device *dev, u8 *data)
> +{
> + int timeout = 100;
> +
> + do {
> + ksz_read8(dev, REG_IND_DATA_CHECK, data);
> + timeout--;
> + } while ((*data & DYNAMIC_MAC_TABLE_NOT_READY) && timeout);
> +
> + /* Entry is not ready for accessing. */
> + if (*data & DYNAMIC_MAC_TABLE_NOT_READY) {
> + return 1;
> + /* Entry is ready for accessing. */
> + } else {
> + ksz_read8(dev, REG_IND_DATA_8, data);
> +
> + /* There is no valid entry in the table. */
> + if (*data & DYNAMIC_MAC_TABLE_MAC_EMPTY)
> + return 2;
> + }
> + return 0;
> +}
Normal calling convention is 0 / -ERROR, not 0,1,2.
> +static void ksz_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
> +{
> + struct ksz_port *port;
> + u8 ctrl;
> + u8 restart;
> + u8 link;
> + u8 speed;
> + u8 force;
> + u8 p = phy;
> + u16 data = 0;
> + int processed = true;
> +
> + port = &dev->ports[p];
> + switch (reg) {
> + case PHY_REG_CTRL:
> + ksz_pread8(dev, p, P_LOCAL_CTRL, &ctrl);
> + ksz_pread8(dev, p, P_NEG_RESTART_CTRL, &restart);
> + ksz_pread8(dev, p, P_SPEED_STATUS, &speed);
> + ksz_pread8(dev, p, P_FORCE_CTRL, &force);
> + if (restart & PORT_PHY_LOOPBACK)
> + data |= PHY_LOOPBACK;
> + if (force & PORT_FORCE_100_MBIT)
> + data |= PHY_SPEED_100MBIT;
> + if (!(force & PORT_AUTO_NEG_DISABLE))
> + data |= PHY_AUTO_NEG_ENABLE;
> + if (restart & PORT_POWER_DOWN)
> + data |= PHY_POWER_DOWN;
> + if (restart & PORT_AUTO_NEG_RESTART)
> + data |= PHY_AUTO_NEG_RESTART;
> + if (force & PORT_FORCE_FULL_DUPLEX)
> + data |= PHY_FULL_DUPLEX;
> + if (speed & PORT_HP_MDIX)
> + data |= PHY_HP_MDIX;
> + if (restart & PORT_FORCE_MDIX)
> + data |= PHY_FORCE_MDIX;
> + if (restart & PORT_AUTO_MDIX_DISABLE)
> + data |= PHY_AUTO_MDIX_DISABLE;
> + if (restart & PORT_TX_DISABLE)
> + data |= PHY_TRANSMIT_DISABLE;
> + if (restart & PORT_LED_OFF)
> + data |= PHY_LED_DISABLE;
> + break;
> + case PHY_REG_STATUS:
> + ksz_pread8(dev, p, P_LINK_STATUS, &link);
> + ksz_pread8(dev, p, P_SPEED_STATUS, &speed);
> + data = PHY_100BTX_FD_CAPABLE |
> + PHY_100BTX_CAPABLE |
> + PHY_10BT_FD_CAPABLE |
> + PHY_10BT_CAPABLE |
> + PHY_AUTO_NEG_CAPABLE;
> + if (link & PORT_AUTO_NEG_COMPLETE)
> + data |= PHY_AUTO_NEG_ACKNOWLEDGE;
> + if (link & PORT_STAT_LINK_GOOD) {
> + data |= PHY_LINK_STATUS;
> + if (!port->link_up) {
> + port->link_up = 1;
> + dev->live_ports |= (1 << phy) & dev->on_ports;
> + }
> + } else if (port->link_up) {
> + port->link_down = 1;
> + port->link_up = 0;
> + dev->live_ports &= ~(1 << phy);
> + }
> + break;
> + case PHY_REG_ID_1:
> + data = KSZ8795_ID_HI;
> + break;
> + case PHY_REG_ID_2:
> + data = KSZ8795_ID_LO;
> + break;
> + case PHY_REG_AUTO_NEGOTIATION:
> + ksz_pread8(dev, p, P_LOCAL_CTRL, &ctrl);
> + data = PHY_AUTO_NEG_802_3;
> + if (ctrl & PORT_AUTO_NEG_SYM_PAUSE)
> + data |= PHY_AUTO_NEG_SYM_PAUSE;
> + if (ctrl & PORT_AUTO_NEG_100BTX_FD)
> + data |= PHY_AUTO_NEG_100BTX_FD;
> + if (ctrl & PORT_AUTO_NEG_100BTX)
> + data |= PHY_AUTO_NEG_100BTX;
> + if (ctrl & PORT_AUTO_NEG_10BT_FD)
> + data |= PHY_AUTO_NEG_10BT_FD;
> + if (ctrl & PORT_AUTO_NEG_10BT)
> + data |= PHY_AUTO_NEG_10BT;
> + break;
> + case PHY_REG_REMOTE_CAPABILITY:
> + ksz_pread8(dev, p, P_REMOTE_STATUS, &link);
> + data = PHY_AUTO_NEG_802_3;
> + if (link & PORT_REMOTE_SYM_PAUSE)
> + data |= PHY_AUTO_NEG_SYM_PAUSE;
> + if (link & PORT_REMOTE_100BTX_FD)
> + data |= PHY_AUTO_NEG_100BTX_FD;
> + if (link & PORT_REMOTE_100BTX)
> + data |= PHY_AUTO_NEG_100BTX;
> + if (link & PORT_REMOTE_10BT_FD)
> + data |= PHY_AUTO_NEG_10BT_FD;
> + if (link & PORT_REMOTE_10BT)
> + data |= PHY_AUTO_NEG_10BT;
> + break;
> + default:
> + processed = false;
> + break;
> + }
> + if (processed)
> + *val = data;
> +}
Similar code will be needed by other drivers, right?
> +static int ksz_port_bridge_join(struct dsa_switch *ds, int port,
> + struct net_device *br)
> +{
> + struct ksz_device *dev = ds->priv;
> +
> + dev->br_member |= (1 << port);
> +
> + /**
> + * port_stp_state_set() will be called after to put the port in
> + * appropriate state so there is no need to do anything.
> + */
/** -> /*. Fix it elsewhere, too. This is not kerneldoc.
> + /* Topology is changed. */
> + if (forward != dev->member)
has changed?
Thanks for the patches,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Download attachment "signature.asc" of type "application/pgp-signature" (182 bytes)
Powered by blists - more mailing lists