netdev - Re: [PATCH v1 RFC 1/1] Add Microchip KSZ8795 DSA driver

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: <20171011205916.GC31288@amd>
Date:   Wed, 11 Oct 2017 22:59:16 +0200
From:   Pavel Machek <pavel@....cz>
To:     Tristram.Ha@...rochip.com
Cc:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Ruediger Schmitt <ruediger.schmitt@...lips.com>,
        muvarov@...il.com, nathan.leigh.conrad@...il.com,
        vivien.didelot@...oirfairelinux.com, UNGLinuxDriver@...rochip.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 RFC 1/1] Add Microchip KSZ8795 DSA driver

Hi!

> +static void ksz8795_set_prio_queue(struct ksz_device *dev, int port, int queue)
> +{
> +	u8 hi;
> +	u8 lo;
> +
> +	/* Number of queues can only be 1, 2, or 4. */
> +	switch (queue) {
> +	case 4:
> +	case 3:
> +		queue = PORT_QUEUE_SPLIT_4;
> +		break;
> +	case 2:
> +		queue = PORT_QUEUE_SPLIT_2;
> +		break;
> +	default:
> +		queue = PORT_QUEUE_SPLIT_1;
> +	}

If only 1, 2 and 4 are valid, it probably should not accept other
values?

> +static void ksz8795_r_mib_cnt(struct ksz_device *dev, int port, u16 addr,
> +			      u64 *cnt)
> +{
> +	u32 data;
> +	u16 ctrl_addr;
> +	u8 check;
> +	int loop;
> +
> +	ctrl_addr = addr + SWITCH_COUNTER_NUM * port;
> +	ctrl_addr |= IND_ACC_TABLE(TABLE_MIB | TABLE_READ);
> +
> +	mutex_lock(&dev->alu_mutex);
> +	ksz_write16(dev, REG_IND_CTRL_0, ctrl_addr);
> +
> +	/* It is almost guaranteed to always read the valid bit because of
> +	 * slow SPI speed.
> +	 */
> +	for (loop = 2; loop > 0; loop--) {
> +		ksz_read8(dev, REG_IND_MIB_CHECK, &check);
> +
> +		if (check & MIB_COUNTER_VALID) {
> +			ksz_read32(dev, REG_IND_DATA_LO, &data);
> +			if (check & MIB_COUNTER_OVERFLOW)
> +				*cnt += MIB_COUNTER_VALUE + 1;
> +			*cnt += data & MIB_COUNTER_VALUE;
> +			break;
> +		}
> +	}

Hmm. Maybe, but should not this at least warn if if it can not get
valid counter?

> +	/* It is almost guaranteed to always read the valid bit because of
> +	 * slow SPI speed.
> +	 */
> +	for (loop = 2; loop > 0; loop--) {
> +		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;
> +		}
> +	}

Same here. Plus, is overflow handling correct? There may be more than
MIB_PACKET_DROPPED + 1 packets dropped between the checks. 

> +static void ksz8795_r_table(struct ksz_device *dev, int table, u16 addr,
> +			    u64 *data)
> +{
> +	u16 ctrl_addr;
> +
> +	ctrl_addr = IND_ACC_TABLE(table | TABLE_READ) | addr;
> +
> +	mutex_lock(&dev->alu_mutex);
> +	ksz_write16(dev, REG_IND_CTRL_0, ctrl_addr);
> +	ksz_get(dev, REG_IND_DATA_HI, data, sizeof(u64));
> +	mutex_unlock(&dev->alu_mutex);
> +	*data = be64_to_cpu(*data);
> +}

It would be a tiny bit nicer to have be64 temporary variable and use
it; having *data change endianness at runtime is "interesting".

> +static int ksz8795_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 -EAGAIN;
> +	/* 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 -ENXIO;
> +	}

You can drop else and one indentation level.

> +	/* At least one valid entry in the table. */
> +	} else {
> +		u64 buf;
> +		int cnt;
> +
> +		ksz_get(dev, REG_IND_DATA_HI, &buf, sizeof(buf));
> +		buf = be64_to_cpu(buf);

Would it make sense to convert endianness inside ksz_get?

									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)